[PATCH v3 0/4] completion: make __git_complete public

[PATCH v3 0/4] 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.

Changes since v2:

 * Improved the safety of __git_have_func with suggestions from René Scharfe
 * Added documentation to the top of git-completion.bash

[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 (4):
  completion: bash: add __git_have_func helper
  completion: bash: improve function detection
  test: completion: add tests for __git_complete
  completion: add proper public __git_complete

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

Range-diff against v2:
1:  0993732142 = 1:  0993732142 completion: bash: add __git_have_func helper
-:  ---------- > 2:  6a3fe8b79b completion: bash: improve function detection
2:  7918c34d0e = 3:  b211eebc0d test: completion: add tests for __git_complete
3:  8a6cc52063 ! 4:  a5bfe5376b completion: add proper public __git_complete
    @@ Commit message
         Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
     
      ## contrib/completion/git-completion.bash ##
    +@@
    + # tell the completion to use commit completion.  This also works with aliases
    + # of form "!sh -c '...'".  For example, "!sh -c ': git commit ; ... '".
    + #
    ++# If you have a command that is not part of git, but you would still
    ++# like completion, you can use __git_complete:
    ++#
    ++#   __git_complete gl git_log
    ++#
    ++# Or if it's a main command (i.e. git or gitk):
    ++#
    ++#   __git_complete gk gitk
    ++#
    + # Compatible with bash 3.2.57.
    + #
    + # You can set the following environment variables to influence the behavior of
     @@ contrib/completion/git-completion.bash: __git_func_wrap ()
      	$1
      }
-- 
2.30.0

[PATCH v3 1/4] 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 v3 2/4] completion: bash: improve function detection

[Felipe Contreras]

 1. We should quote the argument
 2. We don't need two redirections
 3. A safeguard for arguments (-a) would be good

Suggested-by: René Scharfe <l.s.r@web.de>
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 contrib/completion/git-completion.bash | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 869c73ee2c..1150d4bf44 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -3359,7 +3359,7 @@ __git_support_parseopt_helper () {
 }
 
 __git_have_func () {
-	declare -f $1 >/dev/null 2>/dev/null
+	declare -f -- "$1" >/dev/null 2>&1
 }
 
 __git_complete_command () {
-- 
2.30.0

[PATCH v3 3/4] 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 v3 4/4] 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 | 40 +++++++++++++++++++++-----
 t/t9902-completion.sh                  | 14 ++++++++-
 2 files changed, 46 insertions(+), 8 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 1150d4bf44..4b1f4264a6 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -29,6 +29,15 @@
 # tell the completion to use commit completion.  This also works with aliases
 # of form "!sh -c '...'".  For example, "!sh -c ': git commit ; ... '".
 #
+# If you have a command that is not part of git, but you would still
+# like completion, you can use __git_complete:
+#
+#   __git_complete gl git_log
+#
+# Or if it's a main command (i.e. git or gitk):
+#
+#   __git_complete gk gitk
+#
 # Compatible with bash 3.2.57.
 #
 # You can set the following environment variables to influence the behavior of
@@ -3497,10 +3506,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 +3514,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