On Wed, Sep 26, 2018 at 08:30:32AM -0700, Junio C Hamano wrote:
> Taylor Blau <me@xxxxxxxxxxxx> 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
