Re: [PATCH v3] Allow aliases that include other aliases

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Sep 6, 2018, 14:01 UTC
Message-ID: <87pnxqrags.fsf@evledraar.gmail.com>
In-Reply-To: <20180906101658.1865-1-timschumi@gmx.de>

On Thu, Sep 06 2018, Tim Schumacher wrote:

Show 57 quoted lines

> 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@evledraar.gmail.com/ 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.