patch, 4 partscompletion: prioritize ./git-completion.bash

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

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

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

Ramkumar Ramachandra <artagnon@gmail.com> writes:

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.

Junio C Hamano wrote:

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?

Ramkumar Ramachandra <artagnon@gmail.com> writes:

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?

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.

Junio C Hamano wrote:

Right. I was just throwing counterpoints at the wall.

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.

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

Ramkumar Ramachandra <artagnon@gmail.com> writes:

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.

Junio C Hamano wrote:

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.

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.

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

Ramkumar Ramachandra <artagnon@gmail.com> writes:

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

Junio C Hamano wrote:

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?

Ramkumar Ramachandra <artagnon@gmail.com> writes:

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.

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