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

[Jeff King <peff@peff.net>]

On Tue, Jun 13, 2017 at 01:02:49PM +0200, Johannes Schindelin wrote:

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

If there's another test that covers this, I'm happy to use that. I just
didn't notice it.

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

Oops, right.

-Peff