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

> 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.

> 
> 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.