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):
diff --git a/git.c b/git.c
index ca66e24639..06de0bacf3 100644
--- a/git.c
+++ b/git.c
@@ -365,7 +365,7 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
return (*argv) - orig_argv;
}
-static int handle_alias(struct strvec *args)
+static int handle_alias(struct strvec *args, struct string_list *cmd_list)
{
int envchanged = 0, ret = 0, saved_errno = errno;
int count, option_count;
@@ -376,6 +376,8 @@ static int handle_alias(struct strvec *args)
alias_command = args->v[0];
alias_string = alias_lookup(alias_command);
if (alias_string) {
+ struct string_list_item *seen;
+
if (args->nr == 2 && !strcmp(args->v[1], "-h"))
fprintf_ln(stderr, _("'%s' is aliased to '%s'"),
alias_command, alias_string);
@@ -423,6 +425,24 @@ static int handle_alias(struct strvec *args)
if (!strcmp(alias_command, new_argv[0]))
die(_("recursive alias: %s"), alias_command);
+ 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]);
+
trace_argv_printf(new_argv,
"trace: alias expansion: %s =>",
alias_command);
@@ -811,7 +831,6 @@ static int run_argv(struct strvec *args)
{
int done_alias = 0;
struct string_list cmd_list = STRING_LIST_INIT_DUP;
- struct string_list_item *seen;
while (1) {
/*
@@ -821,7 +840,7 @@ static int run_argv(struct strvec *args)
* deprecation complaint in the meantime.
*/
if (is_deprecated_command(args->v[0]) &&
- handle_alias(args)) {
+ handle_alias(args, &cmd_list)) {
done_alias = 1;
continue;
}
@@ -874,30 +893,12 @@ 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
* alias.log = show
*/
- if (!handle_alias(args))
+ if (!handle_alias(args, &cmd_list))
break;
done_alias = 1;
}
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. ;)
-Peff
