Re: [PATCH] bash completion: Fix the . -> .. revision range completion

Previous thread: Re: [PATCH] bash completion: Improve responsiveness of git-log completion by Junio C Hamano on Sunday, July 13, 2008 - 5:38 pm. (2 messages)

Next thread: Re: [PATCH 2/3] add new Git::Repo API by Junio C Hamano on Sunday, July 13, 2008 - 5:38 pm. (2 messages)
To: Petr Baudis <pasky@...>
Cc: Shawn O. Pearce <spearce@...>, <git@...>
Date: Sunday, July 13, 2008 - 5:38 pm

Theoretically we could take a hint from presense of '--' like d773c63
(bash: offer only paths after '--', 2008-07-08) did. If the command line
has double-dash and the token we are looking at is before it, it cannot be
pathname and this check does not have to trigger. But I agree that is not
worth it, because this "theoretical" solution would mean that the user
needs to something awkward like:

git log v1.5.6. --<C-b><C-b><C-b><TAB>

to take advantage of it.

By the way, the above command line is another "dot" related frustration I
always have. If you try:

git log v1.5.6.<TAB>

the completion code adds a dot unconditionally when I want to choose from
the list of v1.5.6.X tags. Of course, I can work this around by dropping
the last dot before asking for completion, so it is not really a very big
deal, but I mention it here because this annoyance is exactly in the same
league as your "git-submodule.<TAB>" example.

"git show v1.5.6.<TAB>" does complete as expected, which is understandable
(the command does not take range, and completion knows about it -- which

There is a slight Yuck factor for using "ls" here but I do not think of a
better alternative offhand.

Will queue on top of Shawn's previous one. Thanks.

--

To: Junio C Hamano <gitster@...>
Cc: Shawn O. Pearce <spearce@...>, <git@...>
Date: Sunday, July 13, 2008 - 7:07 pm

Actually, my original solution to this problem was simply to remove the
. -> .. completion altogether. Maybe this would still be the best course
of action? I don't think the . -> .. is actually very useful for anyone,

I have thought about this hard for some time, but couldn't come up with
anything better than this or

(shopt -s nullglob; completion=("$cur"*); [ -n "$completion" ])

which looks quite awful (and can waste a lot of memory in case of some
really insane completion).

--
Petr "Pasky" Baudis
GNU, n. An animal of South Africa, which in its domesticated state
resembles a horse, a buffalo and a stag. In its wild condition it is
something like a thunderbolt, an earthquake and a cyclone. -- A. Pierce
--

To: Petr Baudis <pasky@...>
Cc: Shawn O. Pearce <spearce@...>, <git@...>
Date: Sunday, July 13, 2008 - 7:25 pm

I think that is what Shawn sent a few minutes ago, so you two are in
agreement, and I will be happy with it, too.

--

To: Junio C Hamano <gitster@...>
Cc: Shawn O. Pearce <spearce@...>, Petr Baudis <pasky@...>, <git@...>
Date: Sunday, July 13, 2008 - 7:52 pm

Does it fix this one too:

git show origin/pu:Makef<tab>

which totally screws up and becomes

git show Makefile

dropping the version specifier?

I don't speak completion-script, so somebody else would have to fix this
really annoying bug (*)

Linus

(*) Ok, so it's rare. I have been bitten by it something like twice in the
last months. But when it happens it's really annoying.
--

To: Linus Torvalds <torvalds@...>
Cc: Junio C Hamano <gitster@...>, Petr Baudis <pasky@...>, <git@...>
Date: Sunday, July 13, 2008 - 8:00 pm

Its unrelated to the issue you described above. But I just tested
your Makefile completion case and it worked as expected, though
I just found out it doesn't add a trailing space once there is a
unique completion made. I'll look at another patch to fix that.

--
Shawn.
--

To: Shawn O. Pearce <spearce@...>
Cc: Junio C Hamano <gitster@...>, Petr Baudis <pasky@...>, <git@...>
Date: Monday, July 14, 2008 - 1:38 am

What version of bash do you have?

It definitely doesn't work for me with

GNU bash, version 3.2.33(1)-release (x86_64-redhat-linux-gnu)

and quite frankly, I don't see how it _can_ work.

Something like this seems to be totally missing, and definitely required.
How can you say that it works for you? I don't see how that is possible
even in theory? Did you actually test it?

Linus

---
contrib/completion/git-completion.bash | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 27332ed..0a87337 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -290,7 +290,7 @@ __git_complete_file ()
ls="$ref"
;;
esac
- COMPREPLY=($(compgen -P "$pfx" \
+ COMPREPLY=($(compgen -P "$ref:$pfx" \
-W "$(git --git-dir="$(__gitdir)" ls-tree "$ls" \
| sed '/^100... blob /s,^.* ,,
/^040000 tree /{
--

To: Linus Torvalds <torvalds@...>
Cc: Junio C Hamano <gitster@...>, Petr Baudis <pasky@...>, <git@...>
Date: Monday, July 14, 2008 - 2:27 am

What is $COMP_WORDBREAKS set to in your shell? In mine it
appears to be:

" \"'@><=;|&(:"

which is the I believe the shell default.

Björn Steinbrink (doener on #git) is running bash 3.2.39 from
Debian and has the same setting, and the completion works correctly
there too. He reports that removing : from COMP_WORDBREAKS will
get the behavior you are reporting as broken.

I have to say, this sounds to me like you (or some package on your
system) modified COMP_WORDBREAKS away from the default that other
distributions use and that is what is breaking us here. Since we
can have only one setting for this variable in the shell I do not
thing it would be a good idea for our completion package to force
a specific setting upon the user.

Though we could try to detect : in there and if it is not present
use the workaround you posted. But I wonder if just asking the
user to include : is easier.

--
Shawn.
--

To: Shawn O. Pearce <spearce@...>
Cc: Junio C Hamano <gitster@...>, Petr Baudis <pasky@...>, <git@...>
Date: Monday, July 14, 2008 - 10:50 am

Umm, if so, git should just set it in the completion script, no?

And doing a google for 'COMP_WORDBREAKS' and 'colon', I don't think I'm
the only one with it unset. In fact, I'm pretty sure I didn't unset it
myself, because I've never heard of that thing. So I think it's a generic
Fedora 9 thing.

(I just checked - that seems to pan out. All the machines with F9 on it I
have access to are missing the ':' - including machines I have not set up
myself like master.kernel.org)

Linus
--

To: Linus Torvalds <torvalds@...>
Cc: Junio C Hamano <gitster@...>, Petr Baudis <pasky@...>, <git@...>
Date: Tuesday, July 15, 2008 - 12:25 am

OK, so it turns out not having : in COMP_WORDBREAKS is a very common
case that we should somehow deal with, to aid our users.

I'm concerned about just setting COMP_WORDBREAKS back to the default
in the git completion script because then we get into an ordering
game with the profile scripts, don't we? If git completion sources
before the gvfs script we don't get our COMP_WORDBREAKS setting.

I think we may need to do two things.

If COMP_WORDBREAKS doesn't contain a :, try to reset it to include
one when the script is sourced. This may "fix" git completion but
make gvfs completion act differently, resulting in a thread on the
gvfs lists. ;-)

If COMP_WORDBREAKS doesn't contain : during a completion event than
we need to do what your original patch asked, which is to include
"$ref:" in the prefix, so the ref isn't lost.

At least we understand the problem now, finally. I'll try to write
up a patch for it tomorrow. Unfortunately packing to move has been
really sucking up my time lately.

--
Shawn.
--

To: Shawn O. Pearce <spearce@...>
Cc: Linus Torvalds <torvalds@...>, Junio C Hamano <gitster@...>, Petr Baudis <pasky@...>, <git@...>
Date: Tuesday, July 15, 2008 - 4:05 am

I beat you to it ;-) This works just fine for me regardless of whether
or not I have a colon in COMP_WORDBREAKS.

--%<--%<--%<--
From: Andreas Ericsson <ae@op5.se>
Subject: git-completion.bash: Handle "rev:path" completion properly

The gvfs package on at least Fedora9 installs its own bash
completion script which removes the colon from COMP_WORDBREAKS,
which acts as a list of characters where bash should consider
as word boundaries. Doing so breaks the git bash completion
script when handling any rev:path style argument.

This patch fixes it by prepending the "rev" part and the colon
(which otherwise gets lost) before adding the "path" part if
COMP_WORDBREAKS doesn't contain the colon we would otherwise
need.

Also fixes a nearby indented-with-spaces issue.

Spotted-by: Linus Torvalds <torvalds@linux-foundation.org>
Investigated-by: Björn Steinbrink <b.steinbrink@gmx.de>
Signed-off-by: Andreas Ericsson <ae@op5.se>
---
contrib/completion/git-completion.bash | 6 +++++-
1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index d268e6f..e138022 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -293,7 +293,11 @@ __git_complete_file ()
*)
ls="$ref"
;;
- esac
+ esac
+ # When completing something like 'rev:path', bash behaves
+ # differently whether or not COMP_WORDBREAKS contains a
+ # colon or not. This lets it handle both cases
+ test "${COMP_WORDBREAKS//:}" = "$COMP_WORDBREAKS" && pfx="$ref:$pfx"
COMPREPLY=($(compgen -P "$pfx" \
-W "$(git --git-dir="$(__gitdir)" ls-tree "$ls" \
| sed '/^100... blob /s,^.* ,,
--
1.5.6.3.315.g10ce0

--
Andreas Ericsson andreas.ericsson@op5.se
OP5 AB www.op5.se
Tel: +46 8-230225 Fax: +46 8-230231

--

To: Andreas Ericsson <ae@...>
Cc: Linus Torvalds <torvalds@...>, Junio C Hamano <gitster@...>, Petr Baudis <pasky@...>, <git@...>
Date: Tuesday, July 15, 2008 - 7:38 pm

Yea, I did more or less the same thing in my patch, but I also
handled this fix in git-fetch and git-push. The : is also used
network IO, possibly over SSH).

So I'm in favor of my patch over yours, but only because of
the fetch and push fixes as well.

--
Shawn.
--

To: Shawn O. Pearce <spearce@...>
Cc: Linus Torvalds <torvalds@...>, Junio C Hamano <gitster@...>, Petr Baudis <pasky@...>, <git@...>
Date: Wednesday, July 16, 2008 - 3:20 am

I agree, although I'd rather not have seen the case statement in
yours. It's bash completion after all, so no need to be "portably
fast" ;-)

I don't care that much though so long as it gets fixed.

--
Andreas Ericsson andreas.ericsson@op5.se
OP5 AB www.op5.se
Tel: +46 8-230225 Fax: +46 8-230231
--

To: Shawn O. Pearce <spearce@...>
Cc: Linus Torvalds <torvalds@...>, Junio C Hamano <gitster@...>, Petr Baudis <pasky@...>, <git@...>
Date: Tuesday, July 15, 2008 - 4:10 am

Andreas Ericsson wrote:

[ a whitespace damaged patch ]

Sorry about that. I'll try again in a short while. It seems sending
patches with thunderbird no longer works like it used to.

--
Andreas Ericsson andreas.ericsson@op5.se
OP5 AB www.op5.se
Tel: +46 8-230225 Fax: +46 8-230231
--

To: Shawn O. Pearce <spearce@...>
Cc: Linus Torvalds <torvalds@...>, Junio C Hamano <gitster@...>, Petr Baudis <pasky@...>, <git@...>
Date: Tuesday, July 15, 2008 - 4:17 am

Apparently it wasn't ws-damaged after all. Just my font-settings
acting up on me.

--
Andreas Ericsson andreas.ericsson@op5.se
OP5 AB www.op5.se
Tel: +46 8-230225 Fax: +46 8-230231
--

To: Shawn O. Pearce <spearce@...>
Cc: Linus Torvalds <torvalds@...>, Junio C Hamano <gitster@...>, Petr Baudis <pasky@...>, <git@...>
Date: Monday, July 14, 2008 - 2:47 am

Seems that gvfs comes with a completion script that deliberately drops
the : from COMP_WORDBREAKS. Do you have that installed Linus?

Björn
--

To: Shawn O. Pearce <spearce@...>
Cc: Linus Torvalds <torvalds@...>, Junio C Hamano <gitster@...>, Petr Baudis <pasky@...>, <git@...>
Date: Monday, July 14, 2008 - 2:50 am

Ah crap, I should have mentioned which file I'm talking about... It's
/etc/profile.d/gvfs-bash-completion.sh

Björn, getting some coffee now
--

To: Björn Steinbrink <B.Steinbrink@...>
Cc: Junio C Hamano <gitster@...>, Shawn O. Pearce <spearce@...>, Petr Baudis <pasky@...>, <git@...>
Date: Monday, July 14, 2008 - 10:51 am

Yup. Part of gvfs-0.2.5-1.fc9.x86_64 for me.

Linus
--

To: Björn Steinbrink <B.Steinbrink@...>
Cc: Linus Torvalds <torvalds@...>, Junio C Hamano <gitster@...>, Shawn O. Pearce <spearce@...>, Petr Baudis <pasky@...>, <git@...>
Date: Monday, July 14, 2008 - 8:39 am

I have that file, and I'm seeing the same issue as Linus.

E13 at http://tiswww.case.edu/php/chet/bash/FAQ mentions it, but
doesn't (imo) give a strong reason why it drops colon from
COMP_WORDBREAKS, as filenames with colons in them aren't exactly
common.

--
Andreas Ericsson andreas.ericsson@op5.se
OP5 AB www.op5.se
Tel: +46 8-230225 Fax: +46 8-230231
--

To: Linus Torvalds <torvalds@...>
Cc: Junio C Hamano <gitster@...>, Petr Baudis <pasky@...>, <git@...>
Date: Monday, July 14, 2008 - 1:57 am

Yes, of course dammit, I did test it before writing an email that

In bash 3.2.17 your change above causes completion to include the
ref name _twice_:

git show jc/html:in<tab>
git show jc/html:jc/html:index.html

This is not good news because it means we need to perform a version
test to identify the correct behavior we need for this shell, and
we also have to figure out what version of 3.2.X this changed in.
*sigh*

--
Shawn.
--

To: Junio C Hamano <gitster@...>
Cc: Petr Baudis <pasky@...>, <git@...>
Date: Sunday, July 13, 2008 - 6:06 pm

This annoys me too Junio. Its a bug I just never got around to fixing.
Based on Petr's comments and yours I'm wondering if this is just not the
better patch to apply here:

--8<--
bash completion: Don't offer "a.." as a completion for "a."

If the user is trying to complete "v1.5.3.<tab>" to see all of
the available maintenance releases for 1.5.3 we should not give
them an extra dot as the completion. Instead if the user wants
a ".." or a "..." operator they should key the two dots out on
their own. Its the same number of keystrokes either way.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
contrib/completion/git-completion.bash | 3 ---
1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 61581fe..9a1c66a 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -324,9 +324,6 @@ __git_complete_revlist ()
cur="${cur#*..}"
__gitcomp "$(__git_refs)" "$pfx" "$cur"
;;
- *.)
- __gitcomp "$cur."
- ;;
*)
__gitcomp "$(__git_refs)"
;;
--
1.5.6.2.393.g45096

--
Shawn.
--

Previous thread: Re: [PATCH] bash completion: Improve responsiveness of git-log completion by Junio C Hamano on Sunday, July 13, 2008 - 5:38 pm. (2 messages)

Next thread: Re: [PATCH 2/3] add new Git::Repo API by Junio C Hamano on Sunday, July 13, 2008 - 5:38 pm. (2 messages)