From: Kristoffer Haugsbakk <code@khaugsbakk.name>
Date: Wed, 10 Sep 2025 17:58:32 GMT
Subject: Re: [PATCH v4 2/7] git: allow alias-shadowing deprecated builtins
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:
>>[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.

> 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