On Thu, Sep 06 2018, Tim Schumacher wrote: > Aliases can only contain non-alias git commands and their > arguments, not other user-defined aliases. Resolving further > (nested) aliases is prevented by breaking the loop after the > first alias was processed. Git then fails with a command-not-found > error. > > Allow resolving nested aliases by not breaking the loop in > run_argv() after the first alias was processed. Instead, continue > incrementing `done_alias` until `handle_alias()` fails, which means that > there are no further aliases that can be processed. Prevent looping > aliases by storing substituted commands in `cmd_list` and checking if > a command has been substituted previously. > > While we're at it, fix a styling issue just below the added code. > --- > git.c | 15 +++++++++++---- > 1 file changed, 11 insertions(+), 4 deletions(-) > > diff --git a/git.c b/git.c > index c27c38738..64f5fbd57 100644 > --- a/git.c > +++ b/git.c > @@ -674,6 +674,7 @@ static void execv_dashed_external(const char **argv) > static int run_argv(int *argcp, const char ***argv) > { > int done_alias = 0; > + struct string_list cmd_list = STRING_LIST_INIT_NODUP; > > while (1) { > /* > @@ -691,17 +692,23 @@ static int run_argv(int *argcp, const char ***argv) > /* .. then try the external ones */ > execv_dashed_external(*argv); > > - /* It could be an alias -- this works around the insanity > + if (string_list_has_string(&cmd_list, *argv[0])) > + die(_("loop alias: %s is called twice"), *argv[0]); > + > + string_list_append(&cmd_list, *argv[0]); > + > + /* > + * It could be an alias -- this works around the insanity > * of overriding "git log" with "git show" by having > * alias.log = show > */ > - if (done_alias) > - break; > if (!handle_alias(argcp, argv)) > break; > - done_alias = 1; > + done_alias++; > } > > + string_list_clear(&cmd_list, 0); > + > return done_alias; > } [In my just-sent https://public-inbox.org/git/87r2i6rbiy.fsf@xxxxxxxxxxxxxxxxxxx/ I should have said "the v3 thread"] Thanks for working on this, comments: If we don't have some test for these sort of aliasing loops that fails now, we really should add that in a 1/2 and fix it in this patch in 2/2. This error reporting is quite bad, consider: [alias] foo = bar bar = baz baz = foo We then say: $ ./git --exec-path=$PWD foo fatal: loop alias: bar is called twice That makes sense from an implementaion perspective, i.e. we lookup "bar" twice. But let's do better. If I have aliase like: a = b b = c c = d d = e e = c It should be telling me that my "e" expansion looped back to the "c = d" expansion. Here's a patch to implement that, feel free to either squash it in with my Signed-Off-By, or tacked onto a v4 version of this, whichever you think makes sense: diff --git a/git.c b/git.c index 64f5fbd572..38f1033e52 100644 --- a/git.c +++ b/git.c @@ -692,8 +692,64 @@ static int run_argv(int *argcp, const char ***argv) /* .. then try the external ones */ execv_dashed_external(*argv); - if (string_list_has_string(&cmd_list, *argv[0])) - die(_("loop alias: %s is called twice"), *argv[0]); + if (string_list_has_string(&cmd_list, *argv[0])) { + struct strbuf sb = STRBUF_INIT; + int i, seen_at_idx = -1; + + /* + * Find the re-entry point for the alias + * loop. TODO: There really should be a + * "return the index of the first matching" + * helper in string-list.c. + */ + for (i = 0; i < cmd_list.nr; i++) { + if (!strcmp(*argv[0], cmd_list.items[i].string)) + seen_at_idx = i; + } + assert(seen_at_idx != -1); + + for (i = 1; i < cmd_list.nr; i++) { + if (i - 1 == seen_at_idx) + /* + * TRANSLATORS: This is a the + * re-enttry point in the list + * printed out by the "alias + * loop" message below. + */ + strbuf_addf(&sb, _(" %d. %s = %s <== The re-entry point in the loop\n"), + i, + cmd_list.items[i - 1].string, + cmd_list.items[i].string); + else + /* + * TRANSLATORS: This is a + * single item in the list + * printed out by the "alias + * loop" message below. + */ + strbuf_addf(&sb, _(" %d. %s = %s\n"), + i, + cmd_list.items[i - 1].string, + cmd_list.items[i].string); + } + /* + * TRANSLATORS: This is the last item in the + * list printed out by the "alias loop" + * message below. + */ + strbuf_addf(&sb, _(" %d. %s = %s <== This is where the loop started!"), + i, + cmd_list.items[i - 1].string, + *argv[0]); + /* + * TRANSLATORS: The %s here at the end is + * going to be a list of aliases as formatted + * by the messages whose comments mention + * "alias loop" above. + */ + die(_("alias loop: When expanding the alias '%s' we ran into a loop:\n%s"), + cmd_list.items[0].string, sb.buf); + } string_list_append(&cmd_list, *argv[0]); Now we'll print errors like: $ ./git --exec-path=$PWD a fatal: alias loop: When expanding the alias 'a' we ran into a loop: 1. a = b 2. b = c 3. c = d <== The re-entry point in the loop 4. d = e 5. e = c <== This is where the loop started! Or, in the much simpler case of foo = bar; bar = foo: $ ./git --exec-path=$PWD foo fatal: alias loop: When expanding the alias 'foo' we ran into a loop: 1. foo = bar <== The re-entry point in the loop 2. bar = foo <== This is where the loop started! I haven't tested this much, so maybe there's some edge cases I haven't thought of / bugs in this reporting code, but hey, that's what the tests I suggested are for :) It's a lot more verbose, but I think it's worth it to produce better error messages.