Re: [PATCH v2 2/2] checkout: proper error message on 'git checkout foo bar --'

From: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Date: Sep 26, 2013, 08:59 UTC
Message-ID: <vpqk3i4klgh.fsf@anie.imag.fr>
In-Reply-To: <20130925224306.GC9464@google.com>

Jonathan Nieder <jrnieder@gmail.com> writes:

>  (a) rename has_dash_dash here to dash_dash_pos, or
>  (b) put the check in the loop, like so:

I agree with (a), but not with (b). I think separating the computation of the position and the diagnosis makes it clearer.

I reworked the code a bit, the diagnosis part now looks like

	if (dash_dash_pos == 0)
		return 1; /* case (2) */
	else if (dash_dash_pos == 1)
		has_dash_dash = 1; /* case (3) or (1) */
	else if (dash_dash_pos >= 2)
		die(_("only one reference expected, %d given."), dash_dash_pos);

Show 20 quoted lines

>> +test_expect_success C_LOCALE_OUTPUT 'accurate error message with more than one ref' '
>> +	test_must_fail git checkout HEAD master -- 2>actual &&
>> +	echo "fatal: only one reference expected, 2 given." >expect &&
>> +	test_cmp expect actual
>
> Nits:
>
>  - if we change this from 'fatal' to 'error' or reword the message as
>    part of a libification some day, it would be a nuisance to have to
>    update this test.  Maybe a 'grep' could make it more flexible.
>
>  - most of the test (though not the interesting part) can run in the
>    !C_LOCALE_OUTPUT case if you use test_i18ncmp or test_i18ngrep
>
>  - even the check for "2" can run in the !C_LOCALE_OUTPUT case. :)
>    e.g. something like
>
> 	test_must_fail ... 2>actual &&
> 	grep 2 actual &&
> 	test_i18ngrep "one reference expected, 2 given" actual

OK, I buy your version. Thanks,

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/