Re: [PATCH v2 5/8] t1308: relax the test verifying that empty alias values are disallowed

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

Hi Junio,

On Sat, 10 Jun 2017, Junio C Hamano wrote:

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > We are about to change the way aliases are expanded, to use the early
> > config machinery.
> >
> > This machinery reports errors in a slightly different manner than the
> > cached config machinery.
> >
> > Let's not get hung up by the precise wording of the message mentioning
> > the lin number. It is really sufficient to verify that all the relevant
> > information is given to the user.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  t/t1308-config-set.sh | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh
> > index ff50960ccae..69a0aa56d6d 100755
> > --- a/t/t1308-config-set.sh
> > +++ b/t/t1308-config-set.sh
> > @@ -215,7 +215,9 @@ test_expect_success 'check line errors for malformed values' '
> >  		br
> >  	EOF
> >  	test_expect_code 128 git br 2>result &&
> > -	test_i18ngrep "fatal: .*alias\.br.*\.git/config.*line 2" result
> > +	test_i18ngrep "missing value for .alias\.br" result &&
> > +	test_i18ngrep "fatal: .*\.git/config" result &&
> > +	test_i18ngrep "fatal: .*line 2" result
> 
> Just judging from the looks of these, it appears that the updated
> messages are more descriptive than the original.  Am I mistaken?

Sadly, I do not think so. It is just different, not better. Maybe less
redundant... See for yourself:

Before:

	error: missing value for 'alias.br'
	fatal: bad config variable 'alias.br' in file '.git/config' at line 2

After:

	error: missing value for 'alias.br'
	fatal: bad config line 2 in file .git/config

> If so I think the log message undersells the changes made by the
> series by sounding as if you are saying "we are too lazy to bother
> giving the same message, so here is a workaround".

I fear this is not underselling anything for an improvement.

The real fix would indeed be (as mentioned by Brandon elsewhere) to unify
the code paths between the cached and the non-cached config machinery, so
as to provide the exact same error message in this case.

It is annoying that I lack the time to make that happen. My official
excuse, though, is that that fix is outside the purpose of this patch
series.

> In any case, s/lin number/line number/ is needed ;-)

Tru. ;-)

Cia,
Dsch