[PATCH 0/4] Fix branch.autosetup(merge|rebase) completion

[PATCH 0/4] Fix branch.autosetup(merge|rebase) completion

[Ramkumar Ramachandra]

Hi,

I figured that branch.autosetupmerge, branch.autosetuprebase and
remote.pushdefault are very tedious to type out in full, and started
looking into fixing their completions this evening. The solution turns
out to be much more complex than I initally imagined, but I'm quite
happy with the solution.

I hope it's an enjoyable read.

Ramkumar Ramachandra (4):
  completion: prioritize ./git-completion.bash
  completion: introduce __gitcomp_2 ()
  completion: fix branch.autosetup(merge|rebase)
  completion: fix remote.pushdefault

 contrib/completion/git-completion.bash | 43 ++++++++++++++++++++++++++++++++--
 contrib/completion/git-completion.zsh  | 12 +++++++++-
 2 files changed, 52 insertions(+), 3 deletions(-)

-- 
1.8.5.2.227.g53f3478

[PATCH 1/4] completion: prioritize ./git-completion.bash

[Ramkumar Ramachandra]

To ease development, prioritize ./git-completion.bash over other
standard system paths.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 contrib/completion/git-completion.zsh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/completion/git-completion.zsh b/contrib/completion/git-completion.zsh
index fac5e71..6fca145 100644
--- a/contrib/completion/git-completion.zsh
+++ b/contrib/completion/git-completion.zsh
@@ -30,10 +30,10 @@ if [ -z "$script" ]; then
 	local -a locations
 	local e
 	locations=(
+		$(dirname ${funcsourcetrace[1]%:*})/git-completion.bash
 		'/etc/bash_completion.d/git' # fedora, old debian
 		'/usr/share/bash-completion/completions/git' # arch, ubuntu, new debian
 		'/usr/share/bash-completion/git' # gentoo
-		$(dirname ${funcsourcetrace[1]%:*})/git-completion.bash
 		)
 	for e in $locations; do
 		test -f $e && script="$e" && break
-- 
1.8.5.2.227.g53f3478

[PATCH 2/4] completion: introduce __gitcomp_2 ()

[Ramkumar Ramachandra]

There are situations where two classes of completions possible. For
example

  branch.<TAB>

should try to complete

  branch.master.
  branch.autosetupmerge
  branch.autosetuprebase

The first candidate has the suffix ".", and the second/ third candidates
have the suffix " ". To facilitate completions of this kind, create a
variation of __gitcomp_nl () that accepts two sets of arguments and two
independent suffixes.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 contrib/completion/git-completion.bash | 30 ++++++++++++++++++++++++++++++
 contrib/completion/git-completion.zsh  | 10 ++++++++++
 2 files changed, 40 insertions(+)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 51c2dd4..64b20b8 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -233,6 +233,36 @@ __gitcomp_nl ()
 	__gitcompadd "$1" "${2-}" "${3-$cur}" "${4- }"
 }
 
+# Generates completion reply from two sets of completion words, with
+# configurable suffixes for each.
+#
+# It accepts 2 to 6 arguments:
+# 1: First set of possible completion words.
+# 2: Second set of possible completion words.
+# 3: A prefix to be added to each completion word (both $1 and $2)
+#    (optional).
+# 4: Generate possible completion matches for this word (optional).
+# 5: A suffix to be appended to each completion word in the first set
+#    ($1) instead of the default space (optional).
+# 6: A suffix to be appended to each completion word in the second set
+#    ($2) instead of the default space (optional).
+__gitcomp_2 ()
+{
+	local pfx="${3-}" cur_="${4-$cur}" sfx="${5- }" sfx2="${6- }" i=0
+	local IFS=$' \t\n'
+
+	for x in $1; do
+		if [[ "$x" == "$cur_"* ]]; then
+			COMPREPLY[i++]="$pfx$x$sfx"
+		fi
+	done
+	for x in $2; do
+		if [[ "$x" == "$cur_"* ]]; then
+			COMPREPLY[i++]="$pfx$x$sfx2"
+		fi
+	done
+}
+
 # Generates completion reply with compgen from newline-separated possible
 # completion filenames.
 # It accepts 1 to 3 arguments:
diff --git a/contrib/completion/git-completion.zsh b/contrib/completion/git-completion.zsh
index 6fca145..261a7f5 100644
--- a/contrib/completion/git-completion.zsh
+++ b/contrib/completion/git-completion.zsh
@@ -76,6 +76,16 @@ __gitcomp_nl ()
 	compadd -Q -S "${4- }" -p "${2-}" -- ${=1} && _ret=0
 }
 
+__gitcomp_2 ()
+{
+	emulate -L zsh
+
+	local IFS=$' \t\n'
+	compset -P '*[=:]'
+	compadd -Q -S "${5- }" -p "${3-}" -- ${=1} && _ret=0
+	compadd -Q -S "${6- }" -p "${3-}" -- ${=2} && _ret=0
+}
+
 __gitcomp_file ()
 {
 	emulate -L zsh
-- 
1.8.5.2.227.g53f3478

Re: [PATCH 2/4] completion: introduce __gitcomp_2 ()

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> There are situations where two classes of completions possible. For
> example
>
>   branch.<TAB>
>
> should try to complete
>
>   branch.master.
>   branch.autosetupmerge
>   branch.autosetuprebase
>
> The first candidate has the suffix ".", and the second/ third candidates
> have the suffix " ". To facilitate completions of this kind, create a
> variation of __gitcomp_nl () that accepts two sets of arguments and two
> independent suffixes.

That sounds like a reasonable issue to address, but I do not quite
get why you need a new helper to do this.

If the original only knows to throw "branch." + branch names +
trailing dot into COMPREPLY[] and does so by calling gitcomp_nl,
isn't it the matter of making another call to gitcomp_nl just after
the existing call to stuff branch.autosetup* with trailing SP to
append them to COMPREPLY[]?

Ahh, is that because the eventual call to __gitcompadd() starts the
iteration starting from zero, essentially forbidding you to
incrementally adding to COMPREPLY[] from multiple callers, even
though it is called comp "add" not "replace with this single thing"?

What I am wondering is if a cleaner solution that can be reused by
later needs that may have more than two data sources (or more than
two suffixes) might be to create a variant of __gitcomp_nl that does
not clear existing entries in COMPREPLY[] array, add a helper to
clear the array, which would make the existing one to:

	__gitcomp_nl () {
        	__gitcomp_clear
                __gitcomp_nl_append "$@"
	}

and then complete branch.* using two calls to __gitcomp_*, letting
the first one clear and later one(s) accumulate:

	__gitcomp_nl "$(__git_heads)" "$pfx" "$cur_" "."
	__gitcomp_nl_append $"autosetupmerge\nautosetuprebase\n" "$pfx" "$cur_" " "

Will queue as-is.

Thanks.

Re: [PATCH 2/4] completion: introduce __gitcomp_2 ()

[Ramkumar Ramachandra]

Junio C Hamano wrote:
>         __gitcomp_nl "$(__git_heads)" "$pfx" "$cur_" "."
>         __gitcomp_nl_append $"autosetupmerge\nautosetuprebase\n" "$pfx" "$cur_" " "

This is not a bad idea at all. I'm just afraid that we might be
leaving open ends: What happens if the $pfx isn't the same in both
cases? Who keeps track of the index "i" of COMPREPLY (it's currently a
local variable)? If we make it global, doesn't every function that
deals with COMPREPLY be careful to reset it?

More importantly, can you see a usecase for more than two completion classes?

Re: [PATCH 2/4] completion: introduce __gitcomp_2 ()

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Junio C Hamano wrote:
>>         __gitcomp_nl "$(__git_heads)" "$pfx" "$cur_" "."
>>         __gitcomp_nl_append $"autosetupmerge\nautosetuprebase\n" "$pfx" "$cur_" " "
>
> This is not a bad idea at all. I'm just afraid that we might be
> leaving open ends: What happens if the $pfx isn't the same in both
> cases? Who keeps track of the index "i" of COMPREPLY (it's currently a
> local variable)? If we make it global, doesn't every function that
> deals with COMPREPLY be careful to reset it?

I am not sure what you are worried about $pfx; what does it do when
you have strings with different prefix in COMPREPLY? If it breaks,
then the answer is "don't do it then".

Doesn't an array know its own length and give you a way to ask?

> More importantly, can you see a usecase for more than two completion classes?

I am not sure why you think it is more important.

Somebody lacked forsight and designed an interface that would allow
only one "completion classes" (whatever that means) and called the
result sufficient. It has been sufficient for a long time.

Later you came, found that one was not enough, and added an inteface
that would allow only two, and called the result sufficient.  See a
pattern?

Anticipating unforseen possibilities for enhancement and preparing
an easy way to support them without overengineering is what the
"prepare an appending variant" suggestion is about, and by
definition, unforseen possibilities have not been seen yet ;-)

Imagine if one is flipping 47 topic branches from 6 contributors
whose names all begin with 'j'.  I can see that such a person would
appreciate if "git config branch.j<TAB>" did not dump all 47 topics
at once but offered "jc/ jk/ jl/ jm/ jn/ js/" instead, and then a
follow-up completion of "git config branch.jk/<TAB>" expanded to
names of topics from that single contributor "jk".  Wouldn't the way
to give these be either to return these two-letter hierarchy names
with slash as the suffix or to return list of two-letter plus a
slash with an empty suffix?  Either way, that is using something
different from a dot or a space, so that may count as the third, I
guess.

Re: [PATCH 2/4] completion: introduce __gitcomp_2 ()

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> I am not sure what you are worried about $pfx; what does it do when
> you have strings with different prefix in COMPREPLY? If it breaks,
> then the answer is "don't do it then".
>
> Doesn't an array know its own length and give you a way to ask?

Right. I was just throwing counterpoints at the wall.

> Imagine if one is flipping 47 topic branches from 6 contributors
> whose names all begin with 'j'.  I can see that such a person would
> appreciate if "git config branch.j<TAB>" did not dump all 47 topics
> at once but offered "jc/ jk/ jl/ jm/ jn/ js/" instead, and then a
> follow-up completion of "git config branch.jk/<TAB>" expanded to
> names of topics from that single contributor "jk".  Wouldn't the way
> to give these be either to return these two-letter hierarchy names
> with slash as the suffix or to return list of two-letter plus a
> slash with an empty suffix?  Either way, that is using something
> different from a dot or a space, so that may count as the third, I
> guess.

Ah, after completing branch.jk/, we would want no suffix: so the
example definitely counts as a third "completion class". I agree with
your reasoning, and will rework the series to do the _append () thing
you suggested.

Thanks.

[PATCH 3/4] completion: fix branch.autosetup(merge|rebase)

[Ramkumar Ramachandra]

When attempting to complete

  $ git config branch.auto<TAB>

'autosetupmerge' and 'autosetuprebase' don't come up. This is because
"$cur" is matched with "branch.*" and a list of branches are
completed. Add 'autosetup(merge|rebase)' to the list of branches using
__gitcomp_2 ().

Also take care to not complete

  $ git config branch.autosetupmerge.<TAB>
  $ git config branch.autosetuprebase.<TAB>

with the usual branch.<name>. candidates.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 contrib/completion/git-completion.bash | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 64b20b8..0bda757 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1851,12 +1851,18 @@ _git_config ()
 		;;
 	branch.*.*)
 		local pfx="${cur%.*}." cur_="${cur##*.}"
+		if [ "$pfx" == "branch.autosetupmerge." ] ||
+			[ "$pfx" == "branch.autosetuprebase." ]; then
+			return
+		fi
 		__gitcomp "remote pushremote merge mergeoptions rebase" "$pfx" "$cur_"
 		return
 		;;
 	branch.*)
 		local pfx="${cur%.*}." cur_="${cur#*.}"
-		__gitcomp_nl "$(__git_heads)" "$pfx" "$cur_" "."
+		__gitcomp_2 "$(__git_heads)" "
+			autosetupmerge autosetuprebase
+			" "$pfx" "$cur_" "."
 		return
 		;;
 	guitool.*.*)
-- 
1.8.5.2.227.g53f3478

Re: [PATCH 3/4] completion: fix branch.autosetup(merge|rebase)

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

>  	branch.*.*)
>  		local pfx="${cur%.*}." cur_="${cur##*.}"
> +		if [ "$pfx" == "branch.autosetupmerge." ] ||
> +			[ "$pfx" == "branch.autosetuprebase." ]; then
> +			return
> +		fi
>  		__gitcomp "remote pushremote merge mergeoptions rebase" "$pfx" "$cur_"
>  		return
>  		;;

I do not quite understand this change.

If we are looking at "branch.autosetupmerge." followed by something,
who typed that final dot?  If you are working on a topic about
auto-setup-merge and named your branch "autosetupmerge", don't you
want to be able to configure various aspect of that branch via
branch.autosetupmerge.{remote,merge} etc., just like you can do so
for your "topic" branch via branch.topic.{remote,merge} etc.,
regardless of your use of "autosetupmerge" option across branches?

Besides, it smells fishy to me that you need to enumerate and
special case these two here, and then you have to repeat them below
in a separate case arm.

>  	branch.*)
>  		local pfx="${cur%.*}." cur_="${cur#*.}"
> -		__gitcomp_nl "$(__git_heads)" "$pfx" "$cur_" "."
> +		__gitcomp_2 "$(__git_heads)" "
> +			autosetupmerge autosetuprebase
> +			" "$pfx" "$cur_" "."
>  		return
>  		;;
>  	guitool.*.*)

Re: [PATCH 3/4] completion: fix branch.autosetup(merge|rebase)

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> If we are looking at "branch.autosetupmerge." followed by something,
> who typed that final dot?

I admit that it's a very unlikely case. The user did:

  $ branch.autosetupmer<TAB>

hit backspace to delete the trailing space, inserted a dot, and hit <TAB> again.

> If you are working on a topic about
> auto-setup-merge and named your branch "autosetupmerge", don't you
> want to be able to configure various aspect of that branch via
> branch.autosetupmerge.{remote,merge} etc., just like you can do so
> for your "topic" branch via branch.topic.{remote,merge} etc.,
> regardless of your use of "autosetupmerge" option across branches?

My reasoning was that being correct was more important that being
complete. So, if by some horrible chance, the user names her branch
"autosetupmerge", we don't aid her in completions.

> Besides, it smells fishy to me that you need to enumerate and
> special case these two here, and then you have to repeat them below
> in a separate case arm.

I'm not too irked about correctness in this odd case; seeing that you
aren't either, I'll resubmit the series without this hunk (+ the hunk
in remote.pushdefault).

Re: [PATCH 3/4] completion: fix branch.autosetup(merge|rebase)

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Junio C Hamano wrote:
>> If we are looking at "branch.autosetupmerge." followed by something,
>> who typed that final dot?
>
> I admit that it's a very unlikely case. The user did:
>
>   $ branch.autosetupmer<TAB>
>
> hit backspace to delete the trailing space, inserted a dot, and hit <TAB> again.
>
>> If you are working on a topic about
>> auto-setup-merge and named your branch "autosetupmerge", don't you
>> want to be able to configure various aspect of that branch via
>> branch.autosetupmerge.{remote,merge} etc., just like you can do so
>> for your "topic" branch via branch.topic.{remote,merge} etc.,
>> regardless of your use of "autosetupmerge" option across branches?
>
> My reasoning was that being correct was more important that being
> complete. So, if by some horrible chance, the user names her branch
> "autosetupmerge", we don't aid her in completions.
>
>> Besides, it smells fishy to me that you need to enumerate and
>> special case these two here, and then you have to repeat them below
>> in a separate case arm.
>
> I'm not too irked about correctness in this odd case; seeing that you
> aren't either, I'll resubmit the series without this hunk (+ the hunk
> in remote.pushdefault).

You seem to be calling it "incorrect" to give the same degree of
completion for a branch the user named "autosetupmerge" as another
branch "topic", but I think it is incorrect not to, so I cannot tell
if we are agreeing or disagreeing.

Puzzled...

Re: [PATCH 3/4] completion: fix branch.autosetup(merge|rebase)

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> You seem to be calling it "incorrect" to give the same degree of
> completion for a branch the user named "autosetupmerge" as another
> branch "topic", but I think it is incorrect not to, so I cannot tell
> if we are agreeing or disagreeing.

No, what's incorrect is providing completions for

  $ git config branch.autosetupmerge.<TAB>

when no branch called "autosetupmerge" exists. The purpose of the hunk
(which I now removed) was to prevent such completions, but it has the
side-effect of also preventing a legitimate completion in the case
when the user really has a branch named "autosetupmerge".

What is your take on the issue?

Re: [PATCH 3/4] completion: fix branch.autosetup(merge|rebase)

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Junio C Hamano wrote:
>> You seem to be calling it "incorrect" to give the same degree of
>> completion for a branch the user named "autosetupmerge" as another
>> branch "topic", but I think it is incorrect not to, so I cannot tell
>> if we are agreeing or disagreeing.
>
> No, what's incorrect is providing completions for
>
>   $ git config branch.autosetupmerge.<TAB>
>
> when no branch called "autosetupmerge" exists.  The purpose of the
> hunk (which I now removed) was to prevent such completions, ...

Hmph, but in a repository without 'foo', I just did

	$ git config branch.foo.<TAB>
        branch.foo.merge          branch.foo.rebase         
        branch.foo.mergeoptions   branch.foo.remote         

and got offered the above. How would that removed hunk that special
cased those autosetupmerge etc. helped such case?

If it _were_ about correctness, and the definition of correctness
were that "completing branch.foo.<TAB> to offer these four variables
is wrong until refs/heads/foo materializes", the "fix" would have
checked if there already is such a branch and refused to complete
otherwise, not special case a few known names such as autosetup*.

As there is no reason to forbid setting configuration variables for
a branch 'foo' you are going to create before you actually create it
with "git branch foo", I do not necessarily agree with the above
definition of correctness, either.

So it was completely bogus hunk and it is good we noticed and
decided to remove it, I guess.

[PATCH 4/4] completion: fix remote.pushdefault

[Ramkumar Ramachandra]

When attempting to complete

  $ git config remote.push<TAB>

'pushdefault' doesn't come up. This is because "$cur" is matched with
"remote.*" and a list of remotes are completed. Add 'pushdefault' to the
list of remotes using __gitcomp_2 ().

Also take care to not complete

  $ git config remote.pushdefault.<TAB>

with the usual remote.<name>. candidates.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 contrib/completion/git-completion.bash | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 0bda757..9e0213d 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1896,6 +1896,9 @@ _git_config ()
 		;;
 	remote.*.*)
 		local pfx="${cur%.*}." cur_="${cur##*.}"
+		if [ "$pfx" == "remote.pushdefault." ]; then
+			return
+		fi
 		__gitcomp "
 			url proxy fetch push mirror skipDefaultUpdate
 			receivepack uploadpack tagopt pushurl
@@ -1904,7 +1907,7 @@ _git_config ()
 		;;
 	remote.*)
 		local pfx="${cur%.*}." cur_="${cur#*.}"
-		__gitcomp_nl "$(__git_remotes)" "$pfx" "$cur_" "."
+		__gitcomp_2 "$(__git_remotes)" "pushdefault" "$pfx" "$cur_" "."
 		return
 		;;
 	url.*.*)
-- 
1.8.5.2.227.g53f3478