threads / patch / 29901

patchfix incorrect line number report for bad gitconfig

Subject: [PATCH] fix incorrect line number report for bad gitconfig

## tl;dr

4 messages between Mar 9, 2012 and Mar 12, 2012. Diffs are folded; open one to read it.

replies: 3people: 2as markdown or json

Martin Stenberg· Mar 9, 2012, 22:30 UTC · lore
From c88f00e06cb877f9d944fdec480f53a7a42dd990 Mon Sep 17 00:00:00 2001
From: Martin Stenberg <martin@gnutiken.se>
Date: Fri, 9 Mar 2012 22:57:54 +0100
Subject: [PATCH] fix incorrect line number report for bad gitconfig

A .gitconfig section with a missing "]" reports the next line as beeing bad, same goes to a value with a missing end quote.

This happens because the error is not detected until the end of the line, when
line number is already increased. Fix this issue by decreasing line number by
one for these cases.
Signed-off-by: Martin Stenberg <martin@gnutiken.se>
---
 config.c |   16 ++++++++++++----
 1 files changed, 12 insertions(+), 4 deletions(-)
Show changes to config.c +12 −4
diff --git a/config.c b/config.c
index ad03908..8d96ba1 100644
--- a/config.c
+++ b/config.c
@@ -196,8 +196,10 @@ static char *parse_value(void)
 	for (;;) {
 		int c = get_next_char();
 		if (c == '\n') {
-			if (quote)
+			if (quote) {
+				cf->linenr--;
 				return NULL;
+			}
 			return cf->value.buf;
 		}
 		if (comment)
@@ -286,8 +288,10 @@ static int get_value(config_fn_t fn, void *data, char *name, unsigned int len)
 static int get_extended_base_var(char *name, int baselen, int c)
 {
 	do {
-		if (c == '\n')
+		if (c == '\n') {
+			cf->linenr--;
 			return -1;
+		}
 		c = get_next_char();
 	} while (isspace(c));
 
@@ -298,14 +302,18 @@ static int get_extended_base_var(char *name, int baselen, int c)
 
 	for (;;) {
 		int c = get_next_char();
-		if (c == '\n')
+		if (c == '\n') {
+			cf->linenr--;
 			return -1;
+		}
 		if (c == '"')
 			break;
 		if (c == '\\') {
 			c = get_next_char();
-			if (c == '\n')
+			if (c == '\n') {
+				cf->linenr--;
 				return -1;
+			}
 		}
 		name[baselen++] = c;
 		if (baselen > MAXNAME / 2)

-- 
1.7.9.1
Junio C Hamano· Mar 9, 2012, 22:58 UTC · re: Martin Stenberg · lore

Re: [PATCH] fix incorrect line number report for bad gitconfig

Martin Stenberg <martin@gnutiken.se> writes:
> From c88f00e06cb877f9d944fdec480f53a7a42dd990 Mon Sep 17 00:00:00 2001
> From: Martin Stenberg <martin@gnutiken.se>
> Date: Fri, 9 Mar 2012 22:57:54 +0100
> Subject: [PATCH] fix incorrect line number report for bad gitconfig

Please drop these four lines. The commit object name is no use to me, and the other lines duplicate what you have in the mail header.

> A .gitconfig section with a missing "]" reports the next line as beeing bad,
> same goes to a value with a missing end quote.

If you say ".gitconfig", it gives a false impression to the readers that this is only limited to parsing of $HOME/.gitconfig, which is not the problem you are addressing. "A section in a config file" would be a better phrasing.

s/beeing/being/;
> This happens because the error is not detected until the end of the line, when
> line number is already increased. Fix this issue by decreasing line number by
> one for these cases.
> Signed-off-by: Martin Stenberg <martin@gnutiken.se>
Please have a paragraph break before your S-o-b: line.
Thanks. Review of the patch text follows.
Show 29 quoted lines
> ---
>  config.c |   16 ++++++++++++----
>  1 files changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/config.c b/config.c
> index ad03908..8d96ba1 100644
> --- a/config.c
> +++ b/config.c
> @@ -196,8 +196,10 @@ static char *parse_value(void)
>  	for (;;) {
>  		int c = get_next_char();
>  		if (c == '\n') {
> -			if (quote)
> +			if (quote) {
> +				cf->linenr--;
>  				return NULL;
> +			}
>  			return cf->value.buf;
>  		}
>  		if (comment)
> @@ -286,8 +288,10 @@ static int get_value(config_fn_t fn, void *data, char *name, unsigned int len)
>  static int get_extended_base_var(char *name, int baselen, int c)
>  {
>  	do {
> -		if (c == '\n')
> +		if (c == '\n') {
> +			cf->linenr--;
> ...
>  		}

We might want to consolidate the error-return codepath in this function to a single place by judicious use of "goto", perhaps like this:

 config.c |    9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)
Show changes to config.c +6 −3
diff --git a/config.c b/config.c
index ad03908..e222e85 100644
--- a/config.c
+++ b/config.c
@@ -287,7 +287,7 @@ static int get_extended_base_var(char *name, int baselen, int c)
 {
 	do {
 		if (c == '\n')
-			return -1;
+			goto error_incomplete_line;
 		c = get_next_char();
 	} while (isspace(c));
 
@@ -299,13 +299,13 @@ static int get_extended_base_var(char *name, int baselen, int c)
 	for (;;) {
 		int c = get_next_char();
 		if (c == '\n')
-			return -1;
+			goto error_incomplete_line;
 		if (c == '"')
 			break;
 		if (c == '\\') {
 			c = get_next_char();
 			if (c == '\n')
-				return -1;
+				goto error_incomplete_line;
 		}
 		name[baselen++] = c;
 		if (baselen > MAXNAME / 2)
@@ -316,6 +316,9 @@ static int get_extended_base_var(char *name, int baselen, int c)
 	if (get_next_char() != ']')
 		return -1;
 	return baselen;
+error_incomplete_line:
+	cf->linenr--;
+	return -1;
 }
 
 static int get_base_var(char *name)
Junio C Hamano· Mar 12, 2012, 05:55 UTC · re: Junio C Hamano · lore

Re: [PATCH] fix incorrect line number report for bad gitconfig

Junio C Hamano <gitster@pobox.com> writes:
Show 14 quoted lines
> Martin Stenberg <martin@gnutiken.se> writes:
>
>> From c88f00e06cb877f9d944fdec480f53a7a42dd990 Mon Sep 17 00:00:00 2001
>> From: Martin Stenberg <martin@gnutiken.se>
>> Date: Fri, 9 Mar 2012 22:57:54 +0100
>> Subject: [PATCH] fix incorrect line number report for bad gitconfig
>
> Please drop these four lines. The commit object name is no use to
> me, and the other lines duplicate what you have in the mail header.
> ...
> Please have a paragraph break before your S-o-b: line.
>
> Thanks. Review of the patch text follows.
> ...

Also could you add a couple of tests (you identified two cases in your log message) to make sure this fix will not be broken in the future, perhaps to t1300?

Thanks.
Martin Stenberg· Mar 12, 2012, 12:01 UTC · re: Junio C Hamano · lore

Re: [PATCH] fix incorrect line number report for bad gitconfig

On Sun, Mar 11, 2012 at 10:55:34PM -0700, Junio C Hamano wrote:
Show 22 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Martin Stenberg <martin@gnutiken.se> writes:
> >
> >> From c88f00e06cb877f9d944fdec480f53a7a42dd990 Mon Sep 17 00:00:00 2001
> >> From: Martin Stenberg <martin@gnutiken.se>
> >> Date: Fri, 9 Mar 2012 22:57:54 +0100
> >> Subject: [PATCH] fix incorrect line number report for bad gitconfig
> >
> > Please drop these four lines. The commit object name is no use to
> > me, and the other lines duplicate what you have in the mail header.
> > ...
> > Please have a paragraph break before your S-o-b: line.
> >
> > Thanks. Review of the patch text follows.
> > ...
> 
> Also could you add a couple of tests (you identified two cases in
> your log message) to make sure this fix will not be broken in the
> future, perhaps to t1300?
> 
> Thanks.
Thanks for the feedback. Fixed the code and added two test cases.
New commit description and patch follows.
fix incorrect line number report for bad config

A section in a config file with a missing "]" reports the next line as being bad, same goes to a value with a missing end quote.

This happens because the error is not detected until the end of the line, when line number is already increased. Fix this issue by decreasing line number by one for these cases.

Signed-off-by: Martin Stenberg <martin@gnutiken.se>
---
 config.c               |   14 ++++++++++----
 t/t1300-repo-config.sh |   36 ++++++++++++++++++++++++++++++++++++
 2 files changed, 46 insertions(+), 4 deletions(-)
Show changes to 2 files +46 −4

config.c, t/t1300-repo-config.sh

diff --git a/config.c b/config.c
index ad03908..0019b1c 100644
--- a/config.c
+++ b/config.c
@@ -197,7 +197,7 @@ static char *parse_value(void)
 		int c = get_next_char();
 		if (c == '\n') {
 			if (quote)
-				return NULL;
+				goto error_incomplete_line;
 			return cf->value.buf;
 		}
 		if (comment)
@@ -245,6 +245,9 @@ static char *parse_value(void)
 		}
 		strbuf_addch(&cf->value, c);
 	}
+error_incomplete_line:
+	cf->linenr--;
+	return NULL;
 }
 
 static inline int iskeychar(int c)
@@ -287,7 +290,7 @@ static int get_extended_base_var(char *name, int baselen, int c)
 {
 	do {
 		if (c == '\n')
-			return -1;
+			goto error_incomplete_line;
 		c = get_next_char();
 	} while (isspace(c));
 
@@ -299,13 +302,13 @@ static int get_extended_base_var(char *name, int baselen, int c)
 	for (;;) {
 		int c = get_next_char();
 		if (c == '\n')
-			return -1;
+			goto error_incomplete_line;
 		if (c == '"')
 			break;
 		if (c == '\\') {
 			c = get_next_char();
 			if (c == '\n')
-				return -1;
+				goto error_incomplete_line;
 		}
 		name[baselen++] = c;
 		if (baselen > MAXNAME / 2)
@@ -316,6 +319,9 @@ static int get_extended_base_var(char *name, int baselen, int c)
 	if (get_next_char() != ']')
 		return -1;
 	return baselen;
+error_incomplete_line:
+	cf->linenr--;
+	return -1;
 }
 
 static int get_base_var(char *name)
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 5f249f6..dac3008 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -985,4 +985,40 @@ test_expect_success 'git config --edit respects core.editor' '
 	test_cmp expect actual
 '
 
+cat > expect <<EOF
+fatal: bad config file line 1 in .git/config
+EOF
+
+cat > .git/config <<\EOF
+[section
+key = value
+EOF
+
+test_expect_success 'incomplete section line' '
+	if git config --get section.key 2>actual
+	then
+		echo config should have failed
+		false
+	fi &&
+	cmp actual expect
+'
+
+cat > expect <<EOF
+fatal: bad config file line 2 in .git/config
+EOF
+
+cat > .git/config <<\EOF
+[section]
+    key = "value
+EOF
+
+test_expect_success 'incomplete quoted line' '
+	if git config --get section.key 2>actual
+	then
+		echo config should have failed
+		false
+	fi &&
+	cmp actual expect
+'
+
 test_done

-- 
1.7.9.1

← back to recent threads