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

Johannes Schindelin [off-list ref] writes:

Hi Junio,

On Fri, 28 Aug 2020, Junio C Hamano wrote:

quoted

Johannes Schindelin [off-list ref] writes:

quoted

Signed-off-by: Johannes Schindelin <redacted>
---
 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.

quoted

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.

quoted

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.