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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Wed, Sep 05 2018, Tim Schumacher wrote:

> 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 agree that this use of a list is better for a completely different
reason (which I'll comment on in the v4 thread), but this reason doesn't
make any sense to me.

If we're looking at performance we're paying a fixed performance cost
for storing this list of strings over a counter for everything we do
with aliases.

It only helps over a counter for the case where we do have a loop, but
at that point who cares? We're going to exit with an erro anyway and the
user has to fix his config, it doesn't matter if that error happens 1
millisecond earlier.