From: Jeff King <peff@peff•net>
To: Kristoffer Haugsbakk <code@khaugsbakk•name>
Cc: Kristoffer Haugsbakk <kristofferhaugsbakk@fastmail•com>,
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: Thu, 11 Sep 2025 16:32:56 -0400 [thread overview]
Message-ID: <20250911203256.GA1894340@coredump.intra.peff.net> (raw)
In-Reply-To: <eefc3524-9ecf-44e5-a6ba-1b4a0aff3158@app.fastmail.com>
On Thu, Sep 11, 2025 at 07:31:04PM +0200, Kristoffer Haugsbakk wrote:
> But it turns out there is a regression in my code with the recursion
> detection. Compare:
>
> $ ./git -c alias.one=two -c alias.two=two one -h
> 'one' is aliased to 'two'
> 'two' is aliased to 'two'
> fatal: recursive alias: two
Your example there is a little funny; "two" is recursive to itself, even
without "one". We detect that case in handle_alias(), and we would still
detect:
git -c alias.whatchanged=whatchanged whatchanged -h
in the same spot. I assume what you meant was:
$ ./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 ==>
which is handled in the caller. And indeed...
> $ ./git -c alias.whatchanged=pack-redundant -c alias.pack-redundant=whatchanged whatchanged -h
> 'whatchanged' is aliased to 'pack-redundant'
> 'pack-redundant' is aliased to 'whatchanged'
> 'whatchanged' is aliased to 'pack-redundant'
> 'pack-redundant' is aliased to 'whatchanged'
> 'whatchanged' is aliased to 'pack-redundant'
> 'pack-redundant' is aliased to 'whatchanged'
> 'whatchanged' is aliased to 'pack-redundant'
> 'pack-redundant' is aliased to 'whatchanged'
> [forever]
...that cannot work with where we've placed the new conditional, because
it continues the loop before hitting the alias check. If we move that to
the top, like the patch below, it works.
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.
-Peff
---
diff --git a/git.c b/git.c
index ca66e24639..17c566a802 100644
--- a/git.c
+++ b/git.c
@@ -814,6 +814,24 @@ static int run_argv(struct strvec *args)
struct string_list_item *seen;
while (1) {
+ seen = unsorted_string_list_lookup(&cmd_list, args->v[0]);
+ if (seen) {
+ struct strbuf sb = STRBUF_INIT;
+ for (size_t i = 0; i < cmd_list.nr; i++) {
+ struct string_list_item *item = &cmd_list.items[i];
+
+ strbuf_addf(&sb, "\n %s", item->string);
+ if (item == seen)
+ strbuf_addstr(&sb, " <==");
+ else if (i == cmd_list.nr - 1)
+ strbuf_addstr(&sb, " ==>");
+ }
+ die(_("alias loop detected: expansion of '%s' does"
+ " not terminate:%s"), cmd_list.items[0].string, sb.buf);
+ }
+
+ string_list_append(&cmd_list, args->v[0]);
+
/*
* Allow deprecated commands to be overridden by aliases. This
* creates a seamless path forward for people who want to keep
@@ -874,24 +892,6 @@ static int run_argv(struct strvec *args)
/* .. then try the external ones */
execv_dashed_external(args->v);
- seen = unsorted_string_list_lookup(&cmd_list, args->v[0]);
- if (seen) {
- struct strbuf sb = STRBUF_INIT;
- for (size_t i = 0; i < cmd_list.nr; i++) {
- struct string_list_item *item = &cmd_list.items[i];
-
- strbuf_addf(&sb, "\n %s", item->string);
- if (item == seen)
- strbuf_addstr(&sb, " <==");
- else if (i == cmd_list.nr - 1)
- strbuf_addstr(&sb, " ==>");
- }
- die(_("alias loop detected: expansion of '%s' does"
- " not terminate:%s"), cmd_list.items[0].string, sb.buf);
- }
-
- string_list_append(&cmd_list, args->v[0]);
-
/*
* It could be an alias -- this works around the insanity
* of overriding "git log" with "git show" by having
next prev parent reply other threads:[~2025-09-11 20:33 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 [this message]
2025-09-11 20:43 ` Jeff King
2025-09-13 14:10 ` Kristoffer Haugsbakk
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=20250911203256.GA1894340@coredump.intra.peff.net \
--to=peff@peff$(echo .)net \
--cc=code@khaugsbakk$(echo .)name \
--cc=git@vger$(echo .)kernel.org \
--cc=kristofferhaugsbakk@fastmail$(echo .)com \
--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