bug#63941: [PATCH] ; always CRLF before non-first boundary in multipart form

[Stefan Monnier]

OK, I'm not super familiar with this code, and while I really appreciate
your tests, I'd rather not completely replace the old with a brand new
code because that makes it hard for me to see what's really changed.

So, as a first step I took the existing code, and did the following:
- "Inline" the (unless (bolp) (insert "\r\n")) into every branch of the
  preceding `cond`.  We know this makes no difference.
- Remove the (unless (bolp) (insert "\r\n")) in the "submit" branch
  since it follows an insertion that ends in "n" and hence doesn't
  do anything.
- Remove the `(unless (bolp)` test from the "file" branch since that's
  what this bug report is about:
  - when `filedata` is a number, this makes no difference (we just
    inserted that number without a trailing \n).
  - when `filedata` is a string that ends in non-\n (a case that
    currently works right) this makes no difference.
  - when it does end in \n this does make a difference which fixes
    this bug.
  - when `filedata` is an empty string, this add an additional \r\n
    compared to the current code.  This seems right to me (I expect the
    decoding software will skip the \r\n at the of the header and then
    look for \r\n<BOUNDARY>, so it's important to have two \r\n).
- In the default case, I left the code as is, but the `(unless (bolp)`
  test is probably not right.

There remain some questions on this patch:

1- When `filedata` is neither a number nor a string this is treated the
   same as an empty string.  Is that right?  Or should it just never
   happen (in which case we should probably catch this case and signal
   an error).
2- Should we also remove the `(unless (bolp)` in the default case?
   I think we do (and my reading of your tests agrees, tho I couldn't
   run your test suite on this version of the code, among other things
   because it contains multiple tests with the same name, so it gives
   me things like (error "Test
   `test-mm-url-encode-multipart-form-data-A-ab-c' redefined")).

I suspect we can also simply this code by moving the first
(insert "--" boundary "\r\n") before the loop, and the second into the
loop so we can make it insert "\r\n--" boundary "\r\n" (and thus remove
\r\n from the end of each of the preceding branches).


        Stefan


diff --git a/lisp/gnus/mm-url.el b/lisp/gnus/mm-url.el
index 11847a79f17..3041ad16264 100644
--- a/lisp/gnus/mm-url.el
+++ b/lisp/gnus/mm-url.el
@@ -430,16 +430,17 @@ mm-url-encode-multipart-form-data
              (insert filedata))
             ;; How can this possibly be useful?
             ((integerp filedata)
-             (insert (number-to-string filedata))))))
+             (insert (number-to-string filedata)))))
+         (insert "\r\n"))
         ((equal name "submit")
          (insert
           "Content-Disposition: form-data; name=\"submit\"\r\n\r\nSubmit\r\n"))
         (t
          (insert (format "Content-Disposition: form-data; name=%S\r\n\r\n"
                          name))
-         (insert value)))
-       (unless (bolp)
-         (insert "\r\n"))))
+         (insert value)
+         (unless (bolp)
+           (insert "\r\n"))))))
     (insert "--" boundary "--\r\n")
     (buffer-string)))