Re: [PATCH v2 2/8] config: report correct line number upon error

[Jeff King <peff@peff.net>]

On Thu, Jun 08, 2017 at 09:53:35PM +0200, Johannes Schindelin wrote:

> When get_value() parses a key/value pair, it is possible that the line
> number is decreased (because the \n has been consumed already) before the
> key/value pair is passed to the callback function, to allow for the
> correct line to be attributed in case of an error.
> 
> However, when git_parse_source() asks get_value() to parse the key/value
> pair, the error reporting is performed *after* get_value() returns.
> 
> Which means that we have to be careful not to increase the line number
> in get_value() after the callback function returned an error.

Good catch. Would we want a test here, like:

diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 13b7851f7..4cad8366a 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -703,6 +703,12 @@ test_expect_success 'invalid unit' '
 	test_i18ngrep "bad numeric config value .1auto. for .aninvalid.unit. in file .git/config: invalid unit" actual
 '
 
+test_expect_success 'invalid path' '
+	echo "[bool]var" >invalid &&
+	test_must_fail git config -f invalid --path bool.var 2>actual &&
+	test_i18ngrep "line 1" actual
+'
+
 test_expect_success 'invalid stdin config' '
 	echo "[broken" | test_must_fail git config --list --file - >output 2>&1 &&
 	test_i18ngrep "bad config line 1 in standard input" output

which currently reports "line 2" instead of line 1.

I wondered if the same bug could be found earlier (e.g,. from
parse_value() when it sees an unpaired quote), but it looks like we do a
cf->linenr-- rollback there already.

> diff --git a/config.c b/config.c
> index 146cb3452ad..9b88531a70d 100644
> --- a/config.c
> +++ b/config.c
> @@ -604,7 +604,8 @@ static int get_value(config_fn_t fn, void *data, struct strbuf *name)
>  	 */
>  	cf->linenr--;
>  	ret = fn(name->buf, value, data);
> -	cf->linenr++;
> +	if (!ret)
> +		cf->linenr++;
>  	return ret;
>  }

I think this should be "if (ret < 0)". The caller only considers it an
error if get_value() returns a negative number. As you have it here, I
think a config callback which returned a positive number would end up
with nonsense line numbers.

-Peff