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

From: Kaartic Sivaraam <kaarticsivaraam91196@gmail.com>
Date: Jul 26, 2017, 13:08 UTC
Message-ID: <1501074537.1792.2.camel@gmail.com>
In-Reply-To: <20170725212110.GG13924@aiede.mtv.corp.google.com>

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