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

[Junio C Hamano <gitster@pobox.com>]

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.