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

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

[Kaartic Sivaraam]

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

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

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

[Kaartic Sivaraam]

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

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

[Jonathan Nieder]

Hi,

Kaartic Sivaraam wrote:

> 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

[...]
> --- 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

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

[Kaartic Sivaraam]

Hello Jonathan Nieder,

Thanks for the reply!

Jonathan Nieder wrote:
> 
> > 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.

> 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.

>  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'


> 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

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

[Junio C Hamano]

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.

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

[Kaartic Sivaraam]

On Wed, 2017-07-26 at 13:09 -0700, Junio C Hamano wrote:
> 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

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

[Junio C Hamano]

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

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

[Kaartic Sivaraam]

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!

-- 
Kaartic

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

[Kaartic Sivaraam]

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

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

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

[Junio C Hamano]

Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> writes:

> 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.

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

[Kaartic Sivaraam]

On Sat, 2017-07-29 at 09:10 -0700, Junio C Hamano wrote:
> 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

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

[Kaartic Sivaraam]

On Sun, 2017-07-30 at 16:17 +0530, Kaartic Sivaraam wrote:
> 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

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

[Kaartic Sivaraam]

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

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

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

[Junio C Hamano]

Kaartic Sivaraam <kaartic.sivaraam@gmail.com> writes:

> 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.

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

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

[Kaartic Sivaraam]

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