[PATCH v3 0/5] completion: trivial cleanups and fixes

[PATCH v3 0/5] completion: trivial cleanups and fixes

[Felipe Contreras]

Hi,

Just a few simpliciations, improvements, and add some missing options.

This series depends on the bash completion tests patch.

Since v2:

 * Add 'git --option <TAB>' fix
 * Improve --exec-path patch

Felipe Contreras (3):
  completion: simplify __gitcomp_1
  completion: trivial simplification
  completion: add missing general options

Jonathan Nieder (1):
  completion: avoid trailing space for --exec-path

SZEDER Gábor (1):
  completion: fix completion after 'git --option <TAB>'

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

-- 
1.7.10.1.g1f19b8.dirty

[PATCH v3 1/5] completion: simplify __gitcomp_1

[Felipe Contreras]

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

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 31f714d..13be9a7 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -307,13 +307,13 @@ __git_ps1 ()
 # __gitcomp_1 requires 2 arguments
 __gitcomp_1 ()
 {
-	local c IFS=' '$'\t'$'\n'
+	local c s 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 " ;;
+		--*=* | *.) s="" ;;
+		*)          s=" " ;;
 		esac
+		echo "$c$2$s"
 	done
 }
 
-- 
1.7.10.1.g1f19b8.dirty

Re: [PATCH v3 1/5] completion: simplify __gitcomp_1

[Thomas Rast]

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

>  # __gitcomp_1 requires 2 arguments
>  __gitcomp_1 ()
>  {
> -	local c IFS=' '$'\t'$'\n'
> +	local c s 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 " ;;
> +		--*=* | *.) s="" ;;
> +		*)          s=" " ;;
>  		esac
> +		echo "$c$2$s"
>  	done
>  }

Sorry for not noticing earlier, but...

I did a double take at the change to 'echo'.  I'm guessing from the
patch that $c$2$s is never just '-e' or some other option taken by the
bash version[1] of echo.  But can you be sure?  Do you know off hand
whether '-nbogus' complains, treats the -n as usual and prints 'bogus',
or echoes '-nbogus'[2]?  Are you sure future bash versions won't break
this?


Also, I can't help but complain about your commit messages (again).
Compare with Jonathan's in 4/5.  His patch is all of a one-line(!)
change

 			--exec-path
+			--exec-path=

yet his commit message is two paragraphs worth of explanations why the
changed behavior is more helpful than what we had before.

On the other hand, your commit message for the above says only

  completion: simplify __gitcomp_1

However, your patch is actually two different changes:

1. the refactoring of the partial command: printf %s$'\n' "$c$2
2. the change to echo

The latter is not in any way explained or justified by your (total
absence of a) commit message.


Footnotes: 
[1]  POSIX states that echo "shall not support any options" and "shall
not recognize the -- argument", but we have printfs all over the code
base because option support is extremely inconsistent

[2]  Spoiler: it prints -nbogus literally

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Re: [PATCH v3 1/5] completion: simplify __gitcomp_1

[Felipe Contreras]

On Sun, Apr 15, 2012 at 1:36 AM, Thomas Rast <trast@inf.ethz.ch> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>>  # __gitcomp_1 requires 2 arguments
>>  __gitcomp_1 ()
>>  {
>> -     local c IFS=' '$'\t'$'\n'
>> +     local c s 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 " ;;
>> +             --*=* | *.) s="" ;;
>> +             *)          s=" " ;;
>>               esac
>> +             echo "$c$2$s"
>>       done
>>  }
>
> Sorry for not noticing earlier, but...
>
> I did a double take at the change to 'echo'.  I'm guessing from the
> patch that $c$2$s is never just '-e' or some other option taken by the
> bash version[1] of echo.  But can you be sure?  Do you know off hand
> whether '-nbogus' complains, treats the -n as usual and prints 'bogus',
> or echoes '-nbogus'[2]?  Are you sure future bash versions won't break
> this?

That doesn't make any sense to me. If you want that, you should do
'eval "echo $foo"', and even if you do 'eval "echo \"$foo\""', that
would be avoided.

> Also, I can't help but complain about your commit messages (again).

Well, we can start a discussion about how "simplify __gitcomp_1" does
not explain sufficiently that this is indeed a simplification of
__gitcomp_1, but from previous experiences you don't really want to
discuss, you just want to be right, and me to follow orders.

Personally, I don't see why every modified line needs an ode.

> 1. the refactoring of the partial command: printf %s$'\n' "$c$2
> 2. the change to echo

Or

1. simplifying __gitcomp_1

> Footnotes:
> [1]  POSIX states that echo "shall not support any options" and "shall
> not recognize the -- argument", but we have printfs all over the code
> base because option support is extremely inconsistent

This is a *bash* completion script.

If you *need* an essay for a commit message for a patch that
essentially does nothing but simplify some code, I'll just drop it;
it's not worth my effort.

Cheers.

-- 
Felipe Contreras

Re: [PATCH v3 1/5] completion: simplify __gitcomp_1

[Felipe Contreras]

On Sun, Apr 15, 2012 at 1:54 AM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> On Sun, Apr 15, 2012 at 1:36 AM, Thomas Rast <trast@inf.ethz.ch> wrote:
>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>>
>>>  # __gitcomp_1 requires 2 arguments
>>>  __gitcomp_1 ()
>>>  {
>>> -     local c IFS=' '$'\t'$'\n'
>>> +     local c s 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 " ;;
>>> +             --*=* | *.) s="" ;;
>>> +             *)          s=" " ;;
>>>               esac
>>> +             echo "$c$2$s"
>>>       done
>>>  }
>>
>> Sorry for not noticing earlier, but...
>>
>> I did a double take at the change to 'echo'.  I'm guessing from the
>> patch that $c$2$s is never just '-e' or some other option taken by the
>> bash version[1] of echo.  But can you be sure?  Do you know off hand
>> whether '-nbogus' complains, treats the -n as usual and prints 'bogus',
>> or echoes '-nbogus'[2]?  Are you sure future bash versions won't break
>> this?
>
> That doesn't make any sense to me. If you want that, you should do
> 'eval "echo $foo"', and even if you do 'eval "echo \"$foo\""', that
> would be avoided.

Actually, after a private discussion with Thomas, I realized that's on
zsh, on bash it doesn't work as I expected, plus, in both 'echo "-n"'
does the unexpected thing (to me).

So, it should be, maybe 'print "%s\n" "$c$2$s"', or 'print "%s$s\n" "$c$2"'.

However, I would like to simplify it even more:

__gitcomp_1 ()
{
	local c w s IFS=' '$'\t'$'\n'
	for c in $1; do
		w="$c$2"
		case "$w" in
		--*=*|*.) s="" ;;
		*)        s=" " ;;
		esac
		printf "%s$s\n" "$w"
	done
}

-- 
Felipe Contreras

Re: [PATCH v3 1/5] completion: simplify __gitcomp_1

[Andreas Schwab]

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

> However, I would like to simplify it even more:
>
> __gitcomp_1 ()
> {
> 	local c w s IFS=' '$'\t'$'\n'
> 	for c in $1; do
> 		w="$c$2"
> 		case "$w" in
> 		--*=*|*.) s="" ;;
> 		*)        s=" " ;;
> 		esac
> 		printf "%s$s\n" "$w"
> 	done
> }

Or even:

__gitcomp_1 ()
{
	local c IFS=$' \t\n'
	for c in $1; do
		c=$c$2
		case $c in
		--*=*|*.) ;;
		*)        c="$c " ;;
		esac
		printf '%s\n' "$c"
	done
}

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

Re: [PATCH v3 1/5] completion: simplify __gitcomp_1

[SZEDER Gábor]

On Sun, Apr 15, 2012 at 10:29:17AM +0200, Andreas Schwab wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
> 
> > However, I would like to simplify it even more:
> >
> > __gitcomp_1 ()
> > {
> > 	local c w s IFS=' '$'\t'$'\n'
> > 	for c in $1; do
> > 		w="$c$2"
> > 		case "$w" in
> > 		--*=*|*.) s="" ;;
> > 		*)        s=" " ;;
> > 		esac
> > 		printf "%s$s\n" "$w"
> > 	done
> > }
> 
> Or even:
> 
> __gitcomp_1 ()
> {
> 	local c IFS=$' \t\n'
> 	for c in $1; do
> 		c=$c$2
> 		case $c in
> 		--*=*|*.) ;;
> 		*)        c="$c " ;;
> 		esac
> 		printf '%s\n' "$c"
> 	done
> }

We can even get rid of the %s with

  printf -- "$c\n"

Re: [PATCH v3 1/5] completion: simplify __gitcomp_1

[Andreas Schwab]

SZEDER Gábor <szeder@ira.uka.de> writes:

> We can even get rid of the %s with
>
>   printf -- "$c\n"

Only if $c can never contain special characters.

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

Re: [PATCH v3 1/5] completion: simplify __gitcomp_1

[SZEDER Gábor]

On Sun, Apr 15, 2012 at 11:49:22AM +0200, Andreas Schwab wrote:
> SZEDER Gábor <szeder@ira.uka.de> writes:
> 
> > We can even get rid of the %s with
> >
> >   printf -- "$c\n"
> 
> Only if $c can never contain special characters.

That's a good point, branch and tag names can contain '%', so the %s
shall stay.

[PATCH v3 2/5] completion: trivial simplification

[Felipe Contreras]

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

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 13be9a7..9d36bb7 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1658,7 +1658,7 @@ _git_notes ()
 		__gitcomp '--ref'
 		;;
 	,*)
-		case "${words[cword-1]}" in
+		case "$prev" in
 		--ref)
 			__gitcomp_nl "$(__git_refs)"
 			;;
@@ -1684,7 +1684,7 @@ _git_notes ()
 	prune,*)
 		;;
 	*)
-		case "${words[cword-1]}" in
+		case "$prev" in
 		-m|-F)
 			;;
 		*)
-- 
1.7.10.1.g1f19b8.dirty

[PATCH v3 3/5] completion: add missing general options

[Felipe Contreras]

And add relevant tests.

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

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 9d36bb7..6a8cf9f 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2640,8 +2640,10 @@ _git ()
 			--version
 			--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 7bd37f5..f24c968 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -95,8 +95,10 @@ test_expect_success 'double dash "git" itself' '
 	--version Z
 	--exec-path Z
 	--html-path Z
+	--info-path Z
 	--work-tree=
 	--namespace=
+	--no-replace-objects Z
 	--help Z
 	EOF
 	test_completion "git --"
@@ -117,4 +119,18 @@ test_expect_success 'double dash "git checkout"' '
 	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_done
-- 
1.7.10.1.g1f19b8.dirty

[PATCH v3 4/5] completion: avoid trailing space for --exec-path

[Felipe Contreras]

From: Jonathan Nieder <jrnieder@gmail.com>

--exec-path looks to the completion script like an unambiguous
successful completion, but it is wrong; the user could be trying to
do

	git --exec-path; # print name of helper directory

or

	git --exec-path=/path/to/alternative/helper/dir <subcommand>

so the most helpful thing to do is to leave out the trailing space and
leave it to the operator to type an equal sign or carriage return
according to the situation.

Cc: Andreas Schwab <schwab@linux-m68k.org>
Reported-by: Felipe Contreras <felipe.contreras@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
[added tests]
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 contrib/completion/git-completion.bash |    1 +
 t/t9902-completion.sh                  |    7 ++++++-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 6a8cf9f..647ee77 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2639,6 +2639,7 @@ _git ()
 			--bare
 			--version
 			--exec-path
+			--exec-path=
 			--html-path
 			--info-path
 			--work-tree=
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index f24c968..dfef809 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -94,6 +94,7 @@ test_expect_success 'double dash "git" itself' '
 	--bare Z
 	--version Z
 	--exec-path Z
+	--exec-path=
 	--html-path Z
 	--info-path Z
 	--work-tree=
@@ -122,7 +123,11 @@ test_expect_success 'double dash "git checkout"' '
 test_expect_success 'general options' '
 	test_completion "git --ver" "--version " &&
 	test_completion "git --hel" "--help " &&
-	test_completion "git --exe" "--exec-path " &&
+	sed -e "s/Z$//" >expected <<-\EOF &&
+	--exec-path Z
+	--exec-path=
+	EOF
+	test_completion "git --exe" &&
 	test_completion "git --htm" "--html-path " &&
 	test_completion "git --pag" "--paginate " &&
 	test_completion "git --no-p" "--no-pager " &&
-- 
1.7.10.1.g1f19b8.dirty

[PATCH v3 5/5] completion: fix completion after 'git --option <TAB>'

[Felipe Contreras]

From: SZEDER Gábor <szeder@ira.uka.de>

Git's bash completion currently doesn't work when certain git options
are specified, e.g. 'git --no-pager <TAB>' errors out with "error:
invalid key: alias.--no-pager".

The main _git() completion function finds out the git command name by
looping through all the words on the command line and searching for
the first word that is not a known option for the git command.
Unfortunately the list of known git options was not updated in a long
time, and newer options are not skipped but mistaken for a git
command.  Such a misrecognized "command" is then passed to
__git_aliased_command(), which in turn passes it to a 'git config'
query, hence the error.

Currently the following options are misrecognized for a git command:

  -c --no-pager --exec-path --html-path --man-path --info-path
  --no-replace-objects --work-tree= --namespace=

To fix this we could just update the list of options to be skipped,
but the same issue will likely arise, if the git command learns a new
option in the future.  Therefore, to make it more future proof against
new options, this patch changes that loop to skip all option-looking
words, i.e. words starting with a dash.

We also have to handle the '-c' option specially, because it takes a
configutation parameter in a separate word, which must be skipped,
too.

Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
[added tests]
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 contrib/completion/git-completion.bash |    3 ++-
 t/t9902-completion.sh                  |   17 +++++++++++++++++
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 647ee77..192e444 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2623,8 +2623,9 @@ _git ()
 		case "$i" in
 		--git-dir=*) __git_dir="${i#--git-dir=}" ;;
 		--bare)      __git_dir="." ;;
-		--version|-p|--paginate) ;;
 		--help) command="help"; break ;;
+		-c) c=$((++c)) ;;
+		-*) ;;
 		*) command="$i"; break ;;
 		esac
 		((c++))
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index dfef809..ab72378 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -138,4 +138,21 @@ test_expect_success 'general options' '
 	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.1.g1f19b8.dirty

Re: [PATCH v3 0/5] completion: trivial cleanups and fixes

[Jonathan Nieder]

Felipe Contreras wrote:

> Just a few simpliciations, improvements, and add some missing options.

I generally like this.  Quick comments:

> Felipe Contreras (3):
>   completion: simplify __gitcomp_1

Thomas and Gábor seem to have this one covered, with some improvements
to it under discussion.

>   completion: trivial simplification

Improves consistency.  The changelog line isn't precise enough to
remind me which change is being mentioned when reading it in the
shortlog, so I guess I'd prefer something like

	completion: simplify by using $prev variable

>   completion: add missing general options

Likewise, a subject line like the following would work better for
me.

	completion: --info-path and --no-replace-objects options for git

Hope that helps,
Jonathan

Re: [PATCH v3 0/5] completion: trivial cleanups and fixes

[Felipe Contreras]

On Sun, Apr 15, 2012 at 4:20 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Felipe Contreras wrote:

>>   completion: trivial simplification
>
> Improves consistency.  The changelog line isn't precise enough to
> remind me which change is being mentioned when reading it in the
> shortlog, so I guess I'd prefer something like
>
>        completion: simplify by using $prev variable

OK.

>>   completion: add missing general options
>
> Likewise, a subject line like the following would work better for
> me.
>
>        completion: --info-path and --no-replace-objects options for git

That's too long, and I think it's less understandable: for git? Of
course it's for git, for what else could it be? :) maybe s/options for
git/general options/, but it's still too long.

I'll leave it like that, anybody else can change it if they wish.

Cheers.

-- 
Felipe Contreras