| Message ID | 20201109215248.461167-1-felipe.contreras@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | completion: bash: support recursive aliases | expand |
Hi Felipe, On Mon, Nov 09, 2020 at 03:52:48PM -0600, Felipe Contreras wrote: > It is possible to have a recursive aliases like: I am not an expert or user of the Bash completion scripts in contrib, so I'll refrain from reviewing that portion of the patch. I would, however, recommend that you avoid the word 'recursive' here. Git rightly detects and rejects recursive and looping aliases. In fact, the example that you give below: > l = log --oneline > lg = l --graph Is not even recursive. I would instead recommend calling 'lg' a "nested" alias. You could argue about whether it is "l", "lg", or both that are nested, but I think renaming the patch to "completion: bash: support nested aliases" and then a s/recursive/nested throughout the patch message would be sufficient. > So the completion should detect such aliases as well. Thanks, Taylor
On Mon, Nov 09, 2020 at 03:52:48PM -0600, Felipe Contreras wrote: > It is possible to have a recursive aliases like: > > l = log --oneline > lg = l --graph > > So the completion should detect such aliases as well. Yeah, agreed that it would be nice to handle this case. > __git_aliased_command () > { > [...] > + if [[ -n "$found" ]]; then > + local expansion=$(__git_aliased_command "$found") > + echo "${expansion:-$found}" > + fi So if we expanded X to Y, we recurse and try to expand Y. That makes sense, but just thinking of some possible drawbacks: - it's an extra process invocation for each alias lookup (to see "nope, this doesn't expand further"). That's probably OK, since this is triggered by human action. I don't think there's a way to avoid this with the current set of Git commands. "git help lg" isn't recursive, and anyway isn't suitable for general use (if there is no such alias, it tries to load the manpage!). - there's no limit on the recursion if we do see a cycle. Doing: git config alias.foo foo git foo <Tab> seems to fork-bomb the system with bash processes (well, perhaps not a true fork-bomb because they expand linearly rather than exponentially, but still...). That's obviously a broken and useless, but the outcome is less than ideal. We could avoid it by looking for repeats in the chain. Doing so in posix shell is pretty painful, but perhaps bash associate arrays would make it not too painful. We do have "git <cmd> --git-completion-helper" these days. I wonder if something like "git --expand-alias-to-command" would be a useful addition, as it would let us directly ask which Git command would be executed (if any). And it would make both downsides go away. I don't mind this solution in the meantime, though. -Peff
Taylor Blau <me@ttaylorr.com> writes: > I am not an expert or user of the Bash completion scripts in contrib, so > I'll refrain from reviewing that portion of the patch. > > I would, however, recommend that you avoid the word 'recursive' here. > Git rightly detects and rejects recursive and looping aliases. In fact, > the example that you give below: > >> l = log --oneline >> lg = l --graph > > Is not even recursive. I would instead recommend calling 'lg' a "nested" > alias. > > You could argue about whether it is "l", "lg", or both that are nested, > but I think renaming the patch to "completion: bash: support nested > aliases" and then a s/recursive/nested throughout the patch message > would be sufficient. > >> So the completion should detect such aliases as well. Two comments. - on design, is it possible to make a set of aliases that form a cycle? do we need to worry about such case? what does the current implementation do for an "alias" in such a cycle? - on implementation, it is done as a recursive call to the same function, but a loop that naturally maps tail recursion would also be a trivial implementation. is it worth rewriting the recursive calls into a loop? if we need to solve the circular references (above) by say limiting the length of the cycle, would such a rewrite make sense as a way to help implementation?
On Mon, Nov 9, 2020 at 4:01 PM Taylor Blau <me@ttaylorr.com> wrote: > > l = log --oneline > > lg = l --graph > > Is not even recursive. I would instead recommend calling 'lg' a "nested" > alias. Perhaps. In my mind these are recursive. It also follows the more general understanding of recursion, for example: "Dorothy thinks witches are dangerous". Is considered a recursive sentence. See [1]. But if anyone else prefers the term "nested" for this, I would gladly update the patch. Cheers. [1] https://en.wikipedia.org/wiki/Recursion#In_language
On Mon, Nov 9, 2020 at 4:19 PM Jeff King <peff@peff.net> wrote: > - there's no limit on the recursion if we do see a cycle. Doing: > > git config alias.foo foo > git foo <Tab> > > seems to fork-bomb the system with bash processes (well, perhaps not > a true fork-bomb because they expand linearly rather than > exponentially, but still...). Yes. I opted for the quick and minimal solution. But if this is a concern it can be handled. > We do have "git <cmd> --git-completion-helper" these days. I wonder if > something like "git --expand-alias-to-command" would be a useful > addition, as it would let us directly ask which Git command would be > executed (if any). And it would make both downsides go away. Yes, but I don't think we need to wait in order to have a solution for both issues. I already sent an updated patch. Additionally, it might not be what the user wants. For example the user might decide to have different completion for each one of the aliases (_git_l, _git_lg, etc.), and if so, we would want __git_aliased_command to exit once we find the correct completion function. Cheers.
On Mon, Nov 9, 2020 at 4:29 PM Junio C Hamano <gitster@pobox.com> wrote: > >> So the completion should detect such aliases as well. > > Two comments. > > - on design, is it possible to make a set of aliases that form a > cycle? do we need to worry about such case? what does the > current implementation do for an "alias" in such a cycle? Yes. The first try would be stuck in a loop until the user types CTRL+C. I already sent a second version that fixes that. > - on implementation, it is done as a recursive call to the same > function, but a loop that naturally maps tail recursion would > also be a trivial implementation. is it worth rewriting the > recursive calls into a loop? if we need to solve the circular > references (above) by say limiting the length of the cycle, would > such a rewrite make sense as a way to help implementation? Yes, that can be done. I opted for the minimal change so it was easy to understand what the code was trying to do. The second version works in the way you suggested.
On Mon, Nov 09, 2020 at 07:06:56PM -0600, Felipe Contreras wrote: > > We do have "git <cmd> --git-completion-helper" these days. I wonder if > > something like "git --expand-alias-to-command" would be a useful > > addition, as it would let us directly ask which Git command would be > > executed (if any). And it would make both downsides go away. > > Yes, but I don't think we need to wait in order to have a solution for > both issues. I already sent an updated patch. Agreed. The cycle-detection in your v2 looks fine to me. > Additionally, it might not be what the user wants. For example the > user might decide to have different completion for each one of the > aliases (_git_l, _git_lg, etc.), and if so, we would want > __git_aliased_command to exit once we find the correct completion > function. Good point. -Peff
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash index 36f5a91c7a..2053e4442d 100644 --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -1121,12 +1121,12 @@ __git_pretty_aliases () # __git_aliased_command requires 1 argument __git_aliased_command () { - local word cmdline=$(__git config --get "alias.$1") + local word cmdline=$(__git config --get "alias.$1") found for word in $cmdline; do case "$word" in \!gitk|gitk) - echo "gitk" - return + found="gitk" + break ;; \!*) : shell command alias ;; -*) : option ;; @@ -1137,10 +1137,15 @@ __git_aliased_command () :) : skip null command ;; \'*) : skip opening quote after sh -c ;; *) - echo "$word" - return + found="$word" + break esac done + + if [[ -n "$found" ]]; then + local expansion=$(__git_aliased_command "$found") + echo "${expansion:-$found}" + fi } # Check whether one of the given words is present on the command line, diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh index 4e943393cf..0e2db6e7fa 100755 --- a/t/t9902-completion.sh +++ b/t/t9902-completion.sh @@ -2195,6 +2195,25 @@ test_expect_success 'complete files' ' test_completion "git add mom" "momified" ' +test_expect_success "simple alias" ' + test_config alias.co checkout && + test_completion "git co m" <<-\EOF + master Z + mybranch Z + mytag Z + EOF +' + +test_expect_success "recursive alias" ' + test_config alias.co checkout && + test_config alias.cod "co --detached" && + test_completion "git cod m" <<-\EOF + master Z + mybranch Z + mytag Z + EOF +' + test_expect_success "completion uses <cmd> completion for alias: !sh -c 'git <cmd> ...'" ' test_config alias.co "!sh -c '"'"'git checkout ...'"'"'" && test_completion "git co m" <<-\EOF
It is possible to have a recursive aliases like: l = log --oneline lg = l --graph So the completion should detect such aliases as well. Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> --- contrib/completion/git-completion.bash | 15 ++++++++++----- t/t9902-completion.sh | 19 +++++++++++++++++++ 2 files changed, 29 insertions(+), 5 deletions(-)