Re: [PATCH v4 2/7] git: allow alias-shadowing deprecated builtins

From: Kristoffer Haugsbakk <code@khaugsbakk.name>
Date: Sep 10, 2025, 17:58 UTC
Message-ID: <b39ba543-24ad-4bd6-be6e-dc1b8a416eb0@app.fastmail.com>
In-Reply-To: <20250910154859.GB562601@coredump.intra.peff.net>

On Wed, Sep 10, 2025, at 17:48, Jeff King wrote:

Show 9 quoted lines

>>[snip]
>> 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 snip]

Thank you. The best I could come up was making `alias.c:alias_exists`. Which must be redundant given the call to `handlke_alias()` that you mention later here.

Show 8 quoted lines

> 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.

This case and the `continue` seemed to make sense to me eventually. Not the `break` case though.

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

Yep.

> 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).

Yeah, that’s subtle.

Do you think this should be a test case?  I guess it doesn’t hurt.

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

Thanks again. My local test suite passes. (Except `t5801-remote-helpers.sh` which never works for me.)

And thanks for pointing this out to begin with.

-- 
Kristoffer