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

[Tim Schumacher <timschumi@gmx.de>]

On 06.09.18 16:57, 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).

I'll try to come up with a few tests (or one at this point, as we only have
a solution for internal aliases so far) and put them as 1/2. However, I don't know
what file I should put those tests into. t0001-init and t1300-config both seem
to test aliases, but I'm unsure if the new tests should go into one of those
files or a completely new one that is dedicated to aliases.

> 
>> 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().

I'll update v4 to use use unsorted_string_list_has_string().
> 
>> +			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?
> 
> 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. ;)
> 
> -Peff
> 

I'll tinker around with both code snippets, we'll see which one is
more convenient for the user.

Thanks to all of you for the input!

Tim