[PATCH v5 0/3] completion: refactor and zsh wrapper

[PATCH v5 0/3] completion: refactor and zsh wrapper

[Felipe Contreras]

Hi,

Here's a bit of reorganition. I'm introducing a new __gitcompadd helper that is
useful to wrapp all changes to COMPREPLY. 2nd and 3rd patches show how it's
useful.

The zsh wrapper is now very very simple, but I haven't received much feedback
yet. I hope it will get in at some point in time.

Felipe Contreras (3):
  completion: add new __gitcompadd helper
  tests: use __gitcompadd to simplify completion tests
  completion: add new zsh completion

 contrib/completion/git-completion.bash | 65 ++++++++++++++++++----------------
 contrib/completion/git-completion.zsh  | 48 +++++++++++++++++++++++++
 t/t9902-completion.sh                  | 29 +++++----------
 3 files changed, 91 insertions(+), 51 deletions(-)
 create mode 100644 contrib/completion/git-completion.zsh

-- 
1.7.12.1

[PATCH v5 1/3] 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 simplifcation of the completino tests (separate
patch).

There should be no functional changes.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 contrib/completion/git-completion.bash | 65 ++++++++++++++++++----------------
 1 file changed, 34 insertions(+), 31 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index d743e56..01325de 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:
@@ -238,13 +243,11 @@ __gitcomp ()
 
 	case "$cur_" in
 	--*=)
-		COMPREPLY=()
+		__gitcompadd
 		;;
 	*)
 		local IFS=$'\n'
-		COMPREPLY=($(compgen -P "${2-}" \
-			-W "$(__gitcomp_1 "${1-}" "${4-}")" \
-			-- "$cur_"))
+		__gitcompadd "$(__gitcomp_1 "${1-}" "${4-}")" "${2-}" "$cur_" ""
 		;;
 	esac
 }
@@ -261,7 +264,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 ()
@@ -486,7 +489,7 @@ __git_complete_remote_or_refspec ()
 			case "$cmd" in
 			push) no_complete_refspec=1 ;;
 			fetch)
-				COMPREPLY=()
+				__gitcompadd
 				return
 				;;
 			*) ;;
@@ -502,7 +505,7 @@ __git_complete_remote_or_refspec ()
 		return
 	fi
 	if [ $no_complete_refspec = 1 ]; then
-		COMPREPLY=()
+		__gitcompadd
 		return
 	fi
 	[ "$remote" = "." ] && remote=
@@ -776,7 +779,7 @@ _git_am ()
 			"
 		return
 	esac
-	COMPREPLY=()
+	__gitcompadd
 }
 
 _git_apply ()
@@ -796,7 +799,7 @@ _git_apply ()
 			"
 		return
 	esac
-	COMPREPLY=()
+	__gitcompadd
 }
 
 _git_add ()
@@ -811,7 +814,7 @@ _git_add ()
 			"
 		return
 	esac
-	COMPREPLY=()
+	__gitcompadd
 }
 
 _git_archive ()
@@ -856,7 +859,7 @@ _git_bisect ()
 		__gitcomp_nl "$(__git_refs)"
 		;;
 	*)
-		COMPREPLY=()
+		__gitcompadd
 		;;
 	esac
 }
@@ -965,7 +968,7 @@ _git_clean ()
 		return
 		;;
 	esac
-	COMPREPLY=()
+	__gitcompadd
 }
 
 _git_clone ()
@@ -989,7 +992,7 @@ _git_clone ()
 		return
 		;;
 	esac
-	COMPREPLY=()
+	__gitcompadd
 }
 
 _git_commit ()
@@ -1023,7 +1026,7 @@ _git_commit ()
 			"
 		return
 	esac
-	COMPREPLY=()
+	__gitcompadd
 }
 
 _git_describe ()
@@ -1154,7 +1157,7 @@ _git_fsck ()
 		return
 		;;
 	esac
-	COMPREPLY=()
+	__gitcompadd
 }
 
 _git_gc ()
@@ -1165,7 +1168,7 @@ _git_gc ()
 		return
 		;;
 	esac
-	COMPREPLY=()
+	__gitcompadd
 }
 
 _git_gitk ()
@@ -1242,7 +1245,7 @@ _git_init ()
 		return
 		;;
 	esac
-	COMPREPLY=()
+	__gitcompadd
 }
 
 _git_ls_files ()
@@ -1261,7 +1264,7 @@ _git_ls_files ()
 		return
 		;;
 	esac
-	COMPREPLY=()
+	__gitcompadd
 }
 
 _git_ls_remote ()
@@ -1377,7 +1380,7 @@ _git_mergetool ()
 		return
 		;;
 	esac
-	COMPREPLY=()
+	__gitcompadd
 }
 
 _git_merge_base ()
@@ -1393,7 +1396,7 @@ _git_mv ()
 		return
 		;;
 	esac
-	COMPREPLY=()
+	__gitcompadd
 }
 
 _git_name_rev ()
@@ -1563,7 +1566,7 @@ _git_send_email ()
 		return
 		;;
 	esac
-	COMPREPLY=()
+	__gitcompadd
 }
 
 _git_stage ()
@@ -1616,7 +1619,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")"
@@ -1676,7 +1679,7 @@ _git_config ()
 		return
 		;;
 	*.*)
-		COMPREPLY=()
+		__gitcompadd
 		return
 		;;
 	esac
@@ -2056,7 +2059,7 @@ _git_remote ()
 		__gitcomp "$c"
 		;;
 	*)
-		COMPREPLY=()
+		__gitcompadd
 		;;
 	esac
 }
@@ -2100,7 +2103,7 @@ _git_rm ()
 		return
 		;;
 	esac
-	COMPREPLY=()
+	__gitcompadd
 }
 
 _git_shortlog ()
@@ -2170,7 +2173,7 @@ _git_stash ()
 			if [ -z "$(__git_find_on_cmdline "$save_opts")" ]; then
 				__gitcomp "$subcommands"
 			else
-				COMPREPLY=()
+				__gitcompadd
 			fi
 			;;
 		esac
@@ -2183,14 +2186,14 @@ _git_stash ()
 			__gitcomp "--index --quiet"
 			;;
 		show,--*|drop,--*|branch,--*)
-			COMPREPLY=()
+			__gitcompadd
 			;;
 		show,*|apply,*|drop,*|pop,*|branch,*)
 			__gitcomp_nl "$(git --git-dir="$(__gitdir)" stash list \
 					| sed -n -e 's/:.*//p')"
 			;;
 		*)
-			COMPREPLY=()
+			__gitcompadd
 			;;
 		esac
 	fi
@@ -2307,7 +2310,7 @@ _git_svn ()
 			__gitcomp "--revision= --parent"
 			;;
 		*)
-			COMPREPLY=()
+			__gitcompadd
 			;;
 		esac
 	fi
@@ -2332,13 +2335,13 @@ _git_tag ()
 
 	case "$prev" in
 	-m|-F)
-		COMPREPLY=()
+		__gitcompadd
 		;;
 	-*|tag)
 		if [ $f = 1 ]; then
 			__gitcomp_nl "$(__git_tags)"
 		else
-			COMPREPLY=()
+			__gitcompadd
 		fi
 		;;
 	*)
-- 
1.7.12.1

Re: [PATCH v5 1/3] completion: add new __gitcompadd helper

[SZEDER Gábor]

On Sun, Oct 14, 2012 at 05:52:49PM +0200, Felipe Contreras wrote:
> 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 simplifcation of the completino tests (separate
> patch).
> 
> There should be no functional changes.
> 
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  contrib/completion/git-completion.bash | 65 ++++++++++++++++++----------------
>  1 file changed, 34 insertions(+), 31 deletions(-)
> 
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index d743e56..01325de 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:
> @@ -238,13 +243,11 @@ __gitcomp ()
>  
>  	case "$cur_" in
>  	--*=)
> -		COMPREPLY=()
> +		__gitcompadd
>  		;;
>  	*)
>  		local IFS=$'\n'
> -		COMPREPLY=($(compgen -P "${2-}" \
> -			-W "$(__gitcomp_1 "${1-}" "${4-}")" \
> -			-- "$cur_"))
> +		__gitcompadd "$(__gitcomp_1 "${1-}" "${4-}")" "${2-}" "$cur_" ""
>  		;;
>  	esac
>  }
> @@ -261,7 +264,7 @@ __gitcomp ()
>  __gitcomp_nl ()
>  {
>  	local IFS=$'\n'
> -	COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$1" -- "${3-$cur}"))
> +	__gitcompadd "$1" "${2-}" "${3-$cur}" "${4- }"
>  }

I feel hesitant about this change.  One of the ways I'm exploring to
fix the issues with shell metacharacters and expansion in compgen is
to actually replace compgen.  We already iterate over all possible
completion words in __gitcomp_1(), so it doesn't make much of a
difference to do the filtering for the current word while we are at
it.  However, the way __gitcompadd() encapsulates COMPREPLY=($(compgen
...)), and tha basic idea of never touching COMPREPLY directly make
this basically impossible.

>  __git_heads ()
> @@ -486,7 +489,7 @@ __git_complete_remote_or_refspec ()
>  			case "$cmd" in
>  			push) no_complete_refspec=1 ;;
>  			fetch)
> -				COMPREPLY=()
> +				__gitcompadd
>  				return
>  				;;
>  			*) ;;
> @@ -502,7 +505,7 @@ __git_complete_remote_or_refspec ()
>  		return
>  	fi
>  	if [ $no_complete_refspec = 1 ]; then
> -		COMPREPLY=()
> +		__gitcompadd
>  		return
>  	fi
>  	[ "$remote" = "." ] && remote=
> @@ -776,7 +779,7 @@ _git_am ()
>  			"
>  		return
>  	esac
> -	COMPREPLY=()
> +	__gitcompadd

These changes effectively run compgen in a subshell to generate an
empty completion reply.  While it doesn't really matter on Linux,
it'll add another half a tenth of a second delay in those cases on my
Windows machine.  At least it should be conditional, i.e. $(compgen
...) shouldn't be executed when there are no possible completion
words.

However, I think those COMPREPLY=() assignments are pointless anyway.
COMPREPLY is always empty when completion functions are invoked, so
there is no need to explicitly set it to an empty array when we don't
provide any words for completion.  Their only use is basically to
explicitly tell us humans that in those cases we don't offer any words
for completion.  But we don't do that consistently: there are several
places without offering words for completion and without COMPREPLY=(),
e.g. the '__git_has_doubledash && return' pattern.

Perhaps it would be time to get rid of these COMPREPLY=() assignments?

>  }
>  
>  _git_apply ()
> @@ -796,7 +799,7 @@ _git_apply ()
>  			"
>  		return
>  	esac
> -	COMPREPLY=()
> +	__gitcompadd
>  }
>  
>  _git_add ()
> @@ -811,7 +814,7 @@ _git_add ()
>  			"
>  		return
>  	esac
> -	COMPREPLY=()
> +	__gitcompadd
>  }
>  
>  _git_archive ()
> @@ -856,7 +859,7 @@ _git_bisect ()
>  		__gitcomp_nl "$(__git_refs)"
>  		;;
>  	*)
> -		COMPREPLY=()
> +		__gitcompadd
>  		;;
>  	esac
>  }
> @@ -965,7 +968,7 @@ _git_clean ()
>  		return
>  		;;
>  	esac
> -	COMPREPLY=()
> +	__gitcompadd
>  }
>  
>  _git_clone ()
> @@ -989,7 +992,7 @@ _git_clone ()
>  		return
>  		;;
>  	esac
> -	COMPREPLY=()
> +	__gitcompadd
>  }
>  
>  _git_commit ()
> @@ -1023,7 +1026,7 @@ _git_commit ()
>  			"
>  		return
>  	esac
> -	COMPREPLY=()
> +	__gitcompadd
>  }
>  
>  _git_describe ()
> @@ -1154,7 +1157,7 @@ _git_fsck ()
>  		return
>  		;;
>  	esac
> -	COMPREPLY=()
> +	__gitcompadd
>  }
>  
>  _git_gc ()
> @@ -1165,7 +1168,7 @@ _git_gc ()
>  		return
>  		;;
>  	esac
> -	COMPREPLY=()
> +	__gitcompadd
>  }
>  
>  _git_gitk ()
> @@ -1242,7 +1245,7 @@ _git_init ()
>  		return
>  		;;
>  	esac
> -	COMPREPLY=()
> +	__gitcompadd
>  }
>  
>  _git_ls_files ()
> @@ -1261,7 +1264,7 @@ _git_ls_files ()
>  		return
>  		;;
>  	esac
> -	COMPREPLY=()
> +	__gitcompadd
>  }
>  
>  _git_ls_remote ()
> @@ -1377,7 +1380,7 @@ _git_mergetool ()
>  		return
>  		;;
>  	esac
> -	COMPREPLY=()
> +	__gitcompadd
>  }
>  
>  _git_merge_base ()
> @@ -1393,7 +1396,7 @@ _git_mv ()
>  		return
>  		;;
>  	esac
> -	COMPREPLY=()
> +	__gitcompadd
>  }
>  
>  _git_name_rev ()
> @@ -1563,7 +1566,7 @@ _git_send_email ()
>  		return
>  		;;
>  	esac
> -	COMPREPLY=()
> +	__gitcompadd
>  }
>  
>  _git_stage ()
> @@ -1616,7 +1619,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")"
> @@ -1676,7 +1679,7 @@ _git_config ()
>  		return
>  		;;
>  	*.*)
> -		COMPREPLY=()
> +		__gitcompadd
>  		return
>  		;;
>  	esac
> @@ -2056,7 +2059,7 @@ _git_remote ()
>  		__gitcomp "$c"
>  		;;
>  	*)
> -		COMPREPLY=()
> +		__gitcompadd
>  		;;
>  	esac
>  }
> @@ -2100,7 +2103,7 @@ _git_rm ()
>  		return
>  		;;
>  	esac
> -	COMPREPLY=()
> +	__gitcompadd
>  }
>  
>  _git_shortlog ()
> @@ -2170,7 +2173,7 @@ _git_stash ()
>  			if [ -z "$(__git_find_on_cmdline "$save_opts")" ]; then
>  				__gitcomp "$subcommands"
>  			else
> -				COMPREPLY=()
> +				__gitcompadd
>  			fi
>  			;;
>  		esac
> @@ -2183,14 +2186,14 @@ _git_stash ()
>  			__gitcomp "--index --quiet"
>  			;;
>  		show,--*|drop,--*|branch,--*)
> -			COMPREPLY=()
> +			__gitcompadd
>  			;;
>  		show,*|apply,*|drop,*|pop,*|branch,*)
>  			__gitcomp_nl "$(git --git-dir="$(__gitdir)" stash list \
>  					| sed -n -e 's/:.*//p')"
>  			;;
>  		*)
> -			COMPREPLY=()
> +			__gitcompadd
>  			;;
>  		esac
>  	fi
> @@ -2307,7 +2310,7 @@ _git_svn ()
>  			__gitcomp "--revision= --parent"
>  			;;
>  		*)
> -			COMPREPLY=()
> +			__gitcompadd
>  			;;
>  		esac
>  	fi
> @@ -2332,13 +2335,13 @@ _git_tag ()
>  
>  	case "$prev" in
>  	-m|-F)
> -		COMPREPLY=()
> +		__gitcompadd
>  		;;
>  	-*|tag)
>  		if [ $f = 1 ]; then
>  			__gitcomp_nl "$(__git_tags)"
>  		else
> -			COMPREPLY=()
> +			__gitcompadd
>  		fi
>  		;;
>  	*)
> -- 
> 1.7.12.1
> 

Re: [PATCH v5 1/3] completion: add new __gitcompadd helper

[Felipe Contreras]

On Wed, Oct 17, 2012 at 7:28 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:
> On Sun, Oct 14, 2012 at 05:52:49PM +0200, Felipe Contreras wrote:

>> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
>> index d743e56..01325de 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:
>> @@ -238,13 +243,11 @@ __gitcomp ()
>>
>>       case "$cur_" in
>>       --*=)
>> -             COMPREPLY=()
>> +             __gitcompadd
>>               ;;
>>       *)
>>               local IFS=$'\n'
>> -             COMPREPLY=($(compgen -P "${2-}" \
>> -                     -W "$(__gitcomp_1 "${1-}" "${4-}")" \
>> -                     -- "$cur_"))
>> +             __gitcompadd "$(__gitcomp_1 "${1-}" "${4-}")" "${2-}" "$cur_" ""
>>               ;;
>>       esac
>>  }
>> @@ -261,7 +264,7 @@ __gitcomp ()
>>  __gitcomp_nl ()
>>  {
>>       local IFS=$'\n'
>> -     COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$1" -- "${3-$cur}"))
>> +     __gitcompadd "$1" "${2-}" "${3-$cur}" "${4- }"
>>  }
>
> I feel hesitant about this change.  One of the ways I'm exploring to
> fix the issues with shell metacharacters and expansion in compgen is
> to actually replace compgen.  We already iterate over all possible
> completion words in __gitcomp_1(), so it doesn't make much of a
> difference to do the filtering for the current word while we are at
> it.  However, the way __gitcompadd() encapsulates COMPREPLY=($(compgen
> ...)), and tha basic idea of never touching COMPREPLY directly make
> this basically impossible.

How is it impossible? You can still replace compgen, all you have to
do is modify __gitcompadd and replace that code with whatever custom
code you want. You can change the arguments and everything. The only
limitation is that it should be the only place where COMPREPLY is
modified, and all is good. Well, it doesn't have to be only _one_
place, but the less functions that do this, the better.

>>  __git_heads ()
>> @@ -486,7 +489,7 @@ __git_complete_remote_or_refspec ()
>>                       case "$cmd" in
>>                       push) no_complete_refspec=1 ;;
>>                       fetch)
>> -                             COMPREPLY=()
>> +                             __gitcompadd
>>                               return
>>                               ;;
>>                       *) ;;
>> @@ -502,7 +505,7 @@ __git_complete_remote_or_refspec ()
>>               return
>>       fi
>>       if [ $no_complete_refspec = 1 ]; then
>> -             COMPREPLY=()
>> +             __gitcompadd
>>               return
>>       fi
>>       [ "$remote" = "." ] && remote=
>> @@ -776,7 +779,7 @@ _git_am ()
>>                       "
>>               return
>>       esac
>> -     COMPREPLY=()
>> +     __gitcompadd
>
> These changes effectively run compgen in a subshell to generate an
> empty completion reply.  While it doesn't really matter on Linux,
> it'll add another half a tenth of a second delay in those cases on my
> Windows machine.  At least it should be conditional, i.e. $(compgen
> ...) shouldn't be executed when there are no possible completion
> words.
>
> However, I think those COMPREPLY=() assignments are pointless anyway.
> COMPREPLY is always empty when completion functions are invoked, so
> there is no need to explicitly set it to an empty array when we don't
> provide any words for completion.  Their only use is basically to
> explicitly tell us humans that in those cases we don't offer any words
> for completion.  But we don't do that consistently: there are several
> places without offering words for completion and without COMPREPLY=(),
> e.g. the '__git_has_doubledash && return' pattern.
>
> Perhaps it would be time to get rid of these COMPREPLY=() assignments?

I'm all for it, I never understood what was the purpose of that. I
believe zsh could benefit from this information to decide whether to
run the default completion (e.g. files) or not, but as you said, if
it's not used consistently for bash, there's no point in trying.

Cheers.

-- 
Felipe Contreras

Re: [PATCH v5 1/3] completion: add new __gitcompadd helper

[SZEDER Gábor]

On Mon, Oct 22, 2012 at 02:41:21AM +0200, Felipe Contreras wrote:
> On Wed, Oct 17, 2012 at 7:28 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:
> > On Sun, Oct 14, 2012 at 05:52:49PM +0200, Felipe Contreras wrote:
> 
> >> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> >> index d743e56..01325de 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:
> >> @@ -238,13 +243,11 @@ __gitcomp ()
> >>
> >>       case "$cur_" in
> >>       --*=)
> >> -             COMPREPLY=()
> >> +             __gitcompadd
> >>               ;;
> >>       *)
> >>               local IFS=$'\n'
> >> -             COMPREPLY=($(compgen -P "${2-}" \
> >> -                     -W "$(__gitcomp_1 "${1-}" "${4-}")" \
> >> -                     -- "$cur_"))
> >> +             __gitcompadd "$(__gitcomp_1 "${1-}" "${4-}")" "${2-}" "$cur_" ""
> >>               ;;
> >>       esac
> >>  }
> >> @@ -261,7 +264,7 @@ __gitcomp ()
> >>  __gitcomp_nl ()
> >>  {
> >>       local IFS=$'\n'
> >> -     COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$1" -- "${3-$cur}"))
> >> +     __gitcompadd "$1" "${2-}" "${3-$cur}" "${4- }"
> >>  }
> >
> > I feel hesitant about this change.  One of the ways I'm exploring to
> > fix the issues with shell metacharacters and expansion in compgen is
> > to actually replace compgen.  We already iterate over all possible
> > completion words in __gitcomp_1(), so it doesn't make much of a
> > difference to do the filtering for the current word while we are at
> > it.  However, the way __gitcompadd() encapsulates COMPREPLY=($(compgen
> > ...)), and tha basic idea of never touching COMPREPLY directly make
> > this basically impossible.
> 
> How is it impossible? You can still replace compgen, all you have to
> do is modify __gitcompadd and replace that code with whatever custom
> code you want. You can change the arguments and everything. The only
> limitation is that it should be the only place where COMPREPLY is
> modified, and all is good. Well, it doesn't have to be only _one_
> place, but the less functions that do this, the better.

That's exactly the problem: there isn't, there can't be one single
"whatever custom code I want".

The compgen() in __gitcomp() will be replaced by an enhanced version
of the loop in __gitcomp_1(), while in __gitcomp_nl() it will be
replaced by a little awk scriptlet.  And then there is the oddball
$(git ls-tree |sed magic) in __git_complete_revlist_file(), where
possible completion words are filenames possibly containing newlines,
therefore requiring yet another approach.

[PATCH v5 2/3] tests: use __gitcompadd to simplify completion tests

[Felipe Contreras]

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 t/t9902-completion.sh | 29 +++++++++--------------------
 1 file changed, 9 insertions(+), 20 deletions(-)

diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 92d7eb4..49c6eb4 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -39,19 +39,18 @@ _get_comp_words_by_ref ()
 	done
 }
 
-print_comp ()
+__gitcompadd ()
 {
-	local IFS=$'\n'
-	echo "${COMPREPLY[*]}" > out
+	compgen -P "${2-}" -S "${4- }" -W "$1" -- "${3-$cur}" > out
 }
 
 run_completion ()
 {
-	local -a COMPREPLY _words
+	local -a _words
 	local _cword
 	_words=( $1 )
 	(( _cword = ${#_words[@]} - 1 ))
-	__git_wrap__git_main && print_comp
+	__git_wrap__git_main
 }
 
 test_completion ()
@@ -70,12 +69,10 @@ test_expect_success '__gitcomp - trailing space - options' '
 	--reset-author Z
 	EOF
 	(
-		local -a COMPREPLY &&
 		cur="--re" &&
 		__gitcomp "--dry-run --reuse-message= --reedit-message=
 				--reset-author" &&
-		IFS="$newline" &&
-		echo "${COMPREPLY[*]}" > out
+		IFS="$newline"
 	) &&
 	test_cmp expected out
 '
@@ -88,12 +85,10 @@ test_expect_success '__gitcomp - trailing space - config keys' '
 	browser.Z
 	EOF
 	(
-		local -a COMPREPLY &&
 		cur="br" &&
 		__gitcomp "branch. branch.autosetupmerge
 				branch.autosetuprebase browser." &&
-		IFS="$newline" &&
-		echo "${COMPREPLY[*]}" > out
+		IFS="$newline"
 	) &&
 	test_cmp expected out
 '
@@ -104,12 +99,10 @@ test_expect_success '__gitcomp - option parameter' '
 	resolve Z
 	EOF
 	(
-		local -a COMPREPLY &&
 		cur="--strategy=re" &&
 		__gitcomp "octopus ours recursive resolve subtree
 			" "" "re" &&
-		IFS="$newline" &&
-		echo "${COMPREPLY[*]}" > out
+		IFS="$newline"
 	) &&
 	test_cmp expected out
 '
@@ -120,12 +113,10 @@ test_expect_success '__gitcomp - prefix' '
 	branch.maint.mergeoptions Z
 	EOF
 	(
-		local -a COMPREPLY &&
 		cur="branch.me" &&
 		__gitcomp "remote merge mergeoptions rebase
 			" "branch.maint." "me" &&
-		IFS="$newline" &&
-		echo "${COMPREPLY[*]}" > out
+		IFS="$newline"
 	) &&
 	test_cmp expected out
 '
@@ -136,12 +127,10 @@ test_expect_success '__gitcomp - suffix' '
 	branch.maint.Z
 	EOF
 	(
-		local -a COMPREPLY &&
 		cur="branch.me" &&
 		__gitcomp "master maint next pu
 			" "branch." "ma" "." &&
-		IFS="$newline" &&
-		echo "${COMPREPLY[*]}" > out
+		IFS="$newline"
 	) &&
 	test_cmp expected out
 '
-- 
1.7.12.1

Re: [PATCH v5 2/3] tests: use __gitcompadd to simplify completion tests

[Felipe Contreras]

On Sun, Oct 14, 2012 at 5:52 PM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:

> -print_comp ()
> +__gitcompadd ()
>  {
> -       local IFS=$'\n'
> -       echo "${COMPREPLY[*]}" > out
> +       compgen -P "${2-}" -S "${4- }" -W "$1" -- "${3-$cur}" > out

Rather:

compgen -W "$1" -P "$2" -S "$4" -- "$3" > out

-- 
Felipe Contreras

Re: [PATCH v5 2/3] tests: use __gitcompadd to simplify completion tests

[SZEDER Gábor]

On Sun, Oct 14, 2012 at 05:52:50PM +0200, Felipe Contreras wrote:
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  t/t9902-completion.sh | 29 +++++++++--------------------
>  1 file changed, 9 insertions(+), 20 deletions(-)
> 
> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
> index 92d7eb4..49c6eb4 100755
> --- a/t/t9902-completion.sh
> +++ b/t/t9902-completion.sh
> @@ -39,19 +39,18 @@ _get_comp_words_by_ref ()
>  	done
>  }
>  
> -print_comp ()
> +__gitcompadd ()
>  {
> -	local IFS=$'\n'
> -	echo "${COMPREPLY[*]}" > out
> +	compgen -P "${2-}" -S "${4- }" -W "$1" -- "${3-$cur}" > out
>  }

Please don't.  Running compgen is a fundamental part of the completion
script, therefore tests must run it as it is in the completion script
and not some copy of it.

>  run_completion ()
>  {
> -	local -a COMPREPLY _words
> +	local -a _words
>  	local _cword
>  	_words=( $1 )
>  	(( _cword = ${#_words[@]} - 1 ))
> -	__git_wrap__git_main && print_comp
> +	__git_wrap__git_main
>  }
>  
>  test_completion ()
> @@ -70,12 +69,10 @@ test_expect_success '__gitcomp - trailing space - options' '
>  	--reset-author Z
>  	EOF
>  	(
> -		local -a COMPREPLY &&

I'm not sure what I was thinking when I wrote this, but using the
local keyword while not within a function but in a subshell doesn't
seem to be that clever ;)  Maybe just a copy-paste from the local
variable declarations of run-completion().

>  		cur="--re" &&
>  		__gitcomp "--dry-run --reuse-message= --reedit-message=
>  				--reset-author" &&
> -		IFS="$newline" &&
> -		echo "${COMPREPLY[*]}" > out

And here I should have used print_comp().

All these can be cleaned up without overriding __gitcompadd() and
potentialy compromising correctness.  Will send a patch in a minute.

> +		IFS="$newline"

This was only necessary for echoing the array.

>  	) &&
>  	test_cmp expected out
>  '
> @@ -88,12 +85,10 @@ test_expect_success '__gitcomp - trailing space - config keys' '
>  	browser.Z
>  	EOF
>  	(
> -		local -a COMPREPLY &&
>  		cur="br" &&
>  		__gitcomp "branch. branch.autosetupmerge
>  				branch.autosetuprebase browser." &&
> -		IFS="$newline" &&
> -		echo "${COMPREPLY[*]}" > out
> +		IFS="$newline"
>  	) &&
>  	test_cmp expected out
>  '
> @@ -104,12 +99,10 @@ test_expect_success '__gitcomp - option parameter' '
>  	resolve Z
>  	EOF
>  	(
> -		local -a COMPREPLY &&
>  		cur="--strategy=re" &&
>  		__gitcomp "octopus ours recursive resolve subtree
>  			" "" "re" &&
> -		IFS="$newline" &&
> -		echo "${COMPREPLY[*]}" > out
> +		IFS="$newline"
>  	) &&
>  	test_cmp expected out
>  '
> @@ -120,12 +113,10 @@ test_expect_success '__gitcomp - prefix' '
>  	branch.maint.mergeoptions Z
>  	EOF
>  	(
> -		local -a COMPREPLY &&
>  		cur="branch.me" &&
>  		__gitcomp "remote merge mergeoptions rebase
>  			" "branch.maint." "me" &&
> -		IFS="$newline" &&
> -		echo "${COMPREPLY[*]}" > out
> +		IFS="$newline"
>  	) &&
>  	test_cmp expected out
>  '
> @@ -136,12 +127,10 @@ test_expect_success '__gitcomp - suffix' '
>  	branch.maint.Z
>  	EOF
>  	(
> -		local -a COMPREPLY &&
>  		cur="branch.me" &&
>  		__gitcomp "master maint next pu
>  			" "branch." "ma" "." &&
> -		IFS="$newline" &&
> -		echo "${COMPREPLY[*]}" > out
> +		IFS="$newline"
>  	) &&
>  	test_cmp expected out
>  '
> -- 
> 1.7.12.1
> 

[PATCH] completion: clean up __gitcomp() tests

[SZEDER Gábor]

Clean up two issues in the tests I added in 74a8c849 (tests: add tests
for the __gitcomp() completion helper function, 2012-04-17):

 - The COMPREPLY array is created using 'local -a' while in a
   subshell.  However, the local keyword should only be used in a
   shell function, and a variable created in a subshell is by
   definition local to that subshell.  Use 'declare -a' instead.

 - The contents of the COMPREPLY array is written through an IFS
   fiddling + echo + redirection combo, although there is the
   print_comp() helper function for exactly this purpose.

Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
---
 t/t9902-completion.sh | 27 ++++++++++-----------------
 1 file changed, 10 insertions(+), 17 deletions(-)

diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index cbd0fb66..cc375ed0 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -70,8 +70,6 @@ test_completion_long ()
 	test_completion "$1"
 }
 
-newline=$'\n'
-
 test_expect_success '__gitcomp - trailing space - options' '
 	sed -e "s/Z$//" >expected <<-\EOF &&
 	--reuse-message=Z
@@ -79,12 +77,11 @@ test_expect_success '__gitcomp - trailing space - options' '
 	--reset-author Z
 	EOF
 	(
-		local -a COMPREPLY &&
+		declare -a COMPREPLY &&
 		cur="--re" &&
 		__gitcomp "--dry-run --reuse-message= --reedit-message=
 				--reset-author" &&
-		IFS="$newline" &&
-		echo "${COMPREPLY[*]}" > out
+		print_comp
 	) &&
 	test_cmp expected out
 '
@@ -97,12 +94,11 @@ test_expect_success '__gitcomp - trailing space - config keys' '
 	browser.Z
 	EOF
 	(
-		local -a COMPREPLY &&
+		declare -a COMPREPLY &&
 		cur="br" &&
 		__gitcomp "branch. branch.autosetupmerge
 				branch.autosetuprebase browser." &&
-		IFS="$newline" &&
-		echo "${COMPREPLY[*]}" > out
+		print_comp
 	) &&
 	test_cmp expected out
 '
@@ -113,12 +109,11 @@ test_expect_success '__gitcomp - option parameter' '
 	resolve Z
 	EOF
 	(
-		local -a COMPREPLY &&
+		declare -a COMPREPLY &&
 		cur="--strategy=re" &&
 		__gitcomp "octopus ours recursive resolve subtree
 			" "" "re" &&
-		IFS="$newline" &&
-		echo "${COMPREPLY[*]}" > out
+		print_comp
 	) &&
 	test_cmp expected out
 '
@@ -129,12 +124,11 @@ test_expect_success '__gitcomp - prefix' '
 	branch.maint.mergeoptions Z
 	EOF
 	(
-		local -a COMPREPLY &&
+		declare -a COMPREPLY &&
 		cur="branch.me" &&
 		__gitcomp "remote merge mergeoptions rebase
 			" "branch.maint." "me" &&
-		IFS="$newline" &&
-		echo "${COMPREPLY[*]}" > out
+		print_comp
 	) &&
 	test_cmp expected out
 '
@@ -145,12 +139,11 @@ test_expect_success '__gitcomp - suffix' '
 	branch.maint.Z
 	EOF
 	(
-		local -a COMPREPLY &&
+		declare -a COMPREPLY &&
 		cur="branch.me" &&
 		__gitcomp "master maint next pu
 			" "branch." "ma" "." &&
-		IFS="$newline" &&
-		echo "${COMPREPLY[*]}" > out
+		print_comp
 	) &&
 	test_cmp expected out
 '
-- 
1.8.0.rc0.83.gc8e1777

Re: [PATCH] completion: clean up __gitcomp() tests

[Felipe Contreras]

On Wed, Oct 17, 2012 at 7:54 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:
> Clean up two issues in the tests I added in 74a8c849 (tests: add tests
> for the __gitcomp() completion helper function, 2012-04-17):
>
>  - The COMPREPLY array is created using 'local -a' while in a
>    subshell.  However, the local keyword should only be used in a
>    shell function, and a variable created in a subshell is by
>    definition local to that subshell.  Use 'declare -a' instead.
>
>  - The contents of the COMPREPLY array is written through an IFS
>    fiddling + echo + redirection combo, although there is the
>    print_comp() helper function for exactly this purpose.

Makes sense. But this code seems awfully similar, a helper function might help.

-- 
Felipe Contreras

Re: [PATCH v5 2/3] tests: use __gitcompadd to simplify completion tests

[Felipe Contreras]

On Wed, Oct 17, 2012 at 7:50 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:
> On Sun, Oct 14, 2012 at 05:52:50PM +0200, Felipe Contreras wrote:
>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>> ---
>>  t/t9902-completion.sh | 29 +++++++++--------------------
>>  1 file changed, 9 insertions(+), 20 deletions(-)
>>
>> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
>> index 92d7eb4..49c6eb4 100755
>> --- a/t/t9902-completion.sh
>> +++ b/t/t9902-completion.sh
>> @@ -39,19 +39,18 @@ _get_comp_words_by_ref ()
>>       done
>>  }
>>
>> -print_comp ()
>> +__gitcompadd ()
>>  {
>> -     local IFS=$'\n'
>> -     echo "${COMPREPLY[*]}" > out
>> +     compgen -P "${2-}" -S "${4- }" -W "$1" -- "${3-$cur}" > out
>>  }
>
> Please don't.  Running compgen is a fundamental part of the completion
> script, therefore tests must run it as it is in the completion script
> and not some copy of it.

All right. I added this patch as an after though to help sell the idea
of __gitcompadd. Either  way I'm not to worried about overriding it,
we are not really exercising any code that could catch issues with
calling compgen; we probably need specialized tests for that. In fact
I amended the quote you are quoting above as it's totally different
from the proposed __gitcompadd, but it still works nonetheless.

-- 
Felipe Contreras

[PATCH v5 3/3] completion: add new zsh completion

[Felipe Contreras]

It seems there's always issues with zsh's bash completion emulation.
I've tried to fix as many as I could and most of the fixes are already
in the latest version of zsh, but still, there are issues.

There is no point in going through all that pain; the emulation is easy
to achieve, and this patch works better than zsh's emulation.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---

v5:

 * Even more simplification by using __gitcompadd

v4:

 * Simplification updates for the latest bash completion

v3:

 * Simplification
 * Avoid COMPREPLY; call compadd directly
 * Fix _get_comp_words_by_ref

 contrib/completion/git-completion.zsh | 48 +++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)
 create mode 100644 contrib/completion/git-completion.zsh

diff --git a/contrib/completion/git-completion.zsh b/contrib/completion/git-completion.zsh
new file mode 100644
index 0000000..dbb5261
--- /dev/null
+++ b/contrib/completion/git-completion.zsh
@@ -0,0 +1,48 @@
+#compdef git gitk
+
+# zsh completion wrapper for git
+#
+# You need git's bash completion script installed somewhere, by default on the
+# same directory as this script.
+#
+# If your script is on ~/.git-completion.sh instead, you can configure it on
+# your ~/.zshrc:
+#
+#  zstyle ':completion:*:*:git:*' script ~/.git-completion.sh
+#
+# The recommended way to install this script is to copy to
+# '~/.zsh/completion/_git', and then add the following to your ~/.zshrc file:
+#
+#  fpath=(~/.zsh/completion $fpath)
+
+complete ()
+{
+	# do nothing
+	return 0
+}
+
+zstyle -s ":completion:*:*:git:*" script script
+test -z "$script" && script="$(dirname ${funcsourcetrace[1]%:*})"/git-completion.bash
+ZSH_VERSION='' . "$script"
+
+__gitcompadd ()
+{
+	compadd -Q -S "$4" -P "$2" -p "${(M)cur#*[=:]}" -- ${=1} && _ret=0
+}
+
+_git ()
+{
+	local _ret=1
+	() {
+		emulate -L ksh
+		local cur cword prev
+		cur=${words[CURRENT-1]}
+		prev=${words[CURRENT-2]}
+		let cword=CURRENT-1
+		__${service}_main
+	}
+	let _ret && _default -S '' && _ret=0
+	return _ret
+}
+
+_git
-- 
1.7.12.1

Re: [PATCH v5 3/3] completion: add new zsh completion

[Felipe Contreras]

On Sun, Oct 14, 2012 at 5:52 PM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:

> +__gitcompadd ()
> +{
> +       compadd -Q -S "$4" -P "$2" -p "${(M)cur#*[=:]}" -- ${=1} && _ret=0

I just found a bug, should be:

compadd -Q -S "$4" -P "${(M)cur#*[=:]}" -p "$2" -- ${=1} && _ret=0

-- 
Felipe Contreras

Re: [PATCH v5 0/3] completion: refactor and zsh wrapper

[Matthieu Moy]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> Hi,
>
> Here's a bit of reorganition. I'm introducing a new __gitcompadd helper that is
> useful to wrapp all changes to COMPREPLY. 2nd and 3rd patches show how it's
> useful.
>
> The zsh wrapper is now very very simple, but I haven't received much feedback
> yet. I hope it will get in at some point in time.

I didn't review the code, but I've installed your patch series, and it
seems to work well.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/