[PATCH v3] completion: add new _GIT_complete helper

[PATCH v3] completion: add new _GIT_complete helper

[Felipe Contreras]

This simplifies the completions, and makes it easier to define aliases:

 _GIT_complete gf git_fetch

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---

Since v3:

 * Rename to _GIT_complete to follow bash completion "guidelines"
 * Get rid of foo_wrap name

Since v2:

 * Remove stuff related to aliases fixes; should work on top of master

 contrib/completion/git-completion.bash |   67 ++++++++++++++------------------
 t/t9902-completion.sh                  |    2 +-
 2 files changed, 31 insertions(+), 38 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 9f56ec7..f300b87 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2603,21 +2603,6 @@ _git ()
 {
 	local i c=1 command __git_dir
 
-	if [[ -n ${ZSH_VERSION-} ]]; then
-		emulate -L bash
-		setopt KSH_TYPESET
-
-		# workaround zsh's bug that leaves 'words' as a special
-		# variable in versions < 4.3.12
-		typeset -h words
-
-		# workaround zsh's bug that quotes spaces in the COMPREPLY
-		# array if IFS doesn't contain spaces.
-		typeset -h IFS
-	fi
-
-	local cur words cword prev
-	_get_comp_words_by_ref -n =: cur words cword prev
 	while [ $c -lt $cword ]; do
 		i="${words[c]}"
 		case "$i" in
@@ -2667,22 +2652,6 @@ _git ()
 
 _gitk ()
 {
-	if [[ -n ${ZSH_VERSION-} ]]; then
-		emulate -L bash
-		setopt KSH_TYPESET
-
-		# workaround zsh's bug that leaves 'words' as a special
-		# variable in versions < 4.3.12
-		typeset -h words
-
-		# workaround zsh's bug that quotes spaces in the COMPREPLY
-		# array if IFS doesn't contain spaces.
-		typeset -h IFS
-	fi
-
-	local cur words cword prev
-	_get_comp_words_by_ref -n =: cur words cword prev
-
 	__git_has_doubledash && return
 
 	local g="$(__gitdir)"
@@ -2703,16 +2672,40 @@ _gitk ()
 	__git_complete_revlist
 }
 
-complete -o bashdefault -o default -o nospace -F _git git 2>/dev/null \
-	|| complete -o default -o nospace -F _git git
-complete -o bashdefault -o default -o nospace -F _gitk gitk 2>/dev/null \
-	|| complete -o default -o nospace -F _gitk gitk
+__git_func_wrap ()
+{
+	if [[ -n ${ZSH_VERSION-} ]]; then
+		emulate -L bash
+		setopt KSH_TYPESET
+
+		# workaround zsh's bug that leaves 'words' as a special
+		# variable in versions < 4.3.12
+		typeset -h words
+
+		# workaround zsh's bug that quotes spaces in the COMPREPLY
+		# array if IFS doesn't contain spaces.
+		typeset -h IFS
+	fi
+	local cur words cword prev
+	_get_comp_words_by_ref -n =: cur words cword prev
+	__git_func "$@"
+}
+
+_GIT_complete ()
+{
+	local name="${2-$1}"
+	eval "$(typeset -f __git_func_wrap | sed -e "s/__git_func/_$name/")"
+	complete -o bashdefault -o default -o nospace -F _${name}_wrap $1 2>/dev/null \
+		|| complete -o default -o nospace -F _${name}_wrap $1
+}
+
+_GIT_complete git
+_GIT_complete gitk
 
 # The following are necessary only for Cygwin, and only are needed
 # when the user has tab-completed the executable name and consequently
 # included the '.exe' suffix.
 #
 if [ Cygwin = "$(uname -o 2>/dev/null)" ]; then
-complete -o bashdefault -o default -o nospace -F _git git.exe 2>/dev/null \
-	|| complete -o default -o nospace -F _git git.exe
+_GIT_complete git.exe git
 fi
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 5bda6b6..331a5b9 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -63,7 +63,7 @@ run_completion ()
 	local _cword
 	_words=( $1 )
 	(( _cword = ${#_words[@]} - 1 ))
-	_git && print_comp
+	_git_wrap && print_comp
 }
 
 test_completion ()
-- 
1.7.10

Re: [PATCH v3] completion: add new _GIT_complete helper

[Jonathan Nieder]

Felipe Contreras wrote:

> Since v3:
>
>  * Rename to _GIT_complete to follow bash completion "guidelines"
>  * Get rid of foo_wrap name

Thanks.  Gábor, does the "all caps _GIT_ prefix for public API
functions" convention look like one we should adopt?  If I understand
correctly, previously contrib/completion/git-completion.bash used
leading double underscores for everything except completion functions,
so this is a change.

Following a convention similar to the bash-completion project's
proposed future convention doesn't really help compatibility.  If we
want to be able to include this function in that project without
change some day, we'd have to call it _BC_git_complete. :)

I personally would be happier with a git_complete function provided
by another script, like this:

	contrib/completion/git-completion.bash:

		__git_complete () {
			...
		}

	contrib/completion/bash-helpers.bash:

		git_complete () {
			__git_complete "$@"
		}

One might object that if the user includes bash-helpers.bash (name is
just a strawman) in .bashrc for interactive shells because he is
defining some custom completion functions,

	git<TAB>

would show the git_complete function.  I think that's fine.  Maybe
the user would enjoy the reminder.  git<TAB> also shows any
dashed-form git commands that happen to be on the $PATH.

Please don't take this as a strong objection.  Maybe the
user-unfriendly version would be called _GIT_complete and someone
interested can provide the friendly git_complete and git_ps1 wrappers
separately, for example.  I just think it is worth thinking carefully
and being consistent about.

Hope that helps,
Jonathan

Re: [PATCH v3] completion: add new _GIT_complete helper

[Felipe Contreras]

On Sat, May 5, 2012 at 5:54 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Felipe Contreras wrote:
>
>> Since v3:
>>
>>  * Rename to _GIT_complete to follow bash completion "guidelines"
>>  * Get rid of foo_wrap name
>
> Thanks.  Gábor, does the "all caps _GIT_ prefix for public API
> functions" convention look like one we should adopt?  If I understand
> correctly, previously contrib/completion/git-completion.bash used
> leading double underscores for everything except completion functions,
> so this is a change.
>
> Following a convention similar to the bash-completion project's
> proposed future convention doesn't really help compatibility.  If we
> want to be able to include this function in that project without
> change some day, we'd have to call it _BC_git_complete. :)

No, that's for bash-completion's functions, this is a git bash
completion function.

And in any case, if they want something different they can change it
themselves, and they could tell us.

But wasn't you the one that suggested we follow the bash-completion's
guidelines, or that was only when the guidelines happened to match
your preference?

There are basically four arguments that have been brought forward.

1) Namespace

You said there were two namespaces:

> _git_*  (completion functions)
>__git_* (everything else, including public interfaces like __git_ps1)

But that's not actually true, we have these:

  __gitfoo (__gitdir, __gitcomp_1, __gitcomp, __gitcomp_nl)
  __git_foo
  _git_foo
  _git, _gitk

And what is used for what is not exactly clear.

Currently the only function meant to be public is __git_ps1, but
there's other __git_foo functions that are not meant to be public, so
clearly there's no namespace for public functions. Everything is
ad-hoc.

It could be assumed that anything that doesn't start with a _ is
reserved for the user. Of course, there's no such guideline anywhere,
so this would be a self-imposed limitation.

2) Guideline

You brought this argument forward, but it turns out the
bash-completion guys have no actual guideline; they are still trying
to decide what would be the naming for public functions.

If there's anything close to a guideline for bash-completion
functions, it would be a _BC_ prefix. Our script thus would be using
_GIT_ for its public functions. This would mean __git_ps1 should be
renamed to _GIT_ps1.

But now it seems you want to separate from this guideline.

3) Conflicts

Another problem is that a user might already have a function named
like that, which means 'git_complete' has higher chances of collision.
But I wrote checks that would ensure this doesn't happen, still,
nobody was interested in those checks.

It seems people are not interested in *real* conflicts, but rather
theoretical namespace collisions.

4) Convenience

git_complete is nicer than _GIT_complete.


It seems to me the push-back away from 'git_complete' is mostly due to
imaginary reasons, and now apparently specious reasons as well. So I
guess there's no point in discussing; no amount of evidence is going
to convince anybody to anything.

Cheers.

-- 
Felipe Contreras

Re: [PATCH v3] completion: add new _GIT_complete helper

[Jonathan Nieder]

Felipe Contreras wrote:
> On Sat, May 5, 2012 at 5:54 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:

>> Following a convention similar to the bash-completion project's
>> proposed future convention doesn't really help compatibility.  If we
>> want to be able to include this function in that project without
>> change some day, we'd have to call it _BC_git_complete. :)
>
> No, that's for bash-completion's functions, this is a git bash
> completion function.

Please read again.  "If we want to be able to include this ...".  I
assume we do not, but that would be the reason to follow their
convention to the letter.

[...]
> But wasn't you the one that suggested we follow the bash-completion's
> guidelines, or that was only when the guidelines happened to match
> your preference?

Sorry for the lack of clarity before.  I like to hope that "Because
Jonathan said so" is _never_ the only justification for putting up
with a technical change you disagree with.  In this instance, my
personal justification was "Because our completion scripts are already
using this convention, which happens to come from bash-completion's
guidelines and here are the reasons behind those".

Also, I think you have misunderstood me.  I was asking Gábor for input
because you were proposing changing a convention and I thought Gábor
had been maintaining the completion scripts.  I was not trying to say
"Please don't do this".

I was not inviting you to argue with me.

Kind regards,
Jonathan

Re: [PATCH v3] completion: add new _GIT_complete helper

[Jonathan Nieder]

Jonathan Nieder wrote:
>> On Sat, May 5, 2012 at 5:54 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:

>>> Following a convention similar to the bash-completion project's
>>> proposed future convention doesn't really help compatibility.  If we
>>> want to be able to include this function in that project without
>>> change some day, we'd have to call it _BC_git_complete. :)
[...]
> Please read again.  "If we want to be able to include this ...".  I
> assume we do not, but that would be the reason to follow their
> convention to the letter.

Quick clarification: I actually think it would be nice to make it easy
to pass maintenance of the git completion script to that project if
automatic option discovery means changes to the script settle down or
if we have less time to work on it some day.  Unfortunately, the
proposed namespace rules[1] (I didn't think the change had happened or
been set in stone yet?) would not make that easy.  Oh well.

[1] http://thread.gmane.org/gmane.comp.shells.bash.completion.scm/2013/focus=3158

Re: [PATCH v3] completion: add new _GIT_complete helper

[Felipe Contreras]

On Sat, May 5, 2012 at 6:47 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Felipe Contreras wrote:
>> On Sat, May 5, 2012 at 5:54 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>
>>> Following a convention similar to the bash-completion project's
>>> proposed future convention doesn't really help compatibility.  If we
>>> want to be able to include this function in that project without
>>> change some day, we'd have to call it _BC_git_complete. :)
>>
>> No, that's for bash-completion's functions, this is a git bash
>> completion function.
>
> Please read again.  "If we want to be able to include this ...".  I
> assume we do not, but that would be the reason to follow their
> convention to the letter.

We don't know what is their convention, so we can't possibly follow it
to the letter. From the looks of it, they want _BC_* for *their*
public APIs, we shouldn't be using that:

http://article.gmane.org/gmane.comp.shells.bash.completion.devel/3158

> [...]
>> But wasn't you the one that suggested we follow the bash-completion's
>> guidelines, or that was only when the guidelines happened to match
>> your preference?
>
> Sorry for the lack of clarity before.  I like to hope that "Because
> Jonathan said so" is _never_ the only justification for putting up
> with a technical change you disagree with.

And I'd like to think that when you filibuster a discussion there's a
good reason for it.

> In this instance, my
> personal justification was "Because our completion scripts are already
> using this convention, which happens to come from bash-completion's
> guidelines and here are the reasons behind those".

I see, so the bash-completion guidelines was not the main point, that
was just extra sauce?

Then it would follow that if the bash-completion guidelines were
different, that wouldn't change your main argument, because the
reasons would be the same. But when I argued that there were no such
bash-completion guidelines you didn't just drop this side-argument,
you pressed on it and even included the bash-completion mailing list.
So all the discussion about the existence of these guidelines was
pointless?

I am going to refrain from expressing what I think of this.

> Also, I think you have misunderstood me.  I was asking Gábor for input
> because you were proposing changing a convention and I thought Gábor
> had been maintaining the completion scripts.  I was not trying to say
> "Please don't do this".
>
> I was not inviting you to argue with me.

Then I'm going to ignore your arguments about bash-completion guidelines.

This is what I propose:

1) We name the new function __git_complete; this would be a temporary
name, the function would not be meant to be public
2) We discuss later what would be the namespace for public functions,
and rename, or add wrappers for them (e.g. _GIT_ps1, _GIT_complete)
3) We standardize the odd functions: __gitdir -> __git_dir

Cheers.

-- 
Felipe Contreras

Jonathan gives feedback --> flamewars inevitable? (Re: [PATCH v3] completion: add new _GIT_complete helper)

[Jonathan Nieder]

Felipe Contreras wrote:

> And I'd like to think that when you filibuster a discussion there's a
> good reason for it.

Kind other people on the list, please enlighten me.  What did I say to
trigger this crap?

Jonathan

Re: [PATCH v3] completion: add new _GIT_complete helper

[Jonathan Nieder]

Felipe Contreras wrote:

> This is what I propose:
>
> 1) We name the new function __git_complete; this would be a temporary
> name, the function would not be meant to be public
> 2) We discuss later what would be the namespace for public functions,
> and rename, or add wrappers for them (e.g. _GIT_ps1, _GIT_complete)
> 3) We standardize the odd functions: __gitdir -> __git_dir

Sounds excellent to me.

Thanks,
Jonathan

Re: Jonathan gives feedback --> flamewars inevitable? (Re: [PATCH v3] completion: add new _GIT_complete helper)

[Felipe Contreras]

On Sat, May 5, 2012 at 7:33 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Felipe Contreras wrote:
>
>> And I'd like to think that when you filibuster a discussion there's a
>> good reason for it.
>
> Kind other people on the list, please enlighten me.  What did I say to
> trigger this crap?

You said "Because our completion scripts are already using this
convention, which happens to come from bash-completion's guidelines
and here are the reasons behind those", so their guidelines are not
essential, only the reasons behind the guidelines, but:

http://article.gmane.org/gmane.comp.version-control.git/195685
http://article.gmane.org/gmane.comp.version-control.git/195689
http://article.gmane.org/gmane.comp.version-control.git/195691
http://article.gmane.org/gmane.comp.shells.bash.completion.devel/3877

You could have skipped this, apparently it was not relevant for the
discussion, it's not feedback for the patch, and the abrasiveness
unnecessary.

http://article.gmane.org/gmane.comp.version-control.git/195719
http://article.gmane.org/gmane.comp.version-control.git/195723
http://article.gmane.org/gmane.comp.version-control.git/195737
http://article.gmane.org/gmane.comp.version-control.git/195742

And more irrelevant bash-completion stuff, and then you even get angry
when I suggest those guidelines were not there, but isn't supposed to
be irrelevant?

http://article.gmane.org/gmane.comp.version-control.git/195744

And then you finally assume that because I say the guidelines were not
there, I must not like namespace conventions. I don't see how that
helps in any way.

So the discussion about whether bash-completion actually had public
API guidelines or not took basically the whole thread, and barely
anything else got discussed. And now you say whether or not they had
this guidelines is not relevant.

If you had said "You know, I think they have this guideline, but it's
not really relevant, what is relevant is X" right when the topic of
bash-completion guidelines popped up, this thread would have looked
much different.

In addition to that you are saying that I shouldn't have took all
those mails as some kind of impediment from you, just feedback, even
though you say: "you refuse to put two and two together", or "OK, you
win", or "it isn't my responsibility to waste time arguing with you"
because I counter-argue your feedback.

I honestly don't know what to think. I guess I will think three times
before replying to your feedback... hopefully you won't take offense
in my silence as well =/

Cheers.

-- 
Felipe Contreras

Re: Jonathan gives feedback --> flamewars inevitable?

[Jonathan Nieder]

Felipe Contreras wrote:

> If you had said "You know, I think they have this guideline, but it's
> not really relevant, what is relevant is X" right when the topic of
> bash-completion guidelines popped up, this thread would have looked
> much different.

Ah, apparently I was still unclear.

The leading underscore is a convention.  They do use it.  They are
thinking of moving to another convention and will probably do so.

I believe that convention is relevant to the git completion script.
Not because we should blindly follow everything the bash-completion
project does, but because there are reasons behind that convention,
following common practices means our behavior is less surprising, and
in fact I do want it to be possible to incorporate git's completion
script in the bash-completion project if that's convenient.

A little changed because their proposed future namespace does not make
it possible for outside contributors to make unofficial extensions and
then include them as something official later without change.  Maybe
that's a flaw.

When I stated what I thought was (and still think is) a fact, and you
decided that the best use of your time was to argue with me about that
instead of talking about the consequences, yes, I was annoyed.  It was
a rabbit hole I shouldn't have followed; you're right.  But I still
don't know what I should have said instead, or how to stop it from
coming up again and again.

Jonathan

Re: Jonathan gives feedback --> flamewars inevitable?

[Jonathan Nieder]

Felipe Contreras wrote:

> http://article.gmane.org/gmane.comp.version-control.git/195685
> http://article.gmane.org/gmane.comp.version-control.git/195689
> http://article.gmane.org/gmane.comp.version-control.git/195691
> http://article.gmane.org/gmane.comp.shells.bash.completion.devel/3877
>
> You could have skipped this

I'm also really confused about this.  Are you saying that trying
to figure out the bash-completion conventions was a waste of time?
It even resulted in a patch to bash-completion being applied that
improved consistency, which I thought was an attribute you valued.

Jonathan

Re: Jonathan gives feedback --> flamewars inevitable? (Re: [PATCH v3] completion: add new _GIT_complete helper)

[Tay Ray Chuan]

On Sun, May 6, 2012 at 1:33 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Felipe Contreras wrote:
>
>> And I'd like to think that when you filibuster a discussion there's a
>> good reason for it.
>
> Kind other people on the list, please enlighten me.  What did I say to
> trigger this crap?

Felipe, Jonathan is and continues to be one of the kindest, diligent
reviewers on this list. The accusation is uncalled for.

-- 
Cheers,
Ray Chuan

Re: [PATCH v3] completion: add new _GIT_complete helper

[SZEDER Gábor]

On Sat, May 05, 2012 at 10:54:23AM -0500, Jonathan Nieder wrote:
> Felipe Contreras wrote:
> 
> > Since v3:
> >
> >  * Rename to _GIT_complete to follow bash completion "guidelines"
> >  * Get rid of foo_wrap name
> 
> Thanks.  Gábor, does the "all caps _GIT_ prefix for public API
> functions" convention look like one we should adopt?  If I understand
> correctly, previously contrib/completion/git-completion.bash used
> leading double underscores for everything except completion functions,
> so this is a change.

Dunno.  I have only three concerns:

- It doesn't contaminate "my" namespace, where my installed programs,
  aliases, and shell functions are, i.e. it begins with an underscore.
- Its name conveys that it's git-specific.
- It's not called _git_complete, so the completion script (in
  particular at the end of _git()) won't misrecognize it as a
  completion function for the 'git complete' command, just in case
  somebody ever happens to have such a command or alias.

I'm not sure about the capital letters, but it fulfills all three.


> I personally would be happier with a git_complete function provided
> by another script, like this:
> 
> 	contrib/completion/git-completion.bash:
> 
> 		__git_complete () {
> 			...
> 		}
> 
> 	contrib/completion/bash-helpers.bash:
> 
> 		git_complete () {
> 			__git_complete "$@"
> 		}
> 
> One might object that if the user includes bash-helpers.bash (name is
> just a strawman) in .bashrc for interactive shells because he is
> defining some custom completion functions,
> 
> 	git<TAB>
> 
> would show the git_complete function.  I think that's fine.

It depends on what else will go into that bash-helpers.bash file.  If
I have to source it to use git completion or the git-specific bash
prompt, then I won't be very happy about it.

> Maybe
> the user would enjoy the reminder.

A reminder for what?

It's a configuration thing, so it will be used in .bashrc; I think
it's quite unlikely that it will be used interactively.


Best,
Gábor

Re: [PATCH v3] completion: add new _GIT_complete helper

[SZEDER Gábor]

Hi,


On Sat, May 05, 2012 at 05:23:20PM +0200, Felipe Contreras wrote:
> This simplifies the completions, and makes it easier to define aliases:
> 
>  _GIT_complete gf git_fetch

So, 'gf' is an alias for 'git fetch', for which the user would like to
use the completion for 'git fetch', right?  But that completion
function is caled _git_fetch(), so the underscore prefix is missing
here.

Besides, this example won't work, because the completion for 'git
fetch' uses __git_complete_remote_or_refspec(), which in turn relies
on finding out the name of the git command from the word on the
command line, and it won't be able to do that from 'gf'.

I remember we discussed this in an earlier round, and you even
suggested a possible fix (passing the command name as argument to
__git_complete_remote_or_refspec()).  I think that's the right thing
to do here.

> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
> 
> Since v3:

_This_ is v3 ;)

>  * Rename to _GIT_complete to follow bash completion "guidelines"
>  * Get rid of foo_wrap name
> 
> Since v2:
> 
>  * Remove stuff related to aliases fixes; should work on top of master
> 
>  contrib/completion/git-completion.bash |   67 ++++++++++++++------------------
>  t/t9902-completion.sh                  |    2 +-
>  2 files changed, 31 insertions(+), 38 deletions(-)
> 
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 9f56ec7..f300b87 100755
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash


> +__git_func_wrap ()

Good.

> +{
> +	if [[ -n ${ZSH_VERSION-} ]]; then
> +		emulate -L bash
> +		setopt KSH_TYPESET
> +
> +		# workaround zsh's bug that leaves 'words' as a special
> +		# variable in versions < 4.3.12
> +		typeset -h words
> +
> +		# workaround zsh's bug that quotes spaces in the COMPREPLY
> +		# array if IFS doesn't contain spaces.
> +		typeset -h IFS
> +	fi
> +	local cur words cword prev
> +	_get_comp_words_by_ref -n =: cur words cword prev
> +	__git_func "$@"

What is this "$@" for and why?  None of the _git_<cmd>() functions
take any arguments, nor does _git() and _gitk(), and AFAICT Bash won't
pass any either.

> +}
> +
> +_GIT_complete ()
> +{
> +	local name="${2-$1}"
> +	eval "$(typeset -f __git_func_wrap | sed -e "s/__git_func/_$name/")"

Still don't like the subshell and sed here ...

> +	complete -o bashdefault -o default -o nospace -F _${name}_wrap $1 2>/dev/null \
> +		|| complete -o default -o nospace -F _${name}_wrap $1
> +}
> +
> +_GIT_complete git
> +_GIT_complete gitk

... because it adds delay when the completion script is loaded.  But I
still don't have ideas how to avoid them.

Re: [PATCH v3] completion: add new _GIT_complete helper

[SZEDER Gábor]

On Sun, May 06, 2012 at 01:14:25PM +0200, SZEDER Gábor wrote:
> On Sat, May 05, 2012 at 05:23:20PM +0200, Felipe Contreras wrote:
> > This simplifies the completions, and makes it easier to define aliases:
> > 
> >  _GIT_complete gf git_fetch
> 
> So, 'gf' is an alias for 'git fetch', for which the user would like to
> use the completion for 'git fetch', right?  But that completion
> function is caled _git_fetch(), so the underscore prefix is missing
> here.
> 
> Besides, this example won't work, because the completion for 'git
> fetch' uses __git_complete_remote_or_refspec(), which in turn relies
> on finding out the name of the git command from the word on the
> command line, and it won't be able to do that from 'gf'.

I scanned the completion script for places where we iterate over the
words on the command line, i.e. for the pattern 'while.*\$cword'.

It seems that with the exception of __git_complete_remote_or_refspec()
all those places seem to be OK to be used with aliases.  They all
start the iteration at the first word on the command line ('git' or
'gf' being the nullth) so they will iterate over all relevant words in
case of aliases, too.  Perhaps this is a heritage of the dashed
commands; back then the completion script had to deal with 'git cmd'
and 'git-cmd', too.  __git_complete_remote_or_refspec() starts at the
second word, so that must be changed.


Best,
Gábor

Re: [PATCH v3] completion: add new _GIT_complete helper

[SZEDER Gábor]

On Sun, May 06, 2012 at 01:30:21PM +0200, SZEDER Gábor wrote:
> On Sun, May 06, 2012 at 01:14:25PM +0200, SZEDER Gábor wrote:
> > On Sat, May 05, 2012 at 05:23:20PM +0200, Felipe Contreras wrote:
> > > This simplifies the completions, and makes it easier to define aliases:
> > > 
> > >  _GIT_complete gf git_fetch
> > 
> > So, 'gf' is an alias for 'git fetch', for which the user would like to
> > use the completion for 'git fetch', right?  But that completion
> > function is caled _git_fetch(), so the underscore prefix is missing
> > here.
> > 
> > Besides, this example won't work, because the completion for 'git
> > fetch' uses __git_complete_remote_or_refspec(), which in turn relies
> > on finding out the name of the git command from the word on the
> > command line, and it won't be able to do that from 'gf'.
> 
> I scanned the completion script for places where we iterate over the
> words on the command line, i.e. for the pattern 'while.*\$cword'.
> 
> It seems that with the exception of __git_complete_remote_or_refspec()
> all those places seem to be OK to be used with aliases.  They all
> start the iteration at the first word on the command line ('git' or
> 'gf' being the nullth) so they will iterate over all relevant words in
> case of aliases, too.  Perhaps this is a heritage of the dashed
> commands; back then the completion script had to deal with 'git cmd'
> and 'git-cmd', too.  __git_complete_remote_or_refspec() starts at the
> second word, so that must be changed.

There is one more odd case, though: __git_config_get_set_variables()
iterates over the words on the command line backwards, i.e. starting
at the index $cword until the index of the word is greater than 1.
This means that the iteration will stop at the second word, so this
must be adjusted, too, just in case someone wants an alias for 'git
config'.

Re: [PATCH v3] completion: add new _GIT_complete helper

[SZEDER Gábor]

Hi,


On Sun, May 06, 2012 at 01:14:25PM +0200, SZEDER Gábor wrote:
> On Sat, May 05, 2012 at 05:23:20PM +0200, Felipe Contreras wrote:
> > +__git_func_wrap ()
> > +{
> > +	if [[ -n ${ZSH_VERSION-} ]]; then
> > +		emulate -L bash
> > +		setopt KSH_TYPESET
> > +
> > +		# workaround zsh's bug that leaves 'words' as a special
> > +		# variable in versions < 4.3.12
> > +		typeset -h words
> > +
> > +		# workaround zsh's bug that quotes spaces in the COMPREPLY
> > +		# array if IFS doesn't contain spaces.
> > +		typeset -h IFS
> > +	fi
> > +	local cur words cword prev
> > +	_get_comp_words_by_ref -n =: cur words cword prev
> > +	__git_func "$@"
> > +}
> > +
> > +_GIT_complete ()
> > +{
> > +	local name="${2-$1}"
> > +	eval "$(typeset -f __git_func_wrap | sed -e "s/__git_func/_$name/")"
> 
> Still don't like the subshell and sed here ...
> 
> > +	complete -o bashdefault -o default -o nospace -F _${name}_wrap $1 2>/dev/null \
> > +		|| complete -o default -o nospace -F _${name}_wrap $1
> > +}
> > +
> > +_GIT_complete git
> > +_GIT_complete gitk
> 
> ... because it adds delay when the completion script is loaded.  But I
> still don't have ideas how to avoid them.

Ok, I think I got it.  How about this on top of Felipe's patch?

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index f300b87d..8c18db92 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2688,19 +2688,19 @@ __git_func_wrap ()
 	fi
 	local cur words cword prev
 	_get_comp_words_by_ref -n =: cur words cword prev
-	__git_func "$@"
+	$1
 }
 
 _GIT_complete ()
 {
-	local name="${2-$1}"
-	eval "$(typeset -f __git_func_wrap | sed -e "s/__git_func/_$name/")"
-	complete -o bashdefault -o default -o nospace -F _${name}_wrap $1 2>/dev/null \
-		|| complete -o default -o nospace -F _${name}_wrap $1
+	local wrapper="__git_wrap_$1"
+	eval "$wrapper () { __git_func_wrap $2 ; }"
+	complete -o bashdefault -o default -o nospace -F $wrapper $1 2>/dev/null \
+		|| complete -o default -o nospace -F $wrapper $1
 }
 
-_GIT_complete git
-_GIT_complete gitk
+_GIT_complete git _git
+_GIT_complete gitk _gitk
 
 # The following are necessary only for Cygwin, and only are needed
 # when the user has tab-completed the executable name and consequently


The point is that __git_func_wrap() is not a template function
processed by _GIT_complete() (or whatever we'll end up calling it)
anymore, but simply a wrapper function around existing completion
functions.  The name of the completion function to be invoked should
be given as argument.  _GIT_complete() then uses 'eval' to create
another wrapper function to invoke __git_func_wrap() with the name of
the desired completion function.  The name of this dynamically created
wrapper function is derived from the name of the command or alias,
i.e. for 'gf' it will be __git_wrap_gf().

The overhead of the additional function call is not even measureable,
while it would spare the overhead of fork()ing a subshell and
fork()+exec()ing 'sed' twice when loading the completion script and
then for each subsequent alias.


Best,
Gábor

Re: [PATCH v3] completion: add new _GIT_complete helper

[Felipe Contreras]

[-- Attachment #1: Type: text/plain, Size: 2812 bytes --]

Hi,

On Sun, May 6, 2012 at 1:14 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:
> On Sat, May 05, 2012 at 05:23:20PM +0200, Felipe Contreras wrote:
>> This simplifies the completions, and makes it easier to define aliases:
>>
>>  _GIT_complete gf git_fetch
>
> So, 'gf' is an alias for 'git fetch', for which the user would like to
> use the completion for 'git fetch', right?  But that completion
> function is caled _git_fetch(), so the underscore prefix is missing
> here.

No, it's not missing:

       local name="${2-$1}"
       eval "$(typeset -f __git_func_wrap | sed -e "s/__git_func/_$name/")"
       complete -o bashdefault -o default -o nospace -F _${name}_wrap
$1 2>/dev/null \
               || complete -o default -o nospace -F _${name}_wrap $1

See how we add '_' before $name? There's not point in burdening the
user with adding a prefix that will _always_ be there anyway.

> Besides, this example won't work, because the completion for 'git
> fetch' uses __git_complete_remote_or_refspec(), which in turn relies
> on finding out the name of the git command from the word on the
> command line, and it won't be able to do that from 'gf'.

That's irrelevant, it's an example; replace with another command that
doesn't use 'words', and it would work.

> I remember we discussed this in an earlier round, and you even
> suggested a possible fix (passing the command name as argument to
> __git_complete_remote_or_refspec()).  I think that's the right thing
> to do here.

Yeah, but I suggested that in order to avoid the eval and the typeset
that I require for future future patches, but it turns out it's still
needed anyway, so my suggestion is to have a 'cmd' variable that
stores the command; __git_func_wrap would take the responsibility of
doing that.

>> +     __git_func "$@"
>
> What is this "$@" for and why?  None of the _git_<cmd>() functions
> take any arguments, nor does _git() and _gitk(), and AFAICT Bash won't
> pass any either.

bash's complete passes 3 arguments. They might not be used, but it
doesn't hurt to pass them either.

>> +}
>> +
>> +_GIT_complete ()
>> +{
>> +     local name="${2-$1}"
>> +     eval "$(typeset -f __git_func_wrap | sed -e "s/__git_func/_$name/")"
>
> Still don't like the subshell and sed here ...

I haven't found any other way.

As for the fix to 'git fetch' and others, I've attached a crude diff
of all the fixes I have queued together, and there it works perfectly
fine, but there's a lot of modifications required to properly traverse
the words array. I've meant to create a special function to traverse
this array, but I haven't had time time for that; I haven't even
landed this patch due to apparently irrelevant discussions.

Cheers.

-- 
Felipe Contreras

[-- Attachment #2: fc-bash-update.diff --]
[-- Type: application/octet-stream, Size: 8851 bytes --]

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 9f56ec7..ec2c3cc 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -721,10 +721,18 @@ __git_complete_revlist ()
 	__git_complete_revlist_file
 }
 
+__git_get_pos ()
+{
+	echo $(( t = cmd_pos - 2 + $1 ))
+}
+
 __git_complete_remote_or_refspec ()
 {
-	local cur_="$cur" cmd="${words[1]}"
-	local i c=2 remote="" pfx="" lhs=1 no_complete_refspec=0
+	local cur_="$cur"
+	local i c remote="" pfx="" lhs=1 no_complete_refspec=0
+
+	c=$(__git_get_pos 2)
+
 	if [ "$cmd" = "remote" ]; then
 		((c++))
 	fi
@@ -976,7 +984,8 @@ __git_aliased_command ()
 # __git_find_on_cmdline requires 1 argument
 __git_find_on_cmdline ()
 {
-	local word subcommand c=1
+	local word subcommand c
+	c=$(__git_get_pos 1)
 	while [ $c -lt $cword ]; do
 		word="${words[c]}"
 		for subcommand in $1; do
@@ -991,7 +1000,8 @@ __git_find_on_cmdline ()
 
 __git_has_doubledash ()
 {
-	local c=1
+	local c
+	c=$(__git_get_pos 1)
 	while [ $c -lt $cword ]; do
 		if [ "--" = "${words[c]}" ]; then
 			return 0
@@ -1111,8 +1121,8 @@ _git_bisect ()
 
 _git_branch ()
 {
-	local i c=1 only_local_ref="n" has_r="n"
-
+	local i c only_local_ref="n" has_r="n"
+	c=$(__git_get_pos 1)
 	while [ $c -lt $cword ]; do
 		i="${words[c]}"
 		case "$i" in
@@ -1142,16 +1152,19 @@ _git_branch ()
 
 _git_bundle ()
 {
-	local cmd="${words[2]}"
-	case "$cword" in
+	local subcommands='create list-heads verify unbundle'
+	local subcommand="$(__git_find_on_cmdline "$subcommands")"
+	local r
+	(( r = cword - cmd_pos + 2 ))
+	case "$r" in
 	2)
-		__gitcomp "create list-heads verify unbundle"
+		__gitcomp "$subcommands"
 		;;
 	3)
 		# looking for a file
 		;;
 	*)
-		case "$cmd" in
+		case "$subcommand" in
 			create)
 				__git_complete_revlist
 			;;
@@ -1427,6 +1440,7 @@ __git_match_ctag() {
 _git_grep ()
 {
 	__git_has_doubledash && return
+	local p=$(__git_get_pos 2)
 
 	case "$cur" in
 	--*)
@@ -1447,7 +1461,7 @@ _git_grep ()
 	esac
 
 	case "$cword,$prev" in
-	2,*|*,-*)
+	$p,*|*,-*)
 		if test -r tags; then
 			__gitcomp_nl "$(__git_match_ctag "$cur" tags)"
 			return
@@ -2562,7 +2576,8 @@ _git_svn ()
 
 _git_tag ()
 {
-	local i c=1 f=0
+	local i c f=0
+	c=$(__git_get_pos 1)
 	while [ $c -lt $cword ]; do
 		i="${words[c]}"
 		case "$i" in
@@ -2599,39 +2614,24 @@ _git_whatchanged ()
 	_git_log
 }
 
-_git ()
+_main_git ()
 {
-	local i c=1 command __git_dir
-
-	if [[ -n ${ZSH_VERSION-} ]]; then
-		emulate -L bash
-		setopt KSH_TYPESET
-
-		# workaround zsh's bug that leaves 'words' as a special
-		# variable in versions < 4.3.12
-		typeset -h words
-
-		# workaround zsh's bug that quotes spaces in the COMPREPLY
-		# array if IFS doesn't contain spaces.
-		typeset -h IFS
-	fi
+	local i c=1 cmd cmd_pos __git_dir
 
-	local cur words cword prev
-	_get_comp_words_by_ref -n =: cur words cword prev
 	while [ $c -lt $cword ]; do
 		i="${words[c]}"
 		case "$i" in
 		--git-dir=*) __git_dir="${i#--git-dir=}" ;;
 		--bare)      __git_dir="." ;;
-		--help) command="help"; break ;;
+		--help) cmd="help"; break ;;
 		-c) c=$((++c)) ;;
 		-*) ;;
-		*) command="$i"; break ;;
+		*) cmd="$i"; break ;;
 		esac
 		((c++))
 	done
 
-	if [ -z "$command" ]; then
+	if [ -z "$cmd" ]; then
 		case "$cur" in
 		--*)   __gitcomp "
 			--paginate
@@ -2655,34 +2655,20 @@ _git ()
 		return
 	fi
 
-	local completion_func="_git_${command//-/_}"
+	(( cmd_pos = c + 1 ))
+
+	local completion_func="_git_${cmd//-/_}"
 	declare -f $completion_func >/dev/null && $completion_func && return
 
-	local expansion=$(__git_aliased_command "$command")
+	local expansion=$(__git_aliased_command "$cmd")
 	if [ -n "$expansion" ]; then
 		completion_func="_git_${expansion//-/_}"
 		declare -f $completion_func >/dev/null && $completion_func
 	fi
 }
 
-_gitk ()
+_main_gitk ()
 {
-	if [[ -n ${ZSH_VERSION-} ]]; then
-		emulate -L bash
-		setopt KSH_TYPESET
-
-		# workaround zsh's bug that leaves 'words' as a special
-		# variable in versions < 4.3.12
-		typeset -h words
-
-		# workaround zsh's bug that quotes spaces in the COMPREPLY
-		# array if IFS doesn't contain spaces.
-		typeset -h IFS
-	fi
-
-	local cur words cword prev
-	_get_comp_words_by_ref -n =: cur words cword prev
-
 	__git_has_doubledash && return
 
 	local g="$(__gitdir)"
@@ -2703,16 +2689,42 @@ _gitk ()
 	__git_complete_revlist
 }
 
-complete -o bashdefault -o default -o nospace -F _git git 2>/dev/null \
-	|| complete -o default -o nospace -F _git git
-complete -o bashdefault -o default -o nospace -F _gitk gitk 2>/dev/null \
-	|| complete -o default -o nospace -F _gitk gitk
+foo_wrap ()
+{
+	local cmd=foo_cmd cmd_pos=1
+	if [[ -n ${ZSH_VERSION-} ]]; then
+		emulate -L bash
+		setopt KSH_TYPESET
+
+		# workaround zsh's bug that leaves 'words' as a special
+		# variable in versions < 4.3.12
+		typeset -h words
+
+		# workaround zsh's bug that quotes spaces in the COMPREPLY
+		# array if IFS doesn't contain spaces.
+		typeset -h IFS
+	fi
+	local cur words cword prev
+	_get_comp_words_by_ref -n =: cur words cword prev
+	foo "$@"
+}
+
+git_complete ()
+{
+	local name="${2-main_$1}"
+	local cmd="${name#git_}"
+	eval "$(typeset -f foo_wrap | sed -e "s/foo/_$name/" -e "s/foo_cmd/$cmd/")"
+	complete -o bashdefault -o default -o nospace -F _${name}_wrap $1 2>/dev/null \
+		|| complete -o default -o nospace -F _${name}_wrap $1
+}
+
+git_complete git
+git_complete gitk
 
 # The following are necessary only for Cygwin, and only are needed
 # when the user has tab-completed the executable name and consequently
 # included the '.exe' suffix.
 #
 if [ Cygwin = "$(uname -o 2>/dev/null)" ]; then
-complete -o bashdefault -o default -o nospace -F _git git.exe 2>/dev/null \
-	|| complete -o default -o nospace -F _git git.exe
+git_complete git.exe main_git
 fi
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index cc12732..2966a18 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -63,7 +63,7 @@ run_completion ()
 	local _cword
 	_words=( $1 )
 	(( _cword = ${#_words[@]} - 1 ))
-	_git && print_comp
+	${comp_wrap-_main_git_wrap} && print_comp
 }
 
 test_completion ()
@@ -73,6 +73,22 @@ test_completion ()
 	test_cmp expected out
 }
 
+setup_repository ()
+{
+	mkdir "$1" && (
+	cd "$1" &&
+	git init &&
+	test_tick &&
+	git commit --allow-empty -m "Initial"
+	)
+}
+
+test_expect_success 'prepare' '
+	setup_repository one &&
+	git clone one test &&
+	cd test
+'
+
 test_expect_success 'basic' '
 	run_completion "git \"\"" &&
 	# built-in
@@ -155,4 +171,71 @@ test_expect_success 'general options plus command' '
 	test_completion "git --no-replace-objects check" "checkout "
 '
 
+test_expect_success 'remote or refspec' '
+	test_completion "git fetch o" "origin " &&
+	test_completion "git fetch origin m" "master:master " &&
+	test_completion "git pull o" "origin " &&
+	test_completion "git pull origin m" "master " &&
+	test_completion "git push o" "origin " &&
+	test_completion "git push origin m" "master "
+'
+
+test_expect_success 'subcomands' '
+	test_completion "git bisect st" "start "
+'
+
+test_expect_success 'has double dash' '
+	test_completion "git add -- foo" ""
+'
+
+test_expect_success 'config' '
+	git config --file=foo color.ui auto &&
+	test_completion "git config --file=foo --get c" "color.ui "
+'
+
+test_expect_success 'other' '
+	cat >expected <<-\EOF &&
+	origin/HEAD 
+	origin/master 
+	EOF
+	test_completion "git branch -r o" &&
+	test_completion "git bundle cr" "create " &&
+
+	echo foobar > tags &&
+	test_completion "git grep f" "foobar " &&
+
+	test_completion "git notes --ref m" "master " &&
+
+	git tag v0.1 &&
+	test_completion "git tag -d v" "v0.1 "
+'
+
+test_expect_success 'global options extra checks' '
+	test_completion "git --no-pager fetch o" "origin " &&
+	test_completion "git --no-pager fetch origin m" "master:master " &&
+	test_completion "git --no-pager pull o" "origin " &&
+	test_completion "git --no-pager pull origin m" "master " &&
+	test_completion "git --no-pager push o" "origin " &&
+	test_completion "git --no-pager push origin m" "master " &&
+	test_completion "git --no-pager bisect st" "start " &&
+	test_completion "git --no-pager add -- foo" "" &&
+	test_completion "git --no-pager config --file=foo --get c" "color.ui " &&
+	cat >expected <<-\EOF &&
+	origin/HEAD 
+	origin/master 
+	EOF
+	test_completion "git --no-pager branch -r o" &&
+	test_completion "git --no-pager bundle cr" "create " &&
+	test_completion "git --no-pager grep f" "foobar " &&
+	test_completion "git --no-pager notes --ref m" "master " &&
+	test_completion "git --no-pager tag -d v" "v0.1 "
+'
+
+test_expect_success 'aliases' '
+	local comp_wrap=_git_fetch_wrap &&
+	git_complete gf git_fetch &&
+	test_completion "gf o" "origin " &&
+	test_completion "gf origin m" "master:master "
+'
+
 test_done

Re: [PATCH v3] completion: add new _GIT_complete helper

[Jonathan Nieder]

SZEDER Gábor wrote:

> It's a configuration thing, so it will be used in .bashrc; I think
> it's quite unlikely that it will be used interactively.

Yeah, makes sense.  So naming the public functions _GIT_complete
and _GIT_ps1 sounds reasonable.

Sorry, usually this kind of discussion would be easy but there is an
element of the human interaction I haven't learned to handle well yet.
Thanks for your help.

Jonathan

Re: [PATCH v3] completion: add new _GIT_complete helper

[Felipe Contreras]

On Sun, May 6, 2012 at 2:12 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:
> Hi,
>
>
> On Sun, May 06, 2012 at 01:14:25PM +0200, SZEDER Gábor wrote:
>> On Sat, May 05, 2012 at 05:23:20PM +0200, Felipe Contreras wrote:
>> > +__git_func_wrap ()
>> > +{
>> > +   if [[ -n ${ZSH_VERSION-} ]]; then
>> > +           emulate -L bash
>> > +           setopt KSH_TYPESET
>> > +
>> > +           # workaround zsh's bug that leaves 'words' as a special
>> > +           # variable in versions < 4.3.12
>> > +           typeset -h words
>> > +
>> > +           # workaround zsh's bug that quotes spaces in the COMPREPLY
>> > +           # array if IFS doesn't contain spaces.
>> > +           typeset -h IFS
>> > +   fi
>> > +   local cur words cword prev
>> > +   _get_comp_words_by_ref -n =: cur words cword prev
>> > +   __git_func "$@"
>> > +}
>> > +
>> > +_GIT_complete ()
>> > +{
>> > +   local name="${2-$1}"
>> > +   eval "$(typeset -f __git_func_wrap | sed -e "s/__git_func/_$name/")"
>>
>> Still don't like the subshell and sed here ...
>>
>> > +   complete -o bashdefault -o default -o nospace -F _${name}_wrap $1 2>/dev/null \
>> > +           || complete -o default -o nospace -F _${name}_wrap $1
>> > +}
>> > +
>> > +_GIT_complete git
>> > +_GIT_complete gitk
>>
>> ... because it adds delay when the completion script is loaded.  But I
>> still don't have ideas how to avoid them.
>
> Ok, I think I got it.  How about this on top of Felipe's patch?
>
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index f300b87d..8c18db92 100755
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -2688,19 +2688,19 @@ __git_func_wrap ()
>        fi
>        local cur words cword prev
>        _get_comp_words_by_ref -n =: cur words cword prev
> -       __git_func "$@"
> +       $1
>  }
>
>  _GIT_complete ()
>  {
> -       local name="${2-$1}"
> -       eval "$(typeset -f __git_func_wrap | sed -e "s/__git_func/_$name/")"
> -       complete -o bashdefault -o default -o nospace -F _${name}_wrap $1 2>/dev/null \
> -               || complete -o default -o nospace -F _${name}_wrap $1
> +       local wrapper="__git_wrap_$1"
> +       eval "$wrapper () { __git_func_wrap $2 ; }"
> +       complete -o bashdefault -o default -o nospace -F $wrapper $1 2>/dev/null \
> +               || complete -o default -o nospace -F $wrapper $1
>  }

This has unnecessary changes, and can be simplified this way:

diff --git a/contrib/completion/git-completion.bash
b/contrib/completion/git-completion.bash
index f300b87..dd1ff33 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2688,13 +2688,13 @@ __git_func_wrap ()
        fi
        local cur words cword prev
        _get_comp_words_by_ref -n =: cur words cword prev
-       __git_func "$@"
+       _$1
 }

 _GIT_complete ()
 {
        local name="${2-$1}"
-       eval "$(typeset -f __git_func_wrap | sed -e "s/__git_func/_$name/")"
+       eval "_${name}_wrap () { __git_func_wrap $name ; }"
        complete -o bashdefault -o default -o nospace -F _${name}_wrap
$1 2>/dev/null \
                || complete -o default -o nospace -F _${name}_wrap $1
 }

Of course, it might make sense to use the $wrapper variable, but that
increases the diff, so I avoided it for reviewing purposes. And I
still see no point forcing users to specify the full name of the
function; they should not be bothered with such details.

Cheers.

-- 
Felipe Contreras

Re: [PATCH v3] completion: add new _GIT_complete helper

[SZEDER Gábor]

Hi,


On Sun, May 06, 2012 at 10:37:06PM +0200, Felipe Contreras wrote:
> On Sun, May 6, 2012 at 1:14 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:
> > On Sat, May 05, 2012 at 05:23:20PM +0200, Felipe Contreras wrote:
> >> This simplifies the completions, and makes it easier to define aliases:
> >>
> >>  _GIT_complete gf git_fetch
> >
> > So, 'gf' is an alias for 'git fetch', for which the user would like to
> > use the completion for 'git fetch', right?  But that completion
> > function is caled _git_fetch(), so the underscore prefix is missing
> > here.
> 
> No, it's not missing:
> 
>        local name="${2-$1}"
>        eval "$(typeset -f __git_func_wrap | sed -e "s/__git_func/_$name/")"
>        complete -o bashdefault -o default -o nospace -F _${name}_wrap
> $1 2>/dev/null \
>                || complete -o default -o nospace -F _${name}_wrap $1
> 
> See how we add '_' before $name?

Indeed, the '_' is added before $name no less than three times.  How
could I have missed that?! ;)  It would be better to do it once and be
done with it.

> There's not point in burdening the
> user with adding a prefix that will _always_ be there anyway.

I don't think it's that much of a burden.  The function is called
_git_fetch, use that as second argument

> > Besides, this example won't work, because the completion for 'git
> > fetch' uses __git_complete_remote_or_refspec(), which in turn relies
> > on finding out the name of the git command from the word on the
> > command line, and it won't be able to do that from 'gf'.
> 
> That's irrelevant, it's an example; 

It's relevant; if you give an example, then at least that example
should work properly, don't you think?

> replace with another command that
> doesn't use 'words', and it would work.

That it doesn't work has nothing to do with $words.  The problem is that
__git_complete_remote_or_refspec() expects to find the git command in
${words[1]}, but in case of an alias it can't.

> > I remember we discussed this in an earlier round, and you even
> > suggested a possible fix (passing the command name as argument to
> > __git_complete_remote_or_refspec()).  I think that's the right thing
> > to do here.
> 
> Yeah, but I suggested that in order to avoid the eval and the typeset
> that I require for future future patches, but it turns out it's still
> needed anyway, so my suggestion is to have a 'cmd' variable that
> stores the command; __git_func_wrap would take the responsibility of
> doing that.

Well, now I suggest to do that to fix
__git_complete_remote_or_refspec(), because that seems to be the
easiest, cleanest, and fastest solution.

> >> +     __git_func "$@"
> >
> > What is this "$@" for and why?  None of the _git_<cmd>() functions
> > take any arguments, nor does _git() and _gitk(), and AFAICT Bash won't
> > pass any either.
> 
> bash's complete passes 3 arguments.

Oh, indeed; the first argument is the command name, the second is the
current word, and the third is the previous word.  All these are
available in our completion functions as ${words[0]}, $cur, and $prev,
respectively.

> They might not be used, but it
> doesn't hurt to pass them either.

They _are_ not used, so passing them has no benefit either.  I would
rather stick to using $cur and $prev than $2 and $3.


Best,
Gábor

Re: [PATCH v3] completion: add new _GIT_complete helper

[Felipe Contreras]

Hi,

On Mon, May 7, 2012 at 1:32 AM, SZEDER Gábor <szeder@ira.uka.de> wrote:
> On Sun, May 06, 2012 at 10:37:06PM +0200, Felipe Contreras wrote:
>> On Sun, May 6, 2012 at 1:14 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:
>> > On Sat, May 05, 2012 at 05:23:20PM +0200, Felipe Contreras wrote:
>> >> This simplifies the completions, and makes it easier to define aliases:
>> >>
>> >>  _GIT_complete gf git_fetch
>> >
>> > So, 'gf' is an alias for 'git fetch', for which the user would like to
>> > use the completion for 'git fetch', right?  But that completion
>> > function is caled _git_fetch(), so the underscore prefix is missing
>> > here.
>>
>> No, it's not missing:
>>
>>        local name="${2-$1}"
>>        eval "$(typeset -f __git_func_wrap | sed -e "s/__git_func/_$name/")"
>>        complete -o bashdefault -o default -o nospace -F _${name}_wrap
>> $1 2>/dev/null \
>>                || complete -o default -o nospace -F _${name}_wrap $1
>>
>> See how we add '_' before $name?
>
> Indeed, the '_' is added before $name no less than three times.  How
> could I have missed that?! ;)  It would be better to do it once and be
> done with it.

Sure.

>> There's not point in burdening the
>> user with adding a prefix that will _always_ be there anyway.
>
> I don't think it's that much of a burden.  The function is called
> _git_fetch, use that as second argument

Perhaps not, but why are you arguing for making users' life more
difficult? Even if it's just a little.

In fact, it could be even simpler:

_GIT_complete gf fetch

>> > Besides, this example won't work, because the completion for 'git
>> > fetch' uses __git_complete_remote_or_refspec(), which in turn relies
>> > on finding out the name of the git command from the word on the
>> > command line, and it won't be able to do that from 'gf'.
>>
>> That's irrelevant, it's an example;
>
> It's relevant; if you give an example, then at least that example
> should work properly, don't you think?

It does work... on my branch.

If you have another example that works, feel free to suggest it, but
I'm going to remove that message, along with _GIT_complete (and
replace it with __git_complete and a note that it's not public API),
because I'm tired of trying to make users' life easier; I just want
the patch in.

>> replace with another command that
>> doesn't use 'words', and it would work.
>
> That it doesn't work has nothing to do with $words.  The problem is that
> __git_complete_remote_or_refspec() expects to find the git command in
> ${words[1]}, but in case of an alias it can't.

${words[1]} is part of $words. And BTW, git bundle also fails similarly.

And then, even if you fetch the command properly,
__git_complete_remote_or_refspec will still fail because it would
assume the remote is 'git'.

Also BTW, git fetch is already broken anyway:

 git --no-pager fetch <TAB>

So don't blame my patch :)

>> > I remember we discussed this in an earlier round, and you even
>> > suggested a possible fix (passing the command name as argument to
>> > __git_complete_remote_or_refspec()).  I think that's the right thing
>> > to do here.
>>
>> Yeah, but I suggested that in order to avoid the eval and the typeset
>> that I require for future future patches, but it turns out it's still
>> needed anyway, so my suggestion is to have a 'cmd' variable that
>> stores the command; __git_func_wrap would take the responsibility of
>> doing that.
>
> Well, now I suggest to do that to fix
> __git_complete_remote_or_refspec(), because that seems to be the
> easiest, cleanest, and fastest solution.

That's not enough to make 'git fetch' work, try it.

>> >> +     __git_func "$@"
>> >
>> > What is this "$@" for and why?  None of the _git_<cmd>() functions
>> > take any arguments, nor does _git() and _gitk(), and AFAICT Bash won't
>> > pass any either.
>>
>> bash's complete passes 3 arguments.
>
> Oh, indeed; the first argument is the command name, the second is the
> current word, and the third is the previous word.  All these are
> available in our completion functions as ${words[0]}, $cur, and $prev,
> respectively.

Yeah, but that's still what bash does, and I saw no reason to change it.

>> They might not be used, but it
>> doesn't hurt to pass them either.
>
> They _are_ not used, so passing them has no benefit either.  I would
> rather stick to using $cur and $prev than $2 and $3.

Sure, but it was just 4 more characters that didn't hurt anybody. Your
version makes passing those arguments more difficult, so I see no need
to try to implement that.

Cheers.

-- 
Felipe Contreras

Re: [PATCH v3] completion: add new _GIT_complete helper

[SZEDER Gábor]

Hi,


On Mon, May 07, 2012 at 02:20:54AM +0200, Felipe Contreras wrote:
> In fact, it could be even simpler:
> 
> _GIT_complete gf fetch

That would be even better; just need to take care of 'cherry-pick' and
the like.

> >> > Besides, this example won't work, because the completion for 'git
> >> > fetch' uses __git_complete_remote_or_refspec(), which in turn relies
> >> > on finding out the name of the git command from the word on the
> >> > command line, and it won't be able to do that from 'gf'.

> It does work... on my branch.

I though the patch was supposed to be applied on git.git's master.  I
didn't know what else you had on your branch, obviously.

> If you have another example that works, feel free to suggest it

I played around with 'gb' as 'git branch' and 'gc' as 'git checkout',
they seemed to work properly, and they both use $words directly or
indirectly.  I suspect most of them Just Works, except those using
__git_complete_remote_or_refspec(), and _git_bundle() as you mention
below, and perhaps a few surprises we hadn't spotted yet.

> >> replace with another command that
> >> doesn't use 'words', and it would work.
> >
> > That it doesn't work has nothing to do with $words.  The problem is that
> > __git_complete_remote_or_refspec() expects to find the git command in
> > ${words[1]}, but in case of an alias it can't.
> 
> ${words[1]} is part of $words.

The problem is not with $words per se, but with the '1'.

> And BTW, git bundle also fails similarly.

Indeed; using __git_find_on_cmdline() and checking which subcommand it
found, if any, would allow us to get rid of all numbers from that
function.

> Also BTW, git fetch is already broken anyway:
> 
>  git --no-pager fetch <TAB>
> 
> So don't blame my patch :)

Yeah, I know, but that's broken since "forever".  However, I think
that would be a separate, though related topic, because it's not
required to make completion for alias work.

> >> > I remember we discussed this in an earlier round, and you even
> >> > suggested a possible fix (passing the command name as argument to
> >> > __git_complete_remote_or_refspec()).  I think that's the right thing
> >> > to do here.

> That's not enough to make 'git fetch' work, try it.

As already mentioned it earlier in this thread, the iteration over
$words in __git_complete_remote_or_refspec() have to be fixed, too.

> >> >> +     __git_func "$@"

> Sure, but it was just 4 more characters that didn't hurt anybody. Your
> version makes passing those arguments more difficult, so I see no need
> to try to implement that.

OK.  I doesn't hurt, but I think it's more important to avoid
fork()+exec() overheads than to pass arguments whose values are
readily available in other variables anyway.


Gábor

Re: Jonathan gives feedback --> flamewars inevitable? (Re: [PATCH v3] completion: add new _GIT_complete helper)

[Felipe Contreras]

On Sun, May 6, 2012 at 7:23 AM, Tay Ray Chuan <rctay89@gmail.com> wrote:
> On Sun, May 6, 2012 at 1:33 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>> Felipe Contreras wrote:
>>
>>> And I'd like to think that when you filibuster a discussion there's a
>>> good reason for it.
>>
>> Kind other people on the list, please enlighten me.  What did I say to
>> trigger this crap?
>
> Felipe, Jonathan is and continues to be one of the kindest, diligent
> reviewers on this list. The accusation is uncalled for.

It's not an accusation; Jonathan's comments did prevent the patches to
move forward, which is a good thing if there's a good reason for that.
That's the whole point of review.

Cheers.

-- 
Felipe Contreras