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

From: "Kristoffer Haugsbakk" <[email-masked]>
Date: Sat, 13 Sep 2025 16:10:57 +0200

On Thu, Sep 11, 2025, at 22:43, Jeff King wrote:
> On Thu, Sep 11, 2025 at 04:32:56PM -0400, Jeff King wrote:
>
>> It is a little funny to do it up front even when we're going to directly
>> run a builtin, but I don't think it harms much. OTOH I find the whole
>> placement a little odd in the first place. We are speculatively adding
>> to the cmd list before even finding out if we have an alias. It kind of
>> feels like this should be part of handle_alias() in the first place.
>> We'd need to pass in the cmd_list variable for it to add to and check.
>
> IOW, something like this (perhaps as a preparatory patch):
>[snip diff]
> And then along with that:
>
>   - a new test at least of the looping deprecated case (we have an
>     existing test for non-deprecated looping aliases)
>
>   - while we are moving all this code, I might consider calling cmd_list
>     something more obvious like "expanded_aliases" or something.
>
>   - the placement above puts it next to the direct-recursion check in
>     handle_alias(). But it does change the output a bit, since we now do
>     the check after the "-h" expansion. So Before we'd print:
>
>       $ git -c alias.one=two -c alias.two=one one -h
>       'one' is aliased to 'two'
>       'two' is aliased to 'one'
>       fatal: alias loop detected: expansion of 'one' does not terminate:
>       one <==
>       two ==>
>
>     And now we'll print:
>
>       $ ./git -c alias.one=two -c alias.two=one one -h
>       'one' is aliased to 'two'
>       'two' is aliased to 'one'
>       'one' is aliased to 'two'
>       fatal: alias loop detected: expansion of 'one' does not terminate:
>         one <==
>         two ==>
>
>     with one extra "-h" line. Probably not a big deal, but that perhaps
>     points to the fact that we could be detecting the cycle one loop
>     sooner. I.e., as soon as we see that "two" points to "one" we know
>     we have cycled. We could probably be checking new_argv there
>     instead, after adding the current name to cmd_list (and in fact that
>     would catch the direct-recursion case automatically and we would not
>     need to do so manually, though maybe people prefer the more specific
>     message it produces).
>
>     I'll leave that as an exercise for the reader. ;)

That’s great, thanks.  I’ve added it as a preparatory patch/commit with
this message.

-----

git: move seen-command bookkeeping into handle_alias(...)

We are about to complicate the command handling by allowing *deprecated*
builtins to be shadowed by aliases.  We need to organize the code in
order to facilitate that.[1]

The code in the `while(1)` speculatively adds commands to the list
before finding out if it’s an alias.  Let’s instead move it inside
`handle_alias(...)` and in turn only run this logic when we have found
an alias.

This is not a refactor since the error output is changed; we will
now print

    '<cmd1>' is aliased to '<cmd2>'

while iterating in addition to the final `fatal` message.

[1]: We will do that by an additional call to `handle_alias(1)` inside
    the loop.  *Not* moving this code leaves a blind spot; we still
    detect regular alias looping/recursion but we miss it when it is
    done using deprecated builtins.

-- 
Kris