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

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

[Felipe Contreras]

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 1/5] completion: get rid of empty COMPREPLY assignments

[Felipe Contreras]

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 <felipe.contreras@gmail.com>
---
 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

[Felipe Contreras]

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 <felipe.contreras@gmail.com>
---
 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 3/5] completion: trivial test improvement

[Felipe Contreras]

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 <felipe.contreras@gmail.com>
---
 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 4/5] completion: get rid of compgen

[Felipe Contreras]

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

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 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

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

[Felipe Contreras]

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 <felipe.contreras@gmail.com>
---
 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

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

[SZEDER Gábor]

[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:
> 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 <felipe.contreras@gmail.com>
> ---
>  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

[SZEDER Gábor]

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.

> Lets do the right thing though.
> 
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  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

[SZEDER Gábor]

On Sat, Nov 17, 2012 at 02:38:17AM +0100, Felipe Contreras wrote:
> The functionality we use is very simple, plus, this fixes a known
> breakage 'complete tree filename with metacharacters'.
> 
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  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

[Felipe Contreras]

On Sat, Nov 17, 2012 at 11:58 AM, SZEDER Gábor <szeder@ira.uka.de> wrote:

>>  # 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

[Felipe Contreras]

On Sat, Nov 17, 2012 at 12:00 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:
> On Sat, Nov 17, 2012 at 02:38:17AM +0100, Felipe Contreras wrote:
>> The functionality we use is very simple, plus, this fixes a known
>> breakage 'complete tree filename with metacharacters'.
>>
>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>> ---
>>  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

[SZEDER Gábor]

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 <szeder@ira.uka.de> wrote:
> > On Sat, Nov 17, 2012 at 02:38:17AM +0100, Felipe Contreras wrote:
> >> The functionality we use is very simple, plus, this fixes a known
> >> breakage 'complete tree filename with metacharacters'.
> >>
> >> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> >> ---
> >>  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

[SZEDER Gábor]

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 <szeder@ira.uka.de> wrote:
> 
> >>  # 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.


> --- 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

[Felipe Contreras]

On Sat, Nov 17, 2012 at 3:12 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:
> 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 <szeder@ira.uka.de> wrote:
>> > On Sat, Nov 17, 2012 at 02:38:17AM +0100, Felipe Contreras wrote:
>> >> The functionality we use is very simple, plus, this fixes a known
>> >> breakage 'complete tree filename with metacharacters'.
>> >>
>> >> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>> >> ---
>> >>  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

[Felipe Contreras]

On Sat, Nov 17, 2012 at 8:33 PM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> On Sat, Nov 17, 2012 at 3:12 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:
>> 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 <szeder@ira.uka.de> wrote:
>>> > On Sat, Nov 17, 2012 at 02:38:17AM +0100, Felipe Contreras wrote:
>>> >> The functionality we use is very simple, plus, this fixes a known
>>> >> breakage 'complete tree filename with metacharacters'.
>>> >>
>>> >> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>>> >> ---
>>> >>  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