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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Peff,

On Sat, 10 Jun 2017, Jeff King wrote:

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

Mmmmkay.

I am always reluctant to add *even more* stuff to the test suite, in
particular since my patch series implicitly changes t1308 to test for this
very thing.

But I guess my fight against the test suite taking longer and longer and
longer is a fight I must lose, as I am pretty alone in that endeavor, and
would have have to fight fellow developers. So I guess it is time to give
up.

I added the test case you suggested, with a title that I find a bit more
descriptive.

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

Right.

The underlying bug, of course, is that we use a different machinery to
handle cached vs non-cached config, but that is a different story.

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

I think you are half-correct: it should be `if (ret >= 0)` (the linenr
needs to be modified back in case of success, not in case of failure, in
case of failure there will be some reporting going on that needs the same
line number as `fn()` had seen).

Ciao,
Dscho