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

Junio C Hamano venit, vidit, dixit 04.09.2009 01:42:

Michael J Gruber [off-list ref] writes:

quoted

quoted

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