Re: [PATCH 1/2] help: add help_unknown_ref

From: Vikrant Varma <vikrant.varma94@gmail.com>
Date: May 1, 2013, 22:26 UTC
Message-ID: <5181967E.9020601@gmail.com>
In-Reply-To: <7vppxa5z01.fsf@alter.siamese.dyndns.org>

On 02-05-2013 00:05, Junio C Hamano wrote:

Show 6 quoted lines

> If you step back a bit, you would notice two things.
>
>   (1) Saying 'foo' when the user means 'origin/foo' is hardly the
>       only (or even the most common) kind of mistake that the code
>       you need to add to 'git merge' would encounter and could help
>       the user with.

Yes. I like your suggestion of using levenshtein.c, similar to what's been done in help.c:help_unknown_cmd. However, where do you draw the line? Do you also suggest 'remotes/origin/foo' for 'remotes/foo'? Also, which would you then prioritize for 'foo': 'fob' (this is local) or 'origin/foo'? In other words, what kind of mistakes are you looking to correct - typos, or forgetful omissions, or both and something more?

Show 7 quoted lines

>   (2) "merge" is not the single command that user may make this kind
>       of mistake the command could help and use the same helper.
>       "git branch myfoo foo" may want to suggest "origin/foo", for
>       example.  I just typed "git checkout mater", which could have
>       been easily corrected to "git checkout master" with a mechanism
>       like this.
>

Of course, once the suggestion mechanism is in place, it can be used to replace unfriendly die()s in every command that takes a ref.

Show 6 quoted lines

>
> An asterisk sticks to the parameter name, not type
>
> Indent with two HT, not HT followed by a run of SPs.
>
> An overlong line can and should be split

 >
 > Do not add trailing blank lines.
 >

Thanks, will fix this.

Show 9 quoted lines

> When you consider the point (2) above, it becomes clear why this
> message does not belong to a helper function with a bland and
> generic name "help unknown ref".
>
> This API is misdesigned.  A possible alternative that may be better
> reusable would be to have a helper that is used to come up with a
> list of suggestions and make the caller responsible for emitting the
> error message.
>

Yes, I think a better name is needed, I was trying to follow along the lines of help_unknown_cmd.

However, making the caller responsible for printing the suggestions may 
not be the best alternative. Borrowing from the way help_unknown_cmd 
works, in help_unknown_ref we could:
1) check if autocorrect is on, returning the corrected refname to the 
calling function, otherwise
2) print an error message, a list of suggestions, and exit()

I think this makes for a clean and reusable API, and requires changing one line of code in every function that currently calls die().