[PATCH 0/4] completion for git-reflog show

[PATCH 0/4] completion for git-reflog show

[Rubén Justo]

When no subcommand is specified to "reflog" we assume "show" [1]:

    $ git reflog -h
    usage: git reflog [show] [<log-options>] [<ref>]
    ...

We are not completing correctly this implicit uses of "show":

With ...

    $ git checkout -b default

... we are not completing "default":

    $ git reflog def<TAB><TAB>

And we are incorrectly returning the "subcommands" when:

    $ git reflog default <TAB><TAB>
    delete expire show

This series fixes this and also adds completion for <log-options> in
"reflog show", so that the user can easily discover uses like:

   $ git reflog --since=1.day.ago

  1. cf39f54efc (git reflog show, 2007-02-08)

Rubén Justo (4):
  completion: introduce __gitcomp_subcommand
  completion: introduce __git_find_subcommand
  completion: reflog with implicit "show"
  completion: reflog show <log-options>

 contrib/completion/git-completion.bash | 63 +++++++++++++++++++++++---
 t/t9902-completion.sh                  | 11 +++++
 2 files changed, 68 insertions(+), 6 deletions(-)

-- 
2.43.0

[PATCH 1/4] completion: introduce __gitcomp_subcommand

[Rubén Justo]

Let's have a function to complete "subcommands" only in the correct
position (right after the command), in commands that follow the syntax:

    git <command> <subcommand>

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 contrib/completion/git-completion.bash | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 8c40ade494..916e137021 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -554,6 +554,27 @@ __gitcomp_file ()
 	true
 }
 
+# Completion for subcommands in commands that follow the syntax:
+#
+#    git <command> <subcommand>
+#
+# 1: List of possible completion words.
+# Returns false if the current word is not a possible subcommand
+# (possitioned after the command), or if no option is found in
+# the list provided.
+__gitcomp_subcommand ()
+{
+	local subcommands="$1"
+
+	if [ $cword -eq $(($__git_cmd_idx + 1)) ]; then
+		__gitcomp "$subcommands"
+
+		test -n "$COMPREPLY"
+	else
+		false
+	fi
+}
+
 # Execute 'git ls-files', unless the --committable option is specified, in
 # which case it runs 'git diff-index' to find out the files that can be
 # committed.  It return paths relative to the directory specified in the first
-- 
2.43.0

Re: [PATCH 1/4] completion: introduce __gitcomp_subcommand

[Junio C Hamano]

Rubén Justo <rjusto@gmail.com> writes:

> +# Completion for subcommands in commands that follow the syntax:
> +#
> +#    git <command> <subcommand>
> +#
> +# 1: List of possible completion words.
> +# Returns false if the current word is not a possible subcommand
> +# (possitioned after the command), or if no option is found in
> +# the list provided.
> +__gitcomp_subcommand ()
> +{
> +	local subcommands="$1"
> +
> +	if [ $cword -eq $(($__git_cmd_idx + 1)) ]; then
> +		__gitcomp "$subcommands"
> +
> +		test -n "$COMPREPLY"
> +	else
> +		false
> +	fi
> +}


I am not at all familiar with the code in this file, so treat this
as a question from somebody who do not know the calling convention
used around here.

This helper function clobbers what was in COMPREPLY[@] before
calling it, from a caller's point of view.  Is it a pattern that
potential callers in this file should already find familiar, and
they do not have to be reminded that they cannot rely on what they
prepared in COMPREPLY to be preserved across a call into this
function?  Otherwise I would suggest mentioning it in the helpful
comment before the function, but I cannot tell if such a comment is
even needed by the intended audience, so...

Re: [PATCH 1/4] completion: introduce __gitcomp_subcommand

[Rubén Justo]

On 26-ene-2024 09:26:44, Junio C Hamano wrote:
> Rubén Justo <rjusto@gmail.com> writes:
> 
> > +# Completion for subcommands in commands that follow the syntax:
> > +#
> > +#    git <command> <subcommand>
> > +#
> > +# 1: List of possible completion words.
> > +# Returns false if the current word is not a possible subcommand
> > +# (possitioned after the command), or if no option is found in
> > +# the list provided.
> > +__gitcomp_subcommand ()
> > +{
> > +	local subcommands="$1"
> > +
> > +	if [ $cword -eq $(($__git_cmd_idx + 1)) ]; then
> > +		__gitcomp "$subcommands"
> > +
> > +		test -n "$COMPREPLY"
> > +	else
> > +		false
> > +	fi
> > +}
> 
> 
> I am not at all familiar with the code in this file, so treat this
> as a question from somebody who do not know the calling convention
> used around here.
> 
> This helper function clobbers what was in COMPREPLY[@] before
> calling it, from a caller's point of view.  Is it a pattern that
> potential callers in this file should already find familiar, and
> they do not have to be reminded that they cannot rely on what they
> prepared in COMPREPLY to be preserved across a call into this
> function?  Otherwise I would suggest mentioning it in the helpful
> comment before the function, but I cannot tell if such a comment is
> even needed by the intended audience, so...

Maybe adding such a comment might suggest at first glance that we're
working different here than in the rest of the __gitcomp_* family of
functions, which is not the intention ... I dunno.

Re: [PATCH 1/4] completion: introduce __gitcomp_subcommand

[Junio C Hamano]

Rubén Justo <rjusto@gmail.com> writes:

> On 26-ene-2024 09:26:44, Junio C Hamano wrote:
>> Rubén Justo <rjusto@gmail.com> writes:
>> 
>> > +# Completion for subcommands in commands that follow the syntax:
>> > +#
>> > +#    git <command> <subcommand>
>> > +#
>> > +# 1: List of possible completion words.
>> > +# Returns false if the current word is not a possible subcommand
>> > +# (possitioned after the command), or if no option is found in
>> > +# the list provided.
>> > +__gitcomp_subcommand ()
>> > +{
>> > +	local subcommands="$1"
>> > +
>> > +	if [ $cword -eq $(($__git_cmd_idx + 1)) ]; then
>> > +		__gitcomp "$subcommands"
>> > +
>> > +		test -n "$COMPREPLY"
>> > +	else
>> > +		false
>> > +	fi
>> > +}
>> 
>> 
>> I am not at all familiar with the code in this file, so treat this
>> as a question from somebody who do not know the calling convention
>> used around here.
>> 
>> This helper function clobbers what was in COMPREPLY[@] before
>> calling it, from a caller's point of view.  Is it a pattern that
>> potential callers in this file should already find familiar, and
>> they do not have to be reminded that they cannot rely on what they
>> prepared in COMPREPLY to be preserved across a call into this
>> function?  Otherwise I would suggest mentioning it in the helpful
>> comment before the function, but I cannot tell if such a comment is
>> even needed by the intended audience, so...
>
> Maybe adding such a comment might suggest at first glance that we're
> working different here than in the rest of the __gitcomp_* family of
> functions, which is not the intention ... I dunno.

Exactly.  That is why I asked.  If it is a norm for all these helper
functions to stomp on COMPREPLY and if it is accepted as a common
pattern by developers who touch this file, then I agree it would be
misleading to have such a comment only to this function.

THanks.

[PATCH 2/4] completion: introduce __git_find_subcommand

[Rubén Justo]

Let's have a function to get the current subcommand when completing
commands that follow the syntax:

    git <command> <subcommand>

As a convenience, let's allow an optional "default subcommand" to be
returned if none is found.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 contrib/completion/git-completion.bash | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 916e137021..5f2e904b56 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -575,6 +575,26 @@ __gitcomp_subcommand ()
 	fi
 }
 
+# Find the current subcommand for commands that follow the syntax:
+#
+#    git <command> <subcommand>
+#
+# 1: List of possible subcommands.
+# 2: Optional subcommand to return when none is found.
+__git_find_subcommand ()
+{
+	local subcommand subcommands="$1" default_subcommand="$2"
+
+	for subcommand in $subcommands; do
+		if [ "$subcommand" = "${words[__git_cmd_idx+1]}" ]; then
+			echo $subcommand
+			return
+		fi
+	done
+
+	echo $default_subcommand
+}
+
 # Execute 'git ls-files', unless the --committable option is specified, in
 # which case it runs 'git diff-index' to find out the files that can be
 # committed.  It return paths relative to the directory specified in the first
-- 
2.43.0

Re: [PATCH 2/4] completion: introduce __git_find_subcommand

[Junio C Hamano]

Rubén Justo <rjusto@gmail.com> writes:

> Let's have a function to get the current subcommand when completing
> commands that follow the syntax:
>
>     git <command> <subcommand>
>
> As a convenience, let's allow an optional "default subcommand" to be
> returned if none is found.
>
> Signed-off-by: Rubén Justo <rjusto@gmail.com>
> ---
>  contrib/completion/git-completion.bash | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
>
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 916e137021..5f2e904b56 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -575,6 +575,26 @@ __gitcomp_subcommand ()
>  	fi
>  }
>  
> +# Find the current subcommand for commands that follow the syntax:
> +#
> +#    git <command> <subcommand>
> +#
> +# 1: List of possible subcommands.
> +# 2: Optional subcommand to return when none is found.
> +__git_find_subcommand ()
> +{
> +	local subcommand subcommands="$1" default_subcommand="$2"

Are the callers expected to form "$1" by concatenating known tokens
with a space?

I am just wondering if we can avoid looping, e.g.

	local nextword=${words[__git_cmd_idx+1]}
	case " $subcommands " in
	*" $nextword "*)
		echo "$nextword"
		return
		;;
	esac

It hopefully should not be a huge deal either way, though. 

> +
> +	for subcommand in $subcommands; do
> +		if [ "$subcommand" = "${words[__git_cmd_idx+1]}" ]; then
> +			echo $subcommand
> +			return
> +		fi
> +	done
> +
> +	echo $default_subcommand
> +}
> +
>  # Execute 'git ls-files', unless the --committable option is specified, in
>  # which case it runs 'git diff-index' to find out the files that can be
>  # committed.  It return paths relative to the directory specified in the first

Re: [PATCH 2/4] completion: introduce __git_find_subcommand

[Rubén Justo]

On 26-ene-2024 09:30:44, Junio C Hamano wrote:
> Rubén Justo <rjusto@gmail.com> writes:
> 
> > Let's have a function to get the current subcommand when completing
> > commands that follow the syntax:
> >
> >     git <command> <subcommand>
> >
> > As a convenience, let's allow an optional "default subcommand" to be
> > returned if none is found.
> >
> > Signed-off-by: Rubén Justo <rjusto@gmail.com>
> > ---
> >  contrib/completion/git-completion.bash | 20 ++++++++++++++++++++
> >  1 file changed, 20 insertions(+)
> >
> > diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> > index 916e137021..5f2e904b56 100644
> > --- a/contrib/completion/git-completion.bash
> > +++ b/contrib/completion/git-completion.bash
> > @@ -575,6 +575,26 @@ __gitcomp_subcommand ()
> >  	fi
> >  }
> >  
> > +# Find the current subcommand for commands that follow the syntax:
> > +#
> > +#    git <command> <subcommand>
> > +#
> > +# 1: List of possible subcommands.
> > +# 2: Optional subcommand to return when none is found.
> > +__git_find_subcommand ()
> > +{
> > +	local subcommand subcommands="$1" default_subcommand="$2"
> 
> Are the callers expected to form "$1" by concatenating known tokens
> with a space?
> 
> I am just wondering if we can avoid looping, e.g.
> 
> 	local nextword=${words[__git_cmd_idx+1]}
> 	case " $subcommands " in
> 	*" $nextword "*)
> 		echo "$nextword"
> 		return
> 		;;
> 	esac
> 

I like the idea; and it works:

     $ words=("a" "b"); __git_cmd_idx=0; __git_find_subcommand "a b" "test"
     b

     $ : simulate that the user moves the cursor backwards
     $ words=("a" "" "b"); __git_cmd_idx=0; __git_find_subcommand "a b" "test"
     test

     $ : simulate that the user moves the cursor backwards
     $ words=("a" " " "b"); __git_cmd_idx=0; __git_find_subcommand "a b" "test"
     test

But functions like __git_find_on_cmdline or __git-find_last_on_cmdline
are already iterating.  I feel we should keep the loop.

Thank you.

[PATCH 3/4] completion: reflog with implicit "show"

[Rubén Justo]

When no subcommand is specified to "reflog", we assume "show" [1]:

    $ git reflog -h
    usage: git reflog [show] [<log-options>] [<ref>]
    ...

We are not completing correctly this implicit uses of "show":

With ...

    $ git checkout -b default

... we are not completing "default":

    $ git reflog def<TAB><TAB>

And we are incorrectly returning the "subcommands" when:

    $ git reflog default <TAB><TAB>
    delete expire show

Let's use __gitcomp_subcommand to correctly handle implicit
subcommands.

  1. cf39f54efc (git reflog show, 2007-02-08)

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 contrib/completion/git-completion.bash | 9 ++++-----
 t/t9902-completion.sh                  | 8 ++++++++
 2 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 5f2e904b56..c41f25aa80 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2448,13 +2448,12 @@ _git_rebase ()
 _git_reflog ()
 {
 	local subcommands="show delete expire"
-	local subcommand="$(__git_find_on_cmdline "$subcommands")"
 
-	if [ -z "$subcommand" ]; then
-		__gitcomp "$subcommands"
-	else
-		__git_complete_refs
+	if __gitcomp_subcommand "$subcommands"; then
+		return
 	fi
+
+	__git_complete_refs
 }
 
 __git_send_email_confirm_options="always never auto cc compose"
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index aa9a614de3..231e17f378 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -2618,6 +2618,14 @@ test_expect_success 'git clone --config= - value' '
 	EOF
 '
 
+test_expect_success 'git reflog show' '
+	test_when_finished "git checkout -" &&
+	git checkout -b shown &&
+	test_completion "git reflog sho" "show " &&
+	test_completion "git reflog show sho" "shown " &&
+	test_completion "git reflog shown sho" "shown "
+'
+
 test_expect_success 'options with value' '
 	test_completion "git merge -X diff-algorithm=" <<-\EOF
 
-- 
2.43.0

Re: [PATCH 3/4] completion: reflog with implicit "show"

[Junio C Hamano]

Rubén Justo <rjusto@gmail.com> writes:

> When no subcommand is specified to "reflog", we assume "show" [1]:
>
>     $ git reflog -h
>     usage: git reflog [show] [<log-options>] [<ref>]
>     ...
>
> We are not completing correctly this implicit uses of "show":
>
> With ...
>
>     $ git checkout -b default
>
> ... we are not completing "default":
>
>     $ git reflog def<TAB><TAB>
>
> And we are incorrectly returning the "subcommands" when:
>
>     $ git reflog default <TAB><TAB>
>     delete expire show
>
> Let's use __gitcomp_subcommand to correctly handle implicit
> subcommands.

As with a good bug report, if you are showing an "incorrect"
behaviour, you should also illustrate what you think is the
"correct" behaviour (see below).

>   1. cf39f54efc (git reflog show, 2007-02-08)
>
> Signed-off-by: Rubén Justo <rjusto@gmail.com>
> ---
>  contrib/completion/git-completion.bash | 9 ++++-----
>  t/t9902-completion.sh                  | 8 ++++++++
>  2 files changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 5f2e904b56..c41f25aa80 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -2448,13 +2448,12 @@ _git_rebase ()
>  _git_reflog ()
>  {
>  	local subcommands="show delete expire"
> -	local subcommand="$(__git_find_on_cmdline "$subcommands")"
>  
> -	if [ -z "$subcommand" ]; then
> -		__gitcomp "$subcommands"
> -	else
> -		__git_complete_refs
> +	if __gitcomp_subcommand "$subcommands"; then
> +		return
>  	fi
> +
> +	__git_complete_refs
>  }

So, when we see something that could be a subcommand we complete it
as a subcommand and return.  In your example, how should

    $ git reflog def<TAB>

work?  We try to see if there is a subcommand that begins with
"def", we see nothing matching, and then run __git_complete_refs?
What if the branch you created earlier were not named "default" but,
say, "delmonte", and you did "git reflog del<TAB>"?  Shouldn't the
user be offered "delete" and "delmonte" as potential completions?

>  __git_send_email_confirm_options="always never auto cc compose"
> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
> index aa9a614de3..231e17f378 100755
> --- a/t/t9902-completion.sh
> +++ b/t/t9902-completion.sh
> @@ -2618,6 +2618,14 @@ test_expect_success 'git clone --config= - value' '
>  	EOF
>  '
>  
> +test_expect_success 'git reflog show' '
> +	test_when_finished "git checkout -" &&
> +	git checkout -b shown &&
> +	test_completion "git reflog sho" "show " &&

IOW, shouldn't this offer both show and shown?

> +	test_completion "git reflog show sho" "shown " &&
> +	test_completion "git reflog shown sho" "shown "
> +'
> +
>  test_expect_success 'options with value' '
>  	test_completion "git merge -X diff-algorithm=" <<-\EOF

Re: [PATCH 3/4] completion: reflog with implicit "show"

[Rubén Justo]

On 26-ene-2024 09:57:25, Junio C Hamano wrote:
> Rubén Justo <rjusto@gmail.com> writes:
> 
> > When no subcommand is specified to "reflog", we assume "show" [1]:
> >
> >     $ git reflog -h
> >     usage: git reflog [show] [<log-options>] [<ref>]
> >     ...
> >
> > We are not completing correctly this implicit uses of "show":
> >
> > With ...
> >
> >     $ git checkout -b default
> >
> > ... we are not completing "default":
> >
> >     $ git reflog def<TAB><TAB>
> >
> > And we are incorrectly returning the "subcommands" when:
> >
> >     $ git reflog default <TAB><TAB>
> >     delete expire show
> >
> > Let's use __gitcomp_subcommand to correctly handle implicit
> > subcommands.
> 
> As with a good bug report, if you are showing an "incorrect"
> behaviour, you should also illustrate what you think is the
> "correct" behaviour (see below).
> 
> >   1. cf39f54efc (git reflog show, 2007-02-08)
> >
> > Signed-off-by: Rubén Justo <rjusto@gmail.com>
> > ---
> >  contrib/completion/git-completion.bash | 9 ++++-----
> >  t/t9902-completion.sh                  | 8 ++++++++
> >  2 files changed, 12 insertions(+), 5 deletions(-)
> >
> > diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> > index 5f2e904b56..c41f25aa80 100644
> > --- a/contrib/completion/git-completion.bash
> > +++ b/contrib/completion/git-completion.bash
> > @@ -2448,13 +2448,12 @@ _git_rebase ()
> >  _git_reflog ()
> >  {
> >  	local subcommands="show delete expire"
> > -	local subcommand="$(__git_find_on_cmdline "$subcommands")"
> >  
> > -	if [ -z "$subcommand" ]; then
> > -		__gitcomp "$subcommands"
> > -	else
> > -		__git_complete_refs
> > +	if __gitcomp_subcommand "$subcommands"; then
> > +		return
> >  	fi
> > +
> > +	__git_complete_refs
> >  }
> 
> So, when we see something that could be a subcommand we complete it
> as a subcommand and return.  In your example, how should
> 
>     $ git reflog def<TAB>
> 
> work?  We try to see if there is a subcommand that begins with
> "def", we see nothing matching, and then run __git_complete_refs?
> What if the branch you created earlier were not named "default" but,
> say, "delmonte", and you did "git reflog del<TAB>"?  Shouldn't the
> user be offered "delete" and "delmonte" as potential completions?
> 
> >  __git_send_email_confirm_options="always never auto cc compose"
> > diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
> > index aa9a614de3..231e17f378 100755
> > --- a/t/t9902-completion.sh
> > +++ b/t/t9902-completion.sh
> > @@ -2618,6 +2618,14 @@ test_expect_success 'git clone --config= - value' '
> >  	EOF
> >  '
> >  
> > +test_expect_success 'git reflog show' '
> > +	test_when_finished "git checkout -" &&
> > +	git checkout -b shown &&
> > +	test_completion "git reflog sho" "show " &&
> 
> IOW, shouldn't this offer both show and shown?

What should we do?

When the user have a branch "show":

   $ git checkout -b show
   $ git reflog sho<TAB><TAB>

And with:

   $ git reflog <TAB><TAB>

Should we suggest the subcommands alongside the branches?

I thought about this, and it's a can of worms.  I concluded that a sane
an instructive for the user approach is to first try the subcommands and
if no option is found, then try the implicit "show".

Maybe I'm overthinking it ...

> 
> > +	test_completion "git reflog show sho" "shown " &&
> > +	test_completion "git reflog shown sho" "shown "
> > +'
> > +
> >  test_expect_success 'options with value' '
> >  	test_completion "git merge -X diff-algorithm=" <<-\EOF

Thanks.

Re: [PATCH 3/4] completion: reflog with implicit "show"

[Junio C Hamano]

Rubén Justo <rjusto@gmail.com> writes:

>> So, when we see something that could be a subcommand we complete it
>> as a subcommand and return.  In your example, how should
>> 
>>     $ git reflog def<TAB>
>> 
>> work?  We try to see if there is a subcommand that begins with
>> "def", we see nothing matching, and then run __git_complete_refs?
>> What if the branch you created earlier were not named "default" but,
>> say, "delmonte", and you did "git reflog del<TAB>"?  Shouldn't the
>> user be offered "delete" and "delmonte" as potential completions?
>> 
>> >  __git_send_email_confirm_options="always never auto cc compose"
>> > diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
>> > index aa9a614de3..231e17f378 100755
>> > --- a/t/t9902-completion.sh
>> > +++ b/t/t9902-completion.sh
>> > @@ -2618,6 +2618,14 @@ test_expect_success 'git clone --config= - value' '
>> >  	EOF
>> >  '
>> >  
>> > +test_expect_success 'git reflog show' '
>> > +	test_when_finished "git checkout -" &&
>> > +	git checkout -b shown &&
>> > +	test_completion "git reflog sho" "show " &&
>> 
>> IOW, shouldn't this offer both show and shown?
>
> What should we do?

I would imagine that we should make the above offer both "show"
(because it can be what the user meant) and "shown" (because it can
also be what the user meant)?  But thinking of it again, because
"show" is a prefix of "shown", this should offer "show" and then
another <TAB> would offer "show" and "shown".  At least, that is
what I would expect from the usual bash completion behaviour, which
would look like:

    $ mkdir /var/tmp/scratch && cd /var/tmp/scratch
    $ : >show 2>shown
    $ echo sho<TAB>

The <TAB> makes the above line expand to

    $ echo show

and place the curser immediately after 'w' (without a space after it).
Giving another <TAB> at this point offers two possible candidates.

    $ echo show<TAB>
    show shown

So, I'd expect a similar completion to happen, i.e.

    $ git reflog sho<TAB>
-->
    $ git reflog show

because there are two candidates, "show" and "shown", and I can type
another <TAB> at that point to see the two candidates.

    $ git reflog show<TAB>
    show shown

If the branch name were "shot" instead of "shown", then the possible
choices would become "show" and "shot", so we'd skip one step from
the above and immediately get the two candidates against "sho<TAB>".

    $ git reflog sho<TAB>
    shot show

That is what I would expect.    

Thanks.

Re: [PATCH 3/4] completion: reflog with implicit "show"

[Rubén Justo]

On Tue, Feb 20, 2024 at 17:46:20 -0800, Junio C Hamano wrote:

>     $ git reflog sho<TAB>
>     shot show
> 
> That is what I would expect.    

Thank you for responding.

Of course that's the logical expectation.

However I'm not sure if it is sensible to offer completion for
sub-commands mixed with branch names.

Furthermore, I am also worried that such an approach implies making the
user pay, probably unnecessarily many times, for __git_complete_refs in
cases such as:

    $ git reflog <TAB><TAB>  ;# the user may be just exploring the sub-commands
    $ git reflog s<TAB>      ;# the user may be lazy and just want "show "
    $ git reflog show<TAB>   ;# likewise, to complete the SP
    $ git reflog expir<TAB>  ;# how often a expir... branch is expected here?

The experienced user, if not most users, should be intuitive enough to
circumvent the corner cases:

    $ git reflog <TAB><TAB>
    delete expire show
    ...
    $ git reflog s<TAB>
    ...
    $ git reflog show s<TAB>
    ...
    $ git reflog show shot

This is why I choose to fallback to __git_complete_ref only when no
other option is available.

If you think, or anyone else, that these concerns don't make sense, I'm
open to make it work as you described.

Re: [PATCH 3/4] completion: reflog with implicit "show"

[Rubén Justo]

On Wed, Feb 21, 2024 at 07:06:36PM +0100, Rubén Justo wrote:
> On Tue, Feb 20, 2024 at 17:46:20 -0800, Junio C Hamano wrote:
> 
> >     $ git reflog sho<TAB>
> >     shot show
> > 
> > That is what I would expect.    
> 
> Thank you for responding.
> 
> Of course that's the logical expectation.
> 
> However I'm not sure if it is sensible to offer completion for
> sub-commands mixed with branch names.
> 
> Furthermore, I am also worried that such an approach implies making the
> user pay, probably unnecessarily many times, for __git_complete_refs in
> cases such as:
> 
>     $ git reflog <TAB><TAB>  ;# the user may be just exploring the sub-commands
>     $ git reflog s<TAB>      ;# the user may be lazy and just want "show "
>     $ git reflog show<TAB>   ;# likewise, to complete the SP
>     $ git reflog expir<TAB>  ;# how often a expir... branch is expected here?
> 
> The experienced user, if not most users, should be intuitive enough to
> circumvent the corner cases:
> 
>     $ git reflog <TAB><TAB>
>     delete expire show
>     ...
>     $ git reflog s<TAB>
>     ...
>     $ git reflog show s<TAB>
>     ...
>     $ git reflog show shot
> 
> This is why I choose to fallback to __git_complete_ref only when no
> other option is available.
> 
> If you think, or anyone else, that these concerns don't make sense, I'm
> open to make it work as you described.

I am happy with the current iteration and I still think that mixing
branch names with options is a source of confusion.

However, this topic in the latest 'What's cooking' is marked with:

 Expecting a reroll.
 cf. <dd106d87-3363-426a-90a2-16e1f2d04661@gmail.com>
 source: <98daf977-dbad-4d3b-a293-6a769895088f@gmail.com>

I am confused about what the expectation is.

Consider this message a ping as maybe the message I'm responding to has
been missed.

Thanks.

Re: [PATCH 3/4] completion: reflog with implicit "show"

[Junio C Hamano]

Rubén Justo <rjusto@gmail.com> writes:

> I am happy with the current iteration and I still think that mixing
> branch names with options is a source of confusion.
>
> However, this topic in the latest 'What's cooking' is marked with:
>
>  Expecting a reroll.
>  cf. <dd106d87-3363-426a-90a2-16e1f2d04661@gmail.com>
>  source: <98daf977-dbad-4d3b-a293-6a769895088f@gmail.com>
>
> I am confused about what the expectation is.

Expectation is to show both possible commands and branch names
available so that users with enough typing can pick from either, as
I do see it even more confusing if only branch names (or only
command names) are visible sometimes (namely, if the prefix is
shared by both) but some other times command names (or branch names)
are completed just fine (namely, if the prefix is unique to only one
set of names between command names and branch names).

[PATCH 4/4] completion: reflog show <log-options>

[Rubén Justo]

Let's add completion for <log-options> in "reflog show" so that the user
can easily discover uses like:

   $ git reflog --since=1.day.ago

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 contrib/completion/git-completion.bash | 13 ++++++++++++-
 t/t9902-completion.sh                  |  5 ++++-
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index c41f25aa80..3deb98389c 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2447,12 +2447,23 @@ _git_rebase ()
 
 _git_reflog ()
 {
-	local subcommands="show delete expire"
+	local subcommand subcommands="show delete expire"
 
 	if __gitcomp_subcommand "$subcommands"; then
 		return
 	fi
 
+	subcommand="$(__git_find_subcommand "$subcommands" "show")"
+
+	case "$subcommand,$cur" in
+	show,--*)
+		__gitcomp "
+			$__git_log_common_options
+			"
+		return
+		;;
+	esac
+
 	__git_complete_refs
 }
 
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 231e17f378..a74d774168 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -2623,7 +2623,10 @@ test_expect_success 'git reflog show' '
 	git checkout -b shown &&
 	test_completion "git reflog sho" "show " &&
 	test_completion "git reflog show sho" "shown " &&
-	test_completion "git reflog shown sho" "shown "
+	test_completion "git reflog shown sho" "shown " &&
+	test_completion "git reflog --unt" "--until=" &&
+	test_completion "git reflog show --unt" "--until=" &&
+	test_completion "git reflog shown --unt" "--until="
 '
 
 test_expect_success 'options with value' '
-- 
2.43.0

[PATCH v2 0/5] completion for git-reflog show

[Rubén Justo]

This iteration has three main changes and two new commits since v2.

The changes:

 - The function __gitcomp_subcommand is no longer needed in this series.
   The patch that introduced it has been removed.
 
 - The description for the behaviour we're fixing in the patch 1/5 has
   been reworded to better explain the behaviour we're expecting.
 
 - Provide completion for both subcommands and references when the
   expectation is such that we're not sure if the implicit subcommand
   "show" is desired.

About the two new commits:

This series is described in the last "What's cooking" with:

 The command line completion script (in contrib/) learned to
 complete "git reflog" better.

So, while we're here I've included two new commits, 4/5 and 5/5, that
fit well in that description.

An important note is in the last patch, reporting the special case
needed for the <log-options> in the "show" subcommand.  This is what has
made me decide to include the new commits in this series.

Rubén Justo (5):
  completion: reflog with implicit "show"
  completion: introduce __git_find_subcommand
  completion: reflog show <log-options>
  completion: factor out __git_builtin
  completion: reflog subcommands and options

 contrib/completion/git-completion.bash | 70 ++++++++++++++++++++++----
 t/t9902-completion.sh                  | 14 ++++++
 2 files changed, 73 insertions(+), 11 deletions(-)

Range-diff against v1:
1:  1a76491362 < -:  ---------- completion: introduce __gitcomp_subcommand
3:  8defb041ac ! 1:  61b9696238 completion: reflog with implicit "show"
    @@ Commit message
             usage: git reflog [show] [<log-options>] [<ref>]
             ...
     
    -    We are not completing correctly this implicit uses of "show":
    -
    -    With ...
    +    This implicit "show" is not being completed correctly:
     
             $ git checkout -b default
    +        $ git reflog def<TAB><TAB>
    +        ... no completion options ...
     
    -    ... we are not completing "default":
    +    The expected result is:
     
    -        $ git reflog def<TAB><TAB>
    +        $ git reflog default
    +
    +    This happens because we're completing references after seeing a valid
    +    subcommand in the command line.  This prevents the implicit "show" from
    +    working properly, but also introduces a new problem: it keeps offering
    +    subcommand options when the subcommand is implicit:
    +
    +        $ git checkout -b explore
    +        $ git reflog default ex<TAB>
    +        ...
    +        $ git reflog default expire
     
    -    And we are incorrectly returning the "subcommands" when:
    +    The expected result is:
     
    -        $ git reflog default <TAB><TAB>
    -        delete expire show
    +        $ git reflog default explore
     
    -    Let's use __gitcomp_subcommand to correctly handle implicit
    -    subcommands.
    +    To fix this, complete references even if no subcommand is present, or in
    +    other words when the subcommand is implicit "show".
    +
    +    Also, only include completion options for subcommands when completing
    +    the right position in the command line.
     
           1. cf39f54efc (git reflog show, 2007-02-08)
     
    @@ contrib/completion/git-completion.bash: _git_rebase ()
     -		__gitcomp "$subcommands"
     -	else
     -		__git_complete_refs
    -+	if __gitcomp_subcommand "$subcommands"; then
    -+		return
    - 	fi
    -+
     +	__git_complete_refs
    ++
    ++	if [ $((cword - __git_cmd_idx)) -eq 1 ]; then
    ++		__gitcompappend "$subcommands" "" "$cur" " "
    + 	fi
      }
      
    - __git_send_email_confirm_options="always never auto cc compose"
     
      ## t/t9902-completion.sh ##
     @@ t/t9902-completion.sh: test_expect_success 'git clone --config= - value' '
    @@ t/t9902-completion.sh: test_expect_success 'git clone --config= - value' '
      '
      
     +test_expect_success 'git reflog show' '
    -+	test_when_finished "git checkout -" &&
    ++	test_when_finished "git checkout - && git branch -d shown" &&
     +	git checkout -b shown &&
    -+	test_completion "git reflog sho" "show " &&
    ++	test_completion "git reflog sho" <<-\EOF &&
    ++	show Z
    ++	shown Z
    ++	EOF
     +	test_completion "git reflog show sho" "shown " &&
     +	test_completion "git reflog shown sho" "shown "
     +'
2:  b1aad9a667 ! 2:  b3133c69d3 completion: introduce __git_find_subcommand
    @@ Commit message
         Signed-off-by: Rubén Justo <rjusto@gmail.com>
     
      ## contrib/completion/git-completion.bash ##
    -@@ contrib/completion/git-completion.bash: __gitcomp_subcommand ()
    - 	fi
    +@@ contrib/completion/git-completion.bash: __gitcomp_file ()
    + 	true
      }
      
     +# Find the current subcommand for commands that follow the syntax:
4:  4d3fb1d563 ! 3:  e6e526b436 completion: reflog show <log-options>
    @@ Commit message
     
      ## contrib/completion/git-completion.bash ##
     @@ contrib/completion/git-completion.bash: _git_rebase ()
    - 
      _git_reflog ()
      {
    --	local subcommands="show delete expire"
    -+	local subcommand subcommands="show delete expire"
    - 
    - 	if __gitcomp_subcommand "$subcommands"; then
    - 		return
    - 	fi
    - 
    -+	subcommand="$(__git_find_subcommand "$subcommands" "show")"
    + 	local subcommands="show delete expire"
    ++	local subcommand="$(__git_find_subcommand "$subcommands" "show")"
     +
     +	case "$subcommand,$cur" in
     +	show,--*)
    @@ contrib/completion/git-completion.bash: _git_rebase ()
     +		return
     +		;;
     +	esac
    -+
    + 
      	__git_complete_refs
    - }
      
     
      ## t/t9902-completion.sh ##
     @@ t/t9902-completion.sh: test_expect_success 'git reflog show' '
    - 	git checkout -b shown &&
    - 	test_completion "git reflog sho" "show " &&
    + 	shown Z
    + 	EOF
      	test_completion "git reflog show sho" "shown " &&
     -	test_completion "git reflog shown sho" "shown "
     +	test_completion "git reflog shown sho" "shown " &&
-:  ---------- > 4:  dfed95d495 completion: factor out __git_builtin
-:  ---------- > 5:  8193b7f4f9 completion: reflog subcommands and options
-- 
2.44.0

[PATCH v2 1/5] completion: reflog with implicit "show"

[Rubén Justo]

When no subcommand is specified to "reflog", we assume "show" [1]:

    $ git reflog -h
    usage: git reflog [show] [<log-options>] [<ref>]
    ...

This implicit "show" is not being completed correctly:

    $ git checkout -b default
    $ git reflog def<TAB><TAB>
    ... no completion options ...

The expected result is:

    $ git reflog default

This happens because we're completing references after seeing a valid
subcommand in the command line.  This prevents the implicit "show" from
working properly, but also introduces a new problem: it keeps offering
subcommand options when the subcommand is implicit:

    $ git checkout -b explore
    $ git reflog default ex<TAB>
    ...
    $ git reflog default expire

The expected result is:

    $ git reflog default explore

To fix this, complete references even if no subcommand is present, or in
other words when the subcommand is implicit "show".

Also, only include completion options for subcommands when completing
the right position in the command line.

  1. cf39f54efc (git reflog show, 2007-02-08)

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 contrib/completion/git-completion.bash |  9 ++++-----
 t/t9902-completion.sh                  | 11 +++++++++++
 2 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 8c40ade494..ff216f1c65 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2407,12 +2407,11 @@ _git_rebase ()
 _git_reflog ()
 {
 	local subcommands="show delete expire"
-	local subcommand="$(__git_find_on_cmdline "$subcommands")"
 
-	if [ -z "$subcommand" ]; then
-		__gitcomp "$subcommands"
-	else
-		__git_complete_refs
+	__git_complete_refs
+
+	if [ $((cword - __git_cmd_idx)) -eq 1 ]; then
+		__gitcompappend "$subcommands" "" "$cur" " "
 	fi
 }
 
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index aa9a614de3..dbd57e6a28 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -2618,6 +2618,17 @@ test_expect_success 'git clone --config= - value' '
 	EOF
 '
 
+test_expect_success 'git reflog show' '
+	test_when_finished "git checkout - && git branch -d shown" &&
+	git checkout -b shown &&
+	test_completion "git reflog sho" <<-\EOF &&
+	show Z
+	shown Z
+	EOF
+	test_completion "git reflog show sho" "shown " &&
+	test_completion "git reflog shown sho" "shown "
+'
+
 test_expect_success 'options with value' '
 	test_completion "git merge -X diff-algorithm=" <<-\EOF
 
-- 
2.44.0

[PATCH v2 3/5] completion: reflog show <log-options>

[Rubén Justo]

Let's add completion for <log-options> in "reflog show" so that the user
can easily discover uses like:

   $ git reflog --since=1.day.ago

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 contrib/completion/git-completion.bash | 10 ++++++++++
 t/t9902-completion.sh                  |  5 ++++-
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 849d191b02..dc5f73a9f3 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2427,6 +2427,16 @@ _git_rebase ()
 _git_reflog ()
 {
 	local subcommands="show delete expire"
+	local subcommand="$(__git_find_subcommand "$subcommands" "show")"
+
+	case "$subcommand,$cur" in
+	show,--*)
+		__gitcomp "
+			$__git_log_common_options
+			"
+		return
+		;;
+	esac
 
 	__git_complete_refs
 
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index dbd57e6a28..04f3620e5b 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -2626,7 +2626,10 @@ test_expect_success 'git reflog show' '
 	shown Z
 	EOF
 	test_completion "git reflog show sho" "shown " &&
-	test_completion "git reflog shown sho" "shown "
+	test_completion "git reflog shown sho" "shown " &&
+	test_completion "git reflog --unt" "--until=" &&
+	test_completion "git reflog show --unt" "--until=" &&
+	test_completion "git reflog shown --unt" "--until="
 '
 
 test_expect_success 'options with value' '
-- 
2.44.0

[PATCH v2 2/5] completion: introduce __git_find_subcommand

[Rubén Justo]

Let's have a function to get the current subcommand when completing
commands that follow the syntax:

    git <command> <subcommand>

As a convenience, let's allow an optional "default subcommand" to be
returned if none is found.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 contrib/completion/git-completion.bash | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index ff216f1c65..849d191b02 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -554,6 +554,26 @@ __gitcomp_file ()
 	true
 }
 
+# Find the current subcommand for commands that follow the syntax:
+#
+#    git <command> <subcommand>
+#
+# 1: List of possible subcommands.
+# 2: Optional subcommand to return when none is found.
+__git_find_subcommand ()
+{
+	local subcommand subcommands="$1" default_subcommand="$2"
+
+	for subcommand in $subcommands; do
+		if [ "$subcommand" = "${words[__git_cmd_idx+1]}" ]; then
+			echo $subcommand
+			return
+		fi
+	done
+
+	echo $default_subcommand
+}
+
 # Execute 'git ls-files', unless the --committable option is specified, in
 # which case it runs 'git diff-index' to find out the files that can be
 # committed.  It return paths relative to the directory specified in the first
-- 
2.44.0

[PATCH v2 4/5] completion: factor out __git_resolve_builtins

[Rubén Justo]

We're going to use the result of "git xxx --git-completion-helper" not
only for feeding COMPREPLY.

Therefore, factor out the execution and the caching of its results in
__gitcomp_builtin, to a new function __git_resolve_builtins.

While we're here, move an important comment we have in the function to
its header, so it gains visibility.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---

I've changed my mind last minute, and renamed the function to
__git_resolve_builtins, so the subject of this patch is different
from the one included in the cover letter.

 contrib/completion/git-completion.bash | 31 +++++++++++++++++++++-----
 1 file changed, 25 insertions(+), 6 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index dc5f73a9f3..f9fbf1f703 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -452,16 +452,18 @@ fi
 
 # This function is equivalent to
 #
-#    __gitcomp "$(git xxx --git-completion-helper) ..."
+#    ___git_resolved_builtins=$(git xxx --git-completion-helper)
 #
-# except that the output is cached. Accept 1-3 arguments:
+# except that the result of the execution is cached.
+#
+# Accept 1-3 arguments:
 # 1: the git command to execute, this is also the cache key
+#    (use "_" when the command contains spaces, e.g. "remote add"
+#    becomes "remote_add")
 # 2: extra options to be added on top (e.g. negative forms)
 # 3: options to be excluded
-__gitcomp_builtin ()
+__git_resolve_builtins ()
 {
-	# spaces must be replaced with underscore for multi-word
-	# commands, e.g. "git remote add" becomes remote_add.
 	local cmd="$1"
 	local incl="${2-}"
 	local excl="${3-}"
@@ -487,7 +489,24 @@ __gitcomp_builtin ()
 		eval "$var=\"$options\""
 	fi
 
-	__gitcomp "$options"
+	___git_resolved_builtins="$options"
+}
+
+# This function is equivalent to
+#
+#    __gitcomp "$(git xxx --git-completion-helper) ..."
+#
+# except that the output is cached. Accept 1-3 arguments:
+# 1: the git command to execute, this is also the cache key
+#    (use "_" when the command contains spaces, e.g. "remote add"
+#    becomes "remote_add")
+# 2: extra options to be added on top (e.g. negative forms)
+# 3: options to be excluded
+__gitcomp_builtin ()
+{
+	__git_resolve_builtins "$1" "$2" "$3"
+
+	__gitcomp "$___git_resolved_builtins"
 }
 
 # Variation of __gitcomp_nl () that appends to the existing list of
-- 
2.44.0

[PATCH v2 5/5] completion: reflog subcommands and options

[Rubén Justo]

Make generic the completion for reflog subcommands and its options.

Note that we still need to special case the options for "show".

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 contrib/completion/git-completion.bash | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index f9fbf1f703..c5c9e9de2d 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2445,8 +2445,12 @@ _git_rebase ()
 
 _git_reflog ()
 {
-	local subcommands="show delete expire"
-	local subcommand="$(__git_find_subcommand "$subcommands" "show")"
+	local subcommands subcommand
+
+	__git_resolve_builtins "reflog"
+
+	subcommands="$___git_resolved_builtins"
+	subcommand="$(__git_find_subcommand "$subcommands" "show")"
 
 	case "$subcommand,$cur" in
 	show,--*)
@@ -2455,6 +2459,10 @@ _git_reflog ()
 			"
 		return
 		;;
+	$subcommand,--*)
+		__gitcomp_builtin "reflog_$subcommand"
+		return
+		;;
 	esac
 
 	__git_complete_refs
-- 
2.44.0