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

[Michael J Gruber <git@drmicha.warpmail.net>]

Junio C Hamano venit, vidit, dixit 04.09.2009 01:42:
> Michael J Gruber <git@drmicha.warpmail.net> writes:
> 
>>> Why was I CC'ed, if the patch wasn't even self tested?
>>
>> Because
>> - not CC'ing you would have meant culling you from the existing CC,
>> - we've discussed v1 of this patch before,
>> - I asked in this patch (v2) whether to go for an alternative.
> 
> Oh, so you did not mean this was for inclusion but as another round
> of RFC.  I misread your intent.  Sorry about that.

OK. I'll try to distinguish better between RFC and RFI in the future.
And, yes, usually I run *all* tests before sending patches. I had even
grepped all tests for "config" in order not to miss any, but failed to
note how the patch affects all other commands as well.

> 
> The eralier analysis of the cause of the breakage indicated that the
> implementation in this patch was flawed.  What it essentially did was to
> re-define all die() to warn() in the codepaths around configuration
> variable handling [*1*].
> 
> However, it does not mean that the idea of "ignoring syntax errors while
> keeping other errors still noticed for all commands, not limited to config
> nor not limited to 'config -e'" is necessarily flawed.
> 
> For example, the test I noticed the breakage with was stuffing an invalid
> value to branch.autosetuprebase and wanted to see "git branch" fail.
> 
> Obviously we do want to fail a "git branch newbranch origin/master" when
> the value given to branch.autosetuprebase is misspelled, to avoid creating
> bogus settings the user did not intend to.  We do care about semantic
> errors (i.e. this variable can take only one of these values, but the
> value given in the file is bogus) in such a case.  But if you are running
> "git branch" only to view, but not to create, there is no reason for us to
> care about the branch.autosetuprebase variable [*2*].
> 
> This observation suggests that it may make sense to make the error
> handling _even looser_ than what you intended to do in your patch (which I
> assume was "we ignore syntax errors and try to recover, pretending that we
> saw a comment until the end of line, but we still keep the validation of
> values assigned to variables that the existing commands rely on").
> 
> Ideally, the rule would be "we care about the values of variables we are
> going to use, but allow misspelled values in variables that are not used
> by us---we have no business complaining about them."  Unfortunately that
> is much harder to arrange with the current code structure, but under that
> rule, "config -e" would care only about "core.editor" and nothing else, so
> as long as that variable can be sanely read, it should be able to start.
> 
> 
> [Footnotes]
> 
> *1* The additional test that came with the patch only checked for the
> positive case (i.e. "does the system with this patch treats errors looser
> than before?"), but failed to check the negative case (i.e. "does the
> change too much and stop catching errors that should be caught?"), which
> unfortunately is a common mistake that easily lets bugs go unnoticed.
> 
> *2* Worse yet, the parsing of branch.autosetuprebase is part of the
> default_config and commands that do not have anything to do with new
> branch creation will fail with the current setup.

Thanks for the thorough explanations. Especially *2* makes me think that
quite some restructuring would be necessary in order to "do it right".
That would be above my head (and time constraints).

Given that, I think the practical options are

a) make "git config" parse leniently (both -e and others)
b) leave as is and document how to recover from bad config
c) launch the editor on a tmp copy, check and refuse or loop on fail...

I still think of "git config -e" as a shortcut only (meaning it doesn't
warrant large specific code efforts), and it's problematic because the
user base is split in two sets:

- Those who know their way around .git/config and editors.
- Those who should stick with the get and set modes of "git config".

"git config -e" helps users from the 2nd group shoot themselves in their
feet badly enough that they can recover only with insight from the first
group...

Michael