threads / patch / 30182

patch, 12 partscompletion: simplify __gitcomp_1

Subject: [PATCH 03/12] completion: simplify __gitcomp_1

## tl;dr

48 messages between Apr 8, 2012 and Apr 12, 2012. Diffs are folded; open one to read it.

replies: 47people: 8as markdown or json

Felipe Contreras· Apr 8, 2012, 03:07 UTC · lore

[PATCH 00/12] Bash completion rework

Hi,

Here's a bunch of patches for bash completion. First of all some tests are added to verify that there are no regressions, then simple cleanups, then some fixes, and finally support for external aliases.

So now you can do something like 'git_complete gf git_fetch' and it would just work :)
Cheers.
Felipe Contreras (12):
  tests: add initial bash completion tests
  completion: simplify __gitcomp
  completion: simplify __gitcomp_1
  completion: trivial simplification
  completion: add missing global options
  tests: add more bash completion tests
  completion: simplify command stuff
  completion: simplify _git_bundle
  completion: calculate argument position properly
  completion: add new git_complete helper
  test: add tests for aliases in bash completion
  completion: rename _git and _gitk
 contrib/completion/git-completion.bash |  153 +++++++++-------
 t/t9902-completion.sh                  |  314 ++++++++++++++++++++++++++++++++
 2 files changed, 398 insertions(+), 69 deletions(-)
 create mode 100755 t/t9902-completion.sh
-- 
1.7.10.3.g5a738d
Felipe Contreras· Apr 8, 2012, 03:07 UTC · re: Felipe Contreras · lore

[PATCH 01/12] tests: add initial bash completion tests

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 t/t9902-completion.sh |  201 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 201 insertions(+)
 create mode 100755 t/t9902-completion.sh
Show changes to t/t9902-completion.sh +201 −0
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
new file mode 100755
index 0000000..7eb80dd
--- /dev/null
+++ b/t/t9902-completion.sh
@@ -0,0 +1,201 @@
+#!/bin/sh
+#
+# Copyright (c) 2012 Felipe Contreras
+#
+
+if type bash >/dev/null 2>&1
+then
+	# execute inside bash
+	test -z "$BASH" && exec bash "$0" "$@"
+else
+	echo '1..0 #SKIP skipping bash completion tests; bash not available'
+	exit 0
+fi
+
+test_description='test bash completion'
+
+. ./test-lib.sh
+
+complete ()
+{
+	# do nothing
+	return 0
+}
+
+. "$GIT_BUILD_DIR/contrib/completion/git-completion.bash"
+
+print_comp ()
+{
+	local IFS=$'\n'
+	echo "${COMPREPLY[*]}" > out
+}
+
+_get_comp_words_by_ref ()
+{
+	while [ $# -gt 0 ]; do
+		case "$1" in
+		cur)
+			cur=${_words[_cword]}
+			;;
+		prev)
+			prev=${_words[_cword-1]}
+			;;
+		words)
+			words=("${_words[@]}")
+			;;
+		cword)
+			cword=$_cword
+			;;
+		esac
+		shift
+	done
+}
+
+test_completion ()
+{
+	local -a COMPREPLY _words
+	local _cword
+	_words=( $1 )
+	test $# -gt 1 && echo "$2" > expected
+	(( _cword = ${#_words[@]} - 1 ))
+	_git && print_comp &&
+	test_cmp expected out
+}
+
+test_expect_success 'basic' '
+	cat >expected <<-\EOF &&
+	help 
+	add 
+	gc 
+	reflog 
+	get-tar-commit-id 
+	relink 
+	grep 
+	relink.perl 
+	am 
+	remote 
+	am.sh 
+	help 
+	annotate 
+	apply 
+	archimport.perl 
+	imap-send 
+	archive 
+	bisect 
+	init 
+	repack 
+	bisect.sh 
+	instaweb 
+	repack.sh 
+	blame 
+	instaweb.sh 
+	replace 
+	branch 
+	log 
+	bundle 
+	request-pull 
+	lost-found.sh 
+	request-pull.sh 
+	reset 
+	checkout 
+	cherry 
+	revert 
+	cherry-pick 
+	merge 
+	rm 
+	clean 
+	send-email 
+	clone 
+	send-email.perl 
+	commit 
+	config 
+	shortlog 
+	credential-cache 
+	show 
+	show-branch 
+	credential-store 
+	cvsexportcommit.perl 
+	stage 
+	stash 
+	cvsimport.perl 
+	stash.sh 
+	mergetool 
+	status 
+	cvsserver.perl 
+	mergetool.sh 
+	submodule 
+	describe 
+	submodule.sh 
+	diff 
+	mv 
+	svn 
+	name-rev 
+	svn.perl 
+	notes 
+	tag 
+	difftool 
+	difftool.perl 
+	fetch 
+	pull 
+	pull.sh 
+	filter-branch 
+	push 
+	filter-branch.sh 
+	quiltimport.sh 
+	format-patch 
+	rebase 
+	fsck 
+	rebase.sh 
+	whatchanged 
+	cccmd 
+	proxy 
+	send-pull 
+	EOF
+	test_completion "git" &&
+
+	cat >expected <<-\EOF &&
+	help 
+	help 
+	EOF
+	test_completion "git he"
+
+	cat >expected <<-\EOF &&
+	fetch 
+	filter-branch 
+	filter-branch.sh 
+	format-patch 
+	fsck 
+	EOF
+	test_completion "git f"
+'
+
+test_expect_success 'double dash' '
+	cat >expected <<-\EOF &&
+	--paginate 
+	--no-pager 
+	--git-dir=
+	--bare 
+	--version 
+	--exec-path 
+	--html-path 
+	--work-tree=
+	--namespace=
+	--help 
+	EOF
+	test_completion "git --"
+
+	cat >expected <<-\EOF &&
+	--quiet 
+	--ours 
+	--theirs 
+	--track 
+	--no-track 
+	--merge 
+	--conflict=
+	--orphan 
+	--patch 
+	EOF
+	test_completion "git checkout --"
+'
+
+test_done
-- 
1.7.10.3.g5a738d
Junio C Hamano· Apr 8, 2012, 04:25 UTC · re: Felipe Contreras · lore

Re: [PATCH 01/12] tests: add initial bash completion tests

Felipe Contreras <felipe.contreras@gmail.com> writes:
Show 25 quoted lines
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  t/t9902-completion.sh |  201 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 201 insertions(+)
>  create mode 100755 t/t9902-completion.sh
>
> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
> new file mode 100755
> index 0000000..7eb80dd
> --- /dev/null
> +++ b/t/t9902-completion.sh
> @@ -0,0 +1,201 @@
> +#!/bin/sh
> +#
> +# Copyright (c) 2012 Felipe Contreras
> +#
> +
> +if type bash >/dev/null 2>&1
> +then
> +	# execute inside bash
> +	test -z "$BASH" && exec bash "$0" "$@"
> +else
> +	echo '1..0 #SKIP skipping bash completion tests; bash not available'
> +	exit 0
> +fi

What shell do you use on your system as /bin/sh (or if you use SHELL_PATH in the Makefile to override it, what do you use)?

Bash has a special case code to behave in a "posixly correct" mode, where a lot of extensions are disabled, when invoked as "sh". Unfortunately, it still defines BASH_* variables, including BASH, when operating in this mode.

This unfortunately breaks the above code on a system where /bin/sh is symlinked to "bash". "make" runs test scripts using with SHELL_PATH, defaulting to /bin/sh, and you say "Ah, bash is available, oh, we have BASH defined so let's keep going" without doing an "exec", so later,

> +. "$GIT_BUILD_DIR/contrib/completion/git-completion.bash"
may trigger errors like this:
  sh: .../git-completion.bash: line 128: syntax error near unexpected token `<'
  sh: .../git-completion.bash: line 128: `    done < <(git config -z --get...

depending on what bash-ism is used in the then-current version of that file.

So we may want to be a bit more careful like this:
if test "z${BASH%/bash}" != "z${BASH}"
then
	: ends with bash so we are already running bash as bash
elif type bash >/dev/null 2>&1
then
	exec bash "$0" "$@"
else
	echo '1..0 #SKIP skipping bash completion tests; bash not available'
	exit 0
fi

This incidentally avoids running type twice when we are already running as bash.

Show 39 quoted lines
> +
> +test_description='test bash completion'
> +
> +. ./test-lib.sh
> +
> +complete ()
> +{
> +	# do nothing
> +	return 0
> +}
> +
> +. "$GIT_BUILD_DIR/contrib/completion/git-completion.bash"
> +
> +print_comp ()
> +{
> +	local IFS=$'\n'
> +	echo "${COMPREPLY[*]}" > out
> +}
> +
> +_get_comp_words_by_ref ()
> +{
> +	while [ $# -gt 0 ]; do
> +		case "$1" in
> +		cur)
> +			cur=${_words[_cword]}
> +			;;
> +		prev)
> +			prev=${_words[_cword-1]}
> +			;;
> +		words)
> +			words=("${_words[@]}")
> +			;;
> +		cword)
> +			cword=$_cword
> +			;;
> +		esac
> +		shift
> +	done
> +}

Could you add an explanation before this function to describe what is going on? The completion script being tested relies on the shell function of the same name defined there to behave one way, and this overrides it with a different implementation. Often such an override is an effective way to intercept and expose the internal state to test framework, but the above does not seem to be such an override, and it is unclear what is going on here.

Jeff King· Apr 8, 2012, 04:48 UTC · re: Junio C Hamano · lore

Re: [PATCH 01/12] tests: add initial bash completion tests

On Sat, Apr 07, 2012 at 09:25:29PM -0700, Junio C Hamano wrote:
Show 10 quoted lines
> Bash has a special case code to behave in a "posixly correct" mode, where
> a lot of extensions are disabled, when invoked as "sh".  Unfortunately, it
> still defines BASH_* variables, including BASH, when operating in this
> mode.
> [...]
> So we may want to be a bit more careful like this:
> 
> if test "z${BASH%/bash}" != "z${BASH}"
> then
> 	: ends with bash so we are already running bash as bash

If bash is in posix mode (including "bash --posix" and being invoked as "bin/sh"), it will set POSIXLY_CORRECT (but not export it). Similarly, if POSIXLY_CORRECT is set in the outer environment, it will act more like sh. So maybe that would be a better test.

-Peff
Junio C Hamano· Apr 8, 2012, 05:41 UTC · re: Jeff King · lore

Re: [PATCH 01/12] tests: add initial bash completion tests

Jeff King <peff@peff.net> writes:
Show 17 quoted lines
> On Sat, Apr 07, 2012 at 09:25:29PM -0700, Junio C Hamano wrote:
>
>> Bash has a special case code to behave in a "posixly correct" mode, where
>> a lot of extensions are disabled, when invoked as "sh".  Unfortunately, it
>> still defines BASH_* variables, including BASH, when operating in this
>> mode.
>> [...]
>> So we may want to be a bit more careful like this:
>> 
>> if test "z${BASH%/bash}" != "z${BASH}"
>> then
>> 	: ends with bash so we are already running bash as bash
>
> If bash is in posix mode (including "bash --posix" and being invoked as
> "bin/sh"), it will set POSIXLY_CORRECT (but not export it). Similarly,
> if POSIXLY_CORRECT is set in the outer environment, it will act more
> like sh. So maybe that would be a better test.

Yes, but the check needs to be careful to make sure the shell that is running the check is indeed bash, so that it will explicitly exec bash for somebody who is running dash but exports POSIXLY_CORRECT to make GNU programs (other than bash) behave more standard compliant way.

In other words,
	if test -n "$POSIXLY_CORRECT" && test -n "$BASH"
	then
        	: we are running bash under posix mode
	elif ...
or somesuch.
Jeff King· Apr 8, 2012, 05:42 UTC · re: Junio C Hamano · lore

Re: [PATCH 01/12] tests: add initial bash completion tests

On Sat, Apr 07, 2012 at 10:41:32PM -0700, Junio C Hamano wrote:
Show 9 quoted lines
> > If bash is in posix mode (including "bash --posix" and being invoked as
> > "bin/sh"), it will set POSIXLY_CORRECT (but not export it). Similarly,
> > if POSIXLY_CORRECT is set in the outer environment, it will act more
> > like sh. So maybe that would be a better test.
> 
> Yes, but the check needs to be careful to make sure the shell that is
> running the check is indeed bash, so that it will explicitly exec bash for
> somebody who is running dash but exports POSIXLY_CORRECT to make GNU
> programs (other than bash) behave more standard compliant way.
Sorry, I thought that was obvious. Yes, this:
Show 8 quoted lines
> In other words,
> 
> 	if test -n "$POSIXLY_CORRECT" && test -n "$BASH"
> 	then
>         	: we are running bash under posix mode
> 	elif ...
> 
> or somesuch.

is what I meant. Replace the "does it end in /bash" bit with "POSIXLY_CORRECT" but, keep the $BASH check.

-Peff
Jeff King· Apr 8, 2012, 08:12 UTC · re: Jeff King · lore

Re: [PATCH 01/12] tests: add initial bash completion tests

On Sun, Apr 08, 2012 at 01:42:51AM -0400, Jeff King wrote:
Show 18 quoted lines
> > Yes, but the check needs to be careful to make sure the shell that is
> > running the check is indeed bash, so that it will explicitly exec bash for
> > somebody who is running dash but exports POSIXLY_CORRECT to make GNU
> > programs (other than bash) behave more standard compliant way.
> 
> Sorry, I thought that was obvious. Yes, this:
> 
> > In other words,
> > 
> > 	if test -n "$POSIXLY_CORRECT" && test -n "$BASH"
> > 	then
> >         	: we are running bash under posix mode
> > 	elif ...
> > 
> > or somesuch.
> 
> is what I meant. Replace the "does it end in /bash" bit with
> "POSIXLY_CORRECT" but, keep the $BASH check.

BTW, if you intend to exec bash when we see a posix bash, I think you would want to unset POSIXLY_CORRECT in case it is exported. Otherwise, doing this:

 POSIXLY_CORRECT=1 bash ./t9902-*

would loop forever, trying to restart bash, but ending up in the POSIX version each time. So all together:

  if test -n "$BASH" && test -z "$POSIXLY_CORRECT"; then
          : we are in full-on bash mode. Great.
  elif type bash >/dev/null 2>&1; then
          # We are not bash, or are in posix mode. Run a new bash,
          # making sure not to let any posix environment variable
          # propagate.
          unset POSIXLY_CORRECT
          exec bash "$0" "$@"
  else
          echo '1..0 # SKIP skipping bash completion tests; bash not available'
          exit 0
  fi
-Peff
PS I wondered if we can continue past the "exec" in the second branch if
   it fails, and if we should be exiting explicitly. POSIX specifies
   that "exec" with a command never returns, and that is what dash does.
   Bash will continue after a failed exec. I expected it not to do so
   in posix mode, but it seems to. Which is perhaps a bug in bash, but
   one we might want to deal with. Granted, the likelihood of "type"
   succeeding but the exec failing is low, so it may not be worth caring
   about.
Andreas Schwab· Apr 8, 2012, 09:07 UTC · re: Jeff King · lore

Re: [PATCH 01/12] tests: add initial bash completion tests

Jeff King <peff@peff.net> writes:
>    Bash will continue after a failed exec.

Only when interactive. A non-interactive shell respects the execfail shopt (off by default).

Andreas.
-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."
Jeff King· Apr 8, 2012, 11:04 UTC · re: Andreas Schwab · lore

Re: [PATCH 01/12] tests: add initial bash completion tests

On Sun, Apr 08, 2012 at 11:07:51AM +0200, Andreas Schwab wrote:
> >    Bash will continue after a failed exec.
> 
> Only when interactive.  A non-interactive shell respects the execfail
> shopt (off by default).

Thanks, I didn't know that. Let's not worry about error-handling after the exec then.

-Peff
Felipe Contreras· Apr 11, 2012, 21:59 UTC · re: Junio C Hamano · lore

Re: [PATCH 01/12] tests: add initial bash completion tests

On Sun, Apr 8, 2012 at 7:25 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 30 quoted lines
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>> ---
>>  t/t9902-completion.sh |  201 +++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 201 insertions(+)
>>  create mode 100755 t/t9902-completion.sh
>>
>> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
>> new file mode 100755
>> index 0000000..7eb80dd
>> --- /dev/null
>> +++ b/t/t9902-completion.sh
>> @@ -0,0 +1,201 @@
>> +#!/bin/sh
>> +#
>> +# Copyright (c) 2012 Felipe Contreras
>> +#
>> +
>> +if type bash >/dev/null 2>&1
>> +then
>> +     # execute inside bash
>> +     test -z "$BASH" && exec bash "$0" "$@"
>> +else
>> +     echo '1..0 #SKIP skipping bash completion tests; bash not available'
>> +     exit 0
>> +fi
>
> What shell do you use on your system as /bin/sh (or if you use SHELL_PATH
> in the Makefile to override it, what do you use)?

/bin/sh is a symlink to bash on my system, and although bash defines POSIXLY_CORRECT, the tests pass just fine. I even tried to run /bin/sh --posix.

-- 
Felipe Contreras
Junio C Hamano· Apr 11, 2012, 23:49 UTC · re: Felipe Contreras · lore

Re: [PATCH 01/12] tests: add initial bash completion tests

Felipe Contreras <felipe.contreras@gmail.com> writes:
Show 8 quoted lines
> On Sun, Apr 8, 2012 at 7:25 AM, Junio C Hamano <gitster@pobox.com> wrote:
> ...
>> What shell do you use on your system as /bin/sh (or if you use SHELL_PATH
>> in the Makefile to override it, what do you use)?
>
> /bin/sh is a symlink to bash on my system, and although bash defines
> POSIXLY_CORRECT, the tests pass just fine. I even tried to run /bin/sh
> --posix.

It really depends on what's in contrib/git-completion.bash and what you test.

For example, if a (possibly future) version of completion script had lines like these (taken from 6486ca6d7):

	__some_function ()
        {
		...
                while read key value; do
                        ...
                done < <(git config -z --get-regexp ...
		...
	}
invoking "bash --posix" would have failed like this:
        $ bash --posix
        bash-4.1$ _a ()
        > {
        >   while read x; do
        >     echo "* $x"
        >   done < <(echo a; echo b)
        bash: syntax error near unexpected token `<'

Perhaps the "initial tests" you posted, together with the version of the completion script we happen to have near 'master', didn't have any construct that gives a visible failure like the above, but that is just by dumb luck and is not a reason for us to be complacent and not to be careful.

The early part of your re-roll (i.e. v2) seems to be done carefully to make sure we run the script in full-fledged bash, from a quick glance.

Thanks.
Jeff King· Apr 8, 2012, 05:01 UTC · re: Felipe Contreras · lore

Re: [PATCH 01/12] tests: add initial bash completion tests

On Sun, Apr 08, 2012 at 06:07:48AM +0300, Felipe Contreras wrote:
Show 8 quoted lines
> +	cat >expected <<-\EOF &&
> +	fetch 
> +	filter-branch 
> +	filter-branch.sh 
> +	format-patch 
> +	fsck 
> +	EOF
> +	test_completion "git f"

This test fails for me. The problem is that I have other git-f* programs in my PATH, and the completion finds and displays them. In other words, the environment outside the test suite can pollute the result.

I'm not sure of the right solution. We can't just sanitize the PATH in test-lib.sh, since those git programs might be in /usr/bin or some other directory containing other commands necessary to run the test suite. We could sanitize it temporarily just for the _git completion invocation, which consists only of builtins (and we know we're running under bash, so we can trust that things like "test" are builtins). But it still feels horribly hacky.

That patch might look like this:
Show changes to 2 files +7 −0

t/t9902-completion.sh, t/test-lib.sh

diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 7eb80dd..713f4b1 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -58,8 +58,11 @@ test_completion ()
 	_words=( $1 )
 	test $# -gt 1 && echo "$2" > expected
 	(( _cword = ${#_words[@]} - 1 ))
+	saved_path=$PATH
+	PATH=$MINIMAL_PATH
 	_git && print_comp &&
 	test_cmp expected out
+	PATH=$saved_path
 }
 
 test_expect_success 'basic' '
diff --git a/t/test-lib.sh b/t/test-lib.sh
index b7d7100..348b68d 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -466,12 +466,14 @@ then
 	IFS=$OLDIFS
 	PATH=$GIT_VALGRIND/bin:$PATH
 	GIT_EXEC_PATH=$GIT_VALGRIND/bin
+	MINIMAL_PATH=$GIT_VALGRIND/bin
 	export GIT_VALGRIND
 elif test -n "$GIT_TEST_INSTALLED" ; then
 	GIT_EXEC_PATH=$($GIT_TEST_INSTALLED/git --exec-path)  ||
 	error "Cannot run git from $GIT_TEST_INSTALLED."
 	PATH=$GIT_TEST_INSTALLED:$GIT_BUILD_DIR:$PATH
 	GIT_EXEC_PATH=${GIT_TEST_EXEC_PATH:-$GIT_EXEC_PATH}
+	MINIMAL_PATH=$GIT_TEST_INSTALLED:$GIT_BUILD_DIR
 else # normal case, use ../bin-wrappers only unless $with_dashes:
 	git_bin_dir="$GIT_BUILD_DIR/bin-wrappers"
 	if ! test -x "$git_bin_dir/git" ; then
@@ -482,8 +484,10 @@ else # normal case, use ../bin-wrappers only unless $with_dashes:
 	fi
 	PATH="$git_bin_dir:$PATH"
 	GIT_EXEC_PATH=$GIT_BUILD_DIR
+	MINIMAL_PATH=$git_bin_dir
 	if test -n "$with_dashes" ; then
 		PATH="$GIT_BUILD_DIR:$PATH"
+		MINIMAL_PATH=$MINIMAL_PATH:$GIT_BUILD_DIR
 	fi
 fi
 GIT_TEMPLATE_DIR="$GIT_BUILD_DIR"/templates/blt
Jonathan Nieder· Apr 8, 2012, 10:30 UTC · re: Jeff King · lore

Re: [PATCH 01/12] tests: add initial bash completion tests

(dropping Shawn from cc list since he hasn't been working on the
 completion script for a couple of years now)
Jeff King wrote:
> On Sun, Apr 08, 2012 at 06:07:48AM +0300, Felipe Contreras wrote:
Show 11 quoted lines
>> +	cat >expected <<-\EOF &&
>> +	fetch 
>> +	filter-branch 
>> +	filter-branch.sh 
>> +	format-patch 
>> +	fsck 
>> +	EOF
>> +	test_completion "git f"
>
> This test fails for me. The problem is that I have other git-f* programs
> in my PATH
[...]
Show 7 quoted lines
>                                     We can't just sanitize the PATH in
> test-lib.sh, since those git programs might be in /usr/bin or some other
> directory containing other commands necessary to run the test suite. We
> could sanitize it temporarily just for the _git completion invocation,
> which consists only of builtins (and we know we're running under bash,
> so we can trust that things like "test" are builtins). But it still
> feels horribly hacky.
Not to mention that it would break the TEST_INSTALLED_GIT facility.

Is there some reason we care about the exact list of completions for "git f"? The above also looks a little bogus because if someone were to remove the +x bit on the git-filter-branch.sh source file or ensure it is not on the PATH when tests are run (I have no particular opinion about whether that's a good idea; it's just an example) then this would change the actual output. That doesn't seem particularly important to check for when talking about how the tab completion behaves on the installed system.

Maybe it would make sense to check a few representative items.
	# builtin
	grep "^fetch\$" out &&
	# builtin whose implementation is not in builtin/cmd.c
	grep "^format-patch\$" out &&
	# script
	grep "^filter-branch\$" out &&
	# plumbing
	! grep "^fsck-objects\$" out &&
	sort out >out.sorted &&
	test_cmp out.sorted out
Jeff King· Apr 8, 2012, 11:06 UTC · re: Jonathan Nieder · lore

Re: [PATCH 01/12] tests: add initial bash completion tests

On Sun, Apr 08, 2012 at 05:30:02AM -0500, Jonathan Nieder wrote:
Show 10 quoted lines
> > This test fails for me. The problem is that I have other git-f* programs
> > in my PATH
> [...]
> Is there some reason we care about the exact list of completions for
> "git f"?
> [...]
> Maybe it would make sense to check a few representative items.
> 
> 	# builtin
> 	grep "^fetch\$" out &&

I think your approach (to stop worrying about stopping pollution from the environment, and start being more specific in what the tests care about) is much saner than the snippet I posted.

-Peff
Junio C Hamano· Apr 9, 2012, 19:58 UTC · re: Jeff King · lore

Re: [PATCH 01/12] tests: add initial bash completion tests

Jeff King <peff@peff.net> writes:
Show 16 quoted lines
> On Sun, Apr 08, 2012 at 05:30:02AM -0500, Jonathan Nieder wrote:
>
>> > This test fails for me. The problem is that I have other git-f* programs
>> > in my PATH
>> [...]
>> Is there some reason we care about the exact list of completions for
>> "git f"?
>> [...]
>> Maybe it would make sense to check a few representative items.
>> 
>> 	# builtin
>> 	grep "^fetch\$" out &&
>
> I think your approach (to stop worrying about stopping pollution from
> the environment, and start being more specific in what the tests care
> about) is much saner than the snippet I posted.

I have to agree with that; it might give unexpeceted results if the user has a $HOME/bin/git-update-index or something, though.

John Keeping· Apr 8, 2012, 10:28 UTC · re: Felipe Contreras · lore

Re: [PATCH 01/12] tests: add initial bash completion tests

On Sun, Apr 08, 2012 at 06:07:48AM +0300, Felipe Contreras wrote:
Show 10 quoted lines
> +test_completion ()
> +{
> +	local -a COMPREPLY _words
> +	local _cword
> +	_words=( $1 )
> +	test $# -gt 1 && echo "$2" > expected
> +	(( _cword = ${#_words[@]} - 1 ))
> +	_git && print_comp &&
> +	test_cmp expected out
> +}

Could you check that _git doesn't output anything to stderr here? I can't see any reason why a completion command should ever do so legitimately and it's generally annoying if one does.

-- 
John
Felipe Contreras· Apr 8, 2012, 03:07 UTC · re: Felipe Contreras · lore

[PATCH 02/12] completion: simplify __gitcomp

The suffix is never used.
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 contrib/completion/git-completion.bash |   12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)
Show changes to contrib/completion/git-completion.bash +5 −7
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 31f714d..39f5435 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -304,15 +304,14 @@ __git_ps1 ()
 	fi
 }
 
-# __gitcomp_1 requires 2 arguments
 __gitcomp_1 ()
 {
 	local c IFS=' '$'\t'$'\n'
 	for c in $1; do
-		case "$c$2" in
-		--*=*) printf %s$'\n' "$c$2" ;;
-		*.)    printf %s$'\n' "$c$2" ;;
-		*)     printf %s$'\n' "$c$2 " ;;
+		case "$c" in
+		--*=*) printf %s$'\n' "$c" ;;
+		*.)    printf %s$'\n' "$c" ;;
+		*)     printf %s$'\n' "$c " ;;
 		esac
 	done
 }
@@ -479,7 +478,6 @@ fi
 # 1: List of possible completion words.
 # 2: A prefix to be added to each possible completion word (optional).
 # 3: Generate possible completion matches for this word (optional).
-# 4: A suffix to be appended to each possible completion word (optional).
 __gitcomp ()
 {
 	local cur_="${3-$cur}"
@@ -491,7 +489,7 @@ __gitcomp ()
 	*)
 		local IFS=$'\n'
 		COMPREPLY=($(compgen -P "${2-}" \
-			-W "$(__gitcomp_1 "${1-}" "${4-}")" \
+			-W "$(__gitcomp_1 "${1-}")" \
 			-- "$cur_"))
 		;;
 	esac
-- 
1.7.10.3.g5a738d
SZEDER Gábor· Apr 8, 2012, 12:46 UTC · re: Felipe Contreras · lore

Re: [PATCH 02/12] completion: simplify __gitcomp

Hi,
On Sun, Apr 08, 2012 at 06:07:49AM +0300, Felipe Contreras wrote:
> The suffix is never used.

The suffix was used to offer completion after 'git config branch.<TAB>' ending with a period. It is not used by git's completion script anymore, because nowadays this case is handled by the __gitcomp_nl helper() function.

However, users can provide custom completion functions to their aliases or additional git commands, and there might be a custom completion function out there that passes a suffix to __gitcomp().

We don't want to break those custom completion functions, so I think the suffix should stay.

Best, Gábor

Show 49 quoted lines
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  contrib/completion/git-completion.bash |   12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 31f714d..39f5435 100755
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -304,15 +304,14 @@ __git_ps1 ()
>  	fi
>  }
>  
> -# __gitcomp_1 requires 2 arguments
>  __gitcomp_1 ()
>  {
>  	local c IFS=' '$'\t'$'\n'
>  	for c in $1; do
> -		case "$c$2" in
> -		--*=*) printf %s$'\n' "$c$2" ;;
> -		*.)    printf %s$'\n' "$c$2" ;;
> -		*)     printf %s$'\n' "$c$2 " ;;
> +		case "$c" in
> +		--*=*) printf %s$'\n' "$c" ;;
> +		*.)    printf %s$'\n' "$c" ;;
> +		*)     printf %s$'\n' "$c " ;;
>  		esac
>  	done
>  }
> @@ -479,7 +478,6 @@ fi
>  # 1: List of possible completion words.
>  # 2: A prefix to be added to each possible completion word (optional).
>  # 3: Generate possible completion matches for this word (optional).
> -# 4: A suffix to be appended to each possible completion word (optional).
>  __gitcomp ()
>  {
>  	local cur_="${3-$cur}"
> @@ -491,7 +489,7 @@ __gitcomp ()
>  	*)
>  		local IFS=$'\n'
>  		COMPREPLY=($(compgen -P "${2-}" \
> -			-W "$(__gitcomp_1 "${1-}" "${4-}")" \
> +			-W "$(__gitcomp_1 "${1-}")" \
>  			-- "$cur_"))
>  		;;
>  	esac
> -- 
> 1.7.10.3.g5a738d
> 
Felipe Contreras· Apr 8, 2012, 13:07 UTC · re: SZEDER Gábor · lore

Re: [PATCH 02/12] completion: simplify __gitcomp

2012/4/8 SZEDER Gábor <szeder@ira.uka.de>:
Show 11 quoted lines
> On Sun, Apr 08, 2012 at 06:07:49AM +0300, Felipe Contreras wrote:
>> The suffix is never used.
>
> The suffix was used to offer completion after 'git config
> branch.<TAB>' ending with a period.  It is not used by git's
> completion script anymore, because nowadays this case is handled by
> the __gitcomp_nl helper() function.
>
> However, users can provide custom completion functions to their
> aliases or additional git commands, and there might be a custom
> completion function out there that passes a suffix to __gitcomp().

Yes, there _might_ be, and they would have to be fixed; APIs change. However, this is highly theoretical, what suffix could anybody possibly want? I believe __gitcomp_1 already does the job of adding sensible suffixes, and there's nothing else anybody else could possibly want.

In any case, shouldn't compgen -S "${4-}" do the trick?
> We don't want to break those custom completion functions, so I think
> the suffix should stay.

So we should never make any cleanups because some custom completion functions might theoretically break? I think at some point we need to accept that nobody is using this argument anyway (and they should).

Cheers.
-- 
Felipe Contreras
Jonathan Nieder· Apr 8, 2012, 13:27 UTC · re: Felipe Contreras · lore

Re: [PATCH 02/12] completion: simplify __gitcomp

Felipe Contreras wrote:
> So we should never make any cleanups because some custom completion
> functions might theoretically break?

No, tasteful cleanups that don't break custom completion functions are very welcome.

Also if you have evidence that this is definitely (not just possibly) only a theoretical problem, then it would not be a regression. But I actually find it likely that custom completion code would have imitated the old completion code in git that used the suffix argument, so...

Can you remind me what the benefit that the user is getting from this patch in exchange for us breaking their tab completion? The title ("simplify __gitcomp") tells me exactly nothing about its impact except that it is perhaps supposed to result in no behavior change.

Sorry for the fuss and hope that helps, Jonathan

Felipe Contreras· Apr 8, 2012, 13:47 UTC · re: Jonathan Nieder · lore

Re: [PATCH 02/12] completion: simplify __gitcomp

On Sun, Apr 8, 2012 at 4:27 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 13 quoted lines
> Felipe Contreras wrote:
>
>> So we should never make any cleanups because some custom completion
>> functions might theoretically break?
>
> No, tasteful cleanups that don't break custom completion functions are
> very welcome.
>
> Also if you have evidence that this is definitely (not just possibly)
> only a theoretical problem, then it would not be a regression.  But I
> actually find it likely that custom completion code would have
> imitated the old completion code in git that used the suffix argument,
> so...
Show me an example. If you do something like:
 __gitcomp "foo bar opt=" "" "" "suffix"

The result would be 'foo suffix' 'bar suffix' 'opt=suffix'. How could that *possibly* be useful?

> Can you remind me what the benefit that the user is getting from this
> patch
Absolutely none.
> in exchange for us breaking their tab completion?
Whoa! Breaking their tab completion? Where? Can you show me some evidence?
>  The title
> ("simplify __gitcomp") tells me exactly nothing about its impact
> except that it is perhaps supposed to result in no behavior change.

That is correct. No functional changes. I would need to update the tests if something changed, right?

Cheers.
-- 
Felipe Contreras
Jonathan Nieder· Apr 8, 2012, 14:36 UTC · re: Felipe Contreras · lore

Re: [PATCH 02/12] completion: simplify __gitcomp

(dropping Shawn from cc since I'm not under the impression he works
 on the completion script these days)
Felipe Contreras wrote:
> Whoa! Breaking their tab completion? Where? Can you show me some evidence?
If you weren't listening before, I'm not sure what I can add now[*].

Luckily, I already said what I needed to say. Yes, cleanups can be good when they don't break things, and no, cleanups that break things are not good. Sometimes it is not obvious which category each case falls into. As you well know, "tests pass" is not enough (e.g., sometimes there are no tests!). If you want someone to argue with, you can find someone else.

Sorry, Jonathan

[*] To be extra, extra clear: I never said your patch breaks people's custom tab completion scripts. I said it might do so and that that is not very comforting to apply the patch when no one seems to have thought carefully about how to investigate that and mitigate the damage if there is any.

Felipe Contreras· Apr 8, 2012, 14:58 UTC · re: Jonathan Nieder · lore

Re: [PATCH 02/12] completion: simplify __gitcomp

On Sun, Apr 8, 2012 at 5:36 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 7 quoted lines
> (dropping Shawn from cc since I'm not under the impression he works
>  on the completion script these days)
> Felipe Contreras wrote:
>
>> Whoa! Breaking their tab completion? Where? Can you show me some evidence?
>
> If you weren't listening before, I'm not sure what I can add now[*].

This is what you said; "in exchange for us breaking their tab completion". There's a big difference between "breaking", and "*potentially* breaking". I have never seen any evidence of tab completion actually being broken.

Show 6 quoted lines
> Luckily, I already said what I needed to say.  Yes, cleanups can be
> good when they don't break things, and no, cleanups that break things
> are not good.  Sometimes it is not obvious which category each case
> falls into.  As you well know, "tests pass" is not enough (e.g.,
> sometimes there are no tests!).  If you want someone to argue with,
> you can find someone else.

Fortunately there's no evidence of anything being broken; all the issues mentioned are *theoretical*, and there's not even an example of a custom completion that would be broken by this.

-- 
Felipe Contreras
Johannes Sixt· Apr 9, 2012, 18:57 UTC · re: Felipe Contreras · lore

Re: [PATCH 02/12] completion: simplify __gitcomp

Am 08.04.2012 16:58, schrieb Felipe Contreras:
Show 13 quoted lines
> On Sun, Apr 8, 2012 at 5:36 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>> (dropping Shawn from cc since I'm not under the impression he works
>>  on the completion script these days)
>> Felipe Contreras wrote:
>>
>>> Whoa! Breaking their tab completion? Where? Can you show me some evidence?
>>
>> If you weren't listening before, I'm not sure what I can add now[*].
> 
> This is what you said; "in exchange for us breaking their tab
> completion". There's a big difference between "breaking", and
> "*potentially* breaking". I have never seen any evidence of tab
> completion actually being broken.
I have:

http://thread.gmane.org/gmane.comp.version-control.msysgit/13310/focus=13335 http://thread.gmane.org/gmane.comp.version-control.git/185184/focus=185189

-- Hannes
Felipe Contreras· Apr 9, 2012, 19:12 UTC · re: Johannes Sixt · lore

Re: [PATCH 02/12] completion: simplify __gitcomp

On Mon, Apr 9, 2012 at 9:57 PM, Johannes Sixt <j6t@kdbg.org> wrote:
Show 19 quoted lines
> Am 08.04.2012 16:58, schrieb Felipe Contreras:
>> On Sun, Apr 8, 2012 at 5:36 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>>> (dropping Shawn from cc since I'm not under the impression he works
>>>  on the completion script these days)
>>> Felipe Contreras wrote:
>>>
>>>> Whoa! Breaking their tab completion? Where? Can you show me some evidence?
>>>
>>> If you weren't listening before, I'm not sure what I can add now[*].
>>
>> This is what you said; "in exchange for us breaking their tab
>> completion". There's a big difference between "breaking", and
>> "*potentially* breaking". I have never seen any evidence of tab
>> completion actually being broken.
>
> I have:
>
> http://thread.gmane.org/gmane.comp.version-control.msysgit/13310/focus=13335
> http://thread.gmane.org/gmane.comp.version-control.git/185184/focus=185189
I meant being broken by this patch.
Plus this patch series actually fixes those issues.
-- 
Felipe Contreras
Felipe Contreras· Apr 8, 2012, 14:11 UTC · re: Jonathan Nieder · lore

Re: [PATCH 02/12] completion: simplify __gitcomp

Hi,

I've looked at the history of __gitcomp and __gitcomp_nl, now I think it makes sense to keep their arguments similar, at least for the time being.

So I suggest this patch to be dropped.
On Sun, Apr 8, 2012 at 4:27 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 21 quoted lines
> Felipe Contreras wrote:
>
>> So we should never make any cleanups because some custom completion
>> functions might theoretically break?
>
> No, tasteful cleanups that don't break custom completion functions are
> very welcome.
>
> Also if you have evidence that this is definitely (not just possibly)
> only a theoretical problem, then it would not be a regression.  But I
> actually find it likely that custom completion code would have
> imitated the old completion code in git that used the suffix argument,
> so...
>
> Can you remind me what the benefit that the user is getting from this
> patch in exchange for us breaking their tab completion?  The title
> ("simplify __gitcomp") tells me exactly nothing about its impact
> except that it is perhaps supposed to result in no behavior change.
>
> Sorry for the fuss and hope that helps,
> Jonathan
Jonathan Nieder· Apr 8, 2012, 14:39 UTC · re: Felipe Contreras · lore

Re: [PATCH 02/12] completion: simplify __gitcomp

(-cc: Shawn) Felipe Contreras wrote:

Show 5 quoted lines
> I've looked at the history of __gitcomp and __gitcomp_nl, now I think
> it makes sense to keep their arguments similar, at least for the time
> being.
>
> So I suggest this patch to be dropped.

Thanks for looking into it, and sorry I responded to a heated response instead of waiting for a calmer one.

I do encourage you to examine your communication style.

Cheers, Jonathan

Junio C Hamano· Apr 9, 2012, 18:22 UTC · re: Jonathan Nieder · lore

Re: [PATCH 02/12] completion: simplify __gitcomp

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 11 quoted lines
> (-cc: Shawn)
> Felipe Contreras wrote:
>
>> I've looked at the history of __gitcomp and __gitcomp_nl, now I think
>> it makes sense to keep their arguments similar, at least for the time
>> being.
>>
>> So I suggest this patch to be dropped.
>
> Thanks for looking into it, and sorry I responded to a heated response
> instead of waiting for a calmer one.
Good to see a civilized discussion of patches on technical merits.
Thanks.
Felipe Contreras· Apr 8, 2012, 03:07 UTC · re: Felipe Contreras · lore
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 contrib/completion/git-completion.bash |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
Show changes to contrib/completion/git-completion.bash +4 −4
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 39f5435..5c66674 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -306,13 +306,13 @@ __git_ps1 ()
 
 __gitcomp_1 ()
 {
-	local c IFS=' '$'\t'$'\n'
+	local c s IFS=' '$'\t'$'\n'
 	for c in $1; do
 		case "$c" in
-		--*=*) printf %s$'\n' "$c" ;;
-		*.)    printf %s$'\n' "$c" ;;
-		*)     printf %s$'\n' "$c " ;;
+		--*=* | *.) s="" ;;
+		*)          s=" " ;;
 		esac
+		echo "$c$s"
 	done
 }
 
-- 
1.7.10.3.g5a738d
Felipe Contreras· Apr 8, 2012, 03:07 UTC · re: Felipe Contreras · lore

[PATCH 04/12] completion: trivial simplification

cword-1 is the previous word ($prev).
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 contrib/completion/git-completion.bash |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Show changes to contrib/completion/git-completion.bash +2 −2
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 5c66674..3bc8d85 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1656,7 +1656,7 @@ _git_notes ()
 		__gitcomp '--ref'
 		;;
 	,*)
-		case "${words[cword-1]}" in
+		case "$prev" in
 		--ref)
 			__gitcomp_nl "$(__git_refs)"
 			;;
@@ -1682,7 +1682,7 @@ _git_notes ()
 	prune,*)
 		;;
 	*)
-		case "${words[cword-1]}" in
+		case "$prev" in
 		-m|-F)
 			;;
 		*)
-- 
1.7.10.3.g5a738d
Felipe Contreras· Apr 8, 2012, 03:07 UTC · re: Felipe Contreras · lore

[PATCH 05/12] completion: add missing global options

Otherwise 'git --foo bar<tab>' fails, also, other options are completely missing.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 contrib/completion/git-completion.bash |    9 +++++++--
 t/t9902-completion.sh                  |   31 +++++++++++++++++++++++++++++++
 2 files changed, 38 insertions(+), 2 deletions(-)
Show changes to 2 files +38 −2

contrib/completion/git-completion.bash, t/t9902-completion.sh

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 3bc8d85..c9672b2 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2621,7 +2621,10 @@ _git ()
 		case "$i" in
 		--git-dir=*) __git_dir="${i#--git-dir=}" ;;
 		--bare)      __git_dir="." ;;
-		--version|-p|--paginate) ;;
+		--version) ;;
+		-p|--paginate|--no-pager) ;;
+		--exec-path=*|--html-path|--info-path) ;;
+		--work-tree=*|--namespace=*|--no-replace-objects) ;;
 		--help) command="help"; break ;;
 		*) command="$i"; break ;;
 		esac
@@ -2636,10 +2639,12 @@ _git ()
 			--git-dir=
 			--bare
 			--version
-			--exec-path
+			--exec-path=
 			--html-path
+			--info-path
 			--work-tree=
 			--namespace=
+			--no-replace-objects
 			--help
 			"
 			;;
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 7eb80dd..ee5654d 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -198,4 +198,35 @@ test_expect_success 'double dash' '
 	test_completion "git checkout --"
 '
 
+test_expect_success 'general options' '
+	test_completion "git --ver" "--version " &&
+	test_completion "git --hel" "--help " &&
+	test_completion "git --exe" "--exec-path=" &&
+	test_completion "git --htm" "--html-path " &&
+	test_completion "git --pag" "--paginate " &&
+	test_completion "git --no-p" "--no-pager " &&
+	test_completion "git --git" "--git-dir=" &&
+	test_completion "git --wor" "--work-tree=" &&
+	test_completion "git --nam" "--namespace=" &&
+	test_completion "git --bar" "--bare " &&
+	test_completion "git --inf" "--info-path " &&
+	test_completion "git --no-r" "--no-replace-objects "
+'
+
+test_expect_success 'general options plus command' '
+	test_completion "git --version check" "checkout " &&
+	test_completion "git --paginate check" "checkout " &&
+	test_completion "git --git-dir=foo check" "checkout " &&
+	test_completion "git --bare check" "checkout " &&
+	test_completion "git --help des" "describe " &&
+	test_completion "git --exec-path=foo check" "checkout " &&
+	test_completion "git --html-path check" "checkout " &&
+	test_completion "git --no-pager check" "checkout " &&
+	test_completion "git --work-tree=foo check" "checkout " &&
+	test_completion "git --namespace=foo check" "checkout " &&
+	test_completion "git --paginate check" "checkout " &&
+	test_completion "git --info-path check" "checkout " &&
+	test_completion "git --no-replace-objects check" "checkout "
+'
+
 test_done
-- 
1.7.10.3.g5a738d
John Keeping· Apr 8, 2012, 10:22 UTC · re: Felipe Contreras · lore

Re: [PATCH 05/12] completion: add missing global options

On Sun, Apr 08, 2012 at 06:07:52AM +0300, Felipe Contreras wrote:
Show 8 quoted lines
> @@ -2621,7 +2621,10 @@ _git ()
>  		case "$i" in
>  		--git-dir=*) __git_dir="${i#--git-dir=}" ;;
>  		--bare)      __git_dir="." ;;
> -		--version|-p|--paginate) ;;
> +		--version) ;;
> +		-p|--paginate|--no-pager) ;;
> +		--exec-path=*|--html-path|--info-path) ;;
Since exec-path's argument is optional, shouldn't this be:
  +		--exec-path*|--html-path|--info-path) ;;
> +		--work-tree=*|--namespace=*|--no-replace-objects) ;;
It would be nice to accept options with argument with and without the
equals sign, as the git command does, so both:
    git --git-dir=/foo/bar
and
    git --git-dir /foo/bar
I guess this looks like (adding in -c as well):
		--git-dir)  ((c++)) ; __git_dir="${words[c]}" ;;
                --work-tree|--namespacei|-c) ((c++)) ;;
Show 9 quoted lines
>  		--help) command="help"; break ;;
>  		*) command="$i"; break ;;
>  		esac
> @@ -2636,10 +2639,12 @@ _git ()
>  			--git-dir=
>  			--bare
>  			--version
> -			--exec-path
> +			--exec-path=

I think this change is incorrect since giving --exec-path without an argument is perfectly fine and has a defined meaning.

Show 6 quoted lines
>  			--html-path
> +			--info-path
>  			--work-tree=
>  			--namespace=
> +			--no-replace-objects
>  			--help
Should "-c" be in this list as well?
>  			"
>  			;;
-- 
John
SZEDER Gábor· Apr 8, 2012, 12:36 UTC · re: Felipe Contreras · lore

Re: [PATCH 05/12] completion: add missing global options

Hi,
On Sun, Apr 08, 2012 at 06:07:52AM +0300, Felipe Contreras wrote:
Show 25 quoted lines
> Otherwise 'git --foo bar<tab>' fails, also, other options are completely
> missing.
> 
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  contrib/completion/git-completion.bash |    9 +++++++--
>  t/t9902-completion.sh                  |   31 +++++++++++++++++++++++++++++++
>  2 files changed, 38 insertions(+), 2 deletions(-)
> 
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 3bc8d85..c9672b2 100755
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -2621,7 +2621,10 @@ _git ()
>  		case "$i" in
>  		--git-dir=*) __git_dir="${i#--git-dir=}" ;;
>  		--bare)      __git_dir="." ;;
> -		--version|-p|--paginate) ;;
> +		--version) ;;
> +		-p|--paginate|--no-pager) ;;
> +		--exec-path=*|--html-path|--info-path) ;;
> +		--work-tree=*|--namespace=*|--no-replace-objects) ;;
>  		--help) command="help"; break ;;
>  		*) command="$i"; break ;;
>  		esac

This is not future proof, it will break again when we add yet another global option. This is also incomplete, because the '-c <configvar=value>' option is missing.

I send a patch to fix this issue a couple of months ago but it got dropped on the floor.

  http://thread.gmane.org/gmane.comp.version-control.git/180650

My patch handles the -c global option and it won't break when we add a new global option, so I think it should replace the above hunk.

Please feel free to copy the commit message, too, for the benefit of other developers who might read the log message a year later.

Show 15 quoted lines
> +test_expect_success 'general options plus command' '
> +	test_completion "git --version check" "checkout " &&
> +	test_completion "git --paginate check" "checkout " &&
> +	test_completion "git --git-dir=foo check" "checkout " &&
> +	test_completion "git --bare check" "checkout " &&
> +	test_completion "git --help des" "describe " &&
> +	test_completion "git --exec-path=foo check" "checkout " &&
> +	test_completion "git --html-path check" "checkout " &&
> +	test_completion "git --no-pager check" "checkout " &&
> +	test_completion "git --work-tree=foo check" "checkout " &&
> +	test_completion "git --namespace=foo check" "checkout " &&
> +	test_completion "git --paginate check" "checkout " &&
> +	test_completion "git --info-path check" "checkout " &&
> +	test_completion "git --no-replace-objects check" "checkout "
> +'
Then you could add a 
	test_completion "git -c 'name=value' check" "checkout "
test, too.

Best, Gábor

Felipe Contreras· Apr 8, 2012, 03:07 UTC · re: Felipe Contreras · lore

[PATCH 06/12] tests: add more bash completion tests

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(+)
Show changes to t/t9902-completion.sh +55 −0
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index ee5654d..f1b660f 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -62,6 +62,22 @@ test_completion ()
 	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' '
 	cat >expected <<-\EOF &&
 	help 
@@ -229,4 +245,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.3.g5a738d
Felipe Contreras· Apr 8, 2012, 03:07 UTC · re: Felipe Contreras · lore

[PATCH 07/12] completion: simplify command stuff

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(-)
Show changes to contrib/completion/git-completion.bash +7 −7
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index c9672b2..1fe11f4 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -721,7 +721,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++))
@@ -2599,7 +2599,7 @@ _git_whatchanged ()
 
 _git ()
 {
-	local i c=1 command __git_dir
+	local i c=1 cmd __git_dir
 
 	if [[ -n ${ZSH_VERSION-} ]]; then
 		emulate -L bash
@@ -2625,13 +2625,13 @@ _git ()
 		-p|--paginate|--no-pager) ;;
 		--exec-path=*|--html-path|--info-path) ;;
 		--work-tree=*|--namespace=*|--no-replace-objects) ;;
-		--help) command="help"; break ;;
-		*) command="$i"; break ;;
+		--help) cmd="help"; break ;;
+		*) cmd="$i"; break ;;
 		esac
 		((c++))
 	done
 
-	if [ -z "$command" ]; then
+	if [ -z "$cmd" ]; then
 		case "$cur" in
 		--*)   __gitcomp "
 			--paginate
@@ -2654,10 +2654,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.3.g5a738d
SZEDER Gábor· Apr 11, 2012, 22:14 UTC · re: Felipe Contreras · lore

Re: [PATCH 07/12] completion: simplify command stuff

Hi,
On Sun, Apr 08, 2012 at 06:07:54AM +0300, Felipe Contreras wrote:
> No need to recalculate it.

Based on this short description I would expect that this "command stuff" is calculated somewhere, and a helper function is changed to use the already calculated value ...

Show 55 quoted lines
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index c9672b2..1fe11f4 100755
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -721,7 +721,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++))
> @@ -2599,7 +2599,7 @@ _git_whatchanged ()
>  
>  _git ()
>  {
> -	local i c=1 command __git_dir
> +	local i c=1 cmd __git_dir
>  
>  	if [[ -n ${ZSH_VERSION-} ]]; then
>  		emulate -L bash
> @@ -2625,13 +2625,13 @@ _git ()
>  		-p|--paginate|--no-pager) ;;
>  		--exec-path=*|--html-path|--info-path) ;;
>  		--work-tree=*|--namespace=*|--no-replace-objects) ;;
> -		--help) command="help"; break ;;
> -		*) command="$i"; break ;;
> +		--help) cmd="help"; break ;;
> +		*) cmd="$i"; break ;;
>  		esac
>  		((c++))
>  	done
>  
> -	if [ -z "$command" ]; then
> +	if [ -z "$cmd" ]; then
>  		case "$cur" in
>  		--*)   __gitcomp "
>  			--paginate
> @@ -2654,10 +2654,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.3.g5a738d
> 

... but the bulk of this change is in the top-level _git() function, as it renames a variable there to match the variable name in the helper function. Perhaps it should be the other way around.

However, this change "promotes" the command (or cmd) variable from being just a variable in _git() to a variable that is used in other completion functions, too. Why not go one more step further, and denote this by adding a __git prefix (i.e. renaming the variable to __git_command)?

Best, Gábor

Felipe Contreras· Apr 11, 2012, 22:21 UTC · re: SZEDER Gábor · lore

Re: [PATCH 07/12] completion: simplify command stuff

2012/4/12 SZEDER Gábor <szeder@ira.uka.de>:
Show 9 quoted lines
> Hi,
>
>
> On Sun, Apr 08, 2012 at 06:07:54AM +0300, Felipe Contreras wrote:
>> No need to recalculate it.
>
> Based on this short description I would expect that this "command
> stuff" is calculated somewhere, and a helper function is changed to
> use the already calculated value ...
[...]
Show 9 quoted lines
> ... but the bulk of this change is in the top-level _git() function,
> as it renames a variable there to match the variable name in the
> helper function.  Perhaps it should be the other way around.
>
> However, this change "promotes" the command (or cmd) variable from
> being just a variable in _git() to a variable that is used in other
> completion functions, too.  Why not go one more step further, and
> denote this by adding a __git prefix (i.e. renaming the variable to
> __git_command)?

After thinking more about it, and analyzing the next patches, I don't think it makes sense to have such a variable; it's only used by __git_complete_remote_or_refspec, and it would be easier to make it an argument to that function; that would simplify this patch series a lot.

Cheers.
-- 
Felipe Contreras
SZEDER Gábor· Apr 11, 2012, 23:01 UTC · re: Felipe Contreras · lore

Re: [PATCH 07/12] completion: simplify command stuff

On Thu, Apr 12, 2012 at 01:21:17AM +0300, Felipe Contreras wrote:
Show 12 quoted lines
> 2012/4/12 SZEDER Gábor <szeder@ira.uka.de>:
> > However, this change "promotes" the command (or cmd) variable from
> > being just a variable in _git() to a variable that is used in other
> > completion functions, too.  Why not go one more step further, and
> > denote this by adding a __git prefix (i.e. renaming the variable to
> > __git_command)?
> 
> After thinking more about it, and analyzing the next patches, I don't
> think it makes sense to have such a variable; it's only used by
> __git_complete_remote_or_refspec, and it would be easier to make it an
> argument to that function; that would simplify this patch series a
> lot.
You mean that _git_fetch() would call
  __git_complete_remote_or_refspec "fetch"

while _git_pull() and _git_push() will pass "pull" and "push", respectively?

Best, Gábor

Felipe Contreras· Apr 11, 2012, 23:45 UTC · re: SZEDER Gábor · lore

Re: [PATCH 07/12] completion: simplify command stuff

2012/4/12 SZEDER Gábor <szeder@ira.uka.de>:
Show 20 quoted lines
> On Thu, Apr 12, 2012 at 01:21:17AM +0300, Felipe Contreras wrote:
>> 2012/4/12 SZEDER Gábor <szeder@ira.uka.de>:
>> > However, this change "promotes" the command (or cmd) variable from
>> > being just a variable in _git() to a variable that is used in other
>> > completion functions, too.  Why not go one more step further, and
>> > denote this by adding a __git prefix (i.e. renaming the variable to
>> > __git_command)?
>>
>> After thinking more about it, and analyzing the next patches, I don't
>> think it makes sense to have such a variable; it's only used by
>> __git_complete_remote_or_refspec, and it would be easier to make it an
>> argument to that function; that would simplify this patch series a
>> lot.
>
> You mean that _git_fetch() would call
>
>  __git_complete_remote_or_refspec "fetch"
>
> while _git_pull() and _git_push() will pass "pull" and "push",
> respectively?
Yeap. Then there would not be any need for the foo_wrap () stuff.
-- 
Felipe Contreras
Felipe Contreras· Apr 12, 2012, 23:08 UTC · re: Felipe Contreras · lore

Re: [PATCH 07/12] completion: simplify command stuff

2012/4/12 Felipe Contreras <felipe.contreras@gmail.com>:
Show 23 quoted lines
> 2012/4/12 SZEDER Gábor <szeder@ira.uka.de>:
>> On Thu, Apr 12, 2012 at 01:21:17AM +0300, Felipe Contreras wrote:
>>> 2012/4/12 SZEDER Gábor <szeder@ira.uka.de>:
>>> > However, this change "promotes" the command (or cmd) variable from
>>> > being just a variable in _git() to a variable that is used in other
>>> > completion functions, too.  Why not go one more step further, and
>>> > denote this by adding a __git prefix (i.e. renaming the variable to
>>> > __git_command)?
>>>
>>> After thinking more about it, and analyzing the next patches, I don't
>>> think it makes sense to have such a variable; it's only used by
>>> __git_complete_remote_or_refspec, and it would be easier to make it an
>>> argument to that function; that would simplify this patch series a
>>> lot.
>>
>> You mean that _git_fetch() would call
>>
>>  __git_complete_remote_or_refspec "fetch"
>>
>> while _git_pull() and _git_push() will pass "pull" and "push",
>> respectively?
>
> Yeap. Then there would not be any need for the foo_wrap () stuff.

Actually, there would be, we need to call _git_fetch(), but if we have generic wrap function we don't know that; all we have is the alias (e.g. gf from 'git fetch'). I guess we could do something like 'eval $(alias gf)' and find out the right function to call, but that already has an 'eval' there, I'm not sure it's worth it.

Any ideas?
-- 
Felipe Contreras
Felipe Contreras· Apr 8, 2012, 03:07 UTC · re: Felipe Contreras · lore

[PATCH 08/12] completion: simplify _git_bundle

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 contrib/completion/git-completion.bash |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)
Show changes to contrib/completion/git-completion.bash +5 −3
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 1fe11f4..c26f96c 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1140,16 +1140,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.3.g5a738d
Felipe Contreras· Apr 8, 2012, 03:07 UTC · re: Felipe Contreras · lore

[PATCH 09/12] completion: calculate argument position properly

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 |   35 +++++++++++++++++++++++---------
 t/t9902-completion.sh                  |   21 +++++++++++++++++++
 2 files changed, 46 insertions(+), 10 deletions(-)
Show changes to 2 files +46 −10

contrib/completion/git-completion.bash, t/t9902-completion.sh

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index c26f96c..60ea224 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -719,10 +719,18 @@ __git_complete_revlist ()
 	__git_complete_revlist_file
 }
 
+__git_get_pos ()
+{
+	echo $(( t = cmd_pos - 2 + $1 ))
+}
+
 __git_complete_remote_or_refspec ()
 {
 	local cur_="$cur"
-	local i c=2 remote="" pfx="" lhs=1 no_complete_refspec=0
+	local i c remote="" pfx="" lhs=1 no_complete_refspec=0
+
+	c=$(__git_get_pos 2)
+
 	if [ "$cmd" = "remote" ]; then
 		((c++))
 	fi
@@ -974,7 +982,8 @@ __git_aliased_command ()
 # __git_find_on_cmdline requires 1 argument
 __git_find_on_cmdline ()
 {
-	local word subcommand c=1
+	local word subcommand c
+	c=$(__git_get_pos 1)
 	while [ $c -lt $cword ]; do
 		word="${words[c]}"
 		for subcommand in $1; do
@@ -989,7 +998,8 @@ __git_find_on_cmdline ()
 
 __git_has_doubledash ()
 {
-	local c=1
+	local c
+	c=$(__git_get_pos 1)
 	while [ $c -lt $cword ]; do
 		if [ "--" = "${words[c]}" ]; then
 			return 0
@@ -1109,8 +1119,8 @@ _git_bisect ()
 
 _git_branch ()
 {
-	local i c=1 only_local_ref="n" has_r="n"
-
+	local i c only_local_ref="n" has_r="n"
+	c=$(__git_get_pos 1)
 	while [ $c -lt $cword ]; do
 		i="${words[c]}"
 		case "$i" in
@@ -1142,8 +1152,9 @@ _git_bundle ()
 {
 	local subcommands='create list-heads verify unbundle'
 	local subcommand="$(__git_find_on_cmdline "$subcommands")"
-
-	case "$cword" in
+	local r
+	(( r = cword - cmd_pos + 2 ))
+	case "$r" in
 	2)
 		__gitcomp "$subcommands"
 		;;
@@ -1427,6 +1438,7 @@ __git_match_ctag() {
 _git_grep ()
 {
 	__git_has_doubledash && return
+	local p=$(__git_get_pos 2)
 
 	case "$cur" in
 	--*)
@@ -1447,7 +1459,7 @@ _git_grep ()
 	esac
 
 	case "$cword,$prev" in
-	2,*|*,-*)
+	$p,*|*,-*)
 		if test -r tags; then
 			__gitcomp_nl "$(__git_match_ctag "$cur" tags)"
 			return
@@ -2562,7 +2574,8 @@ _git_svn ()
 
 _git_tag ()
 {
-	local i c=1 f=0
+	local i c f=0
+	c=$(__git_get_pos 1)
 	while [ $c -lt $cword ]; do
 		i="${words[c]}"
 		case "$i" in
@@ -2601,7 +2614,7 @@ _git_whatchanged ()
 
 _git ()
 {
-	local i c=1 cmd __git_dir
+	local i c=1 cmd cmd_pos __git_dir
 
 	if [[ -n ${ZSH_VERSION-} ]]; then
 		emulate -L bash
@@ -2656,6 +2669,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 f1b660f..b99fb88 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -284,4 +284,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.3.g5a738d
Felipe Contreras· Apr 8, 2012, 03:07 UTC · re: Felipe Contreras · lore

[PATCH 10/12] completion: add new git_complete helper

This simplifies the completions, and makes it easier to define aliases:
 git_complete gf git_fetch
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 contrib/completion/git-completion.bash |   69 +++++++++++++++-----------------
 t/t9902-completion.sh                  |    2 +-
 2 files changed, 33 insertions(+), 38 deletions(-)
Show changes to 2 files +33 −38

contrib/completion/git-completion.bash, t/t9902-completion.sh

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 60ea224..6cf1d98 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2616,21 +2616,6 @@ _git ()
 {
 	local i c=1 cmd cmd_pos __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
@@ -2683,22 +2668,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)"
@@ -2719,16 +2688,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
+foo_wrap ()
+{
+	local cmd=foo_cmd cmd_pos=1
+	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
+	foo "$@"
+}
+
+git_complete ()
+{
+	local name="${2-$1}"
+	local cmd="${name#git_}"
+	eval "$(typeset -f foo_wrap | sed -e "s/foo_cmd/$cmd/" -e "s/foo/_$name/")"
+	complete -o bashdefault -o default -o nospace -F _${name}_wrap $1 2>/dev/null \
+		|| complete -o default -o nospace -F _${name}_wrap $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 b99fb88..7b87f4c 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -58,7 +58,7 @@ test_completion ()
 	_words=( $1 )
 	test $# -gt 1 && echo "$2" > expected
 	(( _cword = ${#_words[@]} - 1 ))
-	_git && print_comp &&
+	_git_wrap && print_comp &&
 	test_cmp expected out
 }
 
-- 
1.7.10.3.g5a738d
SZEDER Gábor· Apr 11, 2012, 22:50 UTC · re: Felipe Contreras · lore

Re: [PATCH 10/12] completion: add new git_complete helper

Hi,
On Sun, Apr 08, 2012 at 06:07:57AM +0300, Felipe Contreras wrote:
Show 56 quoted lines
> This simplifies the completions, and makes it easier to define aliases:
> 
>  git_complete gf git_fetch
> 
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  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 60ea224..6cf1d98 100755
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -2616,21 +2616,6 @@ _git ()
>  {
>  	local i c=1 cmd cmd_pos __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
> @@ -2683,22 +2668,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
> -
Eliminating code duplication, great!
Show 11 quoted lines
>  	__git_has_doubledash && return
>  
>  	local g="$(__gitdir)"
> @@ -2719,16 +2688,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
More greatness.
> +foo_wrap ()

This git-completion-specific internal helper function will pollute the user's environment. It should get a __git_ prefix.

Show 20 quoted lines
> +{
> +	local cmd=foo_cmd cmd_pos=1
> +	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
> +	foo "$@"
> +}
> +
> +git_complete ()

Maybe this should also get a prefix, but I'm not sure, because this is, strictly speaking, not a bash completion function.

Show 10 quoted lines
> +{
> +	local name="${2-$1}"
> +	local cmd="${name#git_}"
> +	eval "$(typeset -f foo_wrap | sed -e "s/foo_cmd/$cmd/" -e "s/foo/_$name/")"
> +	complete -o bashdefault -o default -o nospace -F _${name}_wrap $1 2>/dev/null \
> +		|| complete -o default -o nospace -F _${name}_wrap $1
> +}
> +
> +git_complete git
> +git_complete gitk
Clever ;)

But it needs a subshell and a sed invocation, so Windows people might not be all too happy about this. Perhaps it's not a big deal in the default case, because it will be done only twice at startup time. However, if someone wants several aliases on Windows, it can cause noticeable delay. It's a startup speed vs. convenience trade-off. Anyway, I don't have any ideas how those fork()s and exec() could be avoided.

Best, Gábor

Felipe Contreras· Apr 11, 2012, 23:44 UTC · re: SZEDER Gábor · lore

Re: [PATCH 10/12] completion: add new git_complete helper

2012/4/12 SZEDER Gábor <szeder@ira.uka.de>:
Show 20 quoted lines
>> +{
>> +     local name="${2-$1}"
>> +     local cmd="${name#git_}"
>> +     eval "$(typeset -f foo_wrap | sed -e "s/foo_cmd/$cmd/" -e "s/foo/_$name/")"
>> +     complete -o bashdefault -o default -o nospace -F _${name}_wrap $1 2>/dev/null \
>> +             || complete -o default -o nospace -F _${name}_wrap $1
>> +}
>> +
>> +git_complete git
>> +git_complete gitk
>
> Clever ;)
>
> But it needs a subshell and a sed invocation, so Windows people might
> not be all too happy about this.  Perhaps it's not a big deal in the
> default case, because it will be done only twice at startup time.
> However, if someone wants several aliases on Windows, it can cause
> noticeable delay.  It's a startup speed vs. convenience trade-off.
> Anyway, I don't have any ideas how those fork()s and exec() could be
> avoided.

Well, if you look at the foo_wrap function you would notice there is not much 'foo' in it. We could have a __git_wrap () function instead, but we would need to find an alternative way to set the 'cmd' variable. The easiest would be to just don't set it at all. As I suggested, only __git_complete_remote_or_refspec needs it, and can be easily worked around.

-- 
Felipe Contreras
Felipe Contreras· Apr 8, 2012, 03:07 UTC · re: Felipe Contreras · lore

[PATCH 11/12] test: add tests for aliases in bash completion

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 t/t9902-completion.sh |   10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)
Show changes to t/t9902-completion.sh +8 −2
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 7b87f4c..6c61e7a 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -54,11 +54,12 @@ _get_comp_words_by_ref ()
 test_completion ()
 {
 	local -a COMPREPLY _words
-	local _cword
+	local _cword wrap
 	_words=( $1 )
 	test $# -gt 1 && echo "$2" > expected
+	wrap="${3-_git_wrap}"
 	(( _cword = ${#_words[@]} - 1 ))
-	_git_wrap && print_comp &&
+	$wrap && print_comp &&
 	test_cmp expected out
 }
 
@@ -305,4 +306,9 @@ test_expect_success 'global options extra checks' '
 	test_completion "git --no-pager tag -d v" "v0.1 "
 '
 
+test_expect_success 'aliases' '
+	git_complete gf git_fetch &&
+	test_completion "gf o" "origin " "_git_fetch_wrap"
+'
+
 test_done
-- 
1.7.10.3.g5a738d
Felipe Contreras· Apr 8, 2012, 03:20 UTC · re: Felipe Contreras · lore

Re: [PATCH 11/12] test: add tests for aliases in bash completion

On Sun, Apr 8, 2012 at 6:07 AM, Felipe Contreras <felipe.contreras@gmail.com> wrote:

> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  t/t9902-completion.sh |   10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
Maybe something like this would be better:
Show changes to t/t9902-completion.sh +8 −1
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -58,7 +58,7 @@ test_completion ()
        _words=( $1 )
        test $# -gt 1 && echo "$2" > expected
        (( _cword = ${#_words[@]} - 1 ))
-       _git_wrap && print_comp &&
+       ${comp_wrap-_git_wrap} && print_comp &&
        test_cmp expected out
 }

@@ -305,4 +305,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
-- 
Felipe Contreras
Felipe Contreras· Apr 8, 2012, 03:07 UTC · re: Felipe Contreras · lore

[PATCH 12/12] completion: rename _git and _gitk

Would be helpful to reuse these for zsh completion; it uses _git, and it cannot be changed.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 contrib/completion/git-completion.bash |    8 ++++----
 t/t9902-completion.sh                  |    2 +-
 2 files changed, 5 insertions(+), 5 deletions(-)
Show changes to 2 files +5 −5

contrib/completion/git-completion.bash, t/t9902-completion.sh

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 6cf1d98..7d663f7 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2612,7 +2612,7 @@ _git_whatchanged ()
 	_git_log
 }
 
-_git ()
+_main_git ()
 {
 	local i c=1 cmd cmd_pos __git_dir
 
@@ -2666,7 +2666,7 @@ _git ()
 	fi
 }
 
-_gitk ()
+_main_gitk ()
 {
 	__git_has_doubledash && return
 
@@ -2710,7 +2710,7 @@ foo_wrap ()
 
 git_complete ()
 {
-	local name="${2-$1}"
+	local name="${2-main_$1}"
 	local cmd="${name#git_}"
 	eval "$(typeset -f foo_wrap | sed -e "s/foo_cmd/$cmd/" -e "s/foo/_$name/")"
 	complete -o bashdefault -o default -o nospace -F _${name}_wrap $1 2>/dev/null \
@@ -2725,5 +2725,5 @@ git_complete gitk
 # included the '.exe' suffix.
 #
 if [ Cygwin = "$(uname -o 2>/dev/null)" ]; then
-git_complete git.exe git
+git_complete git.exe main_git
 fi
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 6c61e7a..ef1323f 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -57,7 +57,7 @@ test_completion ()
 	local _cword wrap
 	_words=( $1 )
 	test $# -gt 1 && echo "$2" > expected
-	wrap="${3-_git_wrap}"
+	wrap="${3-_main_git_wrap}"
 	(( _cword = ${#_words[@]} - 1 ))
 	$wrap && print_comp &&
 	test_cmp expected out
-- 
1.7.10.3.g5a738d

← back to recent threads