diff mbox series

help: allow redirecting to help for aliased command

Message ID 20180926102636.30691-1-rv@rasmusvillemoes.dk (mailing list archive)
State New, archived
Headers show
Series help: allow redirecting to help for aliased command | expand

Commit Message

Rasmus Villemoes Sept. 26, 2018, 10:26 a.m. UTC
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.

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.

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(-)

Comments

Taylor Blau Sept. 26, 2018, 2:37 p.m. UTC | #1
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
Duy Nguyen Sept. 26, 2018, 3:16 p.m. UTC | #2
On Wed, Sep 26, 2018 at 12:29 PM Rasmus Villemoes <rv@rasmusvillemoes.dk> 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.
>
> 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.
>
> The documentation in config.txt could probably be improved.

While at there, maybe you could also mention the behavior of "git
help" when given an alias, in git-help.txt. And you could also add a
hint to suggest this new config help.followAlias there.
Junio C Hamano Sept. 26, 2018, 3:16 p.m. UTC | #3
Rasmus Villemoes <rv@rasmusvillemoes.dk> writes:

> 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.
>
> 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.

While I do agree with you that it would sometimes be very handy if
"git cp --help" behaved identically to "git cherry-pick --help" just
like "git cp -h" behaves identically to "git cherry-pick -h" when
you have "[alias] cp = cherry-pick", I do not think help.followAlias
configuration is a good idea.  I may know, perhaps because I use it
all the time, by heart that "cp" is aliased to "cherry-pick" and
want "git cp --help" to directly give me the manpage, but I may not
remember if "co" was commit or checkout and want to be concisely
told that it is aliased to checkout without seeing the full manpage.
Which means you'd want some way to command line override anyway, and
having to say "git -c help.followAlias=false cp --help" is not a
great solution.

If we expect users to use "git cp --help" a lot more often than "git
help cp" (or the other way around), one way to give a nicer experience
may be to unconditionally make "git cp --help" to directly show the
manpage of cherry-pick, while keeping "git help cp" to never do
that.  Then those who want to remember what "co" is aliased to can
ask "git help co".

> +		/*
> +		 * 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));
> +
> +		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;

If you have "[alias] cp = cherry-pick -n", split_cmdline discards
"-n" and the follow-alias prompt does not even tell you that it did
so, and you get "git help cherry-pick".  This code somehow expects
you to know to jump to the section that describes the "--no-commit"
option.  I do not think that is a reasonable expectation.

When you have "[alias] cp = cherry-pick -n", "git cp --help" should
not do "git help cherry-pick".  Only a single word that exactly
matches a git command should get this treatment.

>  	}
>  
>  	if (exclude_guides)
Duy Nguyen Sept. 26, 2018, 3:19 p.m. UTC | #4
On Wed, Sep 26, 2018 at 4:42 PM Taylor Blau <me@ttaylorr.com> wrote:
> > +
> > +             /*
> > +              * 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.

Yes! And another nit. die(), error(), warning()... usually start the
message with a lowercase letter because we already start the sentence
with a prefix, like

fatal: bad alias.blah blah
Junio C Hamano Sept. 26, 2018, 3:30 p.m. UTC | #5
Taylor Blau <me@ttaylorr.com> writes:

>> +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.

That is, as you read in the next paragraph, because it gives the
number of deciseconds to show a prompt before showing the manpage.

Not that I think this configuration is a good idea (see my review).

>> +	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?

It may be that the description for the "execute the likely typoed
command" configuration is poorly written and this merely copied the
badness from it.  Over there the prompt gives a chance to ^C out,
which serves useful purpose, and if that is not documented, we should.

On the other hand, I'd rather see this prompt in the new code
removed, because I do not think the prompt given in the new code
here is all that useful.

>> @@ -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.

I actually think this should always give the output to standard output.

>> +
>> +		/*
>> +		 * 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.

I didn't bother to check the original but this is mimicking an
existing code that lets configuration to be set to num-deciseconds
to pause and give chance to ^C out, and also allows it to be set to
negative to immediately go ahead.  follow-alias at this point cannot
be zero in the codeflow, but it still can be negative.
Taylor Blau Sept. 26, 2018, 6:09 p.m. UTC | #6
On Wed, Sep 26, 2018 at 08:30:32AM -0700, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
>
> >> +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.
>
> That is, as you read in the next paragraph, because it gives the
> number of deciseconds to show a prompt before showing the manpage.
>
> Not that I think this configuration is a good idea (see my review).
>
> >> +	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?
>
> It may be that the description for the "execute the likely typoed
> command" configuration is poorly written and this merely copied the
> badness from it.  Over there the prompt gives a chance to ^C out,
> which serves useful purpose, and if that is not documented, we should.
>
> On the other hand, I'd rather see this prompt in the new code
> removed, because I do not think the prompt given in the new code
> here is all that useful.
>
> >> @@ -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.
>
> I actually think this should always give the output to standard output.
>
> >> +
> >> +		/*
> >> +		 * 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.
>
> I didn't bother to check the original but this is mimicking an
> existing code that lets configuration to be set to num-deciseconds
> to pause and give chance to ^C out, and also allows it to be set to
> negative to immediately go ahead.  follow-alias at this point cannot
> be zero in the codeflow, but it still can be negative.

I think that this is the most compelling argument _for_ the configuration
that you are not in favor of. I understood your previous review as "I
know that 'git cp' is a synonym of 'git cherry-pick', but I want to use
'git co --help' for when I don't remember what 'git co' is a synonym
of."

This pause (though I'm a little surprised by it when reviewing the
code), I think strikes a good balance between the two, i.e., that you
can get help for whatever it is aliased to, and see what that alias is.

Thanks,
Taylor
Taylor Blau Sept. 26, 2018, 6:12 p.m. UTC | #7
On Wed, Sep 26, 2018 at 08:16:36AM -0700, Junio C Hamano wrote:
> Rasmus Villemoes <rv@rasmusvillemoes.dk> writes:
>
> > 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.
> >
> > 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.
>
> While I do agree with you that it would sometimes be very handy if
> "git cp --help" behaved identically to "git cherry-pick --help" just
> like "git cp -h" behaves identically to "git cherry-pick -h" when
> you have "[alias] cp = cherry-pick", I do not think help.followAlias
> configuration is a good idea.  I may know, perhaps because I use it
> all the time, by heart that "cp" is aliased to "cherry-pick" and
> want "git cp --help" to directly give me the manpage, but I may not
> remember if "co" was commit or checkout and want to be concisely
> told that it is aliased to checkout without seeing the full manpage.
> Which means you'd want some way to command line override anyway, and
> having to say "git -c help.followAlias=false cp --help" is not a
> great solution.

I think I responded partially to this hunk in another thread, but I
think I can add some additional information here where it is more
relevant.

One approach to take when digesting this is that 'git co --help' shows
you the manual page for 'git-checkout(1)' (or whatever you have it
aliased to), so that answers the question for the caller who has a
terminal open.

In the case where you are scripting (and want to know what 'git co'
means for programmatic usage), I think that there are two options. One,
which you note above, is the 'git -c help.followAlias=false ...'
approach, which I don't think is so bad for callers, given the tradeoff.

Another way to go is 'git config alias.co', which should provide the
same answer. I think that either would be fine.

Perhaps we could assume that 'help.followAlias' is false when it is
unset, _and_ isatty(2) says that we aren't a TTY. Otherwise, since I
feel that this is a good feature that should be the new default, we can
assume it's set to true.

Thanks,
Taylor
Junio C Hamano Sept. 26, 2018, 6:20 p.m. UTC | #8
Taylor Blau <me@ttaylorr.com> writes:

> This pause (though I'm a little surprised by it when reviewing the
> code), I think strikes a good balance between the two, i.e., that you
> can get help for whatever it is aliased to, and see what that alias is.

And I need to react to it within subsecond with ^C when I want a
compact answer to "what is this aliased to"?  No, thanks.
Jeff King Sept. 26, 2018, 6:49 p.m. UTC | #9
On Wed, Sep 26, 2018 at 08:16:36AM -0700, Junio C Hamano wrote:

> > 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.
> 
> While I do agree with you that it would sometimes be very handy if
> "git cp --help" behaved identically to "git cherry-pick --help" just
> like "git cp -h" behaves identically to "git cherry-pick -h" when
> you have "[alias] cp = cherry-pick", I do not think help.followAlias
> configuration is a good idea.  I may know, perhaps because I use it
> all the time, by heart that "cp" is aliased to "cherry-pick" and
> want "git cp --help" to directly give me the manpage, but I may not
> remember if "co" was commit or checkout and want to be concisely
> told that it is aliased to checkout without seeing the full manpage.
> Which means you'd want some way to command line override anyway, and
> having to say "git -c help.followAlias=false cp --help" is not a
> great solution.
> 
> If we expect users to use "git cp --help" a lot more often than "git
> help cp" (or the other way around), one way to give a nicer experience
> may be to unconditionally make "git cp --help" to directly show the
> manpage of cherry-pick, while keeping "git help cp" to never do
> that.  Then those who want to remember what "co" is aliased to can
> ask "git help co".

I like that direction much better. I also wondered if we could leverage
the "-h" versus "--help" distinction. The problem with printing the
alias definition along with "--help" is that the latter will start a
pager that obliterates what we wrote before (and hence all of this delay
trickery).

But for "-h" we generally expect the command to output a usage message.

So what if the rules were:

  - "git help cp" shows "cp is an alias for cherry-pick" (as it does
    now)

  - "git cp -h" shows "cp is an alias for cherry-pick", followed by
    actually running "cherry-pick -h", which will show the usage
    message. For a single-word command that does very little, since the
    usage message starts with "cherry-pick". But if your alias is
    actually "cp = cherry-pick -n", then it _is_ telling you extra
    information. And this could even work with "!" aliases: we define
    it, and then it is up to the alias to handle "-h" sensibly.

  - "git cp --help" opens the manpage for cherry-pick. We don't bother
    with the alias definition, as it's available through other means
    (and thus we skip the obliteration/timing thing totally).

    This really only works for non-! aliases. Those would continue to
    show the alias definition.


> If you have "[alias] cp = cherry-pick -n", split_cmdline discards
> "-n" and the follow-alias prompt does not even tell you that it did
> so, and you get "git help cherry-pick".  This code somehow expects
> you to know to jump to the section that describes the "--no-commit"
> option.  I do not think that is a reasonable expectation.
> 
> When you have "[alias] cp = cherry-pick -n", "git cp --help" should
> not do "git help cherry-pick".  Only a single word that exactly
> matches a git command should get this treatment.

I'm not sure I agree. A plausible scenario (under the rules I gave
above) is:

  $ git cp -h
  'cp' is aliased to 'cherry-pick -n'
  usage: git cherry-pick ...

  $ git cp --help

I.e., you already know the "-n" part, and now you want to dig further.
Of course one could just type "git cherry-pick --help" since you also
know that, too. But by that rationale, one could already do:

  $ git help cp
  $ git help cherry-pick

without this patch at all.

-Peff
Junio C Hamano Sept. 26, 2018, 7:31 p.m. UTC | #10
Jeff King <peff@peff.net> writes:

>> When you have "[alias] cp = cherry-pick -n", "git cp --help" should
>> not do "git help cherry-pick".  Only a single word that exactly
>> matches a git command should get this treatment.
>
> I'm not sure I agree. A plausible scenario (under the rules I gave
> above) is:
>
>   $ git cp -h
>   'cp' is aliased to 'cherry-pick -n'
>   usage: git cherry-pick ...

With that additional rule, I can buy "it is fine for 'git cp --help'
to completely ignore -n and behave as if 'git help cherry-pick' was
given", I think.  People already expect "git cp --help" to give the
alias expansion, so to them any change will be a regression any way
we cut it---but I think this is the least bad approach.

>   $ git cp --help
>
> I.e., you already know the "-n" part, and now you want to dig further.

One very good thing about the "make '--help' go directly to the
manpage, while teaching '-h' to report also alias expansion" is that
people already expect "-h" is more concise than "--help".  The
current output from "git cp --help" violates that expectation, and
the change you suggest rectifies it.

> Of course one could just type "git cherry-pick --help" since you also
> know that, too.

Yeah, but that is not an argument.  The user aliased cp because
cherry-pick was quite a mouthful and do not want to type "git
cherry-pick --help" in the first place.
Rasmus Villemoes Sept. 28, 2018, 7:40 a.m. UTC | #11
On 2018-09-26 17:16, Junio C Hamano wrote:
> Rasmus Villemoes <rv@rasmusvillemoes.dk> writes:
> 
>> +		/*
>> +		 * 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));
>> +
>> +		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;
> 
> If you have "[alias] cp = cherry-pick -n", split_cmdline discards
> "-n" and the follow-alias prompt does not even tell you that it did
> so,

That's not really true, as I deliberately did the split_cmdline after
printing the "is an alias for", but before "continuing to help for", so
this would precisely tell you

  cp is an alias for 'cherry-pick -n'
  continuing to help for 'cherry-pick' in 1.5 seconds

> and you get "git help cherry-pick".  This code somehow expects
> you to know to jump to the section that describes the "--no-commit"
> option.  I do not think that is a reasonable expectation.

No, in that case I would not expect git cp --help to jump to that
section anymore than I would expect "git cherry-pick -n --help" to
magically do that (and that would be impossible in general, if more
options are bundled in the alias).

> When you have "[alias] cp = cherry-pick -n", "git cp --help" should
> not do "git help cherry-pick".  Only a single word that exactly
> matches a git command should get this treatment.

I considered that, and could certainly live with that. But it seems the
discussion took a different turn in another part of the thread, so I'll
continue there.

Rasmus
Rasmus Villemoes Sept. 28, 2018, 7:44 a.m. UTC | #12
On 2018-09-26 17:19, Duy Nguyen wrote:
> On Wed, Sep 26, 2018 at 4:42 PM Taylor Blau <me@ttaylorr.com> wrote:
>>> +
>>> +             /*
>>> +              * 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.
> 
> Yes! And another nit. die(), error(), warning()... usually start the
> message with a lowercase letter because we already start the sentence
> with a prefix, like
> 
> fatal: bad alias.blah blah
> 

I'll keep these points in mind, but this was pure copy-paste from git.c.

Rasmus
Rasmus Villemoes Sept. 28, 2018, 7:53 a.m. UTC | #13
On 2018-09-26 20:12, Taylor Blau wrote:
> 
> In the case where you are scripting (and want to know what 'git co'
> means for programmatic usage), I think that there are two options. One,
> which you note above, is the 'git -c help.followAlias=false ...'
> approach, which I don't think is so bad for callers, given the tradeoff.
> 
> Another way to go is 'git config alias.co', which should provide the
> same answer. I think that either would be fine.

The latter seems much more robust, since that will also tell you
precisely whether co is an alias at all, and you don't have to parse
-h/--help output (stripping out the 'is aliased to...' stuff, which
might be complicated by i18n etc. etc.). So I don't think we should
worry too much about scripted use of -h/--help.

Rasmus
Rasmus Villemoes Sept. 28, 2018, 8:18 a.m. UTC | #14
On 2018-09-26 20:49, Jeff King wrote:
> On Wed, Sep 26, 2018 at 08:16:36AM -0700, Junio C Hamano wrote:
> 
>>
>> If we expect users to use "git cp --help" a lot more often than "git
>> help cp" (or the other way around), one way to give a nicer experience
>> may be to unconditionally make "git cp --help" to directly show the
>> manpage of cherry-pick, while keeping "git help cp" to never do
>> that.  Then those who want to remember what "co" is aliased to can
>> ask "git help co".
> 
> I like that direction much better. I also wondered if we could leverage
> the "-h" versus "--help" distinction. The problem with printing the
> alias definition along with "--help" is that the latter will start a
> pager that obliterates what we wrote before (and hence all of this delay
> trickery).
> 
> But for "-h" we generally expect the command to output a usage message.
> 
> So what if the rules were:
> 
>   - "git help cp" shows "cp is an alias for cherry-pick" (as it does
>     now)

Sounds good.

>   - "git cp -h" shows "cp is an alias for cherry-pick", followed by
>     actually running "cherry-pick -h", which will show the usage
>     message. For a single-word command that does very little, since the
>     usage message starts with "cherry-pick". But if your alias is
>     actually "cp = cherry-pick -n", then it _is_ telling you extra
>     information.

Funny, I never noticed this difference, and that '-h' for an alias would
actually give more information than '--help'. I sort-of knew that -h
would give the synopsis, so I guess I've just gotten used to always use
--help, and just noticed that for aliases that doesn't provide much help.

Adding the 'is an alias for' info to -h sounds quite sensible.

And this could even work with "!" aliases: we define
>     it, and then it is up to the alias to handle "-h" sensibly.

I'd be nervous about doing this, though, especially if we introduce this
without a new opt-in config option (which seems to be the direction the
discussion is taking). There are lots of commands that don't respond
with a help message to -h, or that only recognize -h as the first word,
or... There are really too many ways this could cause headaches.

But, now that I test it, it seems that we already let the alias handle
-h (and any other following words, with --help as the first word
special-cased). So what you're suggesting is (correct me if I'm wrong)
to _also_ intercept -h as the first word, and then print the alias info,
in addition to spawning the alias with the entire argv as usual. The
alias info would probably need to go to stderr in this case.

>   - "git cp --help" opens the manpage for cherry-pick. We don't bother
>     with the alias definition, as it's available through other means
>     (and thus we skip the obliteration/timing thing totally).

It sounds like you suggest doing this unconditionally, and without any
opt-in via config option or a short wait? That would certainly work for
me. It is, in fact, how I expect 'git cp --help' to work, until I get
reminded that it does not... Also, as Junio noted, is consistent with
--help generally providing more information than -h - except that one
loses the 'is an alias for' part for --help.

>     This really only works for non-! aliases. Those would continue to
>     show the alias definition.

Yes.

Thanks,
Rasmus
Junio C Hamano Sept. 28, 2018, 5 p.m. UTC | #15
Rasmus Villemoes <rv@rasmusvillemoes.dk> writes:

>>> +		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;
>> 
>> If you have "[alias] cp = cherry-pick -n", split_cmdline discards
>> "-n" and the follow-alias prompt does not even tell you that it did
>> so,
>
> That's not really true, as I deliberately did the split_cmdline after
> printing the "is an alias for", but before "continuing to help for", so
> this would precisely tell you
>
>   cp is an alias for 'cherry-pick -n'
>   continuing to help for 'cherry-pick' in 1.5 seconds

Yes, but notice that cherry-pick appears twice---I do not know about
you, but I know at least my eyes will be drawn to the last mention
that does not have '-n' stronger than the one before/above that
line.

In any case, I think Peff's "Let's teach 'git cp -h' to prefix what
'cp' is aliased to before invoking 'git cherry-pick -n -h' (and let
it fail)" approach is much more robust, so let's do that without
emulating that command-typo-correction codepath.
Jeff King Sept. 29, 2018, 8:21 a.m. UTC | #16
On Fri, Sep 28, 2018 at 10:18:05AM +0200, Rasmus Villemoes wrote:

> >     it, and then it is up to the alias to handle "-h" sensibly.
> 
> I'd be nervous about doing this, though, especially if we introduce this
> without a new opt-in config option (which seems to be the direction the
> discussion is taking). There are lots of commands that don't respond
> with a help message to -h, or that only recognize -h as the first word,
> or... There are really too many ways this could cause headaches.
> 
> But, now that I test it, it seems that we already let the alias handle
> -h (and any other following words, with --help as the first word
> special-cased). So what you're suggesting is (correct me if I'm wrong)
> to _also_ intercept -h as the first word, and then print the alias info,
> in addition to spawning the alias with the entire argv as usual. The
> alias info would probably need to go to stderr in this case.

Right, I'm proposing only to add the extra message and then continue as
usual.

It is a little funny, I guess, if you have a script which doesn't
respond to "-h", because you'd get our "foo is aliased to git-bar"
message to stderr, followed by who-knows-what. But as long as it's to
stderr (and not stdout), I think it's not likely to _break_ anything.

> >   - "git cp --help" opens the manpage for cherry-pick. We don't bother
> >     with the alias definition, as it's available through other means
> >     (and thus we skip the obliteration/timing thing totally).
> 
> It sounds like you suggest doing this unconditionally, and without any
> opt-in via config option or a short wait? That would certainly work for
> me. It is, in fact, how I expect 'git cp --help' to work, until I get
> reminded that it does not... Also, as Junio noted, is consistent with
> --help generally providing more information than -h - except that one
> loses the 'is an alias for' part for --help.

Yes, I'd suggest doing it always. No config, no wait.

-Peff
Junio C Hamano Sept. 29, 2018, 5:39 p.m. UTC | #17
Jeff King <peff@peff.net> writes:

> Right, I'm proposing only to add the extra message and then continue as
> usual.
>
> It is a little funny, I guess, if you have a script which doesn't
> respond to "-h", because you'd get our "foo is aliased to git-bar"
> message to stderr, followed by who-knows-what. But as long as it's to
> stderr (and not stdout), I think it's not likely to _break_ anything.
>
>> >   - "git cp --help" opens the manpage for cherry-pick. We don't bother
>> >     with the alias definition, as it's available through other means
>> >     (and thus we skip the obliteration/timing thing totally).
>> 
>> It sounds like you suggest doing this unconditionally, and without any
>> opt-in via config option or a short wait? That would certainly work for
>> me. It is, in fact, how I expect 'git cp --help' to work, until I get
>> reminded that it does not... Also, as Junio noted, is consistent with
>> --help generally providing more information than -h - except that one
>> loses the 'is an alias for' part for --help.
>
> Yes, I'd suggest doing it always. No config, no wait.

While I do think your suggestion is the best among various ones
floated in the thread, I just realized there is one potential glitch
even with that approach.  

Suppose "git foo" is aliased to a command "git bar".

The best case is when "git bar -h" knows that it is asked to give us
a short usage.  We get "foo is aliased to bar" followed by the short
usage for "bar" and everything is visible above the shell prompt
after all that happens.

The second best case is when "git bar" simply does not support "-h"
but actively notices an unknown option on the command line to give
the usage message.  We see "foo is aliased to bar" followed by "-h
is an unknown option; supported options are ..." and everything is
visible above the shell prompt after all that happens.

The worst case is when "git bar" supports or ignores "-h" and
produces reams of output.  Sending the "aliased to" message to the
standard error means that it is scrolled out when the output is
done, or lost even when "git foo -h | less" attempts to let the
reader read before the early part of the output scrolls away.

Even the first two "better" cases share the same glitch if the "foo
is aliased to bar" goes to the standard error output.  Parse-options
enabled commands tend to show a long "-h" output that you would need
to say "git grep -h | less", losing the "aliased to" message.

At least it seems to me an improvement to use standard output,
instead of standard error, for the alias information.

In practice, however, what the command that "git foo" is aliased to
does when given "-h" is probably unknown (because the user is asking
what "git foo" is in the first place), so perhaps I am worried too
much.  When the user does not know if the usage text comes to the
standard output or to the standard error, and if the usage text is
very long or not, they probably would learn quickly that the safest
thing to do is to

	$ git unknown-command -h >&2 | less

And at that point, it does not matter which between the standard
output and the standard error streams we write "unknown-command is
aliased to ...".

So I dunno.
Jeff King Sept. 30, 2018, 4:27 a.m. UTC | #18
On Sat, Sep 29, 2018 at 10:39:54AM -0700, Junio C Hamano wrote:

> Suppose "git foo" is aliased to a command "git bar".
> 
> The best case is when "git bar -h" knows that it is asked to give us
> a short usage.  We get "foo is aliased to bar" followed by the short
> usage for "bar" and everything is visible above the shell prompt
> after all that happens.
> 
> The second best case is when "git bar" simply does not support "-h"
> but actively notices an unknown option on the command line to give
> the usage message.  We see "foo is aliased to bar" followed by "-h
> is an unknown option; supported options are ..." and everything is
> visible above the shell prompt after all that happens.

Right, these are the ones we hope for.

> The worst case is when "git bar" supports or ignores "-h" and
> produces reams of output.  Sending the "aliased to" message to the
> standard error means that it is scrolled out when the output is
> done, or lost even when "git foo -h | less" attempts to let the
> reader read before the early part of the output scrolls away.

This is the "who-knows-what" case I meant here:

>> It is a little funny, I guess, if you have a script which doesn't
>> respond to "-h", because you'd get our "foo is aliased to git-bar"
>> message to stderr, followed by who-knows-what. But as long as it's to
>> stderr (and not stdout), I think it's not likely to _break_ anything.

And I think this has to be stderr. We're polluting the output of the
aliased command with our extra message, so we have two choices:

  1. Pollute stderr, and risk copious stdout (or a pager) scrolling it
     off the screen.

  2. Pollute stdout, at which point our message may be confused as part
     of the actual output of the command (and that may not even be
     immediately noticed if it is passed through a shell pipeline or
     into a file).

Choice (2) seems like a regression to me. Choice (1) is unfortunate in
some cases, but is no worse than today's behavior.

(Obviously I'm not including choices like not running the sub-command at
all, but I think that would be even worse).

> Even the first two "better" cases share the same glitch if the "foo
> is aliased to bar" goes to the standard error output.  Parse-options
> enabled commands tend to show a long "-h" output that you would need
> to say "git grep -h | less", losing the "aliased to" message.
> 
> At least it seems to me an improvement to use standard output,
> instead of standard error, for the alias information.

...so I'd disagree with this.

> In practice, however, what the command that "git foo" is aliased to
> does when given "-h" is probably unknown (because the user is asking
> what "git foo" is in the first place), so perhaps I am worried too
> much.  When the user does not know if the usage text comes to the
> standard output or to the standard error, and if the usage text is
> very long or not, they probably would learn quickly that the safest
> thing to do is to
> 
> 	$ git unknown-command -h >&2 | less
> 
> And at that point, it does not matter which between the standard
> output and the standard error streams we write "unknown-command is
> aliased to ...".

Yeah. I think if "git foo -h" produces a bunch of output you didn't
expect, then "git help foo" or "git foo --help" may be the next thing
you reach for. That's not so different than running the command even
without any aliases involved.

-Peff
Junio C Hamano Sept. 30, 2018, 5:27 a.m. UTC | #19
Jeff King <peff@peff.net> writes:

> And I think this has to be stderr. We're polluting the output of the
> aliased command with our extra message, so we have two choices:
>
>   1. Pollute stderr, and risk copious stdout (or a pager) scrolling it
>      off the screen.
>
>   2. Pollute stdout, at which point our message may be confused as part
>      of the actual output of the command (and that may not even be
>      immediately noticed if it is passed through a shell pipeline or
>      into a file).
>
> Choice (2) seems like a regression to me. Choice (1) is unfortunate in
> some cases, but is no worse than today's behavior.

I think the output of "git foo -h" changing (i.e. has "aliased
to..."  message in front) is about the same degree of regression as
"git foo --help" no longer giving "aliased to..." information
anywhere, though.

>> Even the first two "better" cases share the same glitch if the "foo
>> ...
>> thing to do is to
>> 
>> 	$ git unknown-command -h >&2 | less
>> 
>> And at that point, it does not matter which between the standard
>> output and the standard error streams we write "unknown-command is
>> aliased to ...".
>
> Yeah. I think if "git foo -h" produces a bunch of output you didn't
> expect, then "git help foo" or "git foo --help" may be the next thing
> you reach for. That's not so different than running the command even
> without any aliases involved.

Hmmm.  With the "teach 'git foo -h' to output 'foo is aliased to
bar' to the standard error before running 'git bar -h'", plus "'git
foo --help' now goes straight to 'git bar --help'", "git foo --help"
no longer tells us that foo is aliased to bar.  Presumably "git help
foo" will still give "foo is bar" and stop?
Jeff King Sept. 30, 2018, 5:53 a.m. UTC | #20
On Sat, Sep 29, 2018 at 10:27:15PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > And I think this has to be stderr. We're polluting the output of the
> > aliased command with our extra message, so we have two choices:
> >
> >   1. Pollute stderr, and risk copious stdout (or a pager) scrolling it
> >      off the screen.
> >
> >   2. Pollute stdout, at which point our message may be confused as part
> >      of the actual output of the command (and that may not even be
> >      immediately noticed if it is passed through a shell pipeline or
> >      into a file).
> >
> > Choice (2) seems like a regression to me. Choice (1) is unfortunate in
> > some cases, but is no worse than today's behavior.
> 
> I think the output of "git foo -h" changing (i.e. has "aliased
> to..."  message in front) is about the same degree of regression as
> "git foo --help" no longer giving "aliased to..." information
> anywhere, though.

Hmm. They seem quite different to me. Changing "--help" output is
something that's only going to impact what the user sees (a manpage
versus the alias message). And the user can follow-up by asking for what
they wanted.

Whereas if I have an alias that currently understands "-h", and I do
something like:

  git foo -h | wc -l

if we output to stdout, that's going to produce subtly broken results.
But if we output to stderr instead, then they may see the extra message,
but it's obvious what's happening, and it's probably an annoyance at
worst).

> > Yeah. I think if "git foo -h" produces a bunch of output you didn't
> > expect, then "git help foo" or "git foo --help" may be the next thing
> > you reach for. That's not so different than running the command even
> > without any aliases involved.
> 
> Hmmm.  With the "teach 'git foo -h' to output 'foo is aliased to
> bar' to the standard error before running 'git bar -h'", plus "'git
> foo --help' now goes straight to 'git bar --help'", "git foo --help"
> no longer tells us that foo is aliased to bar.  Presumably "git help
> foo" will still give "foo is bar" and stop?

Yes, that was the intent in the behavior I laid out earlier.

-Peff
diff mbox series

Patch

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
+	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.
+
 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;
+	}
 	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);
+
+		/*
+		 * 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));
+
+		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;
 	}
 
 	if (exclude_guides)