[v2,1/3] help: redirect to aliased commands for "git cmd --help"
diff mbox series

Message ID 20181001112107.28956-1-rv@rasmusvillemoes.dk
State New
Headers show
Series
  • [v2,1/3] help: redirect to aliased commands for "git cmd --help"
Related show

Commit Message

Rasmus Villemoes Oct. 1, 2018, 11:21 a.m. UTC
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(-)

Comments

Jeff King Oct. 3, 2018, 2:13 a.m. UTC | #1
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
Rasmus Villemoes Oct. 3, 2018, 6:24 a.m. UTC | #2
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
Jeff King Oct. 3, 2018, 7:06 a.m. UTC | #3
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

Patch
diff mbox series

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)