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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Jun 13, 2017, 11:02 UTC
Message-ID: <alpine.DEB.2.21.1.1706131256400.171564@virtualbox>
In-Reply-To: <20170610090424.zibu4sgd2lr7ghfk@sigill.intra.peff.net>

Hi Peff,

On Sat, 10 Jun 2017, Jeff King wrote:

Show 34 quoted lines

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

Show 18 quoted lines

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