[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Bug-wget] [Patch] fix bug #44628 not honoring RFC 6266 in --content
From: |
Miquel Llobet |
Subject: |
Re: [Bug-wget] [Patch] fix bug #44628 not honoring RFC 6266 in --content-disposition |
Date: |
Fri, 3 Apr 2015 04:12:31 +0200 |
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 <address@hidden> wrote:
> Miquel Llobet <address@hidden> writes:
>
> > From d71c7cc43689fc752dedb2a3500673f9981f7fc9 Mon Sep 17 00:00:00 2001
> > From: Miquel Llobet <address@hidden>
> > 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