threads / rfc / 46469

RFC patchsetup: update error message to be more meaningful

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

## tl;dr

15 messages between Jul 25, 2017 and Oct 4, 2017. Diffs are folded; open one to read it.

replies: 14people: 3as markdown or json

Kaartic Sivaraam· Jul 25, 2017, 17:57 UTC · lore

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. This results in the following output,

        $ git rev-parse commit.c --flags
        commit.c
        --flags
        fatal: found flag '--flags' in place of a filename
        $ git grep "test" -n
        fatal: found flag '-n' in place of a filename
Signed-off-by: Kaartic Sivaraam <kaarticsivaraam91196@gmail.com>
---
 setup.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to setup.c +1 −2
diff --git a/setup.c b/setup.c
index 860507e1fdb2d..0433d7889e6b3 100644
--- 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);
 	if (looks_like_pathspec(arg) || check_filename(prefix, arg))
 		return;
 	die_verify_filename(prefix, arg, diagnose_misspelt_rev);

--
https://github.com/git/git/pull/388
Kaartic Sivaraam· Jul 25, 2017, 18:16 UTC · re: Kaartic Sivaraam · lore

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

I've added RFC to this patch's subject as I'm not sure if the new message is suitable. Suggestions for messages that are more suitable are welcome.

It seems that the function "verify_filename" is invoked by plumbing commands like 'rev-parse', let me know if I've missed something about them.

I further noted that it's used by 'reset' but I wasn't able to make 'reset' to invoke "verify_filename". In what case does 'reset' invoke the concerned function ?

-- 
Kaartic
Jonathan Nieder· Jul 25, 2017, 21:21 UTC · re: Kaartic Sivaraam · lore

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

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

Kaartic Sivaraam· Jul 26, 2017, 13:08 UTC · re: Jonathan Nieder · lore

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

Hello Jonathan Nieder,
Thanks for the reply!
Jonathan Nieder wrote:
Show 23 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.
>

I've fixed that. The new commit message is found at the end of this message.

Show 11 quoted lines
> 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.
> 
Sorry, I actually didn't that. Could you rephrase it a little.
Show 5 quoted lines
>  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
>

I used "filename" as the function indeed check if the argument given to it is a filename. How about,

    fatal: expecting filename; found '-n'
???
In the context it looks as follows,
    $ git grep "some random regex" -n
    fatal: expected filename; found '-n' 
    $ git rev-parse --flags
    README.md
    --flags
    fatal: expected filename,
    found '--flags'
Show 5 quoted lines
> 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.
>

That's surprising, I thought the phrase "in place of filename" was more explanatory as it doesn't make assumptions about the previous arguments and specifies what was expected.

> Thanks and hope that helps,
Yep

The modified commit message is found below. If it still seems to be missing the *why*, let me know.

    setup: update error message to be more meaningful
    
    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.
    
            $ git ls-files
            README.md
            test-file
    
    Correct case,
    
            $ git rev-parse README.md --flags
            README.md
            --flags
            fatal: bad flag '--flags' used after filename
    
    Incorrect case,
    
            $ git grep "some random regex" -n
            fatal: bad flag '-n' used after filename
    
    The above case is incorrect as "some random regex" isn't a filename
    in this case.
    
    Change the error message to be general and communicative. This results
    in the following output,
    
            $ git rev-parse README.md --flags
            README.md
            --flags
            fatal: found flag '--flags' in place of a filename
    
            $ git grep "some random regex" -n
            fatal: found flag '-n' in place of a filename
    -- 
    Kaartic
Junio C Hamano· Jul 26, 2017, 20:09 UTC · re: Jonathan Nieder · lore

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

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 9 quoted lines
> For an initial guess: in the example
>
> 	git grep test -n
>
> ...
>  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

I think this one is the most sensible. There may or may not be a file called "test" in the working tree, and the user may or may not meant to look for a pattern "test". What is wrong in the sample command line is that "test" is not a dashed option and yet it has a dashed option "-n" after it, and your version clearly explains it.

Kaartic Sivaraam· Jul 29, 2017, 02:44 UTC · re: Junio C Hamano · lore

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

On Wed, 2017-07-26 at 13:09 -0700, Junio C Hamano wrote:
Show 17 quoted lines
> Jonathan Nieder <jrnieder@gmail.com> writes:
> 
> > For an initial guess: in the example
> > 
> > 	git grep test -n
> > 
> > ...
> >  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
> 
> I think this one is the most sensible.  There may or may not be a
> file called "test" in the working tree, and the user may or may not
> meant to look for a pattern "test".  What is wrong in the sample
> command line is that "test" is not a dashed option and yet it has a
> dashed option "-n" after it, and your version clearly explains it.

Though the message seems to be most fitting one, I'm a little reluctant to use it as it "might" create a wrong picture on the minds of the user making them think this would be the case in other cases too, which we know is not true. For example,

    git log -p --full-diff master --stat
    git commit -v Makefile --amend

The above are valid commands and produce expected result even though the 'option argument' comes after the 'non-option' argument. Thus, I thought a "general statement" like the one above might create a wrong picture on the minds of user. Any thoughts about how to overcome this?

-- 
Kaartic
Junio C Hamano· Jul 29, 2017, 03:53 UTC · re: Kaartic Sivaraam · lore

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

Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> writes:
Show 9 quoted lines
> Though the message seems to be most fitting one, I'm a little reluctant
> to use it as it "might" create a wrong picture on the minds of the user
> making them think this would be the case in other cases too, which we
> know is not true. For example,
>
>
>     git log -p --full-diff master --stat
>
>     git commit -v Makefile --amend
These are accepted not by design but by accident.

I do not think we even document that you are allowed to do these in "log" and "commit" manual pages, and we should discourage them (I do not even mind eventually removing these with the usual deprecation dance, but I do not think it is worth the noise).

Kaartic Sivaraam· Jul 29, 2017, 12:13 UTC · re: Junio C Hamano · lore

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

On Fri, 2017-07-28 at 20:53 -0700, Junio C Hamano wrote:
Show 19 quoted lines
> Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> writes:
> 
> > Though the message seems to be most fitting one, I'm a little reluctant
> > to use it as it "might" create a wrong picture on the minds of the user
> > making them think this would be the case in other cases too, which we
> > know is not true. For example,
> > 
> > 
> >     git log -p --full-diff master --stat
> > 
> >     git commit -v Makefile --amend
> 
> These are accepted not by design but by accident.  
> 
> I do not think we even document that you are allowed to do these in
> "log" and "commit" manual pages, and we should discourage them (I do
> not even mind eventually removing these with the usual deprecation
> dance, but I do not think it is worth the noise).
>

That's interesting. In that case, I'll go with the suggested statement, happily!

-- 
Kaartic
Kaartic Sivaraam· Jul 29, 2017, 12:41 UTC · re: Kaartic Sivaraam · lore

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

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.

        $ git ls-files
        README.md
        test-file
Correct case,
        $ git rev-parse README.md --flags
        README.md
        --flags
        fatal: bad flag '--flags' used after filename
Incorrect case,
        $ git grep "some random regex" -n
        fatal: bad flag '-n' used after filename

The above case is incorrect as "some random regex" isn't a filename in this case.

Change the error message to be general and communicative. This results in the following output,

        $ git rev-parse README.md --flags
        README.md
        --flags
        fatal: option '--flags' must come before non-option arguments
        $ git grep "some random regex" -n
        fatal: option '-n' must come before non-option arguments
Signed-off-by: Kaartic Sivaraam <kaarticsivaraam91196@gmail.com>
---
 setup.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to setup.c +1 −2
diff --git a/setup.c b/setup.c
index 860507e1fdb2d..09c79328247dd 100644
--- 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("option '%s' must come before non-option arguments", arg);
 	if (looks_like_pathspec(arg) || check_filename(prefix, arg))
 		return;
 	die_verify_filename(prefix, arg, diagnose_misspelt_rev);

--
https://github.com/git/git/pull/393
Junio C Hamano· Jul 29, 2017, 16:10 UTC · re: Kaartic Sivaraam · lore

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

Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> writes:
Show 22 quoted lines
> On Fri, 2017-07-28 at 20:53 -0700, Junio C Hamano wrote:
>> Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> writes:
>> 
>> > Though the message seems to be most fitting one, I'm a little reluctant
>> > to use it as it "might" create a wrong picture on the minds of the user
>> > making them think this would be the case in other cases too, which we
>> > know is not true. For example,
>> > 
>> > 
>> >     git log -p --full-diff master --stat
>> > 
>> >     git commit -v Makefile --amend
>> 
>> These are accepted not by design but by accident.  
>> 
>> I do not think we even document that you are allowed to do these in
>> "log" and "commit" manual pages, and we should discourage them (I do
>> not even mind eventually removing these with the usual deprecation
>> dance, but I do not think it is worth the noise).
>> 
> That's interesting. In that case, I'll go with the suggested statement,
> happily!

It is not interesting at all. It actually is disturbing that you had the notion that these are "valid" command lines.

We perhaps need to somehow make sure new users won't be led to the misunderstanding. Improving our documentation is a good first step. We might want to have a group of volunteers who actively monitor the internets (e.g. stackoverflow and other places like that) and correct people who spread the same misunderstanding when they do.

Kaartic Sivaraam· Jul 30, 2017, 10:47 UTC · re: Junio C Hamano · lore

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

On Sat, 2017-07-29 at 09:10 -0700, Junio C Hamano wrote:
Show 8 quoted lines
> Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> writes:
> > 
> > That's interesting. In that case, I'll go with the suggested statement,
> > happily!
> 
> It is not interesting at all.  It actually is disturbing that you
> had the notion that these are "valid" command lines.
>

Seems I've used a sloppy statement, sorry about that. I actually thought these were valid because they "worked", no other reason.

> We perhaps need to somehow make sure new users won't be led to the
> misunderstanding.  Improving our documentation is a good first step.
That's something I could help with.
-- 
Kaartic
Kaartic Sivaraam· Jul 30, 2017, 11:03 UTC · re: Kaartic Sivaraam · lore

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

On Sun, 2017-07-30 at 16:17 +0530, Kaartic Sivaraam wrote:
Show 6 quoted lines
> On Sat, 2017-07-29 at 09:10 -0700, Junio C Hamano wrote:
> > We perhaps need to somehow make sure new users won't be led to the
> > misunderstanding.  Improving our documentation is a good first step.
> 
> That's something I could help with.
>

I seem to be stuck a little with this. I'm not sure which other sub- commands other than log and commit have similar behaviour as I'm aware of "why" they behave that way. I tried a little but thought it was better ask this one.

What could be the source of this issue? (I guess this may help identify the possible commands if the source is common.)

Which documentation(s) could be candidates for the suggested change? How about "Documentation/git.txt"?

-- 
Kaartic
Kaartic Sivaraam· Oct 2, 2017, 17:30 UTC · re: Junio C Hamano · lore

[PATCH v2] setup: update error message to be more meaningful

From: Kaartic Sivaraam <kaarticsivaraam91196@gmail.com>

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.

        $ git ls-files
        README.md
        test-file
Correct case,
        $ git rev-parse README.md --flags
        README.md
        --flags
        fatal: bad flag '--flags' used after filename
Incorrect case,
        $ git grep "some random regex" -n
        fatal: bad flag '-n' used after filename

The above case is incorrect as "some random regex" isn't a filename in this case.

Change the error message to be general and communicative. This results in the following output,

        $ git rev-parse README.md --flags
        README.md
        --flags
        fatal: option '--flags' must come before non-option arguments
        $ git grep "some random regex" -n
        fatal: option '-n' must come before non-option arguments
Signed-off-by: Kaartic Sivaraam <kaarticsivaraam91196@gmail.com>
---
 Changes in v2:
    Change in error message.
 setup.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to setup.c +1 −1
diff --git a/setup.c b/setup.c
index 860507e1f..09c793282 100644
--- 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("option '%s' must come before non-option arguments", arg);
 	if (looks_like_pathspec(arg) || check_filename(prefix, arg))
 		return;
 	die_verify_filename(prefix, arg, diagnose_misspelt_rev);

-- 
2.14.1.935.ge2b2bcd8a
Junio C Hamano· Oct 3, 2017, 00:32 UTC · re: Kaartic Sivaraam · lore

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

Kaartic Sivaraam <kaartic.sivaraam@gmail.com> writes:
Show 7 quoted lines
> Incorrect case,
>
>         $ git grep "some random regex" -n
>         fatal: bad flag '-n' used after filename
>
> The above case is incorrect as "some random regex" isn't a filename
> in this case.

The command line rule is to have dashed options first and then other arguments, so I agree that "option '-n' used after non-option argument(s)" would be a better alternative.

"grep" is an oddball, as it allows you to be lazy and omit the "-e" option when there is only one pattern, making a perfectly reasonable "grep -e regex -n" into an invalid "grep regex -n".

As an aside, I wonder if we want to _() the message. It's outside the scope of this fix, obviously.

Show 16 quoted lines
>  setup.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/setup.c b/setup.c
> index 860507e1f..09c793282 100644
> --- 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("option '%s' must come before non-option arguments", arg);
>  	if (looks_like_pathspec(arg) || check_filename(prefix, arg))
>  		return;
>  	die_verify_filename(prefix, arg, diagnose_misspelt_rev);
Kaartic Sivaraam· Oct 4, 2017, 14:18 UTC · re: Junio C Hamano · lore

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

On Tue, 2017-10-03 at 09:32 +0900, Junio C Hamano wrote:
> 
> As an aside, I wonder if we want to _() the message.  It's outside
> the scope of this fix, obviously.
> 
I'm surprised it isn't already! I think it should.

As an aside, I wanted to know whether we should add 'test_expect_failure's for commands of the following sort,

    git log -p --full-diff master --stat
     
    git commit -v Makefile --amend

just to keep us reminded that these are accepted not by design but by accident ?

--- Kaartic

← back to recent threads