Re: [RFC PATCH v2] Allow aliases that include other aliases

[Tim Schumacher <timschumi@gmx.de>]

On 05.09.18 19:34, Jeff King wrote:
> On Wed, Sep 05, 2018 at 10:54:27AM +0200, Tim Schumacher wrote:
> 
>> 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
>> incrementing `done_alias` 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.
>> ---
>>
>> This is what I've come up with to prevent looping aliases. I'm not too
>> happy with the number of indentations needed, but this seemed to be the
>> easiest way to search an array for a value.
> 
> I think this approach is OK, though I wonder if we'd also be fine with
> just:
> 
>    if (done_alias++ > 100)
> 	die("woah, is your alias looping?");
> 
> The point is just to prevent a runaway infinite loop, and this does that
> while keeping the cost very low for the common case (not that one string
> insertion is probably breaking the bank).

I'd opt to use the list-approach instead of aborting when the
counter reaches 100 (or any other value), because it aborts
at the earliest known looping point. I didn't run any tests
comparing both solutions, but I assume the list would perform
faster than the hard-limit, even if it requires slightly more
memory and lines of code.

I hope that I can put the string-list struct to some use,
so that the solution using lists becomes an equally good
solution code-wise.

> 
> It could also extend to ! aliases if we wanted (i.e., my '!git foo'
> example from earlier), but you'd have to carry the counter through the
> environment between processes.

That is a question about "shooting oneself in the foot" again,
but I think trying to prevent that would require more changes
than I can make, and it is definitely out-of-scope for this
patch.

> 
> -Peff
> 
Thanks for reviewing,

Tim