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