[RFC/PATCH v4 0/6] completion: __git_complete and other stuff

[RFC/PATCH v4 0/6] completion: __git_complete and other stuff

[Felipe Contreras]

Hi,

In order to simplify the discussion I've cleaned up my patches related to
_GIT_complete, aliasing stuff, and such.

I'm only intending the firat patch for inclusion for now.

Cheers.

Felipe Contreras (6):
  completion: add new __git_complete helper
  tests: add more bash completion tests
  completion: simplify _git_bundle
  completion: simplify command stuff
  completion: calculate argument position properly
  completion: add public _GIT_complete helper

 contrib/completion/git-completion.bash |  103 ++++++++++++++++----------------
 t/t9902-completion.sh                  |   85 +++++++++++++++++++++++++-
 2 files changed, 135 insertions(+), 53 deletions(-)

-- 
1.7.10

[RFC/PATCH v4 1/6] completion: add new __git_complete helper

[Felipe Contreras]

This simplifies the completions, and would make it easier to define
aliases in the future.

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

 * Get rid of typeset and subshell

Since v2:

 * Rename to _GIT_complete to follow bash completion "guidelines"
 * Get rid of foo_wrap name

Since v1:

 * Remove stuff related to aliases fixes; should work on top of master

 contrib/completion/git-completion.bash |   69 +++++++++++++++-----------------
 t/t9902-completion.sh                  |    2 +-
 2 files changed, 33 insertions(+), 38 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 9f56ec7..4098902 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2603,21 +2603,6 @@ _git ()
 {
 	local i c=1 command __git_dir
 
-	if [[ -n ${ZSH_VERSION-} ]]; then
-		emulate -L bash
-		setopt KSH_TYPESET
-
-		# workaround zsh's bug that leaves 'words' as a special
-		# variable in versions < 4.3.12
-		typeset -h words
-
-		# workaround zsh's bug that quotes spaces in the COMPREPLY
-		# array if IFS doesn't contain spaces.
-		typeset -h IFS
-	fi
-
-	local cur words cword prev
-	_get_comp_words_by_ref -n =: cur words cword prev
 	while [ $c -lt $cword ]; do
 		i="${words[c]}"
 		case "$i" in
@@ -2667,22 +2652,6 @@ _git ()
 
 _gitk ()
 {
-	if [[ -n ${ZSH_VERSION-} ]]; then
-		emulate -L bash
-		setopt KSH_TYPESET
-
-		# workaround zsh's bug that leaves 'words' as a special
-		# variable in versions < 4.3.12
-		typeset -h words
-
-		# workaround zsh's bug that quotes spaces in the COMPREPLY
-		# array if IFS doesn't contain spaces.
-		typeset -h IFS
-	fi
-
-	local cur words cword prev
-	_get_comp_words_by_ref -n =: cur words cword prev
-
 	__git_has_doubledash && return
 
 	local g="$(__gitdir)"
@@ -2703,16 +2672,42 @@ _gitk ()
 	__git_complete_revlist
 }
 
-complete -o bashdefault -o default -o nospace -F _git git 2>/dev/null \
-	|| complete -o default -o nospace -F _git git
-complete -o bashdefault -o default -o nospace -F _gitk gitk 2>/dev/null \
-	|| complete -o default -o nospace -F _gitk gitk
+__git_func_wrap ()
+{
+	if [[ -n ${ZSH_VERSION-} ]]; then
+		emulate -L bash
+		setopt KSH_TYPESET
+
+		# workaround zsh's bug that leaves 'words' as a special
+		# variable in versions < 4.3.12
+		typeset -h words
+
+		# workaround zsh's bug that quotes spaces in the COMPREPLY
+		# array if IFS doesn't contain spaces.
+		typeset -h IFS
+	fi
+	local cur words cword prev
+	_get_comp_words_by_ref -n =: cur words cword prev
+	_$1
+}
+
+# this is NOT a public function; use at your own risk
+__git_complete ()
+{
+	local name="${2-$1}"
+	local wrapper="_${name}_wrap"
+	eval "$wrapper () { __git_func_wrap $name ; }"
+	complete -o bashdefault -o default -o nospace -F $wrapper $1 2>/dev/null \
+		|| complete -o default -o nospace -F $wrapper $1
+}
+
+__git_complete git
+__git_complete gitk
 
 # The following are necessary only for Cygwin, and only are needed
 # when the user has tab-completed the executable name and consequently
 # included the '.exe' suffix.
 #
 if [ Cygwin = "$(uname -o 2>/dev/null)" ]; then
-complete -o bashdefault -o default -o nospace -F _git git.exe 2>/dev/null \
-	|| complete -o default -o nospace -F _git git.exe
+__git_complete git.exe git
 fi
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 5bda6b6..331a5b9 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -63,7 +63,7 @@ run_completion ()
 	local _cword
 	_words=( $1 )
 	(( _cword = ${#_words[@]} - 1 ))
-	_git && print_comp
+	_git_wrap && print_comp
 }
 
 test_completion ()
-- 
1.7.10

Re: [RFC/PATCH v4 1/6] completion: add new __git_complete helper

[SZEDER Gábor]

Hi,

On Mon, May 07, 2012 at 03:23:15AM +0200, Felipe Contreras wrote:
> +__git_func_wrap ()
> +{
> +	if [[ -n ${ZSH_VERSION-} ]]; then
> +		emulate -L bash
> +		setopt KSH_TYPESET
> +
> +		# workaround zsh's bug that leaves 'words' as a special
> +		# variable in versions < 4.3.12
> +		typeset -h words
> +
> +		# workaround zsh's bug that quotes spaces in the COMPREPLY
> +		# array if IFS doesn't contain spaces.
> +		typeset -h IFS
> +	fi
> +	local cur words cword prev
> +	_get_comp_words_by_ref -n =: cur words cword prev
> +	_$1
> +}
> +
> +# this is NOT a public function; use at your own risk
> +__git_complete ()
> +{
> +	local name="${2-$1}"

Please add the underscore prefix here right away, so we don't need
that "_$1" above in __git_func_wrap().

There is still no documentation about the parameters of
__git_complete(), so...

It seems to set up completion for the command given as first argument
to invoke the completion function given as second argument, right?
But the completion function argument is optional, and if it's not
specified, its name is derived from the command name by adding an
underscore prefix (in __git_func_wrap()).  So, in case of 'git', $name
becomes 'git', hence the completion function to be invoked is _git().
So far so good.

> +	local wrapper="_${name}_wrap"

But then the wrapper function becomes _git_wrap().  Uh, oh.

> +	eval "$wrapper () { __git_func_wrap $name ; }"

... because this would then define the function _git_wrap(), which is
not good, because that's supposed to be the completion function for
the 'git wrap' command.

That's exactly why I wrote

  local wrapper="__git_wrap_$1"

in my earlier post, and it was not an unnecessary change that could be
simplified away.


> +	complete -o bashdefault -o default -o nospace -F $wrapper $1 2>/dev/null \
> +		|| complete -o default -o nospace -F $wrapper $1
> +}
> +
> +__git_complete git
> +__git_complete gitk
>  
>  # The following are necessary only for Cygwin, and only are needed
>  # when the user has tab-completed the executable name and consequently
>  # included the '.exe' suffix.
>  #
>  if [ Cygwin = "$(uname -o 2>/dev/null)" ]; then
> -complete -o bashdefault -o default -o nospace -F _git git.exe 2>/dev/null \
> -	|| complete -o default -o nospace -F _git git.exe
> +__git_complete git.exe git
>  fi
> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
> index 5bda6b6..331a5b9 100755
> --- a/t/t9902-completion.sh
> +++ b/t/t9902-completion.sh
> @@ -63,7 +63,7 @@ run_completion ()
>  	local _cword
>  	_words=( $1 )
>  	(( _cword = ${#_words[@]} - 1 ))
> -	_git && print_comp
> +	_git_wrap && print_comp
>  }
>  
>  test_completion ()
> -- 
> 1.7.10
> 

Re: [RFC/PATCH v4 1/6] completion: add new __git_complete helper

[Junio C Hamano]

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


> +# this is NOT a public function; use at your own risk
> +__git_complete ()

The comment is enough to scare away people who might use it and then come
back to complain when this changes its implementation detail, but saying
"This is not X" without saying "This does Y" does not help those who want
to contribute updates to git-completion script.  Is there a short and
sweet description of what this is for and/or what this does?

> +{
> +	local name="${2-$1}"
> +	local wrapper="_${name}_wrap"
> +	eval "$wrapper () { __git_func_wrap $name ; }"
> +	complete -o bashdefault -o default -o nospace -F $wrapper $1 2>/dev/null \
> +		|| complete -o default -o nospace -F $wrapper $1
> +}
> +
> +__git_complete git
> +__git_complete gitk
>  
>  # The following are necessary only for Cygwin, and only are needed
>  # when the user has tab-completed the executable name and consequently
>  # included the '.exe' suffix.
>  #
>  if [ Cygwin = "$(uname -o 2>/dev/null)" ]; then
> -complete -o bashdefault -o default -o nospace -F _git git.exe 2>/dev/null \
> -	|| complete -o default -o nospace -F _git git.exe
> +__git_complete git.exe git
>  fi
> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
> index 5bda6b6..331a5b9 100755
> --- a/t/t9902-completion.sh
> +++ b/t/t9902-completion.sh
> @@ -63,7 +63,7 @@ run_completion ()
>  	local _cword
>  	_words=( $1 )
>  	(( _cword = ${#_words[@]} - 1 ))
> -	_git && print_comp
> +	_git_wrap && print_comp
>  }
>  
>  test_completion ()

Re: [RFC/PATCH v4 1/6] completion: add new __git_complete helper

[Felipe Contreras]

On Mon, May 7, 2012 at 8:51 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>
>> +# this is NOT a public function; use at your own risk
>> +__git_complete ()
>
> The comment is enough to scare away people who might use it and then come
> back to complain when this changes its implementation detail, but saying
> "This is not X" without saying "This does Y" does not help those who want
> to contribute updates to git-completion script.  Is there a short and
> sweet description of what this is for and/or what this does?

No. And this patch is not meant to help those people; I don't even
need this, it's not my itch, I just did it because other people
complained. Those people would have to scratch their own itch if they
want some description of __git_complete (), I only want to reorganize
the code and already did more than my fair share.

-- 
Felipe Contreras

[RFC/PATCH v4 2/6] tests: add more bash completion tests

[Felipe Contreras]

These tests try to excercise code that deals with 'words' and 'cword'.

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

 t/t9902-completion.sh |   55 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)

diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 331a5b9..bb66848 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -158,6 +158,22 @@ test_expect_success '__gitcomp - suffix' '
 	test_cmp expected out
 '
 
+setup_repository ()
+{
+	mkdir "$1" && (
+	cd "$1" &&
+	git init &&
+	test_tick &&
+	git commit --allow-empty -m "Initial"
+	)
+}
+
+test_expect_success 'prepare' '
+	setup_repository one &&
+	git clone one test &&
+	cd test
+'
+
 test_expect_success 'basic' '
 	run_completion "git \"\"" &&
 	# built-in
@@ -240,4 +256,43 @@ test_expect_success 'general options plus command' '
 	test_completion "git --no-replace-objects check" "checkout "
 '
 
+test_expect_success 'remote or refspec' '
+	test_completion "git fetch o" "origin " &&
+	test_completion "git fetch origin m" "master:master " &&
+	test_completion "git pull o" "origin " &&
+	test_completion "git pull origin m" "master " &&
+	test_completion "git push o" "origin " &&
+	test_completion "git push origin m" "master "
+'
+
+test_expect_success 'subcomands' '
+	test_completion "git bisect st" "start "
+'
+
+test_expect_success 'has double dash' '
+	test_completion "git add -- foo" ""
+'
+
+test_expect_success 'config' '
+	git config --file=foo color.ui auto &&
+	test_completion "git config --file=foo --get c" "color.ui "
+'
+
+test_expect_success 'other' '
+	cat >expected <<-\EOF &&
+	origin/HEAD 
+	origin/master 
+	EOF
+	test_completion "git branch -r o" &&
+	test_completion "git bundle cr" "create " &&
+
+	echo foobar > tags &&
+	test_completion "git grep f" "foobar " &&
+
+	test_completion "git notes --ref m" "master " &&
+
+	git tag v0.1 &&
+	test_completion "git tag -d v" "v0.1 "
+'
+
 test_done
-- 
1.7.10

[RFC/PATCH v4 3/6] completion: simplify _git_bundle

[Felipe Contreras]

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

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 4098902..8de0358 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1142,16 +1142,18 @@ _git_branch ()
 
 _git_bundle ()
 {
-	local cmd="${words[2]}"
+	local subcommands='create list-heads verify unbundle'
+	local subcommand="$(__git_find_on_cmdline "$subcommands")"
+
 	case "$cword" in
 	2)
-		__gitcomp "create list-heads verify unbundle"
+		__gitcomp "$subcommands"
 		;;
 	3)
 		# looking for a file
 		;;
 	*)
-		case "$cmd" in
+		case "$subcommand" in
 			create)
 				__git_complete_revlist
 			;;
-- 
1.7.10

[RFC/PATCH v4 4/6] completion: simplify command stuff

[Felipe Contreras]

No need to recalculate it.

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

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 8de0358..e648d7c 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -723,7 +723,7 @@ __git_complete_revlist ()
 
 __git_complete_remote_or_refspec ()
 {
-	local cur_="$cur" cmd="${words[1]}"
+	local cur_="$cur"
 	local i c=2 remote="" pfx="" lhs=1 no_complete_refspec=0
 	if [ "$cmd" = "remote" ]; then
 		((c++))
@@ -2603,22 +2603,22 @@ _git_whatchanged ()
 
 _git ()
 {
-	local i c=1 command __git_dir
+	local i c=1 cmd __git_dir
 
 	while [ $c -lt $cword ]; do
 		i="${words[c]}"
 		case "$i" in
 		--git-dir=*) __git_dir="${i#--git-dir=}" ;;
 		--bare)      __git_dir="." ;;
-		--help) command="help"; break ;;
+		--help) cmd="help"; break ;;
 		-c) c=$((++c)) ;;
 		-*) ;;
-		*) command="$i"; break ;;
+		*) cmd="$i"; break ;;
 		esac
 		((c++))
 	done
 
-	if [ -z "$command" ]; then
+	if [ -z "$cmd" ]; then
 		case "$cur" in
 		--*)   __gitcomp "
 			--paginate
@@ -2642,10 +2642,10 @@ _git ()
 		return
 	fi
 
-	local completion_func="_git_${command//-/_}"
+	local completion_func="_git_${cmd//-/_}"
 	declare -f $completion_func >/dev/null && $completion_func && return
 
-	local expansion=$(__git_aliased_command "$command")
+	local expansion=$(__git_aliased_command "$cmd")
 	if [ -n "$expansion" ]; then
 		completion_func="_git_${expansion//-/_}"
 		declare -f $completion_func >/dev/null && $completion_func
-- 
1.7.10

Re: [RFC/PATCH v4 4/6] completion: simplify command stuff

[SZEDER Gábor]

Hi,

On Mon, May 07, 2012 at 03:23:18AM +0200, Felipe Contreras wrote:
> No need to recalculate it.
> 
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  contrib/completion/git-completion.bash |   14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)

This seems to be the same patch you sent earlier as 7/12 in the first
round.  I had a few comments back then, see

  http://thread.gmane.org/gmane.comp.version-control.git/194958/focus=195276

But this $cmd variable will be only used in
__git_complete_remote_or_refspec(), so perhaps your idea later in that
thread about invoking it from _git_fetch() as

  __git_complete_remote_or_refspec "fetch"

would be better.  That would only require one modification in its four
callers, but neither in _git() in this patch nor in __git_complete()
later in the series to set $cmd.


Best,
Gábor


> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 8de0358..e648d7c 100755
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -723,7 +723,7 @@ __git_complete_revlist ()
>  
>  __git_complete_remote_or_refspec ()
>  {
> -	local cur_="$cur" cmd="${words[1]}"
> +	local cur_="$cur"
>  	local i c=2 remote="" pfx="" lhs=1 no_complete_refspec=0
>  	if [ "$cmd" = "remote" ]; then
>  		((c++))
> @@ -2603,22 +2603,22 @@ _git_whatchanged ()
>  
>  _git ()
>  {
> -	local i c=1 command __git_dir
> +	local i c=1 cmd __git_dir
>  
>  	while [ $c -lt $cword ]; do
>  		i="${words[c]}"
>  		case "$i" in
>  		--git-dir=*) __git_dir="${i#--git-dir=}" ;;
>  		--bare)      __git_dir="." ;;
> -		--help) command="help"; break ;;
> +		--help) cmd="help"; break ;;
>  		-c) c=$((++c)) ;;
>  		-*) ;;
> -		*) command="$i"; break ;;
> +		*) cmd="$i"; break ;;
>  		esac
>  		((c++))
>  	done
>  
> -	if [ -z "$command" ]; then
> +	if [ -z "$cmd" ]; then
>  		case "$cur" in
>  		--*)   __gitcomp "
>  			--paginate
> @@ -2642,10 +2642,10 @@ _git ()
>  		return
>  	fi
>  
> -	local completion_func="_git_${command//-/_}"
> +	local completion_func="_git_${cmd//-/_}"
>  	declare -f $completion_func >/dev/null && $completion_func && return
>  
> -	local expansion=$(__git_aliased_command "$command")
> +	local expansion=$(__git_aliased_command "$cmd")
>  	if [ -n "$expansion" ]; then
>  		completion_func="_git_${expansion//-/_}"
>  		declare -f $completion_func >/dev/null && $completion_func
> -- 
> 1.7.10
> 

[RFC/PATCH v4 5/6] completion: calculate argument position properly

[Felipe Contreras]

Positions are currently hard-coded, which means completions in the form
of 'git --foo bar' fail, because positions have been shifted.

This fixes the issue that was spotted in the mailing list regarding
certain aliases[1].

[1] http://thread.gmane.org/gmane.comp.version-control.git/185184

Cc: SZEDER Gábor <szeder@ira.uka.de>
Cc: Scott Bronson <bronson@rinspin.com>
Cc: Nathan Broadbent <nathan.f77@gmail.com>
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 contrib/completion/git-completion.bash |   16 +++++++++-------
 t/t9902-completion.sh                  |   21 +++++++++++++++++++++
 2 files changed, 30 insertions(+), 7 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index e648d7c..049110e 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -724,7 +724,8 @@ __git_complete_revlist ()
 __git_complete_remote_or_refspec ()
 {
 	local cur_="$cur"
-	local i c=2 remote="" pfx="" lhs=1 no_complete_refspec=0
+	local i c=$cmd_pos remote="" pfx="" lhs=1 no_complete_refspec=0
+
 	if [ "$cmd" = "remote" ]; then
 		((c++))
 	fi
@@ -1144,12 +1145,11 @@ _git_bundle ()
 {
 	local subcommands='create list-heads verify unbundle'
 	local subcommand="$(__git_find_on_cmdline "$subcommands")"
-
-	case "$cword" in
-	2)
+	case $(( cword - cmd_pos )) in
+	0)
 		__gitcomp "$subcommands"
 		;;
-	3)
+	1)
 		# looking for a file
 		;;
 	*)
@@ -1449,7 +1449,7 @@ _git_grep ()
 	esac
 
 	case "$cword,$prev" in
-	2,*|*,-*)
+	$cmd_pos,*|*,-*)
 		if test -r tags; then
 			__gitcomp_nl "$(__git_match_ctag "$cur" tags)"
 			return
@@ -2603,7 +2603,7 @@ _git_whatchanged ()
 
 _git ()
 {
-	local i c=1 cmd __git_dir
+	local i c=1 cmd cmd_pos __git_dir
 
 	while [ $c -lt $cword ]; do
 		i="${words[c]}"
@@ -2642,6 +2642,8 @@ _git ()
 		return
 	fi
 
+	(( cmd_pos = c + 1 ))
+
 	local completion_func="_git_${cmd//-/_}"
 	declare -f $completion_func >/dev/null && $completion_func && return
 
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index bb66848..6904594 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -295,4 +295,25 @@ test_expect_success 'other' '
 	test_completion "git tag -d v" "v0.1 "
 '
 
+test_expect_success 'global options extra checks' '
+	test_completion "git --no-pager fetch o" "origin " &&
+	test_completion "git --no-pager fetch origin m" "master:master " &&
+	test_completion "git --no-pager pull o" "origin " &&
+	test_completion "git --no-pager pull origin m" "master " &&
+	test_completion "git --no-pager push o" "origin " &&
+	test_completion "git --no-pager push origin m" "master " &&
+	test_completion "git --no-pager bisect st" "start " &&
+	test_completion "git --no-pager add -- foo" "" &&
+	test_completion "git --no-pager config --file=foo --get c" "color.ui " &&
+	cat >expected <<-\EOF &&
+	origin/HEAD 
+	origin/master 
+	EOF
+	test_completion "git --no-pager branch -r o" &&
+	test_completion "git --no-pager bundle cr" "create " &&
+	test_completion "git --no-pager grep f" "foobar " &&
+	test_completion "git --no-pager notes --ref m" "master " &&
+	test_completion "git --no-pager tag -d v" "v0.1 "
+'
+
 test_done
-- 
1.7.10

Re: [RFC/PATCH v4 5/6] completion: calculate argument position properly

[Felipe Contreras]

On Mon, May 7, 2012 at 3:23 AM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:

> @@ -2642,6 +2642,8 @@ _git ()
>                return
>        fi
>
> +       (( cmd_pos = c + 1 ))

Actually, I would prefer cmd_pos=$((c + 1)).

-- 
Felipe Contreras

Re: [RFC/PATCH v4 5/6] completion: calculate argument position properly

[Junio C Hamano]

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

> On Mon, May 7, 2012 at 3:23 AM, Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
>
>> @@ -2642,6 +2642,8 @@ _git ()
>>                return
>>        fi
>>
>> +       (( cmd_pos = c + 1 ))
>
> Actually, I would prefer cmd_pos=$((c + 1)).

Yeah, that would feel more natural.

Together with 4/6, I am guessing that you are using $cmd and $cmd_pos as a
global variable to pass state to/from helper functions.  Are they properly
documented in code (if not, please do so)?

Re: [RFC/PATCH v4 5/6] completion: calculate argument position properly

[Felipe Contreras]

On Mon, May 7, 2012 at 8:59 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> On Mon, May 7, 2012 at 3:23 AM, Felipe Contreras
>> <felipe.contreras@gmail.com> wrote:
>>
>>> @@ -2642,6 +2642,8 @@ _git ()
>>>                return
>>>        fi
>>>
>>> +       (( cmd_pos = c + 1 ))
>>
>> Actually, I would prefer cmd_pos=$((c + 1)).
>
> Yeah, that would feel more natural.
>
> Together with 4/6, I am guessing that you are using $cmd and $cmd_pos as a
> global variable to pass state to/from helper functions.  Are they properly
> documented in code (if not, please do so)?

Like cur, prev, words, and cwords? Wait a second... they are not
documented anywhere. I'm also not going to work on this... not my
itch.

-- 
Felipe Contreras

Re: [RFC/PATCH v4 5/6] completion: calculate argument position properly

[Junio C Hamano]

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

>> Together with 4/6, I am guessing that you are using $cmd and $cmd_pos
>> as a global variable to pass state to/from helper functions. Are they
>> properly documented in code (if not, please do so)?
>
> Like cur, prev, words, and cwords? Wait a second... they are not
> documented anywhere. I'm also not going to work on this... not my
> itch.

The usual thing to do is to clean up after other people's mess while you
are mucking with the vicinity of them anyway, especially if you are making
things worse by adding another.

And I am fully aware that it may not be your itch---otherwise I wouldn't
have asked.  I would have said "This needs to be fixed (or else)".

Why is it that everybody on the list, who is working together well in the
community, seems to have trouble only when interacting with you?  Please
remember that it is not my itch to live with an unnecessarily abrasive
behaviour from you, either.

Re: [RFC/PATCH v4 5/6] completion: calculate argument position properly

[Felipe Contreras]

On Mon, May 7, 2012 at 11:52 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>>> Together with 4/6, I am guessing that you are using $cmd and $cmd_pos
>>> as a global variable to pass state to/from helper functions. Are they
>>> properly documented in code (if not, please do so)?
>>
>> Like cur, prev, words, and cwords? Wait a second... they are not
>> documented anywhere. I'm also not going to work on this... not my
>> itch.
>
> The usual thing to do is to clean up after other people's mess while you
> are mucking with the vicinity of them anyway, especially if you are making
> things worse by adding another.

People have different ideas of what is a mess. You think it's a mess,
I don't. Why should I clean it up?

Of course, you as a maintainer have all the right to reject my patches
because they introduce more "mess", and if I want the patch in, I am
forced to do what you say, but it just so happens that I don't care if
this goes in or not, so I have the right to don't do anything; it's my
own free time.

> And I am fully aware that it may not be your itch---otherwise I wouldn't
> have asked.  I would have said "This needs to be fixed (or else)".
>
> Why is it that everybody on the list, who is working together well in the
> community, seems to have trouble only when interacting with you?  Please
> remember that it is not my itch to live with an unnecessarily abrasive
> behaviour from you, either.

Why am I being abrasive here? The only reason I sent these patches
was, as I said, to simplify the discussion about what would make
'alias gf' work, I did not, and I do not intent to propose these
patches for inclusion (as I know the pain (for me) that would entail).

If you would rather have me send no patches at all rather than to show
what I have (in fact I cleaned them quite a lot) and stop there, I can
do that; less work for me.

To be honest, I don't see what's the problem in sending partial stuff;
in a collaborative effort somebody else that actually has this itch
would pick these and finish the job; nothing wrong with that. Why
should it be me who finish the job? And why am I problematic if I
don't want to?

As I said, I only care about the first patch.

Cheers.

-- 
Felipe Contreras

[RFC/PATCH v4 6/6] completion: add public _GIT_complete helper

[Felipe Contreras]

So that users can easily define aliases, such as:

 _GIT_complete gf git_fetch

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 contrib/completion/git-completion.bash |   10 +++++-----
 t/t9902-completion.sh                  |    9 ++++++++-
 2 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 049110e..2b7ef02 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2678,6 +2678,7 @@ _gitk ()
 
 __git_func_wrap ()
 {
+	local cmd="${1#git_}" cmd_pos=1
 	if [[ -n ${ZSH_VERSION-} ]]; then
 		emulate -L bash
 		setopt KSH_TYPESET
@@ -2695,8 +2696,7 @@ __git_func_wrap ()
 	_$1
 }
 
-# this is NOT a public function; use at your own risk
-__git_complete ()
+_GIT_complete ()
 {
 	local name="${2-$1}"
 	local wrapper="_${name}_wrap"
@@ -2705,13 +2705,13 @@ __git_complete ()
 		|| complete -o default -o nospace -F $wrapper $1
 }
 
-__git_complete git
-__git_complete gitk
+_GIT_complete git
+_GIT_complete gitk
 
 # The following are necessary only for Cygwin, and only are needed
 # when the user has tab-completed the executable name and consequently
 # included the '.exe' suffix.
 #
 if [ Cygwin = "$(uname -o 2>/dev/null)" ]; then
-__git_complete git.exe git
+_GIT_complete git.exe git
 fi
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 6904594..2037452 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -63,7 +63,7 @@ run_completion ()
 	local _cword
 	_words=( $1 )
 	(( _cword = ${#_words[@]} - 1 ))
-	_git_wrap && print_comp
+	${comp_wrap-_git_wrap} && print_comp
 }
 
 test_completion ()
@@ -316,4 +316,11 @@ test_expect_success 'global options extra checks' '
 	test_completion "git --no-pager tag -d v" "v0.1 "
 '
 
+test_expect_success 'aliases' '
+	local comp_wrap=_git_fetch_wrap &&
+	_GIT_complete gf git_fetch &&
+	test_completion "gf o" "origin " &&
+	test_completion "gf origin m" "master:master "
+'
+
 test_done
-- 
1.7.10

Re: [RFC/PATCH v4 6/6] completion: add public _GIT_complete helper

[Junio C Hamano]

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

> So that users can easily define aliases, such as:
>
>  _GIT_complete gf git_fetch
>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  contrib/completion/git-completion.bash |   10 +++++-----
>  t/t9902-completion.sh                  |    9 ++++++++-
>  2 files changed, 13 insertions(+), 6 deletions(-)
>
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 049110e..2b7ef02 100755
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -2678,6 +2678,7 @@ _gitk ()
>  
>  __git_func_wrap ()
>  {
> +	local cmd="${1#git_}" cmd_pos=1
>  	if [[ -n ${ZSH_VERSION-} ]]; then
>  		emulate -L bash
>  		setopt KSH_TYPESET
> @@ -2695,8 +2696,7 @@ __git_func_wrap ()
>  	_$1
>  }
>  
> -# this is NOT a public function; use at your own risk
> -__git_complete ()
> +_GIT_complete ()

If it is now a public function, please have some description as to how to
use it for people who find this in the tarball extract.

I am guessing that

	_GIT_complete frotz git_fetch

is a way to declare that 'git frotz' wants the same kind of completion as
'git fetch' command, but I am not sure, as if it were the case it strikes
me somewhat odd that it is not "_GIT_complete frotz fetch".

Re: [RFC/PATCH v4 6/6] completion: add public _GIT_complete helper

[Felipe Contreras]

On Mon, May 7, 2012 at 8:56 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> So that users can easily define aliases, such as:
>>
>>  _GIT_complete gf git_fetch
>>
>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>> ---
>>  contrib/completion/git-completion.bash |   10 +++++-----
>>  t/t9902-completion.sh                  |    9 ++++++++-
>>  2 files changed, 13 insertions(+), 6 deletions(-)
>>
>> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
>> index 049110e..2b7ef02 100755
>> --- a/contrib/completion/git-completion.bash
>> +++ b/contrib/completion/git-completion.bash
>> @@ -2678,6 +2678,7 @@ _gitk ()
>>
>>  __git_func_wrap ()
>>  {
>> +     local cmd="${1#git_}" cmd_pos=1
>>       if [[ -n ${ZSH_VERSION-} ]]; then
>>               emulate -L bash
>>               setopt KSH_TYPESET
>> @@ -2695,8 +2696,7 @@ __git_func_wrap ()
>>       _$1
>>  }
>>
>> -# this is NOT a public function; use at your own risk
>> -__git_complete ()
>> +_GIT_complete ()
>
> If it is now a public function, please have some description as to how to
> use it for people who find this in the tarball extract.

This is RFC, I'm not planning to push for this, I truly have given up on it.

> I am guessing that
>
>        _GIT_complete frotz git_fetch
>
> is a way to declare that 'git frotz' wants the same kind of completion as
> 'git fetch' command, but I am not sure, as if it were the case it strikes
> me somewhat odd that it is not "_GIT_complete frotz fetch".

Yeah, I think that would be nicer, but one would need to take into
consideration the git/gitk cases.

Hopefully somebody else would pick this up and go through the eternal
nitpicking.

Cheers.

-- 
Felipe Contreras