{"thread":{"id":"54607","subject":"[PATCH] completion: bash: support recursive aliases","startedAt":"2020-11-09T21:52:54Z","lastAt":"2020-11-10T01:19:31Z","messageCount":8,"participants":["Felipe Contreras","Taylor Blau","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"409417","messageId":"20201109215248.461167-1-felipe.contreras@gmail.com","threadId":"54607","inReplyTo":null,"subject":"[PATCH] completion: bash: support recursive aliases","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2020-11-09T21:52:48Z","receivedAt":"2020-11-09T21:52:54Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"It is possible to have a recursive aliases like:\n\n l = log --oneline\n lg = l --graph\n\nSo the completion should detect such aliases as well.\n\nSigned-off-by: Felipe Contreras \n---\n contrib/completion/git-completion.bash | 15 ++++++++++-----\n t/t9902-completion.sh | 19 +++++++++++++++++++\n 2 files changed, 29 insertions(+), 5 deletions(-)\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 36f5a91c7a..2053e4442d 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -1121,12 +1121,12 @@ __git_pretty_aliases ()\n # __git_aliased_command requires 1 argument\n __git_aliased_command ()\n {\n-\tlocal word cmdline=$(__git config --get \"alias.$1\")\n+\tlocal word cmdline=$(__git config --get \"alias.$1\") found\n \tfor word in $cmdline; do\n \t\tcase \"$word\" in\n \t\t\\!gitk|gitk)\n-\t\t\techo \"gitk\"\n-\t\t\treturn\n+\t\t\tfound=\"gitk\"\n+\t\t\tbreak\n \t\t\t;;\n \t\t\\!*)\t: shell command alias ;;\n \t\t-*)\t: option ;;\n@@ -1137,10 +1137,15 @@ __git_aliased_command ()\n \t\t:)\t: skip null command ;;\n \t\t\\'*)\t: skip opening quote after sh -c ;;\n \t\t*)\n-\t\t\techo \"$word\"\n-\t\t\treturn\n+\t\t\tfound=\"$word\"\n+\t\t\tbreak\n \t\tesac\n \tdone\n+\n+\tif [[ -n \"$found\" ]]; then\n+\t\tlocal expansion=$(__git_aliased_command \"$found\")\n+\t\techo \"${expansion:-$found}\"\n+\tfi\n }\n \n # Check whether one of the given words is present on the command line,\ndiff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\nindex 4e943393cf..0e2db6e7fa 100755\n--- a/t/t9902-completion.sh\n+++ b/t/t9902-completion.sh\n@@ -2195,6 +2195,25 @@ test_expect_success 'complete files' '\n \ttest_completion \"git add mom\" \"momified\"\n '\n \n+test_expect_success \"simple alias\" '\n+\ttest_config alias.co checkout &&\n+\ttest_completion \"git co m\" <<-\\EOF\n+\tmaster Z\n+\tmybranch Z\n+\tmytag Z\n+\tEOF\n+'\n+\n+test_expect_success \"recursive alias\" '\n+\ttest_config alias.co checkout &&\n+\ttest_config alias.cod \"co --detached\" &&\n+\ttest_completion \"git cod m\" <<-\\EOF\n+\tmaster Z\n+\tmybranch Z\n+\tmytag Z\n+\tEOF\n+'\n+\n test_expect_success \"completion uses completion for alias: !sh -c 'git ...'\" '\n \ttest_config alias.co \"!sh -c '\"'\"'git checkout ...'\"'\"'\" &&\n \ttest_completion \"git co m\" <<-\\EOF\n-- \n2.29.2\n\n"},{"id":"409419","messageId":"X6m8R9erSxGlPXq2@nand.local","threadId":"54607","inReplyTo":"20201109215248.461167-1-felipe.contreras@gmail.com","subject":"Re: [PATCH] completion: bash: support recursive aliases","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-11-09T22:01:43Z","receivedAt":"2020-11-09T22:01:49Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi Felipe,\n\nOn Mon, Nov 09, 2020 at 03:52:48PM -0600, Felipe Contreras wrote:\n> It is possible to have a recursive aliases like:\n\nI am not an expert or user of the Bash completion scripts in contrib, so\nI'll refrain from reviewing that portion of the patch.\n\nI would, however, recommend that you avoid the word 'recursive' here.\nGit rightly detects and rejects recursive and looping aliases. In fact,\nthe example that you give below:\n\n> l = log --oneline\n> lg = l --graph\n\nIs not even recursive. I would instead recommend calling 'lg' a \"nested\"\nalias.\n\nYou could argue about whether it is \"l\", \"lg\", or both that are nested,\nbut I think renaming the patch to \"completion: bash: support nested\naliases\" and then a s/recursive/nested throughout the patch message\nwould be sufficient.\n\n> So the completion should detect such aliases as well.\n\nThanks,\nTaylor\n"},{"id":"409423","messageId":"20201109221939.GA670413@coredump.intra.peff.net","threadId":"54607","inReplyTo":"20201109215248.461167-1-felipe.contreras@gmail.com","subject":"Re: [PATCH] completion: bash: support recursive aliases","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-11-09T22:19:39Z","receivedAt":"2020-11-09T22:19:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 09, 2020 at 03:52:48PM -0600, Felipe Contreras wrote:\n\n> It is possible to have a recursive aliases like:\n> \n> l = log --oneline\n> lg = l --graph\n> \n> So the completion should detect such aliases as well.\n\nYeah, agreed that it would be nice to handle this case.\n\n> __git_aliased_command ()\n> {\n> [...]\n> +\tif [[ -n \"$found\" ]]; then\n> +\t\tlocal expansion=$(__git_aliased_command \"$found\")\n> +\t\techo \"${expansion:-$found}\"\n> +\tfi\n\nSo if we expanded X to Y, we recurse and try to expand Y. That makes\nsense, but just thinking of some possible drawbacks:\n\n - it's an extra process invocation for each alias lookup (to see \"nope,\n this doesn't expand further\"). That's probably OK, since this is\n triggered by human action.\n\n I don't think there's a way to avoid this with the current set of Git\n commands. \"git help lg\" isn't recursive, and anyway isn't suitable\n for general use (if there is no such alias, it tries to load the\n manpage!).\n\n - there's no limit on the recursion if we do see a cycle. Doing:\n\n git config alias.foo foo\n git foo \n\n seems to fork-bomb the system with bash processes (well, perhaps not\n a true fork-bomb because they expand linearly rather than\n exponentially, but still...).\n\n That's obviously a broken and useless, but the outcome is less than\n ideal. We could avoid it by looking for repeats in the chain. Doing\n so in posix shell is pretty painful, but perhaps bash associate\n arrays would make it not too painful.\n\nWe do have \"git --git-completion-helper\" these days. I wonder if\nsomething like \"git --expand-alias-to-command\" would be a useful\naddition, as it would let us directly ask which Git command would be\nexecuted (if any). And it would make both downsides go away.\n\nI don't mind this solution in the meantime, though.\n\n-Peff\n"},{"id":"409426","messageId":"xmqq4klyb1w2.fsf@gitster.c.googlers.com","threadId":"54607","inReplyTo":"X6m8R9erSxGlPXq2@nand.local","subject":"Re: [PATCH] completion: bash: support recursive aliases","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-11-09T22:29:01Z","receivedAt":"2020-11-09T22:29:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> I am not an expert or user of the Bash completion scripts in contrib, so\n> I'll refrain from reviewing that portion of the patch.\n>\n> I would, however, recommend that you avoid the word 'recursive' here.\n> Git rightly detects and rejects recursive and looping aliases. In fact,\n> the example that you give below:\n>\n>> l = log --oneline\n>> lg = l --graph\n>\n> Is not even recursive. I would instead recommend calling 'lg' a \"nested\"\n> alias.\n>\n> You could argue about whether it is \"l\", \"lg\", or both that are nested,\n> but I think renaming the patch to \"completion: bash: support nested\n> aliases\" and then a s/recursive/nested throughout the patch message\n> would be sufficient.\n>\n>> So the completion should detect such aliases as well.\n\nTwo comments.\n\n - on design, is it possible to make a set of aliases that form a\n cycle? do we need to worry about such case? what does the\n current implementation do for an \"alias\" in such a cycle?\n\n - on implementation, it is done as a recursive call to the same\n function, but a loop that naturally maps tail recursion would\n also be a trivial implementation. is it worth rewriting the\n recursive calls into a loop? if we need to solve the circular\n references (above) by say limiting the length of the cycle, would\n such a rewrite make sense as a way to help implementation?\n\n"},{"id":"409448","messageId":"CAMP44s3CsC_gHNA2SjfAqhPorKHCgK-yG39fN+VzUw4Q+opwNg@mail.gmail.com","threadId":"54607","inReplyTo":"X6m8R9erSxGlPXq2@nand.local","subject":"Re: [PATCH] completion: bash: support recursive aliases","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2020-11-10T00:59:30Z","receivedAt":"2020-11-10T00:59:44Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Mon, Nov 9, 2020 at 4:01 PM Taylor Blau wrote:\n\n> > l = log --oneline\n> > lg = l --graph\n>\n> Is not even recursive. I would instead recommend calling 'lg' a \"nested\"\n> alias.\n\nPerhaps. In my mind these are recursive.\n\nIt also follows the more general understanding of recursion, for example:\n\n\"Dorothy thinks witches are dangerous\".\n\nIs considered a recursive sentence. See [1].\n\nBut if anyone else prefers the term \"nested\" for this, I would gladly\nupdate the patch.\n\nCheers.\n\n[1] https://en.wikipedia.org/wiki/Recursion#In_language\n\n-- \nFelipe Contreras\n"},{"id":"409450","messageId":"CAMP44s2mZK2_aAepOQs-5fmrbYPJ74Jb4VaSgwMAEdzZXS+9ZA@mail.gmail.com","threadId":"54607","inReplyTo":"20201109221939.GA670413@coredump.intra.peff.net","subject":"Re: [PATCH] completion: bash: support recursive aliases","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2020-11-10T01:06:56Z","receivedAt":"2020-11-10T01:07:10Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Mon, Nov 9, 2020 at 4:19 PM Jeff King wrote:\n\n> - there's no limit on the recursion if we do see a cycle. Doing:\n>\n> git config alias.foo foo\n> git foo \n>\n> seems to fork-bomb the system with bash processes (well, perhaps not\n> a true fork-bomb because they expand linearly rather than\n> exponentially, but still...).\n\nYes. I opted for the quick and minimal solution. But if this is a\nconcern it can be handled.\n\n> We do have \"git --git-completion-helper\" these days. I wonder if\n> something like \"git --expand-alias-to-command\" would be a useful\n> addition, as it would let us directly ask which Git command would be\n> executed (if any). And it would make both downsides go away.\n\nYes, but I don't think we need to wait in order to have a solution for\nboth issues. I already sent an updated patch.\n\nAdditionally, it might not be what the user wants. For example the\nuser might decide to have different completion for each one of the\naliases (_git_l, _git_lg, etc.), and if so, we would want\n__git_aliased_command to exit once we find the correct completion\nfunction.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"409451","messageId":"CAMP44s36ThnMondH8w-VDi4vWNL0Cpkx+o1qRkx_giJvyfh0dg@mail.gmail.com","threadId":"54607","inReplyTo":"xmqq4klyb1w2.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] completion: bash: support recursive aliases","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2020-11-10T01:10:11Z","receivedAt":"2020-11-10T01:10:25Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Mon, Nov 9, 2020 at 4:29 PM Junio C Hamano wrote:\n\n> >> So the completion should detect such aliases as well.\n>\n> Two comments.\n>\n> - on design, is it possible to make a set of aliases that form a\n> cycle? do we need to worry about such case? what does the\n> current implementation do for an \"alias\" in such a cycle?\n\nYes. The first try would be stuck in a loop until the user types CTRL+C.\n\nI already sent a second version that fixes that.\n\n> - on implementation, it is done as a recursive call to the same\n> function, but a loop that naturally maps tail recursion would\n> also be a trivial implementation. is it worth rewriting the\n> recursive calls into a loop? if we need to solve the circular\n> references (above) by say limiting the length of the cycle, would\n> such a rewrite make sense as a way to help implementation?\n\nYes, that can be done. I opted for the minimal change so it was easy\nto understand what the code was trying to do.\n\nThe second version works in the way you suggested.\n\n-- \nFelipe Contreras\n"},{"id":"409452","messageId":"20201110011924.GA1270103@coredump.intra.peff.net","threadId":"54607","inReplyTo":"CAMP44s2mZK2_aAepOQs-5fmrbYPJ74Jb4VaSgwMAEdzZXS+9ZA@mail.gmail.com","subject":"Re: [PATCH] completion: bash: support recursive aliases","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-11-10T01:19:24Z","receivedAt":"2020-11-10T01:19:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 09, 2020 at 07:06:56PM -0600, Felipe Contreras wrote:\n\n> > We do have \"git --git-completion-helper\" these days. I wonder if\n> > something like \"git --expand-alias-to-command\" would be a useful\n> > addition, as it would let us directly ask which Git command would be\n> > executed (if any). And it would make both downsides go away.\n> \n> Yes, but I don't think we need to wait in order to have a solution for\n> both issues. I already sent an updated patch.\n\nAgreed. The cycle-detection in your v2 looks fine to me.\n\n> Additionally, it might not be what the user wants. For example the\n> user might decide to have different completion for each one of the\n> aliases (_git_l, _git_lg, etc.), and if so, we would want\n> __git_aliased_command to exit once we find the correct completion\n> function.\n\nGood point.\n\n-Peff\n"}]}