[PATCH v2 0/3] completion: make __git_complete public

[PATCH v2 0/3] completion: make __git_complete public

[Felipe Contreras]

Back in 2012 I argued [1] for the introduction of a helper that would
allow users to specify aliases like:

  git_complete gf git_fetch

There was pushback because there was no clear guideline for public
functions (git_complete vs. _git_complete vs. _GIT_complete), and some
aliases didn't actually work.

Fast-forward to 2020, and there's still no guideline for public
functions, and those aliases still don't work (even though I sent the
fixes).

This has not prevented people from using this function that is clearly
needed to setup custom aliases [2], and in fact that's the recommended
way since there's no other.

But it is cumbersome that the user must type:

  __git_complete gf _git_fetch

Or worse:

  __git_complete gk __gitk_main

8 years is more than enough time to stop waiting for the perfect to
come; let's define a public function (with the same name) that is
actually user-friendly:

  __git_complete gf git_fetch
  __git_complete gk gitk

While also maintaining backwards compatibility.

[1] https://lore.kernel.org/git/1334524814-13581-1-git-send-email-felipe.contreras@gmail.com/
[2] https://stackoverflow.com/questions/342969/how-do-i-get-bash-completion-to-work-with-aliases

Felipe Contreras (3):
  completion: bash: add __git_have_func helper
  test: completion: add tests for __git_complete
  completion: add proper public __git_complete

 contrib/completion/git-completion.bash | 41 +++++++++++++++++++-------
 t/t9902-completion.sh                  | 20 +++++++++++++
 2 files changed, 51 insertions(+), 10 deletions(-)

Range-diff:
-:  ---------- > 1:  0993732142 completion: bash: add __git_have_func helper
1:  ea77b1a140 ! 2:  7918c34d0e test: completion: add tests for __git_complete
    @@ Metadata
      ## Commit message ##
         test: completion: add tests for __git_complete
     
    +    Even though the function was marked as not public, it's already used in
    +    the wild.
    +
    +    We should at least test basic functionality.
    +
         Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
     
      ## t/t9902-completion.sh ##
    @@ t/t9902-completion.sh: test_expect_success 'sourcing the completion script clear
      '
      
     +test_expect_success '__git_complete' '
    ++	unset -f __git_wrap__git_main &&
     +	__git_complete foo __git_main &&
    -+	test "$(type -t __git_wrap__git_main)" == function &&
    ++	__git_have_func __git_wrap__git_main &&
     +	__git_complete gf _git_fetch &&
    -+	test "$(type -t __git_wrap_git_fetch)" == function
    ++	__git_have_func __git_wrap_git_fetch
     +'
     +
      test_done
2:  aec19e61ee ! 3:  8a6cc52063 completion: add proper public __git_complete
    @@ Metadata
      ## Commit message ##
         completion: add proper public __git_complete
     
    -    Back in 2012 I argued [1] for the introduction of a helper that would
    -    allow users to specify aliases like:
    +    When __git_complete was introduced, it was meant to be temporarily, while
    +    a proper guideline for public shell functions was established
    +    (tentatively _GIT_complete), but since that never happened, people
    +    in the wild started to use __git_complete, even though it was marked as
    +    not public.
     
    -      git_complete gf git_fetch
    +    Eight years is more than enough wait, let's mark this function as
    +    public, and make it a bit more user-friendly.
     
    -    Back then there was pushback because there was no clear guideline for
    -    public functions (git_complete vs _git_complete vs _GIT_complete), and
    -    some aliases didn't actually work.
    -
    -    Fast-forward to 2020 and there's still no guideline for public
    -    functions, and those aliases still don't work (even though I sent the
    -    fixes).
    -
    -    This has not prevented people from using this function that is clearly
    -    needed to setup custom aliases [2], and in fact it's the recommended
    -    way. But it is cumbersome that the user must type:
    -
    -      __git_complete gf _git_fetch
    -
    -    Or worse:
    +    So that instead of doing:
     
           __git_complete gk __gitk_main
     
    -    8 years is more than enough time to stop waiting for the perfect to
    -    come; let's define a public function (with the same name) that is
    -    actually user-friendly:
    +    The user can do:
     
    -      __git_complete gf git_fetch
           __git_complete gk gitk
     
    -    While also maintaining backwards compatibility.
    +    And instead of:
     
    -    The logic is:
    +      __git_complete gf _git_fetch
    +
    +    Do:
     
    -     1. If $2 exists, use it directly
    -     2. If not, check if __$2_main exists
    -     3. If not, check if _$2 exists
    -     4. If not, fail
    +      __git_complete gf git_fetch
     
    -    [1] https://lore.kernel.org/git/1334524814-13581-1-git-send-email-felipe.contreras@gmail.com/
    -    [2] https://stackoverflow.com/questions/342969/how-do-i-get-bash-completion-to-work-with-aliases
    +    Backwards compatibility is maintained.
     
         Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
     
    @@ contrib/completion/git-completion.bash: __git_complete ()
     +{
     +	local func
     +
    -+	if [[ "$(type -t $2)" == function ]]; then
    ++	if __git_have_func $2; then
     +		func=$2
    -+	elif [[ "$(type -t __$2_main)" == function ]]; then
    ++	elif __git_have_func __$2_main; then
     +		func=__$2_main
    -+	elif [[ "$(type -t _$2)" == function ]]; then
    ++	elif __git_have_func _$2; then
     +		func=_$2
     +	else
     +		echo "ERROR: could not find function '$2'" 1>&2
    @@ contrib/completion/git-completion.bash: __git_complete ()
     
      ## t/t9902-completion.sh ##
     @@ t/t9902-completion.sh: test_expect_success 'sourcing the completion script clears cached --options' '
    - '
      
      test_expect_success '__git_complete' '
    -+	unset -f __git_wrap__git_main &&
    + 	unset -f __git_wrap__git_main &&
     +
      	__git_complete foo __git_main &&
    - 	test "$(type -t __git_wrap__git_main)" == function &&
    + 	__git_have_func __git_wrap__git_main &&
     +	unset -f __git_wrap__git_main &&
     +
      	__git_complete gf _git_fetch &&
    --	test "$(type -t __git_wrap_git_fetch)" == function
    -+	test "$(type -t __git_wrap_git_fetch)" == function &&
    +-	__git_have_func __git_wrap_git_fetch
    ++	__git_have_func __git_wrap_git_fetch &&
     +
     +	__git_complete foo git &&
    -+	test "$(type -t __git_wrap__git_main)" == function &&
    ++	__git_have_func __git_wrap__git_main &&
     +	unset -f __git_wrap__git_main &&
     +
     +	__git_complete gd git_diff &&
    -+	test "$(type -t __git_wrap_git_diff)" == function &&
    ++	__git_have_func __git_wrap_git_diff &&
     +
     +	test_must_fail __git_complete ga missing
      '
-- 
2.30.0

[PATCH v2 1/3] completion: bash: add __git_have_func helper

[Felipe Contreras]

This makes the code more readable, and also will help when new code
wants to do similar checks.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 contrib/completion/git-completion.bash | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 463a3124da..869c73ee2c 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -3358,15 +3358,19 @@ __git_support_parseopt_helper () {
 	esac
 }
 
+__git_have_func () {
+	declare -f $1 >/dev/null 2>/dev/null
+}
+
 __git_complete_command () {
 	local command="$1"
 	local completion_func="_git_${command//-/_}"
-	if ! declare -f $completion_func >/dev/null 2>/dev/null &&
-		declare -f _completion_loader >/dev/null 2>/dev/null
+	if ! __git_have_func $completion_func &&
+		__git_have_func _completion_loader
 	then
 		_completion_loader "git-$command"
 	fi
-	if declare -f $completion_func >/dev/null 2>/dev/null
+	if __git_have_func $completion_func
 	then
 		$completion_func
 		return 0
-- 
2.30.0

[PATCH v2 2/3] test: completion: add tests for __git_complete

[Felipe Contreras]

Even though the function was marked as not public, it's already used in
the wild.

We should at least test basic functionality.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 t/t9902-completion.sh | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index a1c4f1f6d4..c0b4380eae 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -2380,4 +2380,12 @@ test_expect_success 'sourcing the completion script clears cached --options' '
 	verbose test -z "$__gitcomp_builtin_notes_edit"
 '
 
+test_expect_success '__git_complete' '
+	unset -f __git_wrap__git_main &&
+	__git_complete foo __git_main &&
+	__git_have_func __git_wrap__git_main &&
+	__git_complete gf _git_fetch &&
+	__git_have_func __git_wrap_git_fetch
+'
+
 test_done
-- 
2.30.0

[PATCH v2 3/3] completion: add proper public __git_complete

[Felipe Contreras]

When __git_complete was introduced, it was meant to be temporarily, while
a proper guideline for public shell functions was established
(tentatively _GIT_complete), but since that never happened, people
in the wild started to use __git_complete, even though it was marked as
not public.

Eight years is more than enough wait, let's mark this function as
public, and make it a bit more user-friendly.

So that instead of doing:

  __git_complete gk __gitk_main

The user can do:

  __git_complete gk gitk

And instead of:

  __git_complete gf _git_fetch

Do:

  __git_complete gf git_fetch

Backwards compatibility is maintained.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 contrib/completion/git-completion.bash | 31 ++++++++++++++++++++------
 t/t9902-completion.sh                  | 14 +++++++++++-
 2 files changed, 37 insertions(+), 8 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 869c73ee2c..1179bd2876 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -3497,10 +3497,7 @@ __git_func_wrap ()
 	$1
 }
 
-# Setup completion for certain functions defined above by setting common
-# variables and workarounds.
-# This is NOT a public function; use at your own risk.
-__git_complete ()
+___git_complete ()
 {
 	local wrapper="__git_wrap${2}"
 	eval "$wrapper () { __git_func_wrap $2 ; }"
@@ -3508,13 +3505,33 @@ __git_complete ()
 		|| complete -o default -o nospace -F $wrapper $1
 }
 
-__git_complete git __git_main
-__git_complete gitk __gitk_main
+# Setup the completion for git commands
+# 1: command or alias
+# 2: function to call (e.g. `git`, `gitk`, `git_fetch`)
+__git_complete ()
+{
+	local func
+
+	if __git_have_func $2; then
+		func=$2
+	elif __git_have_func __$2_main; then
+		func=__$2_main
+	elif __git_have_func _$2; then
+		func=_$2
+	else
+		echo "ERROR: could not find function '$2'" 1>&2
+		return 1
+	fi
+	___git_complete $1 $func
+}
+
+___git_complete git __git_main
+___git_complete gitk __gitk_main
 
 # The following are necessary only for Cygwin, and only are needed
 # when the user has tab-completed the executable name and consequently
 # included the '.exe' suffix.
 #
 if [ "$OSTYPE" = cygwin ]; then
-	__git_complete git.exe __git_main
+	___git_complete git.exe __git_main
 fi
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index c0b4380eae..c4a7758409 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -2382,10 +2382,22 @@ test_expect_success 'sourcing the completion script clears cached --options' '
 
 test_expect_success '__git_complete' '
 	unset -f __git_wrap__git_main &&
+
 	__git_complete foo __git_main &&
 	__git_have_func __git_wrap__git_main &&
+	unset -f __git_wrap__git_main &&
+
 	__git_complete gf _git_fetch &&
-	__git_have_func __git_wrap_git_fetch
+	__git_have_func __git_wrap_git_fetch &&
+
+	__git_complete foo git &&
+	__git_have_func __git_wrap__git_main &&
+	unset -f __git_wrap__git_main &&
+
+	__git_complete gd git_diff &&
+	__git_have_func __git_wrap_git_diff &&
+
+	test_must_fail __git_complete ga missing
 '
 
 test_done
-- 
2.30.0

Re: [PATCH v2 1/3] completion: bash: add __git_have_func helper

[René Scharfe]

Am 29.12.20 um 18:08 schrieb Felipe Contreras:
> This makes the code more readable, and also will help when new code
> wants to do similar checks.
>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  contrib/completion/git-completion.bash | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 463a3124da..869c73ee2c 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -3358,15 +3358,19 @@ __git_support_parseopt_helper () {
>  	esac
>  }
>
> +__git_have_func () {
> +	declare -f $1 >/dev/null 2>/dev/null

I stumbled slightly over the lack of quoting.  It doesn't matter for
the callers below, but new callers passing arbitrary strings could
cause strange effects:

	x() { echo x; }
	y() { echo y; }
	__git_have_func "x y" # succeeds

	__git_have_func -a # succeeds

I just skimmed patch 3, but it seems to call __git_have_func with
user-supplied strings, so this might become relevant.

And then I wondered why use declare -f, which prints the function's
body, when there is -F, which just prints the function's name.  And why
repeat /dev/null when redirecting stderr when the more shorter 2>&1
would do the same?  None of hat was introduced by you patch, of course.
Anyway, this seems to work for me:

	__git_have_func () {
		case "$1" in
		-*) return 1 ;;
		esac
		declare -F "$1" >/dev/null 2>&1
	}

> +}
> +
>  __git_complete_command () {
>  	local command="$1"
>  	local completion_func="_git_${command//-/_}"
> -	if ! declare -f $completion_func >/dev/null 2>/dev/null &&
> -		declare -f _completion_loader >/dev/null 2>/dev/null
> +	if ! __git_have_func $completion_func &&
> +		__git_have_func _completion_loader
>  	then
>  		_completion_loader "git-$command"
>  	fi
> -	if declare -f $completion_func >/dev/null 2>/dev/null
> +	if __git_have_func $completion_func
>  	then
>  		$completion_func
>  		return 0
>

Re: [PATCH v2 1/3] completion: bash: add __git_have_func helper

[Felipe Contreras]

René Scharfe wrote:
> Am 29.12.20 um 18:08 schrieb Felipe Contreras:
> > This makes the code more readable, and also will help when new code
> > wants to do similar checks.
> >
> > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> > ---
> >  contrib/completion/git-completion.bash | 10 +++++++---
> >  1 file changed, 7 insertions(+), 3 deletions(-)
> >
> > diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> > index 463a3124da..869c73ee2c 100644
> > --- a/contrib/completion/git-completion.bash
> > +++ b/contrib/completion/git-completion.bash
> > @@ -3358,15 +3358,19 @@ __git_support_parseopt_helper () {
> >  	esac
> >  }
> >
> > +__git_have_func () {
> > +	declare -f $1 >/dev/null 2>/dev/null
> 
> I stumbled slightly over the lack of quoting.  It doesn't matter for
> the callers below, but new callers passing arbitrary strings could
> cause strange effects:
> 
> 	x() { echo x; }
> 	y() { echo y; }
> 	__git_have_func "x y" # succeeds
> 
> 	__git_have_func -a # succeeds
> 
> I just skimmed patch 3, but it seems to call __git_have_func with
> user-supplied strings, so this might become relevant.

Yes. I just just copied the code to minimize the changes, but this is a
valid concern.

> And then I wondered why use declare -f, which prints the function's
> body, when there is -F, which just prints the function's name.  And why
> repeat /dev/null when redirecting stderr when the more shorter 2>&1
> would do the same?  None of hat was introduced by you patch, of course.
> Anyway, this seems to work for me:
> 
> 	__git_have_func () {
> 		case "$1" in
> 		-*) return 1 ;;
> 		esac
> 		declare -F "$1" >/dev/null 2>&1
> 	}

I wondered some of those things too, but opted for the minimal approach.

Your change seems good to me, however I prefer this to the case
statement:

  [[ "$1" == -* ]] && return 1

But doesn't seem to be too welcomed in git's bash style.

Looks like this would be prefered:

  if [[ "$1" == -* ]]; then
    return 1
  fi

I would prefer either one of those to the case statement.

But the other change is good. I'll include that as a separate patch on
the next version.

Cheers.

-- 
Felipe Contreras

Re: [PATCH v2 1/3] completion: bash: add __git_have_func helper

[Felipe Contreras]

Felipe Contreras wrote:
> René Scharfe wrote:
> > And then I wondered why use declare -f, which prints the function's
> > body, when there is -F, which just prints the function's name.  And why
> > repeat /dev/null when redirecting stderr when the more shorter 2>&1
> > would do the same?  None of hat was introduced by you patch, of course.
> > Anyway, this seems to work for me:
> > 
> > 	__git_have_func () {
> > 		case "$1" in
> > 		-*) return 1 ;;
> > 		esac
> > 		declare -F "$1" >/dev/null 2>&1
> > 	}

Actually... How about:

  declare -F -- "$1" >/dev/null 2>&1

?

-- 
Felipe Contreras

Re: [PATCH v2 1/3] completion: bash: add __git_have_func helper

[SZEDER Gábor]

On Wed, Dec 30, 2020 at 06:17:52PM +0100, René Scharfe wrote:
> > +__git_have_func () {
> > +	declare -f $1 >/dev/null 2>/dev/null

> And then I wondered why use declare -f, which prints the function's
> body, when there is -F, which just prints the function's name.  And why
> repeat /dev/null when redirecting stderr when the more shorter 2>&1
> would do the same?  None of hat was introduced by you patch, of course.
> Anyway, this seems to work for me:
> 
> 	__git_have_func () {
> 		case "$1" in
> 		-*) return 1 ;;
> 		esac
> 		declare -F "$1" >/dev/null 2>&1
> 	}

The Bash completion script should be usable from Zsh as well, and Zsh
only supports 'declare -f' but not '-F', see 06f44c3cc5 (completion:
make compatible with zsh, 2010-09-06).  The Zsh version included in
the 16.04 based LTS setup that I have at hand doesn't yet seem to have
a (for us) usable 'declare -F' yet.

Re: [PATCH v2 1/3] completion: bash: add __git_have_func helper

[Felipe Contreras]

SZEDER Gábor wrote:
> On Wed, Dec 30, 2020 at 06:17:52PM +0100, René Scharfe wrote:
> > > +__git_have_func () {
> > > +	declare -f $1 >/dev/null 2>/dev/null
> 
> > And then I wondered why use declare -f, which prints the function's
> > body, when there is -F, which just prints the function's name.  And why
> > repeat /dev/null when redirecting stderr when the more shorter 2>&1
> > would do the same?  None of hat was introduced by you patch, of course.
> > Anyway, this seems to work for me:
> > 
> > 	__git_have_func () {
> > 		case "$1" in
> > 		-*) return 1 ;;
> > 		esac
> > 		declare -F "$1" >/dev/null 2>&1
> > 	}
> 
> The Bash completion script should be usable from Zsh as well, and Zsh
> only supports 'declare -f' but not '-F', see 06f44c3cc5 (completion:
> make compatible with zsh, 2010-09-06).  The Zsh version included in
> the 16.04 based LTS setup that I have at hand doesn't yet seem to have
> a (for us) usable 'declare -F' yet.

Ahh. Good catch.

  declare -f -- "$1" >/dev/null 2>&1

Then.

-- 
Felipe Contreras

Re: [PATCH v2 1/3] completion: bash: add __git_have_func helper

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

>> Anyway, this seems to work for me:
>> 
>> 	__git_have_func () {
>> 		case "$1" in
>> 		-*) return 1 ;;
>> 		esac
>> 		declare -F "$1" >/dev/null 2>&1
>> 	}
>
> I wondered some of those things too, but opted for the minimal approach.
>
> Your change seems good to me, however I prefer this to the case
> statement:
>
>   [[ "$1" == -* ]] && return 1
>
> But doesn't seem to be too welcomed in git's bash style.

Actually, there is no formal "bash style" guide for our codebase.
In bash completion script, you are free to use any bashisms as you
find convenient, as long as it is consistent with the surrounding
code.

As SZEDER and you later agree in the downthread, Zsh compatibility
would be a lot more important than sticking to general POSIX shell
subset (and following our shell style guide for the rest of the
system that tries hard to be independent from bashism) when
improving this script.

> Looks like this would be prefered:
>
>   if [[ "$1" == -* ]]; then
>     return 1
>   fi

This is another example that we do not apply the shell style guide
in Documentation/CodingGuidelines to this script (if we did, we
wouldn't see the semicolon there).

Thanks.