Re: [PATCH v1 3/3] git: catch an attempt to run "git-foo"

[Junio C Hamano <gitster@pobox.com>]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Hi Junio,
>
> On Fri, 28 Aug 2020, Junio C Hamano wrote:
>
>> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>>
>> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>> > ---
>> >  git.c  | 3 ++-
>> >  help.c | 5 ++++-
>> >  2 files changed, 6 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/git.c b/git.c
>> > index 71ef4835b20e..863fd0c58a66 100644
>> > --- a/git.c
>> > +++ b/git.c
>> > @@ -851,7 +851,8 @@ int cmd_main(int argc, const char **argv)
>> >  	 * that one cannot handle it.
>> >  	 */
>> >  	if (skip_prefix(cmd, "git-", &cmd)) {
>> > -		warn_on_dashed_git(argv[0]);
>> > +		strip_extension(&cmd);
>> > +		warn_on_dashed_git(cmd);
>>
>> The argv[0] may have been NULL from the beginning of cmd_main(), and
>> cmd would be "git-help" in such a case. We used to pass NULL to
>> warn_on_dashed_git() in such a case because "git-help" is not what the
>> user typed, but what we internally trigger, so we didn't want
>> warn_on_dashed_git() to do anything based on that internal string.
>
> True. The test suite passes with this, though, which means we haven't
> covered that case.

Yup, it would be a good thing to add to our tests, with or without
this patch.

>> How does your handle_builtin() work, by the way?
>>
>> The original code (even before we added warn_on_dashed_git() in this
>> codepath) did not do any strip_extension(), so handle_builtin() can
>> take commands with ".exe" suffix, but now we are feeding the result
>> of strip_extension() to it, so it can deal with both?
>
> Yes, it can deal with both. It calls `strip_extension()`, which on Windows
> removes the `.exe` suffix, if found.
>
>> Sounds convenient and sloppy (not the handle_builtin's
>> implementation, but its callers that sometimes feeds the full
>> executable name, and feeds only the basename some other times) at
>> the same time.
>
> Right, it does not _require_ the extension. I do not necessarily agree
> that it is sloppy, but I do agree that it is convenient ;-)

Being lenient to its input is not sloppy.  

It just is by being convenient, it allows its callers to be sloppy,
which may hurt them as not all callees they call may not be as
lenient as handle_builtin(), which is the only downside I would be
worried about.  Nothing big.

thanks.