Junio C Hamano venit, vidit, dixit 04.09.2009 01:42: > Michael J Gruber <git@xxxxxxxxxxxxxxxxxxxx> 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html