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

Michael J Gruber <git@drmicha.warpmail.net> writes:

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.