Re: [PATCH v2 1/3] help: redirect to aliased commands for "git cmd --help"

From: Jeff King <peff@peff.net>
To: Rasmus Villemoes <rv@rasmusvillemoes.dk>
Cc: git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>,
	Duy Nguyen <pclouds@gmail.com>, Taylor Blau <me@ttaylorr.com>
Subject: Re: [PATCH v2 1/3] help: redirect to aliased commands for "git cmd --help"
Date: Wed, 3 Oct 2018 03:06:45 -0400	[thread overview]
Message-ID: <20181003070645.GA6019@sigill.intra.peff.net> (raw)
In-Reply-To: <9ab3d69a-033a-e5a0-7459-c6ba8a2ec853@rasmusvillemoes.dk>

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