From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Tim Schumacher <timschumi@gmx.de>
Cc: git@vger.kernel.org, gitster@pobox.com, peff@peff.net, pclouds@gmail.com
Subject: Re: [PATCH v3] Allow aliases that include other aliases
Date: Thu, 06 Sep 2018 16:01:39 +0200 [thread overview]
Message-ID: <87pnxqrags.fsf@evledraar.gmail.com> (raw)
In-Reply-To: <20180906101658.1865-1-timschumi@gmx.de>
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@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.
next prev parent reply other threads:[~2018-09-06 14:01 UTC|newest]
Thread overview: 52+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-09-05 8:54 [RFC PATCH v2] Allow aliases that include other aliases Tim Schumacher
2018-09-05 15:48 ` Duy Nguyen
2018-09-05 19:02 ` Tim Schumacher
2018-09-05 17:12 ` Junio C Hamano
2018-09-05 19:12 ` Tim Schumacher
2018-09-05 17:34 ` Jeff King
2018-09-05 20:02 ` Tim Schumacher
2018-09-06 13:38 ` Ævar Arnfjörð Bjarmason
2018-09-06 14:17 ` Ævar Arnfjörð Bjarmason
2018-10-18 22:57 ` [PATCH] alias: detect loops in mixed execution mode Ævar Arnfjörð Bjarmason
2018-10-19 8:28 ` Ævar Arnfjörð Bjarmason
2018-10-19 22:09 ` Jeff King
2018-10-20 10:52 ` Ævar Arnfjörð Bjarmason
2018-10-19 22:07 ` Jeff King
2018-10-20 11:14 ` Ævar Arnfjörð Bjarmason
2018-10-20 18:58 ` Jeff King
2018-10-20 19:18 ` Ævar Arnfjörð Bjarmason
2018-10-22 21:15 ` Jeff King
2018-10-22 21:28 ` Ævar Arnfjörð Bjarmason
2018-10-22 1:23 ` Junio C Hamano
2018-10-26 8:39 ` Jeff King
2018-10-26 12:44 ` Ævar Arnfjörð Bjarmason
2018-10-29 3:44 ` Junio C Hamano
2018-10-29 14:17 ` Jeff King
2018-09-05 21:51 ` [RFC PATCH v2] Allow aliases that include other aliases Junio C Hamano
2018-09-06 10:16 ` [PATCH v3] " Tim Schumacher
2018-09-06 14:01 ` Ævar Arnfjörð Bjarmason [this message]
2018-09-06 14:57 ` Jeff King
2018-09-06 15:10 ` Ævar Arnfjörð Bjarmason
2018-09-06 16:18 ` Jeff King
2018-09-06 19:05 ` Tim Schumacher
2018-09-06 19:17 ` Jeff King
2018-09-06 14:59 ` Jeff King
2018-09-06 18:40 ` Junio C Hamano
2018-09-06 19:05 ` Jeff King
2018-09-06 19:31 ` Tim Schumacher
2018-09-07 22:44 ` [RFC PATCH v4 1/3] Add support for nested aliases Tim Schumacher
2018-09-07 22:44 ` [RFC PATCH v4 2/3] Show the call history when an alias is looping Tim Schumacher
2018-09-08 13:34 ` Duy Nguyen
2018-09-08 16:29 ` Jeff King
2018-09-07 22:44 ` [RFC PATCH v4 3/3] t0014: Introduce alias testing suite Tim Schumacher
2018-09-07 23:38 ` Eric Sunshine
2018-09-14 23:12 ` Tim Schumacher
2018-09-16 7:21 ` Eric Sunshine
2018-09-08 13:28 ` [RFC PATCH v4 1/3] Add support for nested aliases Duy Nguyen
2018-09-16 7:46 ` Tim Schumacher
2018-09-17 15:37 ` Junio C Hamano
2018-09-21 12:45 ` Tim Schumacher
2018-09-21 15:59 ` Junio C Hamano
2018-09-16 7:50 ` [PATCH v5 " Tim Schumacher
2018-09-16 7:50 ` [PATCH v5 2/3] Show the call history when an alias is looping Tim Schumacher
2018-09-16 7:50 ` [PATCH v5 3/3] t0014: Introduce an alias testing suite Tim Schumacher
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
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87pnxqrags.fsf@evledraar.gmail.com \
--to=avarab@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=pclouds@gmail.com \
--cc=peff@peff.net \
--cc=timschumi@gmx.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).