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

From: "Kristoffer Haugsbakk" <[email-masked]>
Date: Sun, 14 Sep 2025 19:24:14 +0200

On Sun, Sep 14, 2025, at 00:06, Jeff King wrote:
> On Sat, Sep 13, 2025 at 04:10:57PM +0200, Kristoffer Haugsbakk wrote:
>
>> 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.
>
> If you want to get rid of that last paragraph, I think it really is as
> simple as checking the expanded alias new_argv[0] as soon as we see it,
> like:

I tried using both of these changes (the patches) but the `alias...`
test suite started failing.

>[snip]
> You could _also_ ditch the "recursive alias" check above at that point,
> though I think it produces a slightly nicer message:
>
>   $ ./git -c alias.foo=foo foo
>   fatal: recursive alias: foo
>
> vs:
>
>   $ ./git -c alias.foo=foo foo
>   fatal: alias loop detected: expansion of 'foo' does not terminate:
>     foo <==

I didn’t try these.

>[snip]
> I dunno. Maybe it is not worth tinkering with too much. But the "this
> changes the output" justification from your proposed message seems to me
> an indication that the refactor is a little iffy.

I ended up with not changing it for v5.  I missed the first time around
that this informational message is only “logged” in the specific case of
`<git cmd> -h`.  In turn you only get one more line of output when you
are (1) chaining deprecated aliases, and (2) making a loop.

v5 still mentions this but I try to put it off to the side, in other
words a footnote.

Thanks