| Message ID | 20181001112107.28956-1-rv@rasmusvillemoes.dk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/3] help: redirect to aliased commands for "git cmd --help" | expand |
On Mon, Oct 01, 2018 at 01:21:05PM +0200, Rasmus Villemoes wrote: > As discussed in the thread for v1 of this patch [1] [2], this changes the > rules for "git foo --help" when foo is an alias. > > (0) When invoked as "git help foo", we continue to print the "foo is > aliased to bar" message and nothing else. > > (1) If foo is an alias for a shell command, print "foo is aliased to > !bar" as usual. > > (2) Otherwise, break the alias string into words, and pretend that "git > word0 --help" was called. > > At least for me, getting the man page for git-cherry-pick directly with > "git cp --help" is more useful (and how I expect an alias to behave) > than the short "is aliased to" notice. It is also consistent with > "--help" generally providing more comprehensive help than "-h". Makes sense. > I believe that printing the "is aliased to" message also in case (2) has > value: Depending on pager setup, or if the user has help.format=web, the > message is still present immediately above the prompt when the user > quits the pager/returns to the terminal. That serves as an explanation > for why one was redirected to "man git-cherry-pick" from "git cp > --help", and if cp is actually 'cherry-pick -n', it reminds the user > that using cp has some flag implicitly set before firing off the next > command. > > It also provides some useful info in case we end up erroring out, either > in the "bad alias string" check, or in the "No manual entry for gitbar" > case. OK, I buy that line of reasoning. And in the other cases, it shouldn't _hurt_ anything. > diff --git a/builtin/help.c b/builtin/help.c > index 8d4f6dd301..4802a06f37 100644 > --- a/builtin/help.c > +++ b/builtin/help.c > @@ -415,9 +415,29 @@ 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 we were invoked as "git help cmd", or cmd is an > + * alias for a shell command, we inform the user what > + * cmd is an alias for and do nothing else. > + */ > + if (!exclude_guides || alias[0] == '!') { > + printf_ln(_("'%s' is aliased to '%s'"), cmd, alias); > + free(alias); > + exit(0); > + } I'm not sure I understand why exclude_guides is relevant. We check it below when we know that we _don't_ have an alias. Hrm. I guess you're using it here as a proxy for "git foo --help" being used instead of "git help foo". The comment probably needs to spell out that exclude_guides is the same as your "we were invoked as...". I wonder if we could change the name of that option. It is an undocumented, hidden option that we use internally, so it should be OK to do so (or we could always add another one). That might prevent somebody in the future from using --exclude-guides in more places and breaking your assumption here. > + /* > + * Otherwise, we pretend that the command was "git > + * word0 --help. > + */ > + fprintf_ln(stderr, _("'%s' is aliased to '%s'"), cmd, alias); > + count = split_cmdline(alias, &argv); > + if (count < 0) > + die(_("bad alias.%s string: %s"), cmd, > + split_cmdline_strerror(count)); > + return alias; So we split only to find argv[0] here. But then we don't return it. That works because the split is done in place, meaning we must have inserted a NUL in alias. That's sufficiently subtle that it might be worth spelling it out in a comment. We don't need to free alias here as we do above, because we're passing it back. We should free argv, though, I think (not its elements, just the array itself). Unfortunately the caller is going to leak our returned "alias", but I'm not sure we can do much about it. I'm not overly concerned with the memory, but it is going to trigger leak-checkers (and we're trying to quiet them down, not go the other way). I think it may be OK to overlook that and just UNLEAK() it in cmd_help(). -Peff
On 2018-10-03 04:13, Jeff King wrote: >> + /* >> + * If we were invoked as "git help cmd", or cmd is an >> + * alias for a shell command, we inform the user what >> + * cmd is an alias for and do nothing else. >> + */ >> + if (!exclude_guides || alias[0] == '!') { >> + printf_ln(_("'%s' is aliased to '%s'"), cmd, alias); >> + free(alias); >> + exit(0); >> + } > > I'm not sure I understand why exclude_guides is relevant. We check it > below when we know that we _don't_ have an alias. Hrm. I guess you're > using it here as a proxy for "git foo --help" being used instead of "git > help foo". Exactly. Perhaps it's abusing the existing machinery, but I didn't know how else to distinguish the two cases, and didn't feel like introducing another way of passing on the exact same information. > The comment probably needs to spell out that exclude_guides > is the same as your "we were invoked as...". Will do. That will also make the string --exclude-guides (i.e., with a dash) appear in the comment, making it more likely to be found should anyone change when and how --exclude-guides is implied. > I wonder if we could change the name of that option. It is an > undocumented, hidden option that we use internally, so it should be OK > to do so (or we could always add another one). That might prevent > somebody in the future from using --exclude-guides in more places and > breaking your assumption here. Perhaps, but I think that's better left for a separate patch, if really necessary even with the expanded comment. >> + count = split_cmdline(alias, &argv); >> + if (count < 0) >> + die(_("bad alias.%s string: %s"), cmd, >> + split_cmdline_strerror(count)); >> + return alias; > > So we split only to find argv[0] here. But then we don't return it. That > works because the split is done in place, meaning we must have inserted > a NUL in alias. That's sufficiently subtle that it might be worth > spelling it out in a comment. OK, I actually had precisely + /* + * 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. + */ in v1, but thought it might be overly verbose. I'll put it back in. > We don't need to free alias here as we do above, because we're passing > it back. We should free argv, though, I think (not its elements, just > the array itself). Yeah, I thought about this, and removing free(argv) was the last thing I did before sending v1 - because we were going to leak alias anyway. I'm happy to put it back in, along with... > Unfortunately the caller is going to leak our returned "alias", [...] I think it may be OK to overlook > that and just UNLEAK() it in cmd_help(). ...this. Except I'd rather do the UNLEAK in check_git_cmd (the documentation does say "only from cmd_* functions or their direct helpers") to make it a more targeted annotation. Thanks, Rasmus
On Wed, Oct 03, 2018 at 08:24:14AM +0200, Rasmus Villemoes wrote: > > The comment probably needs to spell out that exclude_guides > > is the same as your "we were invoked as...". > > Will do. That will also make the string --exclude-guides (i.e., with a > dash) appear in the comment, making it more likely to be found should > anyone change when and how --exclude-guides is implied. OK. I can live with that. > > So we split only to find argv[0] here. But then we don't return it. That > > works because the split is done in place, meaning we must have inserted > > a NUL in alias. That's sufficiently subtle that it might be worth > > spelling it out in a comment. > > OK, I actually had precisely > > + /* > + * 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. > + */ > > in v1, but thought it might be overly verbose. I'll put it back in. :) That's perfect. > > We don't need to free alias here as we do above, because we're passing > > it back. We should free argv, though, I think (not its elements, just > > the array itself). > > Yeah, I thought about this, and removing free(argv) was the last thing I > did before sending v1 - because we were going to leak alias anyway. I'm > happy to put it back in, along with... Thanks. I think this is different than "alias" because we really do leak it _here_, whereas alias lives on and can be UNLEAKed later. > > Unfortunately the caller is going to leak our returned "alias", [...] I think it may be OK to overlook > > that and just UNLEAK() it in cmd_help(). > > ...this. Except I'd rather do the UNLEAK in check_git_cmd (the > documentation does say "only from cmd_* functions or their direct > helpers") to make it a more targeted annotation. Yeah, I think that's fine. Thanks! -Peff
diff --git a/builtin/help.c b/builtin/help.c index 8d4f6dd301..4802a06f37 100644 --- a/builtin/help.c +++ b/builtin/help.c @@ -415,9 +415,29 @@ 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 we were invoked as "git help cmd", or cmd is an + * alias for a shell command, we inform the user what + * cmd is an alias for and do nothing else. + */ + if (!exclude_guides || alias[0] == '!') { + printf_ln(_("'%s' is aliased to '%s'"), cmd, alias); + free(alias); + exit(0); + } + /* + * Otherwise, we pretend that the command was "git + * word0 --help. + */ + fprintf_ln(stderr, _("'%s' is aliased to '%s'"), cmd, alias); + count = split_cmdline(alias, &argv); + if (count < 0) + die(_("bad alias.%s string: %s"), cmd, + split_cmdline_strerror(count)); + return alias; } if (exclude_guides)
As discussed in the thread for v1 of this patch [1] [2], this changes the rules for "git foo --help" when foo is an alias. (0) When invoked as "git help foo", we continue to print the "foo is aliased to bar" message and nothing else. (1) If foo is an alias for a shell command, print "foo is aliased to !bar" as usual. (2) Otherwise, break the alias string into words, and pretend that "git word0 --help" was called. At least for me, getting the man page for git-cherry-pick directly with "git cp --help" is more useful (and how I expect an alias to behave) than the short "is aliased to" notice. It is also consistent with "--help" generally providing more comprehensive help than "-h". I believe that printing the "is aliased to" message also in case (2) has value: Depending on pager setup, or if the user has help.format=web, the message is still present immediately above the prompt when the user quits the pager/returns to the terminal. That serves as an explanation for why one was redirected to "man git-cherry-pick" from "git cp --help", and if cp is actually 'cherry-pick -n', it reminds the user that using cp has some flag implicitly set before firing off the next command. It also provides some useful info in case we end up erroring out, either in the "bad alias string" check, or in the "No manual entry for gitbar" case. [1] https://public-inbox.org/git/20180926102636.30691-1-rv@rasmusvillemoes.dk/ [2] https://public-inbox.org/git/20180926184914.GC30680@sigill.intra.peff.net/ Signed-off-by: Rasmus Villemoes <rv@rasmusvillemoes.dk> --- builtin/help.c | 26 +++++++++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-)