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

From: Jeff King <peff@peff.net>
Date: Sep 6, 2018, 14:59 UTC
Message-ID: <20180906145942.GB1209@sigill.intra.peff.net>
In-Reply-To: <20180906101658.1865-1-timschumi@gmx.de>

On Thu, Sep 06, 2018 at 12:16:58PM +0200, Tim Schumacher wrote:

Show 7 quoted lines

> @@ -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.

It's unfortunate that string_list makes this so easy to get wrong.

Show 12 quoted lines

> +
> +		/*
> +		 * 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?

-Peff