Re: [PATCH/RFC] setup: update error message to be more meaningful

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Jul 25, 2017, 21:21 UTC
Message-ID: <20170725212110.GG13924@aiede.mtv.corp.google.com>
In-Reply-To: <0102015d7ae53b0a-a6505296-9257-4b0d-84d6-2152e17eb070-000000@eu-west-1.amazonses.com>

Hi,

Kaartic Sivaraam wrote:

Show 17 quoted lines

> The error message shown when a flag is found when expecting a
> filename wasn't clear as it didn't communicate what was wrong
> using the 'suitable' words in *all* cases.
>
> Correct case,
>
>         $ git rev-parse commit.c --flags
>         commit.c
>         --flags
>         fatal: bad flag '--flags' used after filename
>
> Incorrect case,
>
>         $ git grep "test" -n
>         fatal: bad flag '-n' used after filename
>
> Change the error message to be general and communicative.

Thanks for writing this. These examples describe *what* the behavior is but don't describe *why* it is wrong or what is expected in its place.

For an initial guess: in the example

	git grep test -n

it is confusing that it says "bad flag used after filename" because test isn't even a filename! At first glance, I would imagine that any of the following behaviors would be nicer:

 1. Treat the command as "git grep -e test -n", or in other words
    "do what I mean" since it is clear enough, at least to humans.

 2. Focus on "argument" instead of "filename" so that the message
    could still apply: something like

	fatal: option '-n' must come before non-option arguments

[...]

Show 8 quoted lines

> --- a/setup.c
> +++ b/setup.c
> @@ -230,7 +230,7 @@ void verify_filename(const char *prefix,
>  		     int diagnose_misspelt_rev)
>  {
>  	if (*arg == '-')
> -		die("bad flag '%s' used after filename", arg);
> +		die("found flag '%s' in place of a filename", arg);

Probably because of the background I am missing described above, it's not clear to me that the new message is any better (or worse) than the existing one. The old message with "after filename" has the virtue of explaining why an option is not expected there.

Thanks and hope that helps, Jonathan