Re: [PATCH v5 2/2] submodule: pass on http.extraheader config settings

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Apr 29, 2016, 12:35 UTC
Message-ID: <alpine.DEB.2.20.1604291429560.9313@virtualbox>
In-Reply-To: <xmqqshy54j2h.fsf@gitster.mtv.corp.google.com>

Hi Junio,

On Thu, 28 Apr 2016, Junio C Hamano wrote:

Show 10 quoted lines

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > -     if (starts_with(var, "credential."))
> > +     if (starts_with(var, "credential.") ||
> > +                     (starts_with(var, "http.") &&
> > +                      ends_with(var, ".extraheader")))
> 
> I know you are fond of indenting with HT without aligning things,
> but this is going too far in the quest of making the code
> unreadable.

Hah. I am actually not fond of anything there, but I go with the default in my vim when selecting lines and pressing the '=' key...

If you know off-hand how to teach my vim to use your preferred indenting, I'll gladly just brow-beat it into submission.

Show 5 quoted lines

>         if (starts_with(var, "credential.") ||
>             (starts_with(var, "http.") && ends_with(var, ".extraheader")))
> 
> would make iteasier to see what are the top-level items (there are two)
> and how they are related (just one of them needs to be satisfied).

Fine by me!

Show 27 quoted lines

> Assuming that we will discover more variables that can be safely
> passed, I'd rather see the above written like this, though:
> 
>         if (starts_with(var, "credential."))
>                 return 1;
>         if (starts_with(var, "http.") && ends_with(var, ".extraheader"))
>                 return 1;
> 
>         return 0;
> 
> Or even something along this line:
> 
>         struct whitelist {
>                 const char *prefix;
>                 const char *suffix;
>         } whitelist[] = {
>                 { "credential.", NULL },
>                 { "http.", ".extraheader" },
>         };
> 
>         for (i = 0; i < ARRAY_SIZE(whitelist); i++) {
>                 struct whitelist *w = &whitelist[i];
>                 if ((!w->prefix || starts_with(var, w->prefix)) &&
>                     (!w->suffix || ends_with(var, w->suffix)))
>                         return 1;
>         }
>         return 0;

Iff. Iff we go with a white-list.

However, I think you did a really good job arguing that the CONFIG_DATA_ENVIRONMENT filtering is, in fact, overzealous.

Just let me know what to go with, and I'll update the patch accordingly.

Ciao, Dscho