Re: [PATCH] git-config: Parse config files leniently

From: Michael J Gruber <git@drmicha.warpmail.net>
Date: Aug 17, 2009, 18:47 UTC
Message-ID: <4A89A5B8.9040405@drmicha.warpmail.net>
In-Reply-To: <7veirejhqq.fsf@alter.siamese.dyndns.org>

Junio C Hamano venit, vidit, dixit 14.08.2009 21:52:

Show 11 quoted lines

> Michael J Gruber <git@drmicha.warpmail.net> writes:
> 
>> Currently, git config dies as soon as there is a parsing error. This is
>> especially unfortunate in case a user tries to correct config mistakes
>> using git config -e.
>>
>> Instead, issue a warning only and treat the rest of the line as a
>> comment (ignore it). This benefits not only git config -e users.
> 
> ... a broken sentence in the middle?  I would have expected the "not only"
> followed by "but also"; the question is "but also what?"

I don't see any broken sentence here. "Benefit" is a verb as well as a noun.

"not only git config -e users" but also everyone else: Unparseable lines are ignored, the rest is parsed.

> 
> Hopefully the benefit is not that it now allows all the other commands to
> cause unspecified types of damage to the repository by following iffy
> settings obtained from a broken configuration file.

The only possible problem that I see is when a section heading is not parsed because of a forgotten "[", say, and the following settings are put in the wrong section because of that.

Show 14 quoted lines

>> Reported-by: David Reitter <david.reitter@gmail.com>
>> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
> 
>> Test had to be adjusted as well.
> 
> The change to the test demonstrates the issue rather well.  The check()
> shell function does not check the exit value from "git config --get", but
> in a real script that cares to check and stop on error, this change will
> now let the script go on, leaving the breakage unnoticed.  I suspect
> command implemented in C, that call git_config(), will also have the same
> issue, and I cannot convince myself this is a good change in general,
> outside the scope of helping "git config -e".
> 
> But I may be being overly cautious.

My first version had the lenient mode for "git config -e" only, which required a new global int (or, alternatively, changing all callers).

One could issue a warning and return -1 rather than success. I'm afraid git_config() callers don't check return values but rely on git_config() dieing in case there are parsing problems.

> 
> By the way, why did you have to change s/echo/printf/?  Can't you give two
> lines in a single argument without "\n" escape?

Because "printf" is more portable then "echo -e". At least I hope so ;) [ One could use a "here document", of course. Is that preferable? ]

Michael