v3, 5 partscompletion: trivial cleanups and fixes

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

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

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

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

On Sun, Apr 15, 2012 at 1:36 AM, Thomas Rast <trast@inf.ethz.ch> wrote:

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.

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.

Or

1. simplifying __gitcomp_1

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.

On Sun, Apr 15, 2012 at 1:54 AM, Felipe Contreras <felipe.contreras@gmail.com> wrote:

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 <felipe.contreras@gmail.com> writes:

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.

On Sun, Apr 15, 2012 at 10:29:17AM +0200, Andreas Schwab wrote:

We can even get rid of the %s with

  printf -- "$c\n"

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

Only if $c can never contain special characters.

Andreas.

On Sun, Apr 15, 2012 at 11:49:22AM +0200, Andreas Schwab wrote:

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

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

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

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

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

Felipe Contreras wrote:

I generally like this.  Quick comments:

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

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

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

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

OK.

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.