From: Tim Schumacher Date: Thu, 06 Sep 2018 19:31:10 GMT Subject: Re: [PATCH v3] Allow aliases that include other aliases Message-ID: <4f814d67-c384-6d1e-a988-97cc25e32475@gmx.de> In-Reply-To: On 06.09.18 20:40, Junio C Hamano wrote: > Jeff King writes: > >> On Thu, Sep 06, 2018 at 12:16:58PM +0200, Tim Schumacher wrote: >> >>> @@ -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]); >> >> I pointed this out in my response to Ævar, but I want to make sure it >> gets seen. This call assumes the list is sorted, but... >> >>> + string_list_append(&cmd_list, *argv[0]); >> >> This will create an unsorted list. You'd have to use >> string_list_insert() here for a sorted list, or >> unsorted_string_list_has_string() in the earlier call. > > Correct. > > Also, normal users who have never seen this loop that implements > alias expansion would not have a clue when they see "called twice". > > I actually think the caller should also pass cmd to run_argv() and > then we should use it (and not argv[]) in this die() message. Could we just save the first element of the original argv for that purpose? Or alternatively, use the first stored element in the command list? > When > the original command was foo that is aliased to bar, which in turn > is aliased to baz, which in turn is aliased to bar, especially that > "git foo" invocation was in a random script written six weeks ago by > the user, it would be a lot more helpful to see > > "alias loop detected: expansion of 'git foo' does not terminate" > > than > > "loop alias: bar is called twice". > > given that 'bar' is not something the user called, or written in the > script she wrote six weeks ago. Indeed, printing the command that the user called is a better message than the command that is the entry-point of the loop. I'll change it in v4. > >> It's unfortunate that string_list makes this so easy to get wrong. >> >>> + >>> + /* >>> + * 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++; >> >> I don't think anybody cares about done_alias being an accurate count. >> Should we just leave this as-is? > > Good point. The only caller treats it as a bool (i.e. "should the > failure be reported as failure to expand an alias cmd which resulted > in (updated) argv[0] that is not a git command?"). > As the string-list has its own counter, I guess the done_alias variable can be reverted to a simple 0/1 value.