[RFC PATCH] Allow aliases that include other aliases

[RFC PATCH] Allow aliases that include other aliases

[Tim Schumacher]

Aliases can only contain non-alias git commands and arguments,
but not other user-defined aliases. Resolving 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.

---

I submitted this as RFC because I'm not sure whether disallowing
nested aliases was an intentional design choice. The done_alias
check implies that disallowing is intended, but the direct
recursion check for aliases that call themselves opposes that.

Furthermore, including this patch allows creating a looping state,
since the recursion check only checks if an alias is directly calling
itself.
One solution would be to break the loop as soon as done_alias reaches
a certain value, but that requires setting an arbitrary point of "too
many recursions".
A list of already resolved aliases and breaking the loop as soon as
an alias is resolved twice would probably do the trick, but
implementing that is well beyond the point of what I'm capable of
doing.

---
 git.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/git.c b/git.c
index c27c38738..9d3cf5797 100644
--- a/git.c
+++ b/git.c
@@ -695,11 +695,9 @@ static int run_argv(int *argcp, const char ***argv)
 		 * 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++;
 	}
 
 	return done_alias;
-- 
2.19.0.rc1.2.g7460ee143

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

[Junio C Hamano]

Tim Schumacher <timschumi@gmx.de> writes:

> I submitted this as RFC because I'm not sure whether disallowing
> nested aliases was an intentional design choice. The done_alias
> check implies that disallowing is intended, but the direct
> recursion check for aliases that call themselves opposes that.

"direct recursion check for aliases"?  I am not sure what you mean
by that, but anyway.

If I recall correctly, it is intended that we disallow run_argv()
doing handle_alias() twice (or more).  But the ultimate objective is
to forbid infinite loops, "git foo" expanding to "git bar" which in
turn expanding back to "git foo", and the current "do not expand
alias to another" is a simple but too strict implementation.  As
long as a replacement implementation still forbids infinite loops
with reasonable cost and complexity, I do not think we would mind
such an improvement to allow alias expanding to another alias.  

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

[Jeff King]

On Tue, Sep 04, 2018 at 10:55:35AM -0700, Junio C Hamano wrote:

> Tim Schumacher <timschumi@gmx.de> writes:
> 
> > I submitted this as RFC because I'm not sure whether disallowing
> > nested aliases was an intentional design choice. The done_alias
> > check implies that disallowing is intended, but the direct
> > recursion check for aliases that call themselves opposes that.
> 
> "direct recursion check for aliases"?  I am not sure what you mean
> by that, but anyway.
> 
> If I recall correctly, it is intended that we disallow run_argv()
> doing handle_alias() twice (or more).  But the ultimate objective is
> to forbid infinite loops, "git foo" expanding to "git bar" which in
> turn expanding back to "git foo", and the current "do not expand
> alias to another" is a simple but too strict implementation.  As
> long as a replacement implementation still forbids infinite loops
> with reasonable cost and complexity, I do not think we would mind
> such an improvement to allow alias expanding to another alias.

I agree that this could be looser, if all we care about is infinite
loops. But given the issues we've had with aliases and the startup
sequence, I also would not be surprised if there is some weird hidden
effect if we call handle_alias() more than once.

That's not a reason not to pursue this, but just one more thing to look
out for.

As an aside, you can still do this:

  $ git config alias.foo '!git bar'
  $ git config alias.bar '!git foo'
  $ git foo
  [boy, my CPU fan is really spinning]

I don't know how much effort we actually need to put into people not
shooting themselves in the foot, since we can't cover all of the cases
anyway (and in a non-infinite configuration, that's another solution for
Tim's original problem).

-Peff