threads / patch / 54900

v2, 3 partscompletion: make __git_complete public

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

## tl;dr

10 messages between Dec 29, 2020 and Jan 6, 2021. Diffs are folded; open one to read it.

replies: 9people: 4as markdown or json

Felipe Contreras· Dec 29, 2020, 17:08 UTC · lore

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
Felipe Contreras· Dec 29, 2020, 17:08 UTC · re: Felipe Contreras · lore

[PATCH v2 2/3] 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 | 8 ++++++++
 1 file changed, 8 insertions(+)
Show changes to t/t9902-completion.sh +8 −0
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
Felipe Contreras· Dec 29, 2020, 17:08 UTC · re: Felipe Contreras · lore

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

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(-)
Show changes to contrib/completion/git-completion.bash +7 −3
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
René Scharfe· Dec 30, 2020, 17:17 UTC · re: Felipe Contreras · lore

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

Am 29.12.20 um 18:08 schrieb Felipe Contreras:
Show 18 quoted lines
> 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
	}
Show 18 quoted lines
> +}
> +
>  __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
>
Felipe Contreras· Dec 30, 2020, 17:39 UTC · re: René Scharfe · lore

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

René Scharfe wrote:
Show 32 quoted lines
> 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.

Show 12 quoted lines
> 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
Felipe Contreras· Dec 30, 2020, 17:58 UTC · re: Felipe Contreras · lore

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

Felipe Contreras wrote:
Show 13 quoted lines
> 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
Junio C Hamano· Jan 6, 2021, 08:30 UTC · re: Felipe Contreras · lore

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

Felipe Contreras <felipe.contreras@gmail.com> writes:
Show 17 quoted lines
>> 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.

Show 5 quoted lines
> 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.
SZEDER Gábor· Dec 30, 2020, 18:00 UTC · re: René Scharfe · lore

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

On Wed, Dec 30, 2020 at 06:17:52PM +0100, René Scharfe wrote:
> > +__git_have_func () {
> > +	declare -f $1 >/dev/null 2>/dev/null
Show 12 quoted lines
> 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.

Felipe Contreras· Dec 30, 2020, 18:09 UTC · re: SZEDER Gábor · lore

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

SZEDER Gábor wrote:
Show 22 quoted lines
> 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
Felipe Contreras· Dec 29, 2020, 17:08 UTC · re: Felipe Contreras · lore

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

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(-)
Show changes to 2 files +37 −8

contrib/completion/git-completion.bash, t/t9902-completion.sh

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

← back to recent threads