[RFC/PATCH 0/5] completion: compgen/compadd cleanups

STALE3737d

Revision rfc of 2 in this series.

15 messages, 2 authors, 2016-06-15 · open the first message on its own page

[RFC/PATCH 0/5] completion: compgen/compadd cleanups

From: Felipe Contreras <hidden>
Date: 2016-06-15 22:55:18

Hi,

These were hinted by SZEDER, so I went ahead and implemented the changes trying
to keep in mind the new zsh completion werapper. The resulting code should be
more efficient, and a known breakage is fixed.

The first two patches come from another patch series for convenience.

Take them with a pinch of salt.

Felipe Contreras (5):
  completion: get rid of empty COMPREPLY assignments
  completion: add new __gitcompadd helper
  completion: trivial test improvement
  completion: get rid of compgen
  completion: refactor __gitcomp_1

 contrib/completion/git-completion.bash | 68 +++++++++++++---------------------
 t/t9902-completion.sh                  |  3 +-
 2 files changed, 27 insertions(+), 44 deletions(-)

-- 
1.8.0

[RFC/PATCH 3/5] completion: trivial test improvement

From: Felipe Contreras <hidden>
Date: 2016-06-15 22:55:18

Instead of passing a dummy "", let's check if the last character is a
space, and then move the _cword accordingly.

Apparently we were passing "" all the way to compgen, which fortunately
expanded it to nothing.

Lets do the right thing though.

Signed-off-by: Felipe Contreras <redacted>
---
 t/t9902-completion.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index cbd0fb6..0b5e1f5 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -51,6 +51,7 @@ run_completion ()
 	local _cword
 	_words=( $1 )
 	(( _cword = ${#_words[@]} - 1 ))
+	test "${1: -1}" == ' ' && (( _cword += 1 ))
 	__git_wrap__git_main && print_comp
 }
 
@@ -156,7 +157,7 @@ test_expect_success '__gitcomp - suffix' '
 '
 
 test_expect_success 'basic' '
-	run_completion "git \"\"" &&
+	run_completion "git " &&
 	# built-in
 	grep -q "^add \$" out &&
 	# script
-- 
1.8.0

[RFC/PATCH 1/5] completion: get rid of empty COMPREPLY assignments

From: Felipe Contreras <hidden>
Date: 2016-06-15 22:55:18

There's no functional reason for those, the only purpose they are
supposed to serve is to say "we don't provide any words here", but even
for that it's not used consitently.

Signed-off-by: Felipe Contreras <redacted>
---
 contrib/completion/git-completion.bash | 28 ----------------------------
 1 file changed, 28 deletions(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index be800e0..7bdd6a8 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -238,7 +238,6 @@ __gitcomp ()
 
 	case "$cur_" in
 	--*=)
-		COMPREPLY=()
 		;;
 	*)
 		local IFS=$'\n'
@@ -486,7 +485,6 @@ __git_complete_remote_or_refspec ()
 			case "$cmd" in
 			push) no_complete_refspec=1 ;;
 			fetch)
-				COMPREPLY=()
 				return
 				;;
 			*) ;;
@@ -502,7 +500,6 @@ __git_complete_remote_or_refspec ()
 		return
 	fi
 	if [ $no_complete_refspec = 1 ]; then
-		COMPREPLY=()
 		return
 	fi
 	[ "$remote" = "." ] && remote=
@@ -776,7 +773,6 @@ _git_am ()
 			"
 		return
 	esac
-	COMPREPLY=()
 }
 
 _git_apply ()
@@ -796,7 +792,6 @@ _git_apply ()
 			"
 		return
 	esac
-	COMPREPLY=()
 }
 
 _git_add ()
@@ -811,7 +806,6 @@ _git_add ()
 			"
 		return
 	esac
-	COMPREPLY=()
 }
 
 _git_archive ()
@@ -856,7 +850,6 @@ _git_bisect ()
 		__gitcomp_nl "$(__git_refs)"
 		;;
 	*)
-		COMPREPLY=()
 		;;
 	esac
 }
@@ -969,7 +962,6 @@ _git_clean ()
 		return
 		;;
 	esac
-	COMPREPLY=()
 }
 
 _git_clone ()
@@ -993,7 +985,6 @@ _git_clone ()
 		return
 		;;
 	esac
-	COMPREPLY=()
 }
 
 _git_commit ()
@@ -1027,7 +1018,6 @@ _git_commit ()
 			"
 		return
 	esac
-	COMPREPLY=()
 }
 
 _git_describe ()
@@ -1158,7 +1148,6 @@ _git_fsck ()
 		return
 		;;
 	esac
-	COMPREPLY=()
 }
 
 _git_gc ()
@@ -1169,7 +1158,6 @@ _git_gc ()
 		return
 		;;
 	esac
-	COMPREPLY=()
 }
 
 _git_gitk ()
@@ -1246,7 +1234,6 @@ _git_init ()
 		return
 		;;
 	esac
-	COMPREPLY=()
 }
 
 _git_ls_files ()
@@ -1265,7 +1252,6 @@ _git_ls_files ()
 		return
 		;;
 	esac
-	COMPREPLY=()
 }
 
 _git_ls_remote ()
@@ -1381,7 +1367,6 @@ _git_mergetool ()
 		return
 		;;
 	esac
-	COMPREPLY=()
 }
 
 _git_merge_base ()
@@ -1397,7 +1382,6 @@ _git_mv ()
 		return
 		;;
 	esac
-	COMPREPLY=()
 }
 
 _git_name_rev ()
@@ -1567,7 +1551,6 @@ _git_send_email ()
 		return
 		;;
 	esac
-	COMPREPLY=()
 }
 
 _git_stage ()
@@ -1680,7 +1663,6 @@ _git_config ()
 		return
 		;;
 	*.*)
-		COMPREPLY=()
 		return
 		;;
 	esac
@@ -2060,7 +2042,6 @@ _git_remote ()
 		__gitcomp "$c"
 		;;
 	*)
-		COMPREPLY=()
 		;;
 	esac
 }
@@ -2104,7 +2085,6 @@ _git_rm ()
 		return
 		;;
 	esac
-	COMPREPLY=()
 }
 
 _git_shortlog ()
@@ -2173,8 +2153,6 @@ _git_stash ()
 		*)
 			if [ -z "$(__git_find_on_cmdline "$save_opts")" ]; then
 				__gitcomp "$subcommands"
-			else
-				COMPREPLY=()
 			fi
 			;;
 		esac
@@ -2187,14 +2165,12 @@ _git_stash ()
 			__gitcomp "--index --quiet"
 			;;
 		show,--*|drop,--*|branch,--*)
-			COMPREPLY=()
 			;;
 		show,*|apply,*|drop,*|pop,*|branch,*)
 			__gitcomp_nl "$(git --git-dir="$(__gitdir)" stash list \
 					| sed -n -e 's/:.*//p')"
 			;;
 		*)
-			COMPREPLY=()
 			;;
 		esac
 	fi
@@ -2311,7 +2287,6 @@ _git_svn ()
 			__gitcomp "--revision= --parent"
 			;;
 		*)
-			COMPREPLY=()
 			;;
 		esac
 	fi
@@ -2336,13 +2311,10 @@ _git_tag ()
 
 	case "$prev" in
 	-m|-F)
-		COMPREPLY=()
 		;;
 	-*|tag)
 		if [ $f = 1 ]; then
 			__gitcomp_nl "$(__git_tags)"
-		else
-			COMPREPLY=()
 		fi
 		;;
 	*)
-- 
1.8.0

[RFC/PATCH 2/5] completion: add new __gitcompadd helper

From: Felipe Contreras <hidden>
Date: 2016-06-15 22:55:18

The idea is to never touch the COMPREPLY variable directly.

This allows other completion systems override __gitcompadd, and do
something different instead.

Also, this allows the simplification of the completion tests (separate
patch).

There should be no functional changes.

Signed-off-by: Felipe Contreras <redacted>
---
 contrib/completion/git-completion.bash | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 7bdd6a8..975ae13 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -225,6 +225,11 @@ _get_comp_words_by_ref ()
 fi
 fi
 
+__gitcompadd ()
+{
+	COMPREPLY=($(compgen -W "$1" -P "$2" -S "$4" -- "$3"))
+}
+
 # Generates completion reply with compgen, appending a space to possible
 # completion words, if necessary.
 # It accepts 1 to 4 arguments:
@@ -241,9 +246,7 @@ __gitcomp ()
 		;;
 	*)
 		local IFS=$'\n'
-		COMPREPLY=($(compgen -P "${2-}" \
-			-W "$(__gitcomp_1 "${1-}" "${4-}")" \
-			-- "$cur_"))
+		__gitcompadd "$(__gitcomp_1 "${1-}" "${4-}")" "${2-}" "$cur_" ""
 		;;
 	esac
 }
@@ -260,7 +263,7 @@ __gitcomp ()
 __gitcomp_nl ()
 {
 	local IFS=$'\n'
-	COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$1" -- "${3-$cur}"))
+	__gitcompadd "$1" "${2-}" "${3-$cur}" "${4- }"
 }
 
 __git_heads ()
@@ -1603,7 +1606,7 @@ _git_config ()
 		local remote="${prev#remote.}"
 		remote="${remote%.fetch}"
 		if [ -z "$cur" ]; then
-			COMPREPLY=("refs/heads/")
+			__gitcompadd "refs/heads/"
 			return
 		fi
 		__gitcomp_nl "$(__git_refs_remotes "$remote")"
-- 
1.8.0

[RFC/PATCH 5/5] completion: refactor __gitcomp_1

From: Felipe Contreras <hidden>
Date: 2016-06-15 22:55:18

It's only used by __gitcomp, so move some code there and avoid going
through the loop again.

We could get rid of it altogether, but it's useful for zsh's completion
wrapper.

Signed-off-by: Felipe Contreras <redacted>
---
 contrib/completion/git-completion.bash | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index ad3e1fe..d92d11e 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -58,15 +58,12 @@ __gitdir ()
 
 __gitcomp_1 ()
 {
-	local c IFS=$' \t\n'
-	for c in $1; do
-		c="$c$2"
-		case $c in
-		--*=*|*.) ;;
-		*) c="$c " ;;
-		esac
-		printf '%s\n' "$c"
-	done
+	local c=$1
+	case $c in
+	--*=*|*.) ;;
+	*) c="$c " ;;
+	esac
+	printf '%s\n' "$c"
 }
 
 # The following function is based on code from:
@@ -249,10 +246,16 @@ __gitcomp ()
 	--*=)
 		;;
 	*)
-		local IFS=$'\n'
-		__gitcompadd "$(__gitcomp_1 "${1-}" "${4-}")" "${2-}" "$cur_" ""
+		local c IFS=$' \t\n'
+		for c in ${1-}; do
+			c=`__gitcomp_1 "$c${4-}"`
+			if [[ "$c" = "$cur_"* ]]; then
+				COMPREPLY+=("${2-}$c")
+			fi
+		done
 		;;
 	esac
+
 }
 
 # Generates completion reply with compgen from newline-separated possible
-- 
1.8.0

[RFC/PATCH 4/5] completion: get rid of compgen

From: Felipe Contreras <hidden>
Date: 2016-06-15 22:55:18

The functionality we use is very simple, plus, this fixes a known
breakage 'complete tree filename with metacharacters'.

Signed-off-by: Felipe Contreras <redacted>
---
 contrib/completion/git-completion.bash | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 975ae13..ad3e1fe 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -227,7 +227,11 @@ fi
 
 __gitcompadd ()
 {
-	COMPREPLY=($(compgen -W "$1" -P "$2" -S "$4" -- "$3"))
+	for x in $1; do
+		if [[ "$x" = "$3"* ]]; then
+			COMPREPLY+=("$2$x$4")
+		fi
+	done
 }
 
 # Generates completion reply with compgen, appending a space to possible
-- 
1.8.0

Re: [RFC/PATCH 5/5] completion: refactor __gitcomp_1

From: SZEDER Gábor <hidden>
Date: 2016-06-15 22:55:18

[Wow, that's quite the Cc-list above.  I wonder why e.g. Robert ended
up there, when all his "sins" were to add a couple of 'git svn'
options back in 2009.]

On Sat, Nov 17, 2012 at 02:38:18AM +0100, Felipe Contreras wrote:
quoted hunk
It's only used by __gitcomp, so move some code there and avoid going
through the loop again.

We could get rid of it altogether, but it's useful for zsh's completion
wrapper.

Signed-off-by: Felipe Contreras <redacted>
---
 contrib/completion/git-completion.bash | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index ad3e1fe..d92d11e 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -58,15 +58,12 @@ __gitdir ()
 
 __gitcomp_1 ()
 {
-	local c IFS=$' \t\n'
-	for c in $1; do
-		c="$c$2"
-		case $c in
-		--*=*|*.) ;;
-		*) c="$c " ;;
-		esac
-		printf '%s\n' "$c"
-	done
+	local c=$1
+	case $c in
+	--*=*|*.) ;;
+	*) c="$c " ;;
+	esac
+	printf '%s\n' "$c"
 }
 
 # The following function is based on code from:
@@ -249,10 +246,16 @@ __gitcomp ()
 	--*=)
 		;;
 	*)
-		local IFS=$'\n'
-		__gitcompadd "$(__gitcomp_1 "${1-}" "${4-}")" "${2-}" "$cur_" ""
+		local c IFS=$' \t\n'
+		for c in ${1-}; do
+			c=`__gitcomp_1 "$c${4-}"`
1. Backticks.
2. A subshell for every word in the wordlist?
+			if [[ "$c" = "$cur_"* ]]; then
+				COMPREPLY+=("${2-}$c")
This is the first time we use the append operator in the completion
script.  When it came up last time the question was whether the
benefit of using it is large enough for worrying about supported Bash
versions.

  http://article.gmane.org/gmane.comp.version-control.git/206525
+			fi
+		done
 		;;
 	esac
+
 }
 
 # Generates completion reply with compgen from newline-separated possible
-- 
1.8.0

Re: [RFC/PATCH 3/5] completion: trivial test improvement

From: SZEDER Gábor <hidden>
Date: 2016-06-15 22:55:18

On Sat, Nov 17, 2012 at 02:38:16AM +0100, Felipe Contreras wrote:
Instead of passing a dummy "", let's check if the last character is a
space, and then move the _cword accordingly.

Apparently we were passing "" all the way to compgen, which fortunately
expanded it to nothing.
Glad you noticed it, too.
I posted an alternative fix (without any new conditions in the code
path) a while ago:

  http://article.gmane.org/gmane.comp.version-control.git/206525

Will repost it as part of my series shortly.
quoted hunk
Lets do the right thing though.

Signed-off-by: Felipe Contreras <redacted>
---
 t/t9902-completion.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index cbd0fb6..0b5e1f5 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -51,6 +51,7 @@ run_completion ()
 	local _cword
 	_words=( $1 )
 	(( _cword = ${#_words[@]} - 1 ))
+	test "${1: -1}" == ' ' && (( _cword += 1 ))
 	__git_wrap__git_main && print_comp
 }
 
@@ -156,7 +157,7 @@ test_expect_success '__gitcomp - suffix' '
 '
 
 test_expect_success 'basic' '
-	run_completion "git \"\"" &&
+	run_completion "git " &&
 	# built-in
 	grep -q "^add \$" out &&
 	# script
-- 
1.8.0

Re: [RFC/PATCH 4/5] completion: get rid of compgen

From: SZEDER Gábor <hidden>
Date: 2016-06-15 22:55:18

On Sat, Nov 17, 2012 at 02:38:17AM +0100, Felipe Contreras wrote:
quoted hunk
The functionality we use is very simple, plus, this fixes a known
breakage 'complete tree filename with metacharacters'.

Signed-off-by: Felipe Contreras <redacted>
---
 contrib/completion/git-completion.bash | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 975ae13..ad3e1fe 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -227,7 +227,11 @@ fi
 
 __gitcompadd ()
 {
-	COMPREPLY=($(compgen -W "$1" -P "$2" -S "$4" -- "$3"))
+	for x in $1; do
+		if [[ "$x" = "$3"* ]]; then
+			COMPREPLY+=("$2$x$4")
+		fi
+	done
The whole point of creating __gitcomp_nl() back then was to fill
COMPREPLY without iterating through all words in the wordlist, making
completion faster for large number of words, e.g. a lot of refs, or
later a lot of symbols for 'git grep' in a larger project.

The loop here kills that optimization.
 }
 
 # Generates completion reply with compgen, appending a space to possible
-- 
1.8.0

Re: [RFC/PATCH 5/5] completion: refactor __gitcomp_1

From: Felipe Contreras <hidden>
Date: 2016-06-15 22:55:19

On Sat, Nov 17, 2012 at 11:58 AM, SZEDER Gábor [off-list ref] wrote:
quoted
 # The following function is based on code from:
@@ -249,10 +246,16 @@ __gitcomp ()
      --*=)
              ;;
      *)
-             local IFS=$'\n'
-             __gitcompadd "$(__gitcomp_1 "${1-}" "${4-}")" "${2-}" "$cur_" ""
+             local c IFS=$' \t\n'
+             for c in ${1-}; do
+                     c=`__gitcomp_1 "$c${4-}"`
1. Backticks.
2. A subshell for every word in the wordlist?
Fine, lets make it hard for zsh then:
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -56,19 +56,6 @@ __gitdir ()
        fi
 }

-__gitcomp_1 ()
-{
-       local c IFS=$' \t\n'
-       for c in $1; do
-               c="$c$2"
-               case $c in
-               --*=*|*.) ;;
-               *) c="$c " ;;
-               esac
-               printf '%s\n' "$c"
-       done
-}
-
 # The following function is based on code from:
 #
 #   bash_completion - programmable completion functions for bash 3.2+
@@ -241,12 +228,22 @@ __gitcomp ()
                COMPREPLY=()
                ;;
        *)
-               local IFS=$'\n'
-               COMPREPLY=($(compgen -P "${2-}" \
-                       -W "$(__gitcomp_1 "${1-}" "${4-}")" \
-                       -- "$cur_"))
+               local c i IFS=$' \t\n'
+               i=0
+               for c in ${1-}; do
+                       c="$c${4-}"
+                       case $c in
+                       --*=*|*.) ;;
+                       *) c="$c " ;;
+                       esac
+                       if [[ "$c" = "$cur_"* ]]; then
+                               (( i++ ))
+                               COMPREPLY[$i]="${2-}$c"
+                       fi
+               done
                ;;
        esac
+
 }

 # Generates completion reply with compgen from newline-separated possible
-- 
Felipe Contreras

Re: [RFC/PATCH 4/5] completion: get rid of compgen

From: Felipe Contreras <hidden>
Date: 2016-06-15 22:55:19

On Sat, Nov 17, 2012 at 12:00 PM, SZEDER Gábor [off-list ref] wrote:
On Sat, Nov 17, 2012 at 02:38:17AM +0100, Felipe Contreras wrote:
quoted
The functionality we use is very simple, plus, this fixes a known
breakage 'complete tree filename with metacharacters'.

Signed-off-by: Felipe Contreras <redacted>
---
 contrib/completion/git-completion.bash | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 975ae13..ad3e1fe 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -227,7 +227,11 @@ fi

 __gitcompadd ()
 {
-     COMPREPLY=($(compgen -W "$1" -P "$2" -S "$4" -- "$3"))
+     for x in $1; do
+             if [[ "$x" = "$3"* ]]; then
+                     COMPREPLY+=("$2$x$4")
+             fi
+     done
The whole point of creating __gitcomp_nl() back then was to fill
COMPREPLY without iterating through all words in the wordlist, making
completion faster for large number of words, e.g. a lot of refs, or
later a lot of symbols for 'git grep' in a larger project.

The loop here kills that optimization.
So your solution is to move the loop to awk? I fail to see how that
could bring more optimization, specially since it includes an extra
fork now.

-- 
Felipe Contreras

Re: [RFC/PATCH 4/5] completion: get rid of compgen

From: SZEDER Gábor <hidden>
Date: 2016-06-15 22:55:19

On Sat, Nov 17, 2012 at 12:42:38PM +0100, Felipe Contreras wrote:
On Sat, Nov 17, 2012 at 12:00 PM, SZEDER Gábor [off-list ref] wrote:
quoted
On Sat, Nov 17, 2012 at 02:38:17AM +0100, Felipe Contreras wrote:
quoted
The functionality we use is very simple, plus, this fixes a known
breakage 'complete tree filename with metacharacters'.

Signed-off-by: Felipe Contreras <redacted>
---
 contrib/completion/git-completion.bash | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 975ae13..ad3e1fe 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -227,7 +227,11 @@ fi

 __gitcompadd ()
 {
-     COMPREPLY=($(compgen -W "$1" -P "$2" -S "$4" -- "$3"))
+     for x in $1; do
+             if [[ "$x" = "$3"* ]]; then
+                     COMPREPLY+=("$2$x$4")
+             fi
+     done
The whole point of creating __gitcomp_nl() back then was to fill
COMPREPLY without iterating through all words in the wordlist, making
completion faster for large number of words, e.g. a lot of refs, or
later a lot of symbols for 'git grep' in a larger project.

The loop here kills that optimization.
So your solution is to move the loop to awk? I fail to see how that
could bring more optimization, specially since it includes an extra
fork now.
This patch didn't aim for more optimization, but it was definitely a
goal not to waste what we gained by creating __gitcomp_nl() in
a31e6262 (completion: optimize refs completion, 2011-10-15).  However,
as it turns out the new version with awk is actually faster than
current master with compgen:

  Before:

    $ refs="$(for i in {0..9999} ; do echo branch$i ; done)"
    $ time __gitcomp_nl "$refs"

    real    0m0.242s
    user    0m0.220s
    sys     0m0.028s

  After:

    $ time __gitcomp_nl "$refs"

    real    0m0.109s
    user    0m0.096s
    sys     0m0.012s

Re: [RFC/PATCH 5/5] completion: refactor __gitcomp_1

From: SZEDER Gábor <hidden>
Date: 2016-06-15 22:55:19

On Sat, Nov 17, 2012 at 12:27:40PM +0100, Felipe Contreras wrote:
On Sat, Nov 17, 2012 at 11:58 AM, SZEDER Gábor [off-list ref] wrote:
quoted
quoted
 # The following function is based on code from:
@@ -249,10 +246,16 @@ __gitcomp ()
      --*=)
              ;;
      *)
-             local IFS=$'\n'
-             __gitcompadd "$(__gitcomp_1 "${1-}" "${4-}")" "${2-}" "$cur_" ""
+             local c IFS=$' \t\n'
+             for c in ${1-}; do
+                     c=`__gitcomp_1 "$c${4-}"`
1. Backticks.
2. A subshell for every word in the wordlist?
Fine, lets make it hard for zsh then:
No, it's about keeping it usable.  With this change offering the
approximately 170 commands for 'git help <TAB>' would take more than 4
seconds on Windows.

quoted hunk
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -56,19 +56,6 @@ __gitdir ()
        fi
 }

-__gitcomp_1 ()
-{
-       local c IFS=$' \t\n'
-       for c in $1; do
-               c="$c$2"
-               case $c in
-               --*=*|*.) ;;
-               *) c="$c " ;;
-               esac
-               printf '%s\n' "$c"
-       done
-}
-
 # The following function is based on code from:
 #
 #   bash_completion - programmable completion functions for bash 3.2+
@@ -241,12 +228,22 @@ __gitcomp ()
                COMPREPLY=()
                ;;
        *)
-               local IFS=$'\n'
-               COMPREPLY=($(compgen -P "${2-}" \
-                       -W "$(__gitcomp_1 "${1-}" "${4-}")" \
-                       -- "$cur_"))
+               local c i IFS=$' \t\n'
+               i=0
+               for c in ${1-}; do
+                       c="$c${4-}"
+                       case $c in
+                       --*=*|*.) ;;
+                       *) c="$c " ;;
+                       esac
+                       if [[ "$c" = "$cur_"* ]]; then
+                               (( i++ ))
+                               COMPREPLY[$i]="${2-}$c"
+                       fi
+               done
                ;;
        esac
+
 }

 # Generates completion reply with compgen from newline-separated possible
-- 
Felipe Contreras

Re: [RFC/PATCH 4/5] completion: get rid of compgen

From: Felipe Contreras <hidden>
Date: 2016-06-15 22:55:19

On Sat, Nov 17, 2012 at 3:12 PM, SZEDER Gábor [off-list ref] wrote:
On Sat, Nov 17, 2012 at 12:42:38PM +0100, Felipe Contreras wrote:
quoted
On Sat, Nov 17, 2012 at 12:00 PM, SZEDER Gábor [off-list ref] wrote:
quoted
On Sat, Nov 17, 2012 at 02:38:17AM +0100, Felipe Contreras wrote:
quoted
The functionality we use is very simple, plus, this fixes a known
breakage 'complete tree filename with metacharacters'.

Signed-off-by: Felipe Contreras <redacted>
---
 contrib/completion/git-completion.bash | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 975ae13..ad3e1fe 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -227,7 +227,11 @@ fi

 __gitcompadd ()
 {
-     COMPREPLY=($(compgen -W "$1" -P "$2" -S "$4" -- "$3"))
+     for x in $1; do
+             if [[ "$x" = "$3"* ]]; then
+                     COMPREPLY+=("$2$x$4")
+             fi
+     done
The whole point of creating __gitcomp_nl() back then was to fill
COMPREPLY without iterating through all words in the wordlist, making
completion faster for large number of words, e.g. a lot of refs, or
later a lot of symbols for 'git grep' in a larger project.

The loop here kills that optimization.
So your solution is to move the loop to awk? I fail to see how that
could bring more optimization, specially since it includes an extra
fork now.
This patch didn't aim for more optimization, but it was definitely a
goal not to waste what we gained by creating __gitcomp_nl() in
a31e6262 (completion: optimize refs completion, 2011-10-15).  However,
as it turns out the new version with awk is actually faster than
current master with compgen:

  Before:

    $ refs="$(for i in {0..9999} ; do echo branch$i ; done)"
    $ time __gitcomp_nl "$refs"

    real    0m0.242s
    user    0m0.220s
    sys     0m0.028s

  After:

    $ time __gitcomp_nl "$refs"

    real    0m0.109s
    user    0m0.096s
    sys     0m0.012s
This one is even faster:

while read -r x; do
	if [[ "$x" == "$3"* ]]; then
		COMPREPLY+=("$2$x$4")
	fi
done <<< $1

== 10000 ==
one:
real	0m0.148s
user	0m0.134s
sys	0m0.025s
two:
real	0m0.055s
user	0m0.054s
sys	0m0.000s

-- 
Felipe Contreras

Re: [RFC/PATCH 4/5] completion: get rid of compgen

From: Felipe Contreras <hidden>
Date: 2016-06-15 22:55:19

On Sat, Nov 17, 2012 at 8:33 PM, Felipe Contreras
[off-list ref] wrote:
On Sat, Nov 17, 2012 at 3:12 PM, SZEDER Gábor [off-list ref] wrote:
quoted
On Sat, Nov 17, 2012 at 12:42:38PM +0100, Felipe Contreras wrote:
quoted
On Sat, Nov 17, 2012 at 12:00 PM, SZEDER Gábor [off-list ref] wrote:
quoted
On Sat, Nov 17, 2012 at 02:38:17AM +0100, Felipe Contreras wrote:
quoted
The functionality we use is very simple, plus, this fixes a known
breakage 'complete tree filename with metacharacters'.

Signed-off-by: Felipe Contreras <redacted>
---
 contrib/completion/git-completion.bash | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 975ae13..ad3e1fe 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -227,7 +227,11 @@ fi

 __gitcompadd ()
 {
-     COMPREPLY=($(compgen -W "$1" -P "$2" -S "$4" -- "$3"))
+     for x in $1; do
+             if [[ "$x" = "$3"* ]]; then
+                     COMPREPLY+=("$2$x$4")
+             fi
+     done
The whole point of creating __gitcomp_nl() back then was to fill
COMPREPLY without iterating through all words in the wordlist, making
completion faster for large number of words, e.g. a lot of refs, or
later a lot of symbols for 'git grep' in a larger project.

The loop here kills that optimization.
So your solution is to move the loop to awk? I fail to see how that
could bring more optimization, specially since it includes an extra
fork now.
This patch didn't aim for more optimization, but it was definitely a
goal not to waste what we gained by creating __gitcomp_nl() in
a31e6262 (completion: optimize refs completion, 2011-10-15).  However,
as it turns out the new version with awk is actually faster than
current master with compgen:

  Before:

    $ refs="$(for i in {0..9999} ; do echo branch$i ; done)"
    $ time __gitcomp_nl "$refs"

    real    0m0.242s
    user    0m0.220s
    sys     0m0.028s

  After:

    $ time __gitcomp_nl "$refs"

    real    0m0.109s
    user    0m0.096s
    sys     0m0.012s
This one is even faster:

while read -r x; do
        if [[ "$x" == "$3"* ]]; then
                COMPREPLY+=("$2$x$4")
        fi
done <<< $1

== 10000 ==
one:
real    0m0.148s
user    0m0.134s
sys     0m0.025s
two:
real    0m0.055s
user    0m0.054s
sys     0m0.000s
Ah, nevermind, I didn't quote the $1.

However, this one is quite close and much simpler:

	local IFS=$'\n'
	COMPREPLY=($(printf -- "$2%s$4\n" $1 | grep "^$2$3"))

== 10000 ==
awk 1:
real	0m0.064s
user	0m0.062s
sys	0m0.003s
printf:
real	0m0.069s
user	0m0.064s
sys	0m0.020s


-- 
Felipe Contreras
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help