[RFC,v4,1/3] Add support for nested aliases
diff mbox series

Message ID 20180907224430.23859-1-timschumi@gmx.de
State New
Headers show
Series
  • [RFC,v4,1/3] Add support for nested aliases
Related show

Commit Message

Tim Schumacher Sept. 7, 2018, 10:44 p.m. UTC
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
the loop 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.

Signed-off-by: Tim Schumacher <timschumi@gmx.de>
---
Changes since v3:
 - Print the command that the user entered instead of the command
   which caused the loop (and a nicer, more explanatory error message)
 - Use unsorted_string_list_has_string() instead of the sorted version
 - Fix a code style issue just below the modified code
 - done_alias is a simple boolean again (instead of a counter)

 git.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

Comments

Duy Nguyen Sept. 8, 2018, 1:28 p.m. UTC | #1
On Sat, Sep 8, 2018 at 12:44 AM Tim Schumacher <timschumi@gmx.de> wrote:
> +               /*
> +                * It could be an alias -- this works around the insanity
>                  * of overriding "git log" with "git show" by having
>                  * alias.log = show
>                  */

I think this comment block is about the next two lines you just
deleted. So delete it to instead of fixing style.

> -               if (done_alias)
> -                       break;
>                 if (!handle_alias(argcp, argv))
>                         break;
>                 done_alias = 1;
>         }
Tim Schumacher Sept. 16, 2018, 7:46 a.m. UTC | #2
On 08.09.18 15:28, Duy Nguyen wrote:
> On Sat, Sep 8, 2018 at 12:44 AM Tim Schumacher <timschumi@gmx.de> wrote:
>> +               /*
>> +                * It could be an alias -- this works around the insanity
>>                   * of overriding "git log" with "git show" by having
>>                   * alias.log = show
>>                   */
> 
> I think this comment block is about the next two lines you just
> deleted. So delete it to instead of fixing style.

I think that comment is talking about the code that is handing the alias,
so it still would be valid.
The check might have peen placed in between to keep it logically grouped.

> 
>> -               if (done_alias)
>> -                       break;
>>                  if (!handle_alias(argcp, argv))
>>                          break;
>>                  done_alias = 1;
>>          }
Junio C Hamano Sept. 17, 2018, 3:37 p.m. UTC | #3
Tim Schumacher <timschumi@gmx.de> writes:

> On 08.09.18 15:28, Duy Nguyen wrote:
>> On Sat, Sep 8, 2018 at 12:44 AM Tim Schumacher <timschumi@gmx.de> wrote:
>>> +               /*
>>> +                * It could be an alias -- this works around the insanity
>>>                   * of overriding "git log" with "git show" by having
>>>                   * alias.log = show
>>>                   */
>>
>> I think this comment block is about the next two lines you just
>> deleted. So delete it to instead of fixing style.
>
> I think that comment is talking about the code that is handing the alias,
> so it still would be valid.

"this" in "this works around" refers to the fact that we first check
the builtins and on-GIT_EXEC_PATH commands before trying an alias,
which is an effective way to forbid an alias from taking over
existing command names.  So it is not about a particular code but is
about how the two sections of code are laid out.

It probably will make it clear if we reworded and made it a comment
about the whole while() loop may make sense, i.e.

	/*
	 * Check if av[0] is a command before seeing if it is an
	 * alias to avoid the insanity of overriding ...
	 */
	while (1) {
		...

but that can be done after the dust settles as a clean-up, I would
think.
Tim Schumacher Sept. 21, 2018, 12:45 p.m. UTC | #4
On 17.09.18 17:37, Junio C Hamano wrote:
> Tim Schumacher <timschumi@gmx.de> writes:
> 
>> On 08.09.18 15:28, Duy Nguyen wrote:
>>> On Sat, Sep 8, 2018 at 12:44 AM Tim Schumacher <timschumi@gmx.de> wrote:
>>>> +               /*
>>>> +                * It could be an alias -- this works around the insanity
>>>>                    * of overriding "git log" with "git show" by having
>>>>                    * alias.log = show
>>>>                    */
>>>
>>> I think this comment block is about the next two lines you just
>>> deleted. So delete it to instead of fixing style.
>>
>> I think that comment is talking about the code that is handing the alias,
>> so it still would be valid.
> 
> "this" in "this works around" refers to the fact that we first check
> the builtins and on-GIT_EXEC_PATH commands before trying an alias,
> which is an effective way to forbid an alias from taking over
> existing command names.  So it is not about a particular code but is
> about how the two sections of code are laid out.
> 
> It probably will make it clear if we reworded and made it a comment
> about the whole while() loop may make sense, i.e.
> 
> 	/*
> 	 * Check if av[0] is a command before seeing if it is an
> 	 * alias to avoid the insanity of overriding ...
> 	 */
> 	while (1) {
> 		...
> 

Imho, the "insanity" part makes the intention of that comment unclear, even if
it is located at the top of the while() loop. Giving an example is nice, but wouldn't
it be better to say something like the following?

	/*
	 * Check if av[0] is a command before seeing if it is an
	 * alias to avoid taking over existing commands
	 */

> but that can be done after the dust settles as a clean-up, I would
> think.
> 

I'll keep the changed comment in my local repository for now and publish it together
with other changes in v6, but I assume there won't be much additional feedback.
Junio C Hamano Sept. 21, 2018, 3:59 p.m. UTC | #5
Tim Schumacher <timschumi@gmx.de> writes:

> it is located at the top of the while() loop. Giving an example is nice, but wouldn't
> it be better to say something like the following?
>
> 	/*
> 	 * Check if av[0] is a command before seeing if it is an
> 	 * alias to avoid taking over existing commands
> 	 */

If we have more concrete and constructive things to explain why we
choose to forbid it, that may be worth saying, but I agree that it
does not add much value to this comment to declare that an attempt
to take over existing commands is "insane".

Thanks.

Patch
diff mbox series

diff --git a/git.c b/git.c
index c27c38738..15727c17f 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,25 @@  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 (unsorted_string_list_has_string(&cmd_list, *argv[0])) {
+			die(_("alias loop detected: expansion of '%s' does"
+			      " not terminate"), cmd_list.items[0].string);
+		}
+
+		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;
 	}
 
+	string_list_clear(&cmd_list, 0);
+
 	return done_alias;
 }