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

Michael J Gruber [off-list ref] 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 but
also everyone else.

This changes the behaviour enough to break t3200-branch.sh, test #52.

The test stuffs an invalid (but not syntactically incorrect) value used by
"git branch" in the configuration and tries to make sure that "git branch"
diagnoses the breakage, but it does not fail anymore with your patch.

There are probably other breakages as well (e.g. t5304-prune.sh, test #5)
but if you trace "git branch" under the debugger in the trash directory
left after running t3200 with -i, it should be pretty obvious that your
patch is utterly bogus.  get_value() can return negative result after
diagnosing a semantic problem with the value, and that is different from a
syntax error that you would try to recover and continue, pretending you
can ignore the remainder of the line as if it is a comment.

Why was I CC'ed, if the patch wasn't even self tested?

Junio C Hamano venit, vidit, dixit 03.09.2009 09:00:

Michael J Gruber [off-list ref] writes:

quoted

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 but
also everyone else.

This changes the behaviour enough to break t3200-branch.sh, test #52.

The test stuffs an invalid (but not syntactically incorrect) value used by
"git branch" in the configuration and tries to make sure that "git branch"
diagnoses the breakage, but it does not fail anymore with your patch.

There are probably other breakages as well (e.g. t5304-prune.sh, test #5)
but if you trace "git branch" under the debugger in the trash directory
left after running t3200 with -i, it should be pretty obvious that your
patch is utterly bogus.  get_value() can return negative result after
diagnosing a semantic problem with the value, and that is different from a
syntax error that you would try to recover and continue, pretending you
can ignore the remainder of the line as if it is a comment.

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.

Since "git config -e" for broken config is not my itch at all, but the
reporter's, I'll stop my efforts after this response.

Michael

Michael J Gruber [off-list ref] writes:

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.

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.

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

Michael J Gruber [off-list ref] writes:

quoted

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

We need to distinguish at least the "syntax" and "semantics" errors, i.e.
not necessarily "do it right", but "how your patch should have looked
like".

I think a slightly hacky but practical workaround then becomes possible.
We can have a config callback function to parse only core.editor (and
perhaps some other very minimum set of variables needed to launch the
editor on the right config file) extremely loosely, i.e. not even calling
git_config_string() to complain about error-nonbool.  You can use the
callback _only_ from the ACTION_EDIT codepath in builtin-config.c (which
currently uses git_default_config and errors out when some uninteresting
configuration variables have semantic errors).

That would get rid of the issue that the configuration mechanism triggers
semantic errors for unrelated variables, and would give us a usable
recovery editor, no?