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:
diff --git a/git.c b/git.c
index 06de0bacf3..f11ce416a3 100644
--- a/git.c
+++ b/git.c
@@ -425,7 +425,9 @@ static int handle_alias(struct strvec *args, struct string_list *cmd_list)
if (!strcmp(alias_command, new_argv[0]))
die(_("recursive alias: %s"), alias_command);
- seen = unsorted_string_list_lookup(cmd_list, args->v[0]);
+ string_list_append(cmd_list, args->v[0]);
+
+ seen = unsorted_string_list_lookup(cmd_list, new_argv[0]);
if (seen) {
struct strbuf sb = STRBUF_INIT;
for (size_t i = 0; i < cmd_list->nr; i++) {
@@ -441,8 +443,6 @@ static int handle_alias(struct strvec *args, struct string_list *cmd_list)
" not terminate:%s"), cmd_list->items[0].string, sb.buf);
}
- string_list_append(cmd_list, args->v[0]);
-
trace_argv_printf(new_argv,
"trace: alias expansion: %s =>",
alias_command);
It is important to move the append of args->v[0] up (though arguably it
should be alias_command here, which is a local-variable alias that the
rest of the function uses). Adding it up-front is not needed for loop
detection, but it is shown as part of the output when we do find a loop.
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 <==
Though perhaps if the output code marked it as both the start and end of
the loop it would be more sensible. Maybe like:
diff --git a/git.c b/git.c
index f11ce416a3..f1a83f2e6a 100644
--- a/git.c
+++ b/git.c
@@ -422,9 +422,6 @@ static int handle_alias(struct strvec *args, struct string_list *cmd_list)
if (count < 1)
die(_("empty alias for %s"), alias_command);
- if (!strcmp(alias_command, new_argv[0]))
- die(_("recursive alias: %s"), alias_command);
-
string_list_append(cmd_list, args->v[0]);
seen = unsorted_string_list_lookup(cmd_list, new_argv[0]);
@@ -436,8 +433,8 @@ static int handle_alias(struct strvec *args, struct string_list *cmd_list)
strbuf_addf(&sb, "\n %s", item->string);
if (item == seen)
strbuf_addstr(&sb, " <==");
- else if (i == cmd_list->nr - 1)
- strbuf_addstr(&sb, " ==>");
+ if (i == cmd_list->nr - 1)
+ strbuf_addstr(&sb, item == seen ? ">" : " ==>");
}
die(_("alias loop detected: expansion of '%s' does"
" not terminate:%s"), cmd_list->items[0].string, sb.buf);
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.
-Peff
