From: Jeff King <peff@peff.net>
Date: Wed, 10 Sep 2025 15:48:59 GMT
Subject: Re: [PATCH v4 2/7] git: allow alias-shadowing deprecated builtins
Message-ID: <20250910154859.GB562601@coredump.intra.peff.net>
In-Reply-To: <20250910051347.GA556174@coredump.intra.peff.net>

On Wed, Sep 10, 2025 at 01:13:47AM -0400, Jeff King wrote:

> On Tue, Sep 09, 2025 at 09:45:52PM +0200, kristofferhaugsbakk@fastmail.com wrote:
> 
> > +		if (is_deprecated_command(args->v[0]) &&
> > +		    alias_lookup(args->v[0])) {
> > +			if (!handle_alias(args))
> > +				break;
> > +			done_alias = 1;
> > +			continue;
> > +		}
> 
> I think this is failing the SANITIZE=leak jobs, because alias_lookup()
> returns an allocated string with the alias. You need to capture and free
> it, or introduce an alias_exists() wrapper to do so.
> 
> There might also be a way to do it just by calling handle_alias() and
> checking its return value (it seems to indicate whether an alias was
> found, but I didn't look through the whole function carefully, or think
> about the implications of how the done_alias flag works).

I _think_ this is correct:

diff --git a/git.c b/git.c
index 47857d280f..ea9ec77a88 100644
--- a/git.c
+++ b/git.c
@@ -823,9 +823,7 @@ static int run_argv(struct strvec *args)
 		 * deprecation complaint in the meantime.
 		 */
 		if (is_deprecated_command(args->v[0]) &&
-		    alias_lookup(args->v[0])) {
-			if (!handle_alias(args))
-				break;
+		    handle_alias(args)) {
 			done_alias = 1;
 			continue;
 		}

The handle_alias() function will either:

  - return 0 if it does not find an alias

  - return 1 if it finds an alias and we should loop again to run that
    command (but set done_alias so we know to suppress in-process
    builtins below). The "args" array will have been updated with the
    new command name.

  - exit itself if it's a "!" shell alias that we exec

So we just need to loop again when it told us that it found an alias (it
is tempting to just continue in the loop body, but that would miss the
case of a deprecated alias that resolves to another deprecated alias).

Anyway, I believe that is correct and solves the leak issue (because
handle_alias() does the lookup and takes care of cleanup itself).

-Peff