threads / patch / 14330

patchcompletion: add branch options --contains --merged --no-merged

Subject: [PATCH] completion: add branch options --contains --merged --no-merged

## tl;dr

12 messages between Jul 7, 2008 and Jul 9, 2008. Diffs are folded; open one to read it.

replies: 11people: 5as markdown or json

Eric Raible· Jul 7, 2008, 20:41 UTC · lore
Signed-off-by: Eric Raible <raible@gmail.com>
---
 contrib/completion/git-completion.bash |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Show changes to contrib/completion/git-completion.bash +1 −1
diff --git a/contrib/completion/git-completion.bash
b/contrib/completion/git-completion.bash
index 0eb8df0..22e109d 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -546,7 +546,7 @@ _git_branch ()
 	--*)
 		__gitcomp "
 			--color --no-color --verbose --abbrev= --no-abbrev
-			--track --no-track
+			--track --no-track --contains --merged --no-merged
 			"
 		;;
 	*)
-- 
1.5.6.1.1071.g76fb.dirty
Shawn O. Pearce· Jul 8, 2008, 04:49 UTC · re: Eric Raible · lore

Re: [PATCH] completion: add branch options --contains --merged --no-merged

Eric Raible <raible@gmail.com> wrote:
> Signed-off-by: Eric Raible <raible@gmail.com>
Trivially-Acked-by: Shawn O. Pearce <spearce@spearce.org>
;-)

More completion support that probably should go to maint, as the functionality in git-branch is in 1.5.6 but we (again) forgot to make sure the completion was up-to-date prior to release.

Show 20 quoted lines
> ---
>  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 0eb8df0..22e109d 100755
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -546,7 +546,7 @@ _git_branch ()
>  	--*)
>  		__gitcomp "
>  			--color --no-color --verbose --abbrev= --no-abbrev
> -			--track --no-track
> +			--track --no-track --contains --merged --no-merged
>  			"
>  		;;
>  	*)
> -- 
> 1.5.6.1.1071.g76fb.dirty
-- 
Shawn.
Junio C Hamano· Jul 8, 2008, 05:30 UTC · re: Shawn O. Pearce · lore

Re: [PATCH] completion: add branch options --contains --merged --no-merged

"Shawn O. Pearce" <spearce@spearce.org> writes:
Show 10 quoted lines
> Eric Raible <raible@gmail.com> wrote:
>> Signed-off-by: Eric Raible <raible@gmail.com>
>
> Trivially-Acked-by: Shawn O. Pearce <spearce@spearce.org>
>
> ;-)
>
> More completion support that probably should go to maint, as the
> functionality in git-branch is in 1.5.6 but we (again) forgot to
> make sure the completion was up-to-date prior to release.

I am actually getting more worried about completion code getting larger and larger without its performance impact not being looked at nor addressed adequately. In my regular working tree:

	$ echo Docu<TAB>
completes "mentation/" instantly, but:
	$ git log -- Docu<TAB>
takes about 1.5 to 2 seconds to complete the same.
Johannes Schindelin· Jul 8, 2008, 11:36 UTC · re: Junio C Hamano · lore

Re: [PATCH] completion: add branch options --contains --merged --no-merged

Hi,
On Mon, 7 Jul 2008, Junio C Hamano wrote:
Show 24 quoted lines
> "Shawn O. Pearce" <spearce@spearce.org> writes:
> 
> > Eric Raible <raible@gmail.com> wrote:
> >> Signed-off-by: Eric Raible <raible@gmail.com>
> >
> > Trivially-Acked-by: Shawn O. Pearce <spearce@spearce.org>
> >
> > ;-)
> >
> > More completion support that probably should go to maint, as the
> > functionality in git-branch is in 1.5.6 but we (again) forgot to
> > make sure the completion was up-to-date prior to release.
> 
> I am actually getting more worried about completion code getting larger
> and larger without its performance impact not being looked at nor
> addressed adequately.  In my regular working tree:
> 
> 	$ echo Docu<TAB>
> 
> completes "mentation/" instantly, but:
> 
> 	$ git log -- Docu<TAB>
> 
> takes about 1.5 to 2 seconds to complete the same.
I noticed that myself, but did not have time to look into it.

It shows two bugs, actually: completions do not care about "--", and completing refs takes way too long.

Ciao, Dscho

SZEDER Gábor· Jul 8, 2008, 16:56 UTC · re: Johannes Schindelin · lore

[PATCH] bash: offer only paths after '--'

Many git commands use '--' to separate subcommands, options, and refs from paths. However, the programmable completion for several of these commands does not respect the '--', and offer subcommands, options, or refs after a '--', although only paths are permitted. e.g. 'git bisect -- <TAB>' offers subcommands, 'git log -- --<TAB>' offers options and 'git log -- git<TAB>' offers all gitgui tags.

The completion for the following commands share this wrong behaviour:
  am add bisect commit diff log reset shortlog submodule gitk.

To avoid this, we check the presence of a '--' on the command line first and let the shell do filename completion, if one is found.

Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
---
On Tue, Jul 08, 2008 at 01:36:43PM +0200, Johannes Schindelin wrote:
> It shows two bugs, actually: completions do not care about "--", 

I think I have found and corrected all the places where '--' was not handled properly, but might have overlooked something.

Hope that I got the commit message right (;
 contrib/completion/git-completion.bash |   30 ++++++++++++++++++++++++++++++
 1 files changed, 30 insertions(+), 0 deletions(-)
Show changes to contrib/completion/git-completion.bash +30 −0
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 6a15522..e7d8a75 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -451,6 +451,18 @@ __git_find_subcommand ()
 	done
 }
 
+__git_has_doubledash ()
+{
+	local c=1
+	while [ $c -lt $COMP_CWORD ]; do
+		if [ "--" = "${COMP_WORDS[c]}" ]; then
+			return 0
+		fi
+		c=$((++c))
+	done
+	return 1
+}
+
 __git_whitespacelist="nowarn warn error error-all strip"
 
 _git_am ()
@@ -497,6 +509,8 @@ _git_apply ()
 
 _git_add ()
 {
+	__git_has_doubledash && return
+
 	local cur="${COMP_WORDS[COMP_CWORD]}"
 	case "$cur" in
 	--*)
@@ -511,6 +525,8 @@ _git_add ()
 
 _git_bisect ()
 {
+	__git_has_doubledash && return
+
 	local subcommands="start bad good skip reset visualize replay log run"
 	local subcommand="$(__git_find_subcommand "$subcommands")"
 	if [ -z "$subcommand" ]; then
@@ -612,6 +628,8 @@ _git_cherry_pick ()
 
 _git_commit ()
 {
+	__git_has_doubledash && return
+
 	local cur="${COMP_WORDS[COMP_CWORD]}"
 	case "$cur" in
 	--*)
@@ -631,6 +649,8 @@ _git_describe ()
 
 _git_diff ()
 {
+	__git_has_doubledash && return
+
 	local cur="${COMP_WORDS[COMP_CWORD]}"
 	case "$cur" in
 	--*)
@@ -733,6 +753,8 @@ _git_ls_tree ()
 
 _git_log ()
 {
+	__git_has_doubledash && return
+
 	local cur="${COMP_WORDS[COMP_CWORD]}"
 	case "$cur" in
 	--pretty=*)
@@ -1088,6 +1110,8 @@ _git_remote ()
 
 _git_reset ()
 {
+	__git_has_doubledash && return
+
 	local cur="${COMP_WORDS[COMP_CWORD]}"
 	case "$cur" in
 	--*)
@@ -1100,6 +1124,8 @@ _git_reset ()
 
 _git_shortlog ()
 {
+	__git_has_doubledash && return
+
 	local cur="${COMP_WORDS[COMP_CWORD]}"
 	case "$cur" in
 	--*)
@@ -1157,6 +1183,8 @@ _git_stash ()
 
 _git_submodule ()
 {
+	__git_has_doubledash && return
+
 	local subcommands="add status init update"
 	if [ -z "$(__git_find_subcommand "$subcommands")" ]; then
 		local cur="${COMP_WORDS[COMP_CWORD]}"
@@ -1362,6 +1390,8 @@ _git ()
 
 _gitk ()
 {
+	__git_has_doubledash && return
+
 	local cur="${COMP_WORDS[COMP_CWORD]}"
 	local g="$(git rev-parse --git-dir 2>/dev/null)"
 	local merge=""
-- 
1.5.6.1.118.g82b2fef
Eric Raible· Jul 8, 2008, 17:46 UTC · re: SZEDER Gábor · lore

Re: [PATCH] bash: offer only paths after '--'

SZEDER Gábor <szeder <at> ira.uka.de> writes:
Show 5 quoted lines
> 
> Many git commands use '--' to separate subcommands, options, and refs
> from paths.  However, the programmable completion for several of these
> commands does not respect the '--', and offer subcommands, options, or
> refs after a '--', although only paths are permitted.  e.g. 'git bisect

I like this change, but how about also offering a plain '--' as one of the completion choices as a way of reminding newbies that the command in question is one of the ones that takes filenames after all options?

Junio C Hamano· Jul 8, 2008, 20:21 UTC · re: SZEDER Gábor · lore

Re: [PATCH] bash: offer only paths after '--'

SZEDER Gábor <szeder@ira.uka.de> writes:
> Hope that I got the commit message right (;
It was very readable.  Thanks.
Show 8 quoted lines
> +__git_has_doubledash ()
> +{
> +	local c=1
> +	while [ $c -lt $COMP_CWORD ]; do
> +		if [ "--" = "${COMP_WORDS[c]}" ]; then
> +			return 0
> +		fi
> +		c=$((++c))

This assignment is somewhat curious, although it should work as expected either way ;-)

Shawn?
Shawn O. Pearce· Jul 8, 2008, 23:18 UTC · re: Junio C Hamano · lore

Re: [PATCH] bash: offer only paths after '--'

Junio C Hamano <gitster@pobox.com> wrote:
Show 5 quoted lines
> SZEDER Gábor <szeder@ira.uka.de> writes:
> 
> > Hope that I got the commit message right (;
> 
> It was very readable.  Thanks.
Acked-by: Shawn O. Pearce <spearce@spearce.org>
Show 11 quoted lines
> > +__git_has_doubledash ()
> > +{
> > +	local c=1
> > +	while [ $c -lt $COMP_CWORD ]; do
> > +		if [ "--" = "${COMP_WORDS[c]}" ]; then
> > +			return 0
> > +		fi
> > +		c=$((++c))
> 
> This assignment is somewhat curious, although it should work as expected
> either way ;-)

I agree, its damned odd. But we already do this in the same sort of loop inside of _git_branch() (see around line 541 in next). This new patch is only sticking with our current set of conventions in the script, so I say its fine.

-- 
Shawn.
Junio C Hamano· Jul 8, 2008, 23:23 UTC · re: Shawn O. Pearce · lore

Re: [PATCH] bash: offer only paths after '--'

"Shawn O. Pearce" <spearce@spearce.org> writes:
Show 25 quoted lines
> Junio C Hamano <gitster@pobox.com> wrote:
>> SZEDER Gábor <szeder@ira.uka.de> writes:
>> 
>> > Hope that I got the commit message right (;
>> 
>> It was very readable.  Thanks.
>
> Acked-by: Shawn O. Pearce <spearce@spearce.org>
>
>> > +__git_has_doubledash ()
>> > +{
>> > +	local c=1
>> > +	while [ $c -lt $COMP_CWORD ]; do
>> > +		if [ "--" = "${COMP_WORDS[c]}" ]; then
>> > +			return 0
>> > +		fi
>> > +		c=$((++c))
>> 
>> This assignment is somewhat curious, although it should work as expected
>> either way ;-)
>
> I agree, its damned odd.  But we already do this in the same
> sort of loop inside of _git_branch() (see around line 541 in
> next).  This new patch is only sticking with our current set
> of conventions in the script, so I say its fine.
Chuckling...  Thanks for sanity checking and an Ack.
SZEDER Gábor· Jul 8, 2008, 23:51 UTC · re: Shawn O. Pearce · lore

Re: [PATCH] bash: offer only paths after '--'

On Tue, Jul 08, 2008 at 11:18:37PM +0000, Shawn O. Pearce wrote:
Show 11 quoted lines
> Junio C Hamano <gitster@pobox.com> wrote:
> > SZEDER Gábor <szeder@ira.uka.de> writes:
> > > +		c=$((++c))
> > 
> > This assignment is somewhat curious, although it should work as expected
> > either way ;-)
> 
> I agree, its damned odd.  But we already do this in the same
> sort of loop inside of _git_branch() (see around line 541 in
> next).  This new patch is only sticking with our current set
> of conventions in the script, so I say its fine.
Well, according to
  git blame contrib/completion/git-completion.bash  |grep '++'

you started this convention back in 2006, I just copied and modified your code (;

Maybe an old C++ "heritage"? In C++ it matters for class types (e.g. iterators), because the postfix operator might be slower than the prefix.

Best, Gábor

Shawn O. Pearce· Jul 8, 2008, 23:55 UTC · re: SZEDER Gábor · lore

Re: [PATCH] bash: offer only paths after '--'

SZEDER GGGbor <szeder@ira.uka.de> wrote:
Show 14 quoted lines
> On Tue, Jul 08, 2008 at 11:18:37PM +0000, Shawn O. Pearce wrote:
> > Junio C Hamano <gitster@pobox.com> wrote:
> > > SZEDER Gábor <szeder@ira.uka.de> writes:
> > > > +		c=$((++c))
> > > 
> > > This assignment is somewhat curious, although it should work as expected
> > > either way ;-)
>
> Well, according to
> 
>   git blame contrib/completion/git-completion.bash  |grep '++'
> 
> you started this convention back in 2006, I just copied and modified
> your code (;
Yea, I don't doubt it was me that did this.
 
> Maybe an old C++ "heritage"?  In C++ it matters for class types (e.g.
> iterators), because the postfix operator might be slower than the
> prefix.

Unlikely, but maybe. I'm not really a C++ programmer. I tend to avoid C++ when/if I am given the chance to do so.

-- 
Shawn.
Junio C Hamano· Jul 9, 2008, 00:06 UTC · re: SZEDER Gábor · lore

Re: [PATCH] bash: offer only paths after '--'

SZEDER Gábor <szeder@ira.uka.de> writes:
Show 11 quoted lines
> On Tue, Jul 08, 2008 at 11:18:37PM +0000, Shawn O. Pearce wrote:
>> Junio C Hamano <gitster@pobox.com> wrote:
>> > SZEDER Gábor <szeder@ira.uka.de> writes:
>> > > +		c=$((++c))
>> > 
>> > This assignment is somewhat curious, although it should work as expected
>> > either way ;-)
> ...
> Maybe an old C++ "heritage"?  In C++ it matters for class types (e.g.
> iterators), because the postfix operator might be slower than the
> prefix.

Heh, I was not talking about prefix vs postfix but about the assignment into the variable that is incremented as a side effect of evaluating the left hand side. If you know the variable is incremented already there is no point in assigning the resulting value to it ;-)

	c=$(( $c + 1 ))

would have avoided such an uneasy feeling, and would have been more portable. Even though $((x)) and $(($x)) are supposed to evaluate the same, some shells do not like dollar-less variable names in arithmetic expansion, and prefix/postfix increment/decrement are not required to be supported by POSIX.

But this script being bash completion, we can use as much bashism as we want here; perhaps I would have written:

	: $((c++))
        

← back to recent threads