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

From: Taylor Blau <me@ttaylorr.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: Taylor Blau <me@ttaylorr.com>,
	Rasmus Villemoes <rv@rasmusvillemoes.dk>,
	git@vger.kernel.org
Subject: Re: [PATCH] help: allow redirecting to help for aliased command
Date: Wed, 26 Sep 2018 11:09:11 -0700	[thread overview]
Message-ID: <20180926180911.GA63889@syl> (raw)
In-Reply-To: <xmqqtvmcmg2v.fsf@gitster-ct.c.googlers.com>

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