git config -> "fatal: bad config file"

git config -> "fatal: bad config file"

[David Reitter]

I made a mistake editing my config file using "git config -e".  This  
caused git commands to fail.

Trying to fix the problem, I did

git config -e
fatal: bad config file line 7 in .git/config

I think the refusal to edit a broken config file is not a good idea.   
It's easy to fix for me of course by editing .git/config directly, but  
git-config should probably not read the config file at all.

Thanks for your consideration.

Re: git config -> "fatal: bad config file"

[Michael J Gruber]

David Reitter venit, vidit, dixit 14.08.2009 15:38:
> I made a mistake editing my config file using "git config -e".  This  
> caused git commands to fail.
> 
> Trying to fix the problem, I did
> 
> git config -e
> fatal: bad config file line 7 in .git/config
> 
> I think the refusal to edit a broken config file is not a good idea.   
> It's easy to fix for me of course by editing .git/config directly, but  
> git-config should probably not read the config file at all.
> 
> Thanks for your consideration.
> 

git needs to read the file because the editor could be configured there!
The only option would be to make git config -e continue past that error.

Michael

Re: git config -> "fatal: bad config file"

[David Reitter]

On Aug 14, 2009, at 10:18 AM, Michael J Gruber wrote:

> git needs to read the file because the editor could be configured  
> there!
> The only option would be to make git config -e continue past that  
> error.


Syntax errors in .git/config could lead to warnings. Since the file is  
primarily line-oriented anyways (except for groups), recovery should  
be easy.

Also, you could have git-config -e edit a temporary file, check the  
file for errors after editing and then move it to .git/config.

Re: git config -> "fatal: bad config file"

[Jakub Narebski]

Michael J Gruber <git@drmicha.warpmail.net> writes:
> David Reitter venit, vidit, dixit 14.08.2009 15:38:

> > I made a mistake editing my config file using "git config -e".  This  
> > caused git commands to fail.
> > 
> > Trying to fix the problem, I did
> > 
> > git config -e
> > fatal: bad config file line 7 in .git/config
> > 
> > I think the refusal to edit a broken config file is not a good idea.   
> > It's easy to fix for me of course by editing .git/config directly, but  
> > git-config should probably not read the config file at all.
> > 
> > Thanks for your consideration.
> > 
> 
> git needs to read the file because the editor could be configured there!
> The only option would be to make git config -e continue past that error.

Well, it shows you which file the error is, but I think as a special
case "git config [file] --edit" should show also (absolute?) pathname
of the file it tired to open.

core.editor can be in any place.

-- 
Jakub Narebski
Poland
ShadeHawk on #git

[PATCH] git-config: Parse config files leniently

[Michael J Gruber]

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.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
Reported-by: David Reitter <david.reitter@gmail.com>
---
The diff and stat are overstatements. I had to shift that whole for loop
by one tab stop but diff (with or without --color-words) does not recognize
this. Nothing inside the loop is changed.
The outer while makes sure that
* we switch to comment mode after a parsing error
* we return eventually no matter whether the file ends with \n or not.

Test had to be adjusted as well.

 config.c                |   80 ++++++++++++++++++++++++----------------------
 t/t1303-wacky-config.sh |    4 +-
 2 files changed, 44 insertions(+), 40 deletions(-)

diff --git a/config.c b/config.c
index e87edea..5e0af5d 100644
--- a/config.c
+++ b/config.c
@@ -207,50 +207,54 @@ static int git_parse_file(config_fn_t fn, void *data)
 	static const unsigned char *utf8_bom = (unsigned char *) "\xef\xbb\xbf";
 	const unsigned char *bomptr = utf8_bom;
 
-	for (;;) {
-		int c = get_next_char();
-		if (bomptr && *bomptr) {
-			/* We are at the file beginning; skip UTF8-encoded BOM
-			 * if present. Sane editors won't put this in on their
-			 * own, but e.g. Windows Notepad will do it happily. */
-			if ((unsigned char) c == *bomptr) {
-				bomptr++;
+	while (!config_file_eof) {
+		for (;;) {
+			int c = get_next_char();
+			if (bomptr && *bomptr) {
+				/* We are at the file beginning; skip UTF8-encoded BOM
+				 * if present. Sane editors won't put this in on their
+				 * own, but e.g. Windows Notepad will do it happily. */
+				if ((unsigned char) c == *bomptr) {
+					bomptr++;
+					continue;
+				} else {
+					/* Do not tolerate partial BOM. */
+					if (bomptr != utf8_bom)
+						break;
+					/* No BOM at file beginning. Cool. */
+					bomptr = NULL;
+				}
+			}
+			if (c == '\n') {
+				if (config_file_eof)
+					return 0;
+				comment = 0;
 				continue;
-			} else {
-				/* Do not tolerate partial BOM. */
-				if (bomptr != utf8_bom)
+			}
+			if (comment || isspace(c))
+				continue;
+			if (c == '#' || c == ';') {
+				comment = 1;
+				continue;
+			}
+			if (c == '[') {
+				baselen = get_base_var(var);
+				if (baselen <= 0)
 					break;
-				/* No BOM at file beginning. Cool. */
-				bomptr = NULL;
+				var[baselen++] = '.';
+				var[baselen] = 0;
+				continue;
 			}
-		}
-		if (c == '\n') {
-			if (config_file_eof)
-				return 0;
-			comment = 0;
-			continue;
-		}
-		if (comment || isspace(c))
-			continue;
-		if (c == '#' || c == ';') {
-			comment = 1;
-			continue;
-		}
-		if (c == '[') {
-			baselen = get_base_var(var);
-			if (baselen <= 0)
+			if (!isalpha(c))
+				break;
+			var[baselen] = tolower(c);
+			if (get_value(fn, data, var, baselen+1) < 0)
 				break;
-			var[baselen++] = '.';
-			var[baselen] = 0;
-			continue;
 		}
-		if (!isalpha(c))
-			break;
-		var[baselen] = tolower(c);
-		if (get_value(fn, data, var, baselen+1) < 0)
-			break;
+		warning("bad config file line %d in %s", config_linenr, config_file_name);
+		comment = 1;
 	}
-	die("bad config file line %d in %s", config_linenr, config_file_name);
+	return -1;
 }
 
 static int parse_unit_factor(const char *end, unsigned long *val)
diff --git a/t/t1303-wacky-config.sh b/t/t1303-wacky-config.sh
index 080117c..be850c5 100755
--- a/t/t1303-wacky-config.sh
+++ b/t/t1303-wacky-config.sh
@@ -9,7 +9,7 @@ setup() {
 }
 
 check() {
-	echo "$2" >expected
+	printf "$2\n" >expected
 	git config --get "$1" >actual 2>&1
 	test_cmp actual expected
 }
@@ -44,7 +44,7 @@ LONG_VALUE=$(printf "x%01021dx a" 7)
 test_expect_success 'do not crash on special long config line' '
 	setup &&
 	git config section.key "$LONG_VALUE" &&
-	check section.key "fatal: bad config file line 2 in .git/config"
+	check section.key "warning: bad config file line 2 in .git/config\nwarning: bad config file line 2 in .git/config"
 '
 
 test_done
-- 
1.6.4.225.gb589e

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

[Junio C Hamano]

Michael J Gruber <git@drmicha.warpmail.net> 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.

... a broken sentence in the middle?  I would have expected the "not only"
followed by "but also"; the question is "but also what?"

Hopefully the benefit is not that it now allows all the other commands to
cause unspecified types of damage to the repository by following iffy
settings obtained from a broken configuration file.

> Reported-by: David Reitter <david.reitter@gmail.com>
> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>

> Test had to be adjusted as well.

The change to the test demonstrates the issue rather well.  The check()
shell function does not check the exit value from "git config --get", but
in a real script that cares to check and stop on error, this change will
now let the script go on, leaving the breakage unnoticed.  I suspect
command implemented in C, that call git_config(), will also have the same
issue, and I cannot convince myself this is a good change in general,
outside the scope of helping "git config -e".

But I may be being overly cautious.

By the way, why did you have to change s/echo/printf/?  Can't you give two
lines in a single argument without "\n" escape?

> diff --git a/t/t1303-wacky-config.sh b/t/t1303-wacky-config.sh
> index 080117c..be850c5 100755
> --- a/t/t1303-wacky-config.sh
> +++ b/t/t1303-wacky-config.sh
> @@ -9,7 +9,7 @@ setup() {
>  }
>  
>  check() {
> -	echo "$2" >expected
> +	printf "$2\n" >expected
>  	git config --get "$1" >actual 2>&1
>  	test_cmp actual expected
>  }
> @@ -44,7 +44,7 @@ LONG_VALUE=$(printf "x%01021dx a" 7)
>  test_expect_success 'do not crash on special long config line' '
>  	setup &&
>  	git config section.key "$LONG_VALUE" &&
> -	check section.key "fatal: bad config file line 2 in .git/config"
> +	check section.key "warning: bad config file line 2 in .git/config\nwarning: bad config file line 2 in .git/config"
>  '
>  
>  test_done

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

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 14.08.2009 21:52:
> Michael J Gruber <git@drmicha.warpmail.net> 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.
> 
> ... a broken sentence in the middle?  I would have expected the "not only"
> followed by "but also"; the question is "but also what?"

I don't see any broken sentence here. "Benefit" is a verb as well as a noun.

"not only git config -e users" but also everyone else: Unparseable lines
are ignored, the rest is parsed.

> 
> Hopefully the benefit is not that it now allows all the other commands to
> cause unspecified types of damage to the repository by following iffy
> settings obtained from a broken configuration file.

The only possible problem that I see is when a section heading is not
parsed because of a forgotten "[", say, and the following settings are
put in the wrong section because of that.

>> Reported-by: David Reitter <david.reitter@gmail.com>
>> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
> 
>> Test had to be adjusted as well.
> 
> The change to the test demonstrates the issue rather well.  The check()
> shell function does not check the exit value from "git config --get", but
> in a real script that cares to check and stop on error, this change will
> now let the script go on, leaving the breakage unnoticed.  I suspect
> command implemented in C, that call git_config(), will also have the same
> issue, and I cannot convince myself this is a good change in general,
> outside the scope of helping "git config -e".
> 
> But I may be being overly cautious.

My first version had the lenient mode for "git config -e" only, which
required a new global int (or, alternatively, changing all callers).

One could issue a warning and return -1 rather than success. I'm afraid
git_config() callers don't check return values but rely on git_config()
dieing in case there are parsing problems.

> 
> By the way, why did you have to change s/echo/printf/?  Can't you give two
> lines in a single argument without "\n" escape?

Because "printf" is more portable then "echo -e". At least I hope so ;)
[ One could use a "here document", of course. Is that preferable? ]

Michael

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

[Junio C Hamano]

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


> Junio C Hamano venit, vidit, dixit 14.08.2009 21:52:
>> Michael J Gruber <git@drmicha.warpmail.net> writes:
>> ...
>> But I may be being overly cautious.
>
> My first version had the lenient mode for "git config -e" only, which
> required a new global int (or, alternatively, changing all callers).

Without looking at the actual patch, that "single global that is only used
by builtin_config() when using -e" sounds safer.

But still I think I may be being overly cautious.

>> By the way, why did you have to change s/echo/printf/?  Can't you give two
>> lines in a single argument without "\n" escape?
>
> Because "printf" is more portable then "echo -e". At least I hope so ;)
> [ One could use a "here document", of course. Is that preferable? ]

What I meant was to give literally two lines, like this:

    check section.key 'warning: bad config file line 2 in .git/config
warning: bad config file line 2 in .git/config'

I do not see a need for a here-doc.

The rest is tangent you can ignore.

>>> 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.
>> 
>> ... a broken sentence in the middle?  I would have expected the "not only"
>> followed by "but also"; the question is "but also what?"
>
> I don't see any broken sentence here. "Benefit" is a verb as well as a noun.

Yes, and I think you read me correctly.

s/broken/chopped in the middle, missing 'but also X'/;

and you just explained that you meant "everyone else" by that X in the
missing part of the sentence.

[PATCHv2] git-config: Parse config files leniently

[Michael J Gruber]

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.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
Reported-by: David Reitter <david.reitter@gmail.com>
---
 config.c                |   80 ++++++++++++++++++++++++----------------------
 t/t1303-wacky-config.sh |    3 +-
 2 files changed, 44 insertions(+), 39 deletions(-)

So, after a business trip, vacation, ... I'm finally returning to this patch.
I addressed the echo/printf issue as suggested and clarified the commit message.

Regarding the global int for switching on/off lenient parsing: I reinstated my
v0 of the patch only to find out (again) that setup_git_directory_gently is the
problem here. We would have to turn on lenient parsing before we even know that
"-e" is supplied. So, the only option would be to have "git config" use lenient
parsing in all modes (edit/get/set) and have other git commands die fatally on
erroneous config.

So, here's v2 which has lenient config parsing for everyone because I don't see
a way to have it for "git config -e" only. If you prefer to have it for all of
"git config" only, that version is in another branch head now...

Cheers,
Michael

diff --git a/config.c b/config.c
index e87edea..5e0af5d 100644
--- a/config.c
+++ b/config.c
@@ -207,50 +207,54 @@ static int git_parse_file(config_fn_t fn, void *data)
 	static const unsigned char *utf8_bom = (unsigned char *) "\xef\xbb\xbf";
 	const unsigned char *bomptr = utf8_bom;
 
-	for (;;) {
-		int c = get_next_char();
-		if (bomptr && *bomptr) {
-			/* We are at the file beginning; skip UTF8-encoded BOM
-			 * if present. Sane editors won't put this in on their
-			 * own, but e.g. Windows Notepad will do it happily. */
-			if ((unsigned char) c == *bomptr) {
-				bomptr++;
+	while (!config_file_eof) {
+		for (;;) {
+			int c = get_next_char();
+			if (bomptr && *bomptr) {
+				/* We are at the file beginning; skip UTF8-encoded BOM
+				 * if present. Sane editors won't put this in on their
+				 * own, but e.g. Windows Notepad will do it happily. */
+				if ((unsigned char) c == *bomptr) {
+					bomptr++;
+					continue;
+				} else {
+					/* Do not tolerate partial BOM. */
+					if (bomptr != utf8_bom)
+						break;
+					/* No BOM at file beginning. Cool. */
+					bomptr = NULL;
+				}
+			}
+			if (c == '\n') {
+				if (config_file_eof)
+					return 0;
+				comment = 0;
 				continue;
-			} else {
-				/* Do not tolerate partial BOM. */
-				if (bomptr != utf8_bom)
+			}
+			if (comment || isspace(c))
+				continue;
+			if (c == '#' || c == ';') {
+				comment = 1;
+				continue;
+			}
+			if (c == '[') {
+				baselen = get_base_var(var);
+				if (baselen <= 0)
 					break;
-				/* No BOM at file beginning. Cool. */
-				bomptr = NULL;
+				var[baselen++] = '.';
+				var[baselen] = 0;
+				continue;
 			}
-		}
-		if (c == '\n') {
-			if (config_file_eof)
-				return 0;
-			comment = 0;
-			continue;
-		}
-		if (comment || isspace(c))
-			continue;
-		if (c == '#' || c == ';') {
-			comment = 1;
-			continue;
-		}
-		if (c == '[') {
-			baselen = get_base_var(var);
-			if (baselen <= 0)
+			if (!isalpha(c))
+				break;
+			var[baselen] = tolower(c);
+			if (get_value(fn, data, var, baselen+1) < 0)
 				break;
-			var[baselen++] = '.';
-			var[baselen] = 0;
-			continue;
 		}
-		if (!isalpha(c))
-			break;
-		var[baselen] = tolower(c);
-		if (get_value(fn, data, var, baselen+1) < 0)
-			break;
+		warning("bad config file line %d in %s", config_linenr, config_file_name);
+		comment = 1;
 	}
-	die("bad config file line %d in %s", config_linenr, config_file_name);
+	return -1;
 }
 
 static int parse_unit_factor(const char *end, unsigned long *val)
diff --git a/t/t1303-wacky-config.sh b/t/t1303-wacky-config.sh
index 080117c..0599d9f 100755
--- a/t/t1303-wacky-config.sh
+++ b/t/t1303-wacky-config.sh
@@ -44,7 +44,8 @@ LONG_VALUE=$(printf "x%01021dx a" 7)
 test_expect_success 'do not crash on special long config line' '
 	setup &&
 	git config section.key "$LONG_VALUE" &&
-	check section.key "fatal: bad config file line 2 in .git/config"
+	check section.key "warning: bad config file line 2 in .git/config
+warning: bad config file line 2 in .git/config"
 '
 
 test_done
-- 
1.6.4.2.395.ge3d52

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

[Junio C Hamano]

Michael J Gruber <git@drmicha.warpmail.net> 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?

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

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 03.09.2009 09:00:
> Michael J Gruber <git@drmicha.warpmail.net> 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?

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

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

[Junio C Hamano]

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.

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

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 04.09.2009 01:42:
> 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.

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

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

[Junio C Hamano]

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

>> *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?