Re: [PATCH] help: allow redirecting to help for aliased command

[Taylor Blau <me@ttaylorr.com>]

On Wed, Sep 26, 2018 at 12:26:36PM +0200, Rasmus Villemoes wrote:
> I often use 'git <cmd> --help' as a quick way to get the documentation
> for a command. However, I've also trained my muscle memory to use my
> aliases (cp=cherry-pick, co=checkout etc.), which means that I often end
> up doing
>
>   git cp --help
>
> to which git correctly informs me that cp is an alias for
> cherry-pick. However, I already knew that, and what I really wanted was
> the man page for the cherry-pick command.

Neat. I have many of those such aliases myself, and have always wanted
something like this. Thanks for taking the time to put such a patch
together :-).

> This introduces a help.followAlias config option that transparently
> redirects to (the first word of) the alias text (provided of course it
> is not a shell command), similar to the option for autocorrect of
> misspelled commands.

Good. I was curious if you were going to introduce a convention along
the lines of, "If the alias begins with a '!', then pass "--help" to it
and it must respond appropriately." I'm glad that you didn't take that
approach.

> The documentation in config.txt could probably be improved. Also, I
> mimicked the autocorrect case in that the "Continuing to ..." text goes
> to stderr, but because of that, I also print the "is aliased to" text to
> stderr, which is different from the current behaviour of using
> stdout. I'm not sure what the most correct thing is, but I assume --help
> is mostly used interactively with stdout and stderr pointing at the same
> place.
>
> Signed-off-by: Rasmus Villemoes <rv@rasmusvillemoes.dk>
> ---
>  Documentation/config.txt | 10 ++++++++++
>  builtin/help.c           | 36 +++++++++++++++++++++++++++++++++---
>  2 files changed, 43 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index ad0f4510c3..8a1fc8064e 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -2105,6 +2105,16 @@ help.autoCorrect::
>  	value is 0 - the command will be just shown but not executed.
>  	This is the default.
>
> +help.followAlias::
> +	When requesting help for an alias, git prints a line of the
> +	form "'<alias>' is aliased to '<string>'". If this option is
> +	set to a positive integer, git proceeds to show the help for

With regard to "set to a positive integer", I'm not sure why this is the
way that it is. I see below you used 'git_config_int()', but I think
that 'git_config_bool()' would be more appropriate.

The later understands strings like "yes", "on" or "true", which I think
is more of what I would expect from a configuration setting such as
this.

> +	the first word of <string> after the given number of
> +	deciseconds. If the value of this option is negative, the
> +	redirect happens immediately. If the value is 0 (which is the
> +	default), or <string> begins with an exclamation point, no
> +	redirect takes place.

It was unclear to my originlly why this was given as a configuration
knob, but my understanding after reading the patch is that this is to do
_additional_ things besides printing what is aliased to what.

Could you perhaps note this in the documentation?

>  help.htmlPath::
>  	Specify the path where the HTML documentation resides. File system paths
>  	and URLs are supported. HTML pages will be prefixed with this path when
> diff --git a/builtin/help.c b/builtin/help.c
> index 8d4f6dd301..ef1c3f0916 100644
> --- a/builtin/help.c
> +++ b/builtin/help.c
> @@ -34,6 +34,7 @@ enum help_format {
>  };
>
>  static const char *html_path;
> +static int follow_alias;
>
>  static int show_all = 0;
>  static int show_guides = 0;
> @@ -273,6 +274,10 @@ static int git_help_config(const char *var, const char *value, void *cb)
>  		html_path = xstrdup(value);
>  		return 0;
>  	}
> +	if (!strcmp(var, "help.followalias")) {
> +		follow_alias = git_config_int(var, value);
> +		return 0;
> +	}

Good. I think in modern Git, we'd prefer to write this as a series of
`else if`'s, but this matches the style of the surrounding code. I think
that you could optionally clean up this style as a preparatory commit,
but ultimately I don't think it's worth a reroll on its own.

>  	if (!strcmp(var, "man.viewer")) {
>  		if (!value)
>  			return config_error_nonbool(var);
> @@ -415,9 +420,34 @@ 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 (!follow_alias || alias[0] == '!') {
> +			printf_ln(_("'%s' is aliased to '%s'"), cmd, alias);
> +			free(alias);
> +			exit(0);
> +		}
> +		fprintf_ln(stderr, _("'%s' is aliased to '%s'"), cmd, alias);

OK, I think that this is a sensible decision: print to STDERR when
that's not the main purpose of what're doing (e.g., we're going to
follow the alias momentarily), and STDOUT when it's the only thing we're
doing.

Potentially we could call 'fprintf_ln()' only once, and track an `int
fd` at the top of this block.

> +
> +		/*
> +		 * We use split_cmdline() to get the first word of the
> +		 * alias, to ensure that we use the same rules as when
> +		 * the alias is actually used. split_cmdline()
> +		 * modifies alias in-place.
> +		 */
> +		count = split_cmdline(alias, &argv);
> +		if (count < 0)
> +			die("Bad alias.%s string: %s", cmd,
> +			    split_cmdline_strerror(count));

Please wrap this in _() so that translators can translate it.

> +		if (follow_alias > 0) {
> +			fprintf_ln(stderr,
> +				   _("Continuing to help for %s in %0.1f seconds."),
> +				   alias, follow_alias/10.0);
> +			sleep_millisec(follow_alias * 100);
> +		}
> +		return alias;

I'm not sure that this notification is necessary, but I'll defer to the
judgement of others on this one.

Thanks,
Taylor