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

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

[Junio C Hamano]

Kaartic Sivaraam  writes:

> On Fri, 2017-07-28 at 20:53 -0700, Junio C Hamano wrote:
>> Kaartic Sivaraam  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 Fri, 2017-07-28 at 20:53 -0700, Junio C Hamano wrote:
> Kaartic Sivaraam  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

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

[Junio C Hamano]

Kaartic Sivaraam  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 Wed, 2017-07-26 at 13:09 -0700, Junio C Hamano wrote:
> Jonathan Nieder  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]

Jonathan Nieder  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]

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

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

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