public inbox for git@vger.kernel.org 
 help / color / mirror / Atom feed

From: Matthieu Moy <Matthieu.Moy@grenoble-inp•fr>
To: Jonathan Nieder <jrnieder@gmail•com>
Cc: git@vger•kernel.org, gitster@pobox•com, pclouds@gmail•com,
	jc@sahnwaldt•de
Subject: Re: [PATCH v2 2/2] checkout: proper error message on 'git checkout foo bar --'
Date: Thu, 26 Sep 2013 10:59:10 +0200	[thread overview]
Message-ID: <vpqk3i4klgh.fsf@anie.imag.fr> (raw)
In-Reply-To: <20130925224306.GC9464@google.com> (Jonathan Nieder's message of "Wed, 25 Sep 2013 15:43:06 -0700")

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

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

next prev parent reply	other threads:[~2013-09-26  8:59 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-09-25 19:31 [PATCH v2 1/2] checkout: allow dwim for branch creation for "git checkout $branch --" Matthieu Moy
2013-09-25 19:31 ` [PATCH v2 2/2] checkout: proper error message on 'git checkout foo bar --' Matthieu Moy
2013-09-25 22:43   ` Jonathan Nieder
2013-09-26  8:59     ` Matthieu Moy [this message]
2013-09-25 22:33 ` [PATCH v2 1/2] checkout: allow dwim for branch creation for "git checkout $branch --" Jonathan Nieder
2013-09-26  8:03   ` Matthieu Moy
2013-09-26  9:03   ` Matthieu Moy

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=vpqk3i4klgh.fsf@anie.imag.fr \
    --to=matthieu.moy@grenoble-inp$(echo .)fr \
    --cc=git@vger$(echo .)kernel.org \
    --cc=gitster@pobox$(echo .)com \
    --cc=jc@sahnwaldt$(echo .)de \
    --cc=jrnieder@gmail$(echo .)com \
    --cc=pclouds@gmail$(echo .)com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox