Thanks for the review and git tips! I made all the changes you suggested. using filename as a temporary in case !encodedFilename is fine, but > maybe we can make it clearer and use two different local temporary > values (encodedFilename and unencodedFilename) and set *filename > according to which one is available? What do you think?
I think so too, it's easier to understand this way. Miquel Llobet On Thu, Apr 2, 2015 at 3:01 PM, Giuseppe Scrivano <[email protected]> wrote: > Miquel Llobet <[email protected]> writes: > > > From d71c7cc43689fc752dedb2a3500673f9981f7fc9 Mon Sep 17 00:00:00 2001 > > From: Miquel Llobet <[email protected]> > > Date: Wed, 1 Apr 2015 17:32:50 +0200 > > Subject: [PATCH] Fixed #44628 honoring RFC 6266 content-disposition > > > > src/http.c (parse_content_disposition): stores filename* and filename > > separately and choses filename* if available > > src/http.c (test_parse_content_disposition): added new tests for the > > reported bu > > this line seems incomplete. > > > --- > > src/http.c | 27 ++++++++++++++++++++++----- > > 1 file changed, 22 insertions(+), 5 deletions(-) > > > > diff --git a/src/http.c b/src/http.c > > index 9994d13..1f98e5e 100644 > > --- a/src/http.c > > +++ b/src/http.c > > @@ -1202,6 +1202,7 @@ parse_content_disposition (const char *hdr, char > **filename) > > bool is_url_encoded = false; > > > > *filename = NULL; > > + char *encodedFilename = NULL; > > declaration after statement, please move it one line up. > > > for ( ; extract_param (&hdr, &name, &value, ';', &is_url_encoded); > > is_url_encoded = false) > > { > > @@ -1218,17 +1219,27 @@ parse_content_disposition (const char *hdr, char > **filename) > > if (value.b == value.e) > > continue; > > > > - if (*filename) > > - append_value_to_filename (filename, &value, is_url_encoded); > > + /* Check if the name is "filename*" as specified in RFC 6266. > > trailing whitespace here. > > I find useful to have these lines > > [color] > branch = auto > diff = auto > interactive = auto > status = auto > > in my ~/.gitconfig file. > > > > + * Since "filename" could be broken up as "filename*N" (RFC > 2231), > > + * a check is needed to make sure this is not the case */ > > + int isEncodedFilename = *name.e == '*'&& !c_isdigit(*(name.e > + 1)); > > Please use a "bool" instead of "int". > > Space before && and after the function name: c_isdigit (*(name.e + 1)); > > > + char **outFilename = isEncodedFilename ? &encodedFilename : > filename; > > both isEncodedFilename and outFilename must be declared immediately > after the '{' before any code statement. > > > > + if (*outFilename) > > + append_value_to_filename (outFilename, &value, > is_url_encoded); > > else > > { > > - *filename = strdupdelim (value.b, value.e); > > + *outFilename = strdupdelim (value.b, value.e); > > if (is_url_encoded) > > - url_unescape (*filename); > > + url_unescape (*outFilename); > > } > > } > > } > > > > + if (encodedFilename) > > + { > > + xfree (*filename); > > + *filename = encodedFilename; > > + } > > using filename as a temporary in case !encodedFilename is fine, but > maybe we can make it clearer and use two different local temporary > values (encodedFilename and unencodedFilename) and set *filename > according to which one is available? What do you think? > > if (encodedFilename) > { > xfree (unencodedfilename); > *filename = encodedFilename; > } > else > { > xfree (encodedfilename); > *filename = unencodedFilename; > } > > Regards, > Giuseppe >
0001-Fixed-44628-honoring-RFC-6266-content-disposition.patch
Description: Binary data
