From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Jeff King <peff@peff.net>
Cc: Tim Schumacher <timschumi@gmx.de>,
git@vger.kernel.org, gitster@pobox.com, pclouds@gmail.com
Subject: Re: [PATCH v3] Allow aliases that include other aliases
Date: Thu, 06 Sep 2018 17:10:04 +0200 [thread overview]
Message-ID: <87musur7ar.fsf@evledraar.gmail.com> (raw)
In-Reply-To: <20180906145708.GA1209@sigill.intra.peff.net>
On Thu, Sep 06 2018, Jeff King wrote:
> On Thu, Sep 06, 2018 at 04:01:39PM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>> 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.
>
> Yes, I'd agree that this is worth adding a test (especially if the
> output routines get more complex).
>
>> 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:
>
> I don't have a strong opinion on whether this is worth it, but I think
> your implementation could be a little simpler:
>
>> 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);
>
> The string-list code doesn't generally deal in indices. You can use
> string_list_find_insert_index(), but its return value is a little funky
> for the existing case. You can also just do:
>
> struct string_list_item *seen;
> ...
> seen = string_list_lookup(&cmd_list, *argv[0]);
> if (seen) {
> /* we have a loop */
> int idx = seen - cmd_list.items;
>
> That's a little intimate with the string-list implementation as an array
> of string_list, but it's already pretty standard to walk over and
> dereference that list (including in your patch). But also see below.
>
> Side note: there's actually a bigger problem with the original patch:
> the string list is unsorted (because it uses string_list_append(), and
> which is why your linear walk works here). But string_list_has_string()
> assumes it is sorted. So I think we'd actually want to use
> unsorted_string_list_has_string() or unsorted_string_list_lookup().
>
>> + 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);
>
> This is always going to show the right-hand of the equals as the
> left-hand on the next line. Would it be simpler to just show the list?
> Likewise, the last item in the list is always going to be "where the
> loop started". Do we need to say that?
Yeah maybe that's overzealous. I figured in the spirit of clang & GCC
compiler messages these days there's no such thing as too dumbed down :)
> E.g., something like:
>
> seen = unsorted_string_list_lookup(&cmd_list, *argv[0]);
> if (seen) {
> for (i = 0; i < cmd_list.nr; i++) {
> struct string_list *item = cmd_list.items[i];
>
> strbuf_addf(&sb, " %s", item->string);
> if (item == seen)
> strbuf_add(&sb, " <==");
> strbuf_addch(&sb, '\n');
> }
> /* We never added this to the list, but we were about to */
> strbuf_addch(" %s\n", seen->string);
> die(...);
> }
>
> I guess it's not that far off of yours. Not using words to describe the
> loop entry and exit points avoids translation, which avoids notes to
> translators, which is most of what makes your patch long. ;)
This still needs translation for RTL languages. I.e. they'd want to
print out the equivalent of "%s " followed by "==> %s ". We happen to
(unfortunately) not carry such a language yet, but it's worth
future-proofing output as we add it in case we get one.
next prev parent reply other threads:[~2018-09-06 15:10 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
2018-09-06 14:57 ` Jeff King
2018-09-06 15:10 ` Ævar Arnfjörð Bjarmason [this message]
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=87musur7ar.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).