From: "Kristoffer Haugsbakk" <kristofferhaugsbakk@fastmail•com>
To: "Jeff King" <peff@peff•net>,
"Kristoffer Haugsbakk" <code@khaugsbakk•name>
Cc: git@vger•kernel.org, "Eric Sunshine" <sunshine@sunshineco•com>,
"Patrick Steinhardt" <ps@pks•im>
Subject: Re: [PATCH v4 2/7] git: allow alias-shadowing deprecated builtins
Date: Sat, 13 Sep 2025 16:10:57 +0200 [thread overview]
Message-ID: <98d206ca-3dc1-42cc-9f01-e3241d5a24af@app.fastmail.com> (raw)
In-Reply-To: <20250911204302.GA1907101@coredump.intra.peff.net>
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
next prev parent reply other threads:[~2025-09-13 14:11 UTC|newest]
Thread overview: 102+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-08-27 16:29 [PATCH 0/4] you-still-use-that??: improve breaking changes troubleshooting kristofferhaugsbakk
2025-08-27 16:29 ` [PATCH 1/4] usage: help the user help themselves kristofferhaugsbakk
2025-08-27 20:36 ` Kristoffer Haugsbakk
2025-08-27 21:02 ` Eric Sunshine
2025-08-27 21:20 ` Junio C Hamano
2025-08-27 21:27 ` Eric Sunshine
2025-08-27 22:26 ` Junio C Hamano
2025-08-28 6:39 ` Kristoffer Haugsbakk
2025-08-28 15:09 ` Junio C Hamano
2025-09-03 16:50 ` Eric Sunshine
2025-09-03 17:53 ` Kristoffer Haugsbakk
2025-09-03 21:21 ` Eric Sunshine
2025-09-03 21:41 ` Kristoffer Haugsbakk
2025-09-03 21:44 ` Jeff King
2025-09-03 22:11 ` Eric Sunshine
2025-09-04 6:57 ` Kristoffer Haugsbakk
2025-09-05 13:11 ` Jeff King
2025-09-09 20:01 ` Kristoffer Haugsbakk
2025-08-27 21:14 ` Junio C Hamano
2025-08-27 16:29 ` [PATCH 2/4] whatchanged: tell users the git-log(1) equivalent kristofferhaugsbakk
2025-08-27 16:45 ` Junio C Hamano
2025-08-27 16:48 ` Junio C Hamano
2025-08-27 17:08 ` Kristoffer Haugsbakk
2025-08-28 12:07 ` Kristoffer Haugsbakk
2025-08-27 16:29 ` [PATCH 3/4] whatchanged: remove not-even-shorter clause kristofferhaugsbakk
2025-08-27 16:29 ` [PATCH 4/4] BreakingChanges: remove claim about whatchanged reports kristofferhaugsbakk
2025-08-27 16:43 ` [PATCH 0/4] you-still-use-that??: improve breaking changes troubleshooting Junio C Hamano
2025-08-29 15:21 ` [PATCH v2 " kristofferhaugsbakk
2025-08-29 15:21 ` [PATCH v2 1/4] you-still-use-that??: help the user help themselves kristofferhaugsbakk
2025-08-29 15:21 ` [PATCH v2 2/4] whatchanged: tell users the git-log(1) equivalent kristofferhaugsbakk
2025-08-29 15:21 ` [PATCH v2 3/4] whatchanged: remove not-even-shorter clause kristofferhaugsbakk
2025-08-29 15:21 ` [PATCH v2 4/4] BreakingChanges: remove claim about whatchanged reports kristofferhaugsbakk
2025-09-08 15:36 ` [PATCH v3 0/8] you-still-use-that??: improve breaking changes troubleshooting kristofferhaugsbakk
2025-09-08 15:36 ` [PATCH v3 1/8] git: add `deprecated` category to --list-cmds kristofferhaugsbakk
2025-09-09 6:43 ` Patrick Steinhardt
2025-09-09 8:02 ` Kristoffer Haugsbakk
2025-09-08 15:36 ` [PATCH v3 2/8] git: make the two loops look more symmetric kristofferhaugsbakk
2025-09-08 15:36 ` [PATCH v3 3/8] git: allow alias-shadowing deprecated builtins kristofferhaugsbakk
2025-09-08 20:47 ` Junio C Hamano
2025-09-08 21:11 ` Jeff King
2025-09-08 21:55 ` Junio C Hamano
2025-09-09 6:25 ` Kristoffer Haugsbakk
2025-09-08 15:36 ` [PATCH v3 4/8] t0014: test shadowing of aliases for a sample of builtins kristofferhaugsbakk
2025-09-08 15:36 ` [PATCH v3 5/8] you-still-use-that??: help the user help themselves kristofferhaugsbakk
2025-09-08 15:36 ` [PATCH v3 6/8] whatchanged: tell users the git-log(1) equivalent kristofferhaugsbakk
2025-09-08 15:36 ` [PATCH v3 7/8] whatchanged: remove not-even-shorter clause kristofferhaugsbakk
2025-09-08 15:36 ` [PATCH v3 8/8] BreakingChanges: remove claim about whatchanged reports kristofferhaugsbakk
2025-09-09 19:45 ` [PATCH v4 0/7] you-still-use-that??: improve breaking changes troubleshooting kristofferhaugsbakk
2025-09-09 19:45 ` [PATCH v4 1/7] git: add `deprecated` category to --list-cmds kristofferhaugsbakk
2025-09-09 21:44 ` Junio C Hamano
2025-09-10 11:41 ` Patrick Steinhardt
2025-09-10 15:50 ` Jeff King
2025-09-10 21:40 ` Junio C Hamano
2025-09-10 20:23 ` Kristoffer Haugsbakk
2025-09-09 19:45 ` [PATCH v4 2/7] git: allow alias-shadowing deprecated builtins kristofferhaugsbakk
2025-09-10 5:13 ` Jeff King
2025-09-10 15:48 ` Jeff King
2025-09-10 17:58 ` Kristoffer Haugsbakk
2025-09-10 18:34 ` Jeff King
2025-09-11 17:31 ` Kristoffer Haugsbakk
2025-09-11 20:32 ` Jeff King
2025-09-11 20:43 ` Jeff King
2025-09-13 14:10 ` Kristoffer Haugsbakk [this message]
2025-09-13 22:06 ` Jeff King
2025-09-14 17:24 ` Kristoffer Haugsbakk
2025-09-15 1:44 ` Jeff King
2025-09-15 6:27 ` Kristoffer Haugsbakk
2025-09-11 20:44 ` Jeff King
2025-09-11 21:19 ` Junio C Hamano
2025-09-13 21:50 ` Kristoffer Haugsbakk
2025-09-09 19:45 ` [PATCH v4 3/7] t0014: test shadowing of aliases for a sample of builtins kristofferhaugsbakk
2025-09-09 19:45 ` [PATCH v4 4/7] you-still-use-that??: help the user help themselves kristofferhaugsbakk
2025-09-09 19:45 ` [PATCH v4 5/7] whatchanged: tell users the git-log(1) equivalent kristofferhaugsbakk
2025-09-09 19:45 ` [PATCH v4 6/7] whatchanged: remove not-even-shorter clause kristofferhaugsbakk
2025-09-09 19:45 ` [PATCH v4 7/7] BreakingChanges: remove claim about whatchanged reports kristofferhaugsbakk
2025-09-14 19:49 ` [PATCH v5 0/8] you-still-use-that??: improve breaking changes troubleshooting kristofferhaugsbakk
2025-09-14 19:49 ` [PATCH v5 1/8] git: add `deprecated` category to --list-cmds kristofferhaugsbakk
2025-09-14 19:49 ` [PATCH v5 2/8] git: move seen-alias bookkeeping into handle_alias(...) kristofferhaugsbakk
2025-09-14 19:49 ` [PATCH v5 3/8] git: allow alias-shadowing deprecated builtins kristofferhaugsbakk
2025-09-14 19:49 ` [PATCH v5 4/8] t0014: test shadowing of aliases for a sample of builtins kristofferhaugsbakk
2025-09-14 19:49 ` [PATCH v5 5/8] you-still-use-that??: help the user help themselves kristofferhaugsbakk
2025-09-14 19:49 ` [PATCH v5 6/8] whatchanged: hint about git-log(1) and aliasing kristofferhaugsbakk
2025-09-14 19:49 ` [PATCH v5 7/8] whatchanged: remove not-even-shorter clause kristofferhaugsbakk
2025-09-14 19:49 ` [PATCH v5 8/8] BreakingChanges: remove claim about whatchanged reports kristofferhaugsbakk
2025-09-15 19:19 ` [PATCH v5 0/8] you-still-use-that??: improve breaking changes troubleshooting Junio C Hamano
2025-09-16 20:47 ` Kristoffer Haugsbakk
2025-09-16 23:24 ` Jeff King
2025-09-17 15:41 ` Kristoffer Haugsbakk
2025-09-17 16:25 ` Junio C Hamano
2025-09-17 20:24 ` [PATCH v6 0/9] " kristofferhaugsbakk
2025-09-17 20:24 ` [PATCH v6 1/9] Makefile: don’t add whatchanged after it has been removed kristofferhaugsbakk
2025-09-17 20:24 ` [PATCH v6 2/9] git: add `deprecated` category to --list-cmds kristofferhaugsbakk
2025-09-17 20:24 ` [PATCH v6 3/9] git: move seen-alias bookkeeping into handle_alias(...) kristofferhaugsbakk
2025-09-17 20:24 ` [PATCH v6 4/9] git: allow alias-shadowing deprecated builtins kristofferhaugsbakk
2025-09-17 20:24 ` [PATCH v6 5/9] t0014: test shadowing of aliases for a sample of builtins kristofferhaugsbakk
2025-09-17 20:24 ` [PATCH v6 6/9] you-still-use-that??: help the user help themselves kristofferhaugsbakk
2025-09-17 20:24 ` [PATCH v6 7/9] whatchanged: hint about git-log(1) and aliasing kristofferhaugsbakk
2025-09-17 20:24 ` [PATCH v6 8/9] whatchanged: remove not-even-shorter clause kristofferhaugsbakk
2025-09-17 20:24 ` [PATCH v6 9/9] BreakingChanges: remove claim about whatchanged reports kristofferhaugsbakk
2025-09-18 18:31 ` [PATCH v6 0/9] you-still-use-that??: improve breaking changes troubleshooting Jeff King
2025-09-18 20:21 ` Junio C Hamano
2025-09-18 20:28 ` Kristoffer Haugsbakk
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=98d206ca-3dc1-42cc-9f01-e3241d5a24af@app.fastmail.com \
--to=kristofferhaugsbakk@fastmail$(echo .)com \
--cc=code@khaugsbakk$(echo .)name \
--cc=git@vger$(echo .)kernel.org \
--cc=peff@peff$(echo .)net \
--cc=ps@pks$(echo .)im \
--cc=sunshine@sunshineco$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox