{"thread":{"id":"49274","subject":"[RFC PATCH] Allow aliases that include other aliases","startedAt":"2018-09-04T17:39:20Z","lastAt":"2018-09-04T20:11:19Z","messageCount":3,"participants":["Tim Schumacher","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"357311","messageId":"20180904173915.2393-1-timschumi@gmx.de","threadId":"49274","inReplyTo":null,"subject":"[RFC PATCH] Allow aliases that include other aliases","fromName":"Tim Schumacher","fromEmail":"timschumi@gmx.de","sentAt":"2018-09-04T17:39:15Z","receivedAt":"2018-09-04T17:39:20Z","isPatch":true,"sender":{"key":"timschumi@gmx.de","avatar":"https://avatars.githubusercontent.com/u/16820960?v=4"},"body":"Aliases can only contain non-alias git commands and arguments,\nbut not other user-defined aliases. Resolving nested aliases\nis prevented by breaking the loop after the first alias was\nprocessed, git then fails with a command-not-found error.\n\nAllow resolving nested aliases by not breaking the loop in\nrun_argv() after the first alias was processed. Instead, continue\nincrementing done_alias until handle_alias() fails, which means\nthat there are no further aliases that can be processed.\n\n---\n\nI submitted this as RFC because I'm not sure whether disallowing\nnested aliases was an intentional design choice. The done_alias\ncheck implies that disallowing is intended, but the direct\nrecursion check for aliases that call themselves opposes that.\n\nFurthermore, including this patch allows creating a looping state,\nsince the recursion check only checks if an alias is directly calling\nitself.\nOne solution would be to break the loop as soon as done_alias reaches\na certain value, but that requires setting an arbitrary point of \"too\nmany recursions\".\nA list of already resolved aliases and breaking the loop as soon as\nan alias is resolved twice would probably do the trick, but\nimplementing that is well beyond the point of what I'm capable of\ndoing.\n\n---\n git.c | 4 +---\n 1 file changed, 1 insertion(+), 3 deletions(-)\n\ndiff --git a/git.c b/git.c\nindex c27c38738..9d3cf5797 100644\n--- a/git.c\n+++ b/git.c\n@@ -695,11 +695,9 @@ static int run_argv(int *argcp, const char ***argv)\n \t\t * of overriding \"git log\" with \"git show\" by having\n \t\t * alias.log = show\n \t\t */\n-\t\tif (done_alias)\n-\t\t\tbreak;\n \t\tif (!handle_alias(argcp, argv))\n \t\t\tbreak;\n-\t\tdone_alias = 1;\n+\t\tdone_alias++;\n \t}\n \n \treturn done_alias;\n-- \n2.19.0.rc1.2.g7460ee143\n\n"},{"id":"357312","messageId":"xmqqsh2p9mg8.fsf@gitster-ct.c.googlers.com","threadId":"49274","inReplyTo":"20180904173915.2393-1-timschumi@gmx.de","subject":"Re: [RFC PATCH] Allow aliases that include other aliases","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-04T17:55:35Z","receivedAt":"2018-09-04T17:55:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tim Schumacher <timschumi@gmx.de> writes:\n\n> I submitted this as RFC because I'm not sure whether disallowing\n> nested aliases was an intentional design choice. The done_alias\n> check implies that disallowing is intended, but the direct\n> recursion check for aliases that call themselves opposes that.\n\n\"direct recursion check for aliases\"?  I am not sure what you mean\nby that, but anyway.\n\nIf I recall correctly, it is intended that we disallow run_argv()\ndoing handle_alias() twice (or more).  But the ultimate objective is\nto forbid infinite loops, \"git foo\" expanding to \"git bar\" which in\nturn expanding back to \"git foo\", and the current \"do not expand\nalias to another\" is a simple but too strict implementation.  As\nlong as a replacement implementation still forbids infinite loops\nwith reasonable cost and complexity, I do not think we would mind\nsuch an improvement to allow alias expanding to another alias.  \n"},{"id":"357340","messageId":"20180904201115.GD17481@sigill.intra.peff.net","threadId":"49274","inReplyTo":"xmqqsh2p9mg8.fsf@gitster-ct.c.googlers.com","subject":"Re: [RFC PATCH] Allow aliases that include other aliases","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-09-04T20:11:16Z","receivedAt":"2018-09-04T20:11:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 04, 2018 at 10:55:35AM -0700, Junio C Hamano wrote:\n\n> Tim Schumacher <timschumi@gmx.de> writes:\n> \n> > I submitted this as RFC because I'm not sure whether disallowing\n> > nested aliases was an intentional design choice. The done_alias\n> > check implies that disallowing is intended, but the direct\n> > recursion check for aliases that call themselves opposes that.\n> \n> \"direct recursion check for aliases\"?  I am not sure what you mean\n> by that, but anyway.\n> \n> If I recall correctly, it is intended that we disallow run_argv()\n> doing handle_alias() twice (or more).  But the ultimate objective is\n> to forbid infinite loops, \"git foo\" expanding to \"git bar\" which in\n> turn expanding back to \"git foo\", and the current \"do not expand\n> alias to another\" is a simple but too strict implementation.  As\n> long as a replacement implementation still forbids infinite loops\n> with reasonable cost and complexity, I do not think we would mind\n> such an improvement to allow alias expanding to another alias.\n\nI agree that this could be looser, if all we care about is infinite\nloops. But given the issues we've had with aliases and the startup\nsequence, I also would not be surprised if there is some weird hidden\neffect if we call handle_alias() more than once.\n\nThat's not a reason not to pursue this, but just one more thing to look\nout for.\n\nAs an aside, you can still do this:\n\n  $ git config alias.foo '!git bar'\n  $ git config alias.bar '!git foo'\n  $ git foo\n  [boy, my CPU fan is really spinning]\n\nI don't know how much effort we actually need to put into people not\nshooting themselves in the foot, since we can't cover all of the cases\nanyway (and in a non-infinite configuration, that's another solution for\nTim's original problem).\n\n-Peff\n"}]}