Re: [PATCH 1/2] status: really ignore config with --porcelain

From: Ramkumar Ramachandra <artagnon@gmail.com>
Date: Jun 24, 2013, 14:05 UTC
Message-ID: <CALkWK0=F_i95S+53eZmOAJtA+jG=jvi5-sDc3BgW3rNQo=n3Ng@mail.gmail.com>
In-Reply-To: <vpqhagnwraj.fsf@anie.imag.fr>

Matthieu Moy wrote:

> [...]

Before we begin, let me say that this series is written with the emergency-fix mindset, and targets maint. I didn't spend time thinking about how to do it best or even add tests.

Show 11 quoted lines

>> The correct solution is therefore to skip the config parser completely
>> when --porcelain is given.  Unfortunately, to determine that --porcelain
>> is given, we have to run the command-line option parser.  Running the
>> command-line option parser before the config parser is undesirable, as
>> configuration variables would override options on the command-line.  As
>> a fair compromise, check that argv[1] is equal to the string
>> "--porcelain" and skip the config parser in this case.
>
> I really don't like this. If we go for a solution looking explicitely at
> argv[], we should at least iterate over it (also not satisfactory
> because --porcelain could be the argument of another switch).

Yep, that's the compromise.

Show 5 quoted lines

> I think it's possible to have an actually robust solution, either
>
> * running the CLI parser after, if --porcelain is given, reset the
>   effect of the variables. Not very clean because we'd have to reset all
>   the variables to their default, and there is a risk of forgetting one.

Since it's impossible to determine what effect the CLI parser had on various variables (some of which are static global), I'm against this approach.

> * Or, running the CLI parser before, but with different variables to
>   specify what the command-line says and what will actually be done,
>   with something like

Basically, having the CLI parser and the config parser flip two different sets of variables, so we can discriminate who set what. What annoys me is that this is the first instance of such a requirement.

The approach I'm currently tilting towards is extending the parse-options API to allow parsing one special option early. I would argue that this is a good feature that we should have asked for when we saw 6758af89e (Merge branch 'jn/git-cmd-h-bypass-setup', 2010-12-10). What do you think?

Show 5 quoted lines

>>  builtin/commit.c | 7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> No time to contribute one now myself, but this would really deserve a
> test.

Yeah, will do as a follow-up. I'm interested in guarding against such breakages too :)