[PATCH] completion: bash: support recursive aliases

[PATCH] completion: bash: support recursive aliases

[Felipe Contreras]

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(-)

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
-- 
2.29.2

Re: [PATCH] completion: bash: support recursive aliases

[Taylor Blau]

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

Re: [PATCH] completion: bash: support recursive aliases

[Junio C Hamano]

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?

Re: [PATCH] completion: bash: support recursive aliases

[Felipe Contreras]

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.

-- 
Felipe Contreras

Re: [PATCH] completion: bash: support recursive aliases

[Felipe Contreras]

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

-- 
Felipe Contreras

Re: [PATCH] completion: bash: support recursive aliases

[Jeff King]

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

Re: [PATCH] completion: bash: support recursive aliases

[Felipe Contreras]

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.

-- 
Felipe Contreras

Re: [PATCH] completion: bash: support recursive aliases

[Jeff King]

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