From: Tim Schumacher Date: Wed, 05 Sep 2018 20:02:58 GMT Subject: Re: [RFC PATCH v2] Allow aliases that include other aliases Message-ID: In-Reply-To: <20180905173455.GA2336@sigill.intra.peff.net> 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