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
