threads / rfc / 32132

RFC patch, 5 partscompletion: trivial test improvement

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

## tl;dr

15 messages between Nov 17, 2012 and Nov 18, 2012. Diffs are folded; open one to read it.

replies: 14people: 2as markdown or json

Felipe Contreras· Nov 17, 2012, 01:38 UTC · lore

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

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
Felipe Contreras· Nov 17, 2012, 01:38 UTC · re: Felipe Contreras · lore

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

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(-)
Show changes to contrib/completion/git-completion.bash +0 −28
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
Felipe Contreras· Nov 17, 2012, 01:38 UTC · re: Felipe Contreras · lore

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

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(-)
Show changes to contrib/completion/git-completion.bash +8 −5
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
Felipe Contreras· Nov 17, 2012, 01:38 UTC · re: Felipe Contreras · lore

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(-)
Show changes to t/t9902-completion.sh +2 −1
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
SZEDER Gábor· Nov 17, 2012, 10:59 UTC · re: Felipe Contreras · lore

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

On Sat, Nov 17, 2012 at 02:38:16AM +0100, Felipe Contreras wrote:
Show 5 quoted lines
> 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.
Show 31 quoted lines
> 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
>
Felipe Contreras· Nov 17, 2012, 01:38 UTC · re: Felipe Contreras · lore

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

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(-)
Show changes to contrib/completion/git-completion.bash +5 −1
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
SZEDER Gábor· Nov 17, 2012, 11:00 UTC · re: Felipe Contreras · lore

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

On Sat, Nov 17, 2012 at 02:38:17AM +0100, Felipe Contreras wrote:
Show 22 quoted lines
> 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.
Show 5 quoted lines
>  }
>  
>  # Generates completion reply with compgen, appending a space to possible
> -- 
> 1.8.0
Felipe Contreras· Nov 17, 2012, 11:42 UTC · re: SZEDER Gábor · lore

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

On Sat, Nov 17, 2012 at 12:00 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:
Show 30 quoted lines
> 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
SZEDER Gábor· Nov 17, 2012, 14:12 UTC · re: Felipe Contreras · lore

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

On Sat, Nov 17, 2012 at 12:42:38PM +0100, Felipe Contreras wrote:
Show 35 quoted lines
> 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
Felipe Contreras· Nov 17, 2012, 19:33 UTC · re: SZEDER Gábor · lore

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

On Sat, Nov 17, 2012 at 3:12 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:
Show 59 quoted lines
> 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
Felipe Contreras· Nov 18, 2012, 00:53 UTC · re: Felipe Contreras · lore

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

On Sat, Nov 17, 2012 at 8:33 PM, Felipe Contreras <felipe.contreras@gmail.com> wrote:

Show 78 quoted lines
> 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
Felipe Contreras· Nov 17, 2012, 01:38 UTC · re: Felipe Contreras · lore

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

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(-)
Show changes to contrib/completion/git-completion.bash +14 −11
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
SZEDER Gábor· Nov 17, 2012, 10:58 UTC · re: Felipe Contreras · lore

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

[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:
Show 46 quoted lines
> 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
Show 11 quoted lines
> +			fi
> +		done
>  		;;
>  	esac
> +
>  }
>  
>  # Generates completion reply with compgen from newline-separated possible
> -- 
> 1.8.0
>
Felipe Contreras· Nov 17, 2012, 11:27 UTC · re: SZEDER Gábor · lore

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

On Sat, Nov 17, 2012 at 11:58 AM, SZEDER Gábor <szeder@ira.uka.de> wrote:
Show 13 quoted lines
>>  # 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:
Show changes to contrib/completion/git-completion.bash +14 −17
--- 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
SZEDER Gábor· Nov 17, 2012, 14:13 UTC · re: Felipe Contreras · lore

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

On Sat, Nov 17, 2012 at 12:27:40PM +0100, Felipe Contreras wrote:
Show 17 quoted lines
> 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.

Show 52 quoted lines
> --- 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

← back to recent threads