[PATCH 0/2] completion: backwards compatibility fix

[PATCH 0/2] completion: backwards compatibility fix

[Felipe Contreras]

Hi,

These two patches are meant to fix the backwards compatibility of _git, and
_gitk. This also helps my zsh's wrapper.

Felipe Contreras (2):
  completion: rename _git and _gitk
  completion: add support for backwards compatibilit

 contrib/completion/git-completion.bash |   22 +++++++++++++++++-----
 t/t9902-completion.sh                  |    2 +-
 2 files changed, 18 insertions(+), 6 deletions(-)

-- 
1.7.10.2

[PATCH 1/2] completion: rename _git and _gitk

[Felipe Contreras]

Would be useful to provide backwards compatibility for _git. Also, zsh
completion uses _git, and it cannot be changed.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 contrib/completion/git-completion.bash |   10 +++++-----
 t/t9902-completion.sh                  |    2 +-
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index cd92322..3cb106e 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2597,7 +2597,7 @@ _git_whatchanged ()
 	_git_log
 }
 
-_git ()
+_main_git ()
 {
 	local i c=1 command __git_dir
 
@@ -2648,7 +2648,7 @@ _git ()
 	fi
 }
 
-_gitk ()
+_main_gitk ()
 {
 	__git_has_doubledash && return
 
@@ -2700,13 +2700,13 @@ __git_complete ()
 		|| complete -o default -o nospace -F $wrapper $1
 }
 
-__git_complete git _git
-__git_complete gitk _gitk
+__git_complete git _main_git
+__git_complete gitk _main_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
-__git_complete git.exe _git
+__git_complete git.exe _main_git
 fi
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 0f09fd6..9a80c60 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_wrap_git && print_comp
+	__git_wrap_main_git && print_comp
 }
 
 test_completion ()
-- 
1.7.10.2

Re: [PATCH 1/2] completion: rename _git and _gitk

[SZEDER Gábor]

Hi,


On Sat, May 19, 2012 at 04:41:34AM +0200, Felipe Contreras wrote:
> Would be useful to provide backwards compatibility for _git. Also, zsh
> completion uses _git, and it cannot be changed.
> 
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  contrib/completion/git-completion.bash |   10 +++++-----
>  t/t9902-completion.sh                  |    2 +-
>  2 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index cd92322..3cb106e 100755
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -2597,7 +2597,7 @@ _git_whatchanged ()
>  	_git_log
>  }
>  
> -_git ()
> +_main_git ()

> -_gitk ()
> +_main_gitk ()

After all those namespace discussions the names of these functions
should start with _git or __git.


Gábor

Re: [PATCH 1/2] completion: rename _git and _gitk

[Junio C Hamano]

SZEDER Gábor <szeder@ira.uka.de> writes:

>> -_git ()
>> +_main_git ()
>
>> -_gitk ()
>> +_main_gitk ()
>
> After all those namespace discussions the names of these functions
> should start with _git or __git.

As these are pure implementation internal details that are called from
very limited places, I do not think Felipe minds a patch to update them.

Personally I would find _git_git or __git_git even more mysterious and
begs one-liner comments above their definition, though.

Thanks.

Re: [PATCH 1/2] completion: rename _git and _gitk

[Felipe Contreras]

On Tue, May 22, 2012 at 8:00 PM, Junio C Hamano <gitster@pobox.com> wrote:
> SZEDER Gábor <szeder@ira.uka.de> writes:
>
>>> -_git ()
>>> +_main_git ()
>>
>>> -_gitk ()
>>> +_main_gitk ()
>>
>> After all those namespace discussions the names of these functions
>> should start with _git or __git.
>
> As these are pure implementation internal details that are called from
> very limited places, I do not think Felipe minds a patch to update them.

I don't mind updating these to whatever you decide is the namespace
for internal functions, just avoid _git ().

Cheers.

-- 
Felipe Contreras

[PATCH 2/2] completion: add support for backwards compatibility

[Felipe Contreras]

Some people might be relying on _git and _gitk to define custom aliases,
unfortunately, commit 6b179ad (completion: add new __git_complete
helper) broke that support.

  "bash: [: 1: unary operator expected"

This can be easily fixed by using __git_complete, but it's not meant to
be public.

Although _git and _gitk are probably not meant to be public, it's easy
to keep having support for them by having a wrapper to the proper
new function that is fully functional.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 contrib/completion/git-completion.bash |   12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 3cb106e..1689f99 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2700,6 +2700,18 @@ __git_complete ()
 		|| complete -o default -o nospace -F $wrapper $1
 }
 
+# wrapper for backwards compatibility
+_git ()
+{
+	__git_wrap_main_git
+}
+
+# wrapper for backwards compatibility
+_gitk ()
+{
+	__git_wrap_main_gitk
+}
+
 __git_complete git _main_git
 __git_complete gitk _main_gitk
 
-- 
1.7.10.2

Re: [PATCH 0/2] completion: backwards compatibility fix

[Jeff King]

On Sat, May 19, 2012 at 04:41:33AM +0200, Felipe Contreras wrote:

> These two patches are meant to fix the backwards compatibility of _git, and
> _gitk. This also helps my zsh's wrapper.
>
> Felipe Contreras (2):
>   completion: rename _git and _gitk
>   completion: add support for backwards compatibilit

Thanks. This looks like the obviously correct solution. Even if we end
up with a public _GIT_complete or whatever, the backwards-compatibility
is worth it.

-Peff

Re: [PATCH 0/2] completion: backwards compatibility fix

[Carsten Mattner]

On Sat, May 19, 2012 at 4:45 AM, Jeff King <peff@peff.net> wrote:
> On Sat, May 19, 2012 at 04:41:33AM +0200, Felipe Contreras wrote:
>
>> These two patches are meant to fix the backwards compatibility of _git, and
>> _gitk. This also helps my zsh's wrapper.
>>
>> Felipe Contreras (2):
>>   completion: rename _git and _gitk
>>   completion: add support for backwards compatibilit
>
> Thanks. This looks like the obviously correct solution. Even if we end
> up with a public _GIT_complete or whatever, the backwards-compatibility
> is worth it.

Cool. Just though the patches now. Felipe, which variant should I use
to define _main_git completion for the alias g with the 2 patches
applied to git_completion.bash? Using bash-4.2.28.

Re: [PATCH 0/2] completion: backwards compatibility fix

[Jeff King]

On Sat, May 19, 2012 at 09:54:28AM +0200, Carsten Mattner wrote:

> >> Felipe Contreras (2):
> >>   completion: rename _git and _gitk
> >>   completion: add support for backwards compatibilit
> >
> > Thanks. This looks like the obviously correct solution. Even if we end
> > up with a public _GIT_complete or whatever, the backwards-compatibility
> > is worth it.
> 
> Cool. Just though the patches now. Felipe, which variant should I use
> to define _main_git completion for the alias g with the 2 patches
> applied to git_completion.bash? Using bash-4.2.28.

I think the point of the patches is that you can continue to use your
same "complete ... -F _git g" line as always (with these patches you
could also say "__git_complete g _main_git", but that is not backwards
compatible to older versions, if you use the same bashrc on multiple
machines).

-Peff

Re: [PATCH 0/2] completion: backwards compatibility fix

[Carsten Mattner]

On Sat, May 19, 2012 at 5:31 PM, Jeff King <peff@peff.net> wrote:
> On Sat, May 19, 2012 at 09:54:28AM +0200, Carsten Mattner wrote:
>
>> >> Felipe Contreras (2):
>> >>   completion: rename _git and _gitk
>> >>   completion: add support for backwards compatibilit
>> >
>> > Thanks. This looks like the obviously correct solution. Even if we end
>> > up with a public _GIT_complete or whatever, the backwards-compatibility
>> > is worth it.
>>
>> Cool. Just though the patches now. Felipe, which variant should I use
>> to define _main_git completion for the alias g with the 2 patches
>> applied to git_completion.bash? Using bash-4.2.28.
>
> I think the point of the patches is that you can continue to use your
> same "complete ... -F _git g" line as always (with these patches you
> could also say "__git_complete g _main_git", but that is not backwards
> compatible to older versions, if you use the same bashrc on multiple
> machines).

Great, is that short form always enough with the patches applied
and the non-internal function to use for future compatibility?

Re: [PATCH 0/2] completion: backwards compatibility fix

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> These two patches are meant to fix the backwards compatibility of _git, and
> _gitk.

Thanks.

Even though I think these two should be squashed into one "fix regression
that made _git and _gitk behave differently" commit (because otherwise the
result of applying the first one makes _git/_gitk disappear, shifting the
breakage from "older usage fails inside _git" to "older usage fails to
even find _git", the end result looks good.

Re: [PATCH 0/2] completion: backwards compatibility fix

[Carsten Mattner]

On Sat, May 19, 2012 at 4:41 AM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> Hi,
>
> These two patches are meant to fix the backwards compatibility of _git, and
> _gitk. This also helps my zsh's wrapper.
>
> Felipe Contreras (2):
>  completion: rename _git and _gitk
>  completion: add support for backwards compatibilit
>
>  contrib/completion/git-completion.bash |   22 +++++++++++++++++-----
>  t/t9902-completion.sh                  |    2 +-
>  2 files changed, 18 insertions(+), 6 deletions(-)
>
> --
> 1.7.10.2
>

Thanks Felipe. With the 2 patches applied all the following
completion definitions work.

complete -o bashdefault -o default -o nospace -F _git g 2>/dev/null \
   || complete -o default -o nospace -F _git g
__git_complete g _git
__git_complete g _main_git

Ignoring backwards compatibility, which one would you suggest
I use? Keep in mind that I originally copied the first
long completion defintion from git-completion.bash a long time
ago.

Re: [PATCH 0/2] completion: backwards compatibility fix

[Felipe Contreras]

On Sun, May 20, 2012 at 11:02 AM, Carsten Mattner
<carstenmattner@googlemail.com> wrote:
> On Sat, May 19, 2012 at 4:41 AM, Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
>> Hi,
>>
>> These two patches are meant to fix the backwards compatibility of _git, and
>> _gitk. This also helps my zsh's wrapper.
>>
>> Felipe Contreras (2):
>>  completion: rename _git and _gitk
>>  completion: add support for backwards compatibilit
>>
>>  contrib/completion/git-completion.bash |   22 +++++++++++++++++-----
>>  t/t9902-completion.sh                  |    2 +-
>>  2 files changed, 18 insertions(+), 6 deletions(-)
>>
>> --
>> 1.7.10.2
>>
>
> Thanks Felipe. With the 2 patches applied all the following
> completion definitions work.
>
> complete -o bashdefault -o default -o nospace -F _git g 2>/dev/null \
>   || complete -o default -o nospace -F _git g
> __git_complete g _git
> __git_complete g _main_git
>
> Ignoring backwards compatibility, which one would you suggest
> I use? Keep in mind that I originally copied the first
> long completion defintion from git-completion.bash a long time
> ago.

I would use '__git_complete g _git', but keep in mind that none of
these forms have a promise to stay. Eventually the final one would be
'_GIT_complete g _git' (hopefully), but if you want to be safe from
changes maybe the first (original) would be best; even though there's
no promise it won't break, we would probably try our best not to break
it (again).

BTW. If your system has the '-o bashdefault' option you probably don't
need the full form, just:
% complete -o bashdefault -o default -o nospace -F _git g

Or if your system doesn't:
% complete -o default -o nospace -F _git g

Cheers.

-- 
Felipe Contreras

Re: [PATCH 0/2] completion: backwards compatibility fix

[Carsten Mattner]

On Sun, May 20, 2012 at 12:28 PM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> On Sun, May 20, 2012 at 11:02 AM, Carsten Mattner
> <carstenmattner@googlemail.com> wrote:
>> On Sat, May 19, 2012 at 4:41 AM, Felipe Contreras
>> <felipe.contreras@gmail.com> wrote:
>>> Hi,
>>>
>>> These two patches are meant to fix the backwards compatibility of _git, and
>>> _gitk. This also helps my zsh's wrapper.
>>>
>>> Felipe Contreras (2):
>>>  completion: rename _git and _gitk
>>>  completion: add support for backwards compatibilit
>>>
>>>  contrib/completion/git-completion.bash |   22 +++++++++++++++++-----
>>>  t/t9902-completion.sh                  |    2 +-
>>>  2 files changed, 18 insertions(+), 6 deletions(-)
>>>
>>> --
>>> 1.7.10.2
>>>
>>
>> Thanks Felipe. With the 2 patches applied all the following
>> completion definitions work.
>>
>> complete -o bashdefault -o default -o nospace -F _git g 2>/dev/null \
>>   || complete -o default -o nospace -F _git g
>> __git_complete g _git
>> __git_complete g _main_git
>>
>> Ignoring backwards compatibility, which one would you suggest
>> I use? Keep in mind that I originally copied the first
>> long completion defintion from git-completion.bash a long time
>> ago.
>
> I would use '__git_complete g _git', but keep in mind that none of
> these forms have a promise to stay. Eventually the final one would be
> '_GIT_complete g _git' (hopefully), but if you want to be safe from
> changes maybe the first (original) would be best; even though there's
> no promise it won't break, we would probably try our best not to break
> it (again).

What about adding a small note at the top of the completion script
together with the other documentation? Does it make sense to keep
the known to work and public invocation in there?

> BTW. If your system has the '-o bashdefault' option you probably don't
> need the full form, just:
> % complete -o bashdefault -o default -o nospace -F _git g
>
> Or if your system doesn't:
> % complete -o default -o nospace -F _git g

I like the short versions. They seem to work.