Re: [PATCH v2 1/3] help: redirect to aliased commands for "git cmd --help"

From: Jeff King <peff@peff.net>
Date: Oct 3, 2018, 02:13 UTC
Message-ID: <20181003021358.GA20553@sigill.intra.peff.net>
In-Reply-To: <20181001112107.28956-1-rv@rasmusvillemoes.dk>

On Mon, Oct 01, 2018 at 01:21:05PM +0200, Rasmus Villemoes wrote:

Show 16 quoted lines

> As discussed in the thread for v1 of this patch [1] [2], this changes the
> rules for "git foo --help" when foo is an alias.
> 
> (0) When invoked as "git help foo", we continue to print the "foo is
> aliased to bar" message and nothing else.
> 
> (1) If foo is an alias for a shell command, print "foo is aliased to
> !bar" as usual.
> 
> (2) Otherwise, break the alias string into words, and pretend that "git
> word0 --help" was called.
> 
> At least for me, getting the man page for git-cherry-pick directly with
> "git cp --help" is more useful (and how I expect an alias to behave)
> than the short "is aliased to" notice. It is also consistent with
> "--help" generally providing more comprehensive help than "-h".

Makes sense.

Show 12 quoted lines

> I believe that printing the "is aliased to" message also in case (2) has
> value: Depending on pager setup, or if the user has help.format=web, the
> message is still present immediately above the prompt when the user
> quits the pager/returns to the terminal. That serves as an explanation
> for why one was redirected to "man git-cherry-pick" from "git cp
> --help", and if cp is actually 'cherry-pick -n', it reminds the user
> that using cp has some flag implicitly set before firing off the next
> command.
> 
> It also provides some useful info in case we end up erroring out, either
> in the "bad alias string" check, or in the "No manual entry for gitbar"
> case.

OK, I buy that line of reasoning. And in the other cases, it shouldn't _hurt_ anything.

Show 24 quoted lines

> diff --git a/builtin/help.c b/builtin/help.c
> index 8d4f6dd301..4802a06f37 100644
> --- a/builtin/help.c
> +++ b/builtin/help.c
> @@ -415,9 +415,29 @@ static const char *check_git_cmd(const char* cmd)
>  
>  	alias = alias_lookup(cmd);
>  	if (alias) {
> -		printf_ln(_("'%s' is aliased to '%s'"), cmd, alias);
> -		free(alias);
> -		exit(0);
> +		const char **argv;
> +		int count;
> +
> +		/*
> +		 * If we were invoked as "git help cmd", or cmd is an
> +		 * alias for a shell command, we inform the user what
> +		 * cmd is an alias for and do nothing else.
> +		 */
> +		if (!exclude_guides || alias[0] == '!') {
> +			printf_ln(_("'%s' is aliased to '%s'"), cmd, alias);
> +			free(alias);
> +			exit(0);
> +		}

I'm not sure I understand why exclude_guides is relevant. We check it below when we know that we _don't_ have an alias. Hrm. I guess you're using it here as a proxy for "git foo --help" being used instead of "git help foo". The comment probably needs to spell out that exclude_guides is the same as your "we were invoked as...".

I wonder if we could change the name of that option. It is an undocumented, hidden option that we use internally, so it should be OK to do so (or we could always add another one). That might prevent somebody in the future from using --exclude-guides in more places and breaking your assumption here.

Show 10 quoted lines

> +		/*
> +		 * Otherwise, we pretend that the command was "git
> +		 * word0 --help.
> +		 */
> +		fprintf_ln(stderr, _("'%s' is aliased to '%s'"), cmd, alias);
> +		count = split_cmdline(alias, &argv);
> +		if (count < 0)
> +			die(_("bad alias.%s string: %s"), cmd,
> +			    split_cmdline_strerror(count));
> +		return alias;

So we split only to find argv[0] here. But then we don't return it. That works because the split is done in place, meaning we must have inserted a NUL in alias. That's sufficiently subtle that it might be worth spelling it out in a comment.

We don't need to free alias here as we do above, because we're passing it back. We should free argv, though, I think (not its elements, just the array itself).

Unfortunately the caller is going to leak our returned "alias", but I'm not sure we can do much about it. I'm not overly concerned with the memory, but it is going to trigger leak-checkers (and we're trying to quiet them down, not go the other way). I think it may be OK to overlook that and just UNLEAK() it in cmd_help().

-Peff