[PATCH v2] completion: add new git_complete helper

[PATCH v2] 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 v2:

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

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

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 31f714d..abb4ccb 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
@@ -2663,22 +2648,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)"
@@ -2699,16 +2668,41 @@ _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 ()
+{
+	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-$1}"
+	eval "$(typeset -f foo_wrap | sed -e "s/foo/_$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 7bd37f5..0f1a9ec 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 v2] completion: add new git_complete helper

[Jonathan Nieder]

Felipe Contreras wrote:

> This simplifies the completions, and makes it easier to define aliases:
>
>  git_complete gf git_fetch

Sounds neat.  Unfortunately users could already be using a function
with some other purpose named git_complete in their .profile and this
would override it.  The completion script has so far stuck to a
limited namespace:

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

A name like __git_complete should work, presumably.

Hope that helps,
Jonathan

Re: [PATCH v2] completion: add new git_complete helper

[Felipe Contreras]

On Mon, Apr 16, 2012 at 12:37 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Felipe Contreras wrote:
>
>> This simplifies the completions, and makes it easier to define aliases:
>>
>>  git_complete gf git_fetch
>
> Sounds neat.  Unfortunately users could already be using a function
> with some other purpose named git_complete in their .profile and this
> would override it.  The completion script has so far stuck to a
> limited namespace:
>
>        _git_*  (completion functions)
>        __git_* (everything else, including public interfaces like __git_ps1)
>
> A name like __git_complete should work, presumably.

Perhaps it's time to avoid the __ prefix for public interfaces;
otherwise how would people know they are public?

-- 
Felipe Contreras

Re: [PATCH v2] completion: add new git_complete helper

[Junio C Hamano]

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

> On Mon, Apr 16, 2012 at 12:37 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>> The completion script has so far stuck to a limited namespace:
>>
>>        _git_*  (completion functions)
>>        __git_* (everything else, including public interfaces like __git_ps1)
>>
>> A name like __git_complete should work, presumably.
>
> Perhaps it's time to avoid the __ prefix for public interfaces;
> otherwise how would people know they are public?

We could do the same as __git_ps1, no?  People know to use it already;
they know it is public.

I am OK with introducing git_ps1 while keeping __git_ps1 as an equivalent
and declare that git_$anything will be the surface interface for end users
to *use* the machinery we provide, though.  Then git_complete could be
introduced without __git_complete equivalent.  Probably _git_$name has to
stay an implementation detail (i.e. the users can use it but it is their
responsibility to update their using script when the implementation
changes).

Re: [PATCH v2] completion: add new git_complete helper

[Jonathan Nieder]

Junio C Hamano wrote:

> I am OK with introducing git_ps1 while keeping __git_ps1 as an equivalent
> and declare that git_$anything will be the surface interface for end users
> to *use* the machinery we provide, though.

The problem is that completion scripts are often included implicitly
in .profile through the bash completion machinery.  Users do not have
to explicitly ask for them, so there is no reason for users to expect
that the function name "more_cowbell" is dangerous to use in .profile
because some day the completion script for the "more" command will
start using it.

Because of these considerations, the convention is that every
identifier provided by a completion script, including public ones,
starts with an underscore.

Re: [PATCH v2] completion: add new git_complete helper

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Because of these considerations, the convention is that every
> identifier provided by a completion script, including public ones,
> starts with an underscore.

OK.

Re: [PATCH v2] completion: add new git_complete helper

[Felipe Contreras]

On Mon, Apr 16, 2012 at 7:07 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Junio C Hamano wrote:
>
>> I am OK with introducing git_ps1 while keeping __git_ps1 as an equivalent
>> and declare that git_$anything will be the surface interface for end users
>> to *use* the machinery we provide, though.
>
> The problem is that completion scripts are often included implicitly
> in .profile through the bash completion machinery.  Users do not have
> to explicitly ask for them, so there is no reason for users to expect
> that the function name "more_cowbell" is dangerous to use in .profile
> because some day the completion script for the "more" command will
> start using it.

Sure, that's why we would have _git_cowbell instead, so for *most* of
functions the user would have no trouble, it would *only* be
git_completion the one without prefix, and I think the name is quite
safe. There's quite likely few, probably nobody, actually using that
function name in their profiles.

Also, the script would now be loaded only *after*, the user types 'git
<TAB>'. And there's already a bunch of functions that are already
exported by just having bash completion: have(), quote(), dequote(),
quote_readline(). And there's at least one script that uses a function
without a prefix: ri_get_methods()

I would like to see a completion script that actually has a function
supposed to be exported and that still uses the _ prefix anyway.

Cheers.

-- 
Felipe Contreras

Re: [PATCH v2] completion: add new git_complete helper

[Jonathan Nieder]

Felipe Contreras wrote:

> I would like to see a completion script that actually has a function
> supposed to be exported and that still uses the _ prefix anyway.

The /etc/bash_completion library itself exports lots and lots of
functions with a _ prefix.

Re: [PATCH v2] completion: add new git_complete helper

[Felipe Contreras]

On Mon, Apr 16, 2012 at 11:09 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Felipe Contreras wrote:
>
>> I would like to see a completion script that actually has a function
>> supposed to be exported and that still uses the _ prefix anyway.
>
> The /etc/bash_completion library itself exports lots and lots of
> functions with a _ prefix.

We are not making a bash_completion library; I mean a bash completion
script (other than the library).

-- 
Felipe Contreras

Re: [PATCH v2] completion: add new git_complete helper

[Jonathan Nieder]

Felipe Contreras wrote:
> On Mon, Apr 16, 2012 at 11:09 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>> Felipe Contreras wrote:

>>> I would like to see a completion script that actually has a function
>>> supposed to be exported and that still uses the _ prefix anyway.
>>
>> The /etc/bash_completion library itself exports lots and lots of
>> functions with a _ prefix.
>
> We are not making a bash_completion library; I mean a bash completion
> script (other than the library).

Ok.  If you refuse to put two and two together, then I will (as usual
when this happens) just be a little passive aggressive and annoyed and
let you talk to other people.

Cheers,
Jonathan

Re: [PATCH v2] completion: add new git_complete helper

[Felipe Contreras]

On Mon, Apr 16, 2012 at 11:33 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Felipe Contreras wrote:
>> On Mon, Apr 16, 2012 at 11:09 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>>> Felipe Contreras wrote:
>
>>>> I would like to see a completion script that actually has a function
>>>> supposed to be exported and that still uses the _ prefix anyway.
>>>
>>> The /etc/bash_completion library itself exports lots and lots of
>>> functions with a _ prefix.
>>
>> We are not making a bash_completion library; I mean a bash completion
>> script (other than the library).
>
> Ok.  If you refuse to put two and two together, then I will (as usual
> when this happens) just be a little passive aggressive and annoyed and
> let you talk to other people.

Just to be clear; we are making a completion script for the 'git'
command, so if we are looking for guidance on what other 'foo' command
completion scripts do, the bash completion library itself is not good
guidance.

My gut feeling is that there's no completion script that exports
functions meant to be used directly by the user, so there's really no
guidance for this.

Now, even if you use the bash completion library, it still does export
functions without a prefix, so why take the ones with prefix as a
rule, and ignore the other ones? It seems like there's no real
guideline for what gets a prefix and what doesn't, even in the bash
completion library itself, which I don't think should be used a
guideline anyway.

The closest relative of 'git_complete' would be 'complete', both of
course are meant for public usage. I have not seen any other functions
that are similar.

Cheers.

-- 
Felipe Contreras

Re: [PATCH v2] completion: add new git_complete helper

[Jonathan Nieder]

Felipe Contreras wrote:

> Now, even if you use the bash completion library, it still does export
> functions without a prefix

Are you sure?  "complete" is a bash builtin and has nothing to do with
the bash completion library except that the latter uses it.

Re: [PATCH v2] completion: add new git_complete helper

[Felipe Contreras]

On Mon, Apr 16, 2012 at 11:46 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Felipe Contreras wrote:
>
>> Now, even if you use the bash completion library, it still does export
>> functions without a prefix
>
> Are you sure?  "complete" is a bash builtin and has nothing to do with
> the bash completion library except that the latter uses it.

I already provided examples:
have(), quote(), dequote(), quote_readline()

-- 
Felipe Contreras

Re: [PATCH v2] completion: add new git_complete helper

[Jonathan Nieder]

(cc-ing the bash-completion-devel list)
Felipe Contreras wrote:
> On Mon, Apr 16, 2012 at 11:46 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>> Felipe Contreras wrote:

>>> Now, even if you use the bash completion library, it still does export
>>> functions without a prefix
>>
>> Are you sure?  "complete" is a bash builtin and has nothing to do with
>> the bash completion library except that the latter uses it.
>
> I already provided examples:
> have(), quote(), dequote(), quote_readline()

Ah, that's what you mean.  Thanks for the pointers, and sorry to have
misunderstood.

There's a little oddity here.  "have" is clearly an unwanted backward
compatibility feature:

	# @deprecated should no longer be used; generally not needed with dynamically
	#             loaded completions, and _have is suitable for runtime use.
	have()
	[...]
	unset -f have
	unset have

But "quote", "dequote", and "quote_readline" do not get the same
treatment.  Perhaps they are for backward compatibility, too, but are
so widely used that there is no hope of ever getting rid of them.

Hopefully this information helps clarify to what extent the leading
underscores in functions exposed by completion scripts are meant or
are not meant as a convention.

Jonathan

_______________________________________________
Bash-completion-devel mailing list
Bash-completion-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/bash-completion-devel

Re: [PATCH v2] completion: add new git_complete helper

[Ville Skyttä]

On 2012-04-16 23:59, Jonathan Nieder wrote:

> Hopefully this information helps clarify to what extent the leading
> underscores in functions exposed by completion scripts are meant or
> are not meant as a convention.

We've discussed what a real "API" or "namespace" of bash-completion
would look like, but so far nothing concrete has come out of it.

http://thread.gmane.org/gmane.comp.shells.bash.completion.scm/2013/focus=3135

Re: [Bash-completion-devel] [PATCH v2] completion: add new git_complete helper

[Felipe Contreras]

On Sat, Apr 21, 2012 at 10:20 AM, Ville Skyttä <ville.skytta@iki.fi> wrote:
> On 2012-04-16 23:59, Jonathan Nieder wrote:
>
>> Hopefully this information helps clarify to what extent the leading
>> underscores in functions exposed by completion scripts are meant or
>> are not meant as a convention.
>
> We've discussed what a real "API" or "namespace" of bash-completion
> would look like, but so far nothing concrete has come out of it.
>
> http://thread.gmane.org/gmane.comp.shells.bash.completion.scm/2013/focus=3135

Thank you for pointing this out. This means I was correct; there
was/is no convention for public APIs.

According to that thread, the closest there is to a convention would
be to name it _GIT_complete. That would certainly avoid conflicts with
any current namespace, so I feel it's much better than __git_complete.

Still, I don't see the point in avoiding 'git_complete' and making our
lifes more difficult. Bash public functions, like *complete*, don't
have any special namespace, they just snatch them, and that's the end
of it. In the particular case of git, where would have only a couple
(currently 2) public functions, I don't see what's the big deal.

Cheers.

-- 
Felipe Contreras

Re: [Bash-completion-devel] [PATCH v2] completion: add new git_complete helper

[Ville Skyttä]

On 2012-04-21 18:41, Felipe Contreras wrote:

> Still, I don't see the point in avoiding 'git_complete' and making our
> lifes more difficult.

I'm not aware of ways it'd make people's lifes more difficult, but if
git_complete is a function intended for completion purposes, I'd
personally not name it git* because it'd interfere for example with
completing git<TAB> by being included in the suggested completions even
though it's never meant to be used that way.  Prefixing for example with
underscore doesn't avoid the problem completely, but makes it less
likely to happen.

Re: [Bash-completion-devel] [PATCH v2] completion: add new git_complete helper

[Felipe Contreras]

2012/4/21 Ville Skyttä <ville.skytta@iki.fi>:
> On 2012-04-21 18:41, Felipe Contreras wrote:
>
>> Still, I don't see the point in avoiding 'git_complete' and making our
>> lifes more difficult.
>
> I'm not aware of ways it'd make people's lifes more difficult, but if
> git_complete is a function intended for completion purposes, I'd
> personally not name it git* because it'd interfere for example with
> completing git<TAB> by being included in the suggested completions even
> though it's never meant to be used that way.  Prefixing for example with
> underscore doesn't avoid the problem completely, but makes it less
> likely to happen.

That's actually a good point, I forgot that functions are also completed.

Cheers.

-- 
Felipe Contreras

Re: [PATCH v2] completion: add new git_complete helper

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Felipe Contreras wrote:
>> On Mon, Apr 16, 2012 at 11:09 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>>> Felipe Contreras wrote:
>
>>>> I would like to see a completion script that actually has a function
>>>> supposed to be exported and that still uses the _ prefix anyway.
>>>
>>> The /etc/bash_completion library itself exports lots and lots of
>>> functions with a _ prefix.
>>
>> We are not making a bash_completion library; I mean a bash completion
>> script (other than the library).
>
> Ok.  If you refuse to put two and two together, then I will (as usual
> when this happens) just be a little passive aggressive and annoyed and
> let you talk to other people.

I have been assuming that bash_completion library is a sort of metaproject
whose sole purpose is to supply a framework for completion scripts for
other packages like Git to be plugged easily and give niceties like lazy
loading to avoid inflicting undue latencies to the end users.

Earlier I said "Ok" to you based on that assumption; somebody may have his
own git_complete that rebuilds a test integration branch (i.e. completes
it) and having the name git_complete exported by us will indirectly affect
her fingers if she installs bash_completion and Git package on her system.

But that assumption may not be correct, or even if it is correct, Felipe
may be missing that part, so it may not be "refusing to add two and two",
but could be "learned two, but do not know the number of the other side of
addition".

Re: [PATCH v2] completion: add new git_complete helper

[Felipe Contreras]

On Mon, Apr 16, 2012 at 11:51 PM, Junio C Hamano <gitster@pobox.com> wrote:

> Earlier I said "Ok" to you based on that assumption; somebody may have his
> own git_complete that rebuilds a test integration branch (i.e. completes
> it) and having the name git_complete exported by us will indirectly affect
> her fingers if she installs bash_completion and Git package on her system.

She might have had the same problem if she named it _git_complete.
That might be unlinkely, but I also find it unlikely that anybody
would have a git_complete function. In fact, I find it unlikely that
people would write shell functions like that; it's much easier to
write scripts and put them in $PATH.

Even more, I just added a foobar() function in my profile, and I also
added a foobar() function in /etc/bash_completion.d/git. I don't see
my function replaced in any way, even after typing and completing
'git' commands. I don't know how that's possible, but that's why I
don't like to take these types of claims as face value.

Cheers.

-- 
Felipe Contreras

Re: [PATCH v2] completion: add new git_complete helper

[Jonathan Nieder]

Felipe Contreras wrote:

> Even more, I just added a foobar() function in my profile, and I also
> added a foobar() function in /etc/bash_completion.d/git. I don't see
> my function replaced in any way, even after typing and completing
> 'git' commands. I don't know how that's possible, but that's why I
> don't like to take these types of claims as face value.

What version of the bash_completion library do you use?  Lazy-loading
was introduced in version 1.90.

If you put

	foobar() {
		echo hi
	}
	. /etc/bash_completion

then is your private foobar unclobbered?

To answer your demand before for a function in the public interface of
a completion script, not library, which respects or does not respect
the bash-completion convention: see the public _rpm_installed_packages
helper from the rpm completion script[1].

If you don't like the convention or think that I have misunderstood
it, I can understand that and would recommend that you suggest a
different one and get it adopted and documented by the bash_completion
project.

Jonathan

[1] http://lists.alioth.debian.org/pipermail/bash-completion-devel/2012-March/004359.html

Re: [PATCH v2] completion: add new git_complete helper

[Felipe Contreras]

On Tue, Apr 17, 2012 at 1:44 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Felipe Contreras wrote:
>
>> Even more, I just added a foobar() function in my profile, and I also
>> added a foobar() function in /etc/bash_completion.d/git. I don't see
>> my function replaced in any way, even after typing and completing
>> 'git' commands. I don't know how that's possible, but that's why I
>> don't like to take these types of claims as face value.
>
> What version of the bash_completion library do you use?  Lazy-loading
> was introduced in version 1.90.

1.99

> If you put
>
>        foobar() {
>                echo hi
>        }
>        . /etc/bash_completion
>
> then is your private foobar unclobbered?

Yes, in that case it does, but that's not the default behavior, at
least not in my system, and I doubt anybody would define their
functions before loading library scripts.

> To answer your demand before for a function in the public interface of
> a completion script, not library, which respects or does not respect
> the bash-completion convention: see the public _rpm_installed_packages
> helper from the rpm completion script[1].

What makes you think this is public? It's under the section '# helper
functions', which doesn't seem to be public. Plus, it's repeated in
rpm, rpmbuild, and rpmbuild-md5.

The fact that somebody uses it doesn't mean it's public.

Cheers.

-- 
Felipe Contreras

Re: [PATCH v2] completion: add new git_complete helper

[Jonathan Nieder]

Felipe Contreras wrote:
> On Tue, Apr 17, 2012 at 1:44 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:

>> What version of the bash_completion library do you use?  Lazy-loading
>> was introduced in version 1.90.
>
> 1.99

Is your git completion script installed at /etc/bash_completion.d/git,
/usr/share/bash-completion/completions/git, or somewhere else?

Re: [PATCH v2] completion: add new git_complete helper

[Jonathan Nieder]

Felipe Contreras wrote:

> What makes you think this is public? It's under the section '# helper
> functions', which doesn't seem to be public. Plus, it's repeated in
> rpm, rpmbuild, and rpmbuild-md5.

Ok, you win.  I hadn't realized we were having a debate, but now I do,
and you have won.

Can we get back to making the completion script nicer for human
beings that have been using it, please?

The following summary may sound annoyed, because I am.  On the other
hand I know you mean well and am grateful for your work.

I have said that the convention for bash completion scripts is to
precede all exposed identifier names with an underscore.  You
mentioned some old counterexamples that have been grandfathered in.
You mentioned that you do not trust me.  Your bash completion script
gets loaded immediately instead of being lazy-loaded, probably because
it is not in /usr/share/bash-completion/completions/.  You claimed
that nobody would _ever_ ask for bash completion support at the end of
their .profile and after their custom functions in .profile that do
something unrelated, though I used to do that for a long time and
Debian's default .bashrc loads /etc/bash_completion at the end, too.

I still maintain that namespaces are useful and we should follow the
conventional ones when they exist.  What is the next step?

Jonathan

Re: [PATCH v2] completion: add new git_complete helper

[Felipe Contreras]

On Tue, Apr 17, 2012 at 2:16 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Felipe Contreras wrote:
>
>> What makes you think this is public? It's under the section '# helper
>> functions', which doesn't seem to be public. Plus, it's repeated in
>> rpm, rpmbuild, and rpmbuild-md5.
>
> Ok, you win.  I hadn't realized we were having a debate, but now I do,
> and you have won.

I thought we were trying to agree on a good name for possibly the
first (or second, depending how you count) public function. It's
important to choose a good name because we can't just change it again
in a month.

You are the one who brought the argument that even public functions
have the '_' prefix, so it's *your* responsibility to substantiate
that argument. I don't think it would be easy to find a precedent like
this, but you can prove me wrong by providing the evidence; so far, I
don't think that has happened.

> Can we get back to making the completion script nicer for human
> beings that have been using it, please?

Sure, but we need some sort of git_completion function if we want to
make it easy for people to define aliases for git commands. And this
is a known issue that has been brought in the past.

> The following summary may sound annoyed, because I am.  On the other
> hand I know you mean well and am grateful for your work.
>
> I have said that the convention for bash completion scripts is to
> precede all exposed identifier names with an underscore.  You
> mentioned some old counterexamples that have been grandfathered in.
> You mentioned that you do not trust me.  Your bash completion script
> gets loaded immediately instead of being lazy-loaded, probably because
> it is not in /usr/share/bash-completion/completions/.  You claimed
> that nobody would _ever_ ask for bash completion support at the end of
> their .profile and after their custom functions in .profile that do
> something unrelated,

I did not claim such thing. I said that I *doubted* it, not that it
was most definitely the case.

> though I used to do that for a long time and
> Debian's default .bashrc loads /etc/bash_completion at the end, too.

I see. That would be troublesome indeed (if somebody chose to add the
function before that line, and not at the end), *if* somehow they used
git_completion in their scripts.

> I still maintain that namespaces are useful and we should follow the
> conventional ones when they exist.  What is the next step?

What about this?

if ! type git_complete >/dev/null 2>&1; then
git_complete ()
{
	echo "WARNING: This function is not meant for public use; the name" \
		"might change. Use __git_complete for now."
	__git_complete "$@"
}
else
type git_complete | grep -q "WARNING" ||
echo "git_complete is already being used, please notify
git@vger.kernel.org, to" \
	"avoid overriding this function in the future."
fi

We can use that for 1.7.11, and if nobody complains remove the
warnings for 1.7.12.

-- 
Felipe Contreras

Re: [PATCH v2] completion: add new git_complete helper

[Jonathan Nieder]

Felipe Contreras wrote:

> You are the one who brought the argument that even public functions
> have the '_' prefix, so it's *your* responsibility to substantiate
> that argument.

No, honestly it isn't my responsibility to waste time arguing with
you.  What kind of crazy world would work that way?

And I did not mean to bring up any argument.  I only meant to bring up
the _datum_ that, at least in the context of the bash_completion
project, that is the current convention.  And then you started trying
to tell me that I had the facts wrong!  You might even be right, but
you haven't shown any sign of trying to check that, by, say, asking
someone from the bash_completion project what convention they use.

I'm sick of this.  Call it whatever you want.  I don't know why you
think this is productive.

Still annoyed,
Jonathan

Re: [PATCH v2] completion: add new git_complete helper

[Felipe Contreras]

On Tue, Apr 17, 2012 at 10:36 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Felipe Contreras wrote:
>
>> You are the one who brought the argument that even public functions
>> have the '_' prefix, so it's *your* responsibility to substantiate
>> that argument.
>
> No, honestly it isn't my responsibility to waste time arguing with
> you.  What kind of crazy world would work that way?
>
> And I did not mean to bring up any argument.  I only meant to bring up
> the _datum_ that, at least in the context of the bash_completion
> project, that is the current convention.  And then you started trying
> to tell me that I had the facts wrong!  You might even be right, but
> you haven't shown any sign of trying to check that, by, say, asking
> someone from the bash_completion project what convention they use.

I don't understand. I'm proposing the name 'git_complete', I thought
you were arguing against it. If you were only providing random facts,
then we can just ignore them, and it would be OK, right? But I'm
pretty sure you would be angry as well if I just ignored that fact.

Sure, it would be nice to follow bash_completion project's convention
for these kinds of functions, if they had any, and might be useful to
ask them what they think. But we don't *have* to. And nobody is
arguing that we should ask them. Right? Or are you?

> I'm sick of this.  Call it whatever you want.  I don't know why you
> think this is productive.

We don't have to agree on any name right now. I believe it's worth
waiting before deciding on a name for a public function like this, and
try to get some consensus.

But throwing arguments on the air, and they get angry when they get
counter-argued is not helpful. If we are going to discuss, lets
discuss, but that doesn't seem to be what you want. You want me to
blindly use the name you propose without saying a word? Blindly accept
your argument... follow orders?

I took your argument seriously and looked for evidence myself, and in
the absence of evidence I'm still not throwing it away, I'm assuming
it has merit and added code to make sure we don't override some user
function that has the same name. That would solve the problem you
raised, and it would help us find out if this is indeed a real issue,
or a theoretical one, and at the same time making sure people don't
rely too much on this function for the moment.

I believe this is very scientific-like; you make a hypothesis, we make
an experiment, and then we can find out the results, and only then
make a decision.

I don't know what else I can do to move this forward.

Cheers.

-- 
Felipe Contreras

Re: [PATCH v2] completion: add new git_complete helper

[Jonathan Nieder]

Felipe Contreras wrote:

> Sure, it would be nice to follow bash_completion project's convention
> for these kinds of functions, if they had any, and might be useful to
> ask them what they think. But we don't *have* to. And nobody is
> arguing that we should ask them. Right? Or are you?

No, I really am not arguing that we should ask them.  Because we (or
at least I) already know at least what their convention was a few
years ago.  However, I do encourage anyone curious to ask them instead
of making up arguments about why the answer is obvious.

[...]
> But throwing arguments on the air, and they get angry when they get
> counter-argued is not helpful. If we are going to discuss, lets
> discuss, but that doesn't seem to be what you want. You want me to
> blindly use the name you propose without saying a word?

I do not think it is fair to call your position blind.  I really would
be happier if you were able to listen, and, instead of throwing out
protests and debating points, to talk about your real concerns.

I am guessing (but you never said!) that you find the name
__git_complete ugly.  That's fine.  I even agree.  I have mentioned
that there is a namespace that your proposed alternative violates.
Your response is... to refuse to believe that what I said is true or
relevant?

How am I supposed to have a reasonable discussion after that?

This is not a debating floor.  When I give what I think is relevant
information, I am not requesting "please misinterpret me and shoot me
down".

Ciao,
Jonathan

Re: [PATCH v2] completion: add new git_complete helper

[Jonathan Nieder]

Felipe Contreras wrote:

> I took your argument seriously and looked for evidence myself, and in
> the absence of evidence I'm still not throwing it away, I'm assuming
> it has merit and added code to make sure we don't override some user
> function that has the same name. That would solve the problem you
> raised

Ah.  All that talking, but I was not addressing your actual point.

I though you didn't believe that the namespace adopted by the
bash_completion project was "identifiers starting with underscore,
plus a few old identifiers that have been grandfathered in".

But in fact, you just don't like namespace conventions.

Now I'm exhausted so I have no time to explain why a project that
involves implicitly including scripts from a wide variety of authors
into a flat namespace shared with others might end up using a
convention to manage that namespace.  But at least the world seems a
little saner.  Sorry for the wasted time.

Please, next time just say what you mean instead of leaving me talking
about something irrelevant.

Re: [PATCH v2] completion: add new git_complete helper

[Jonathan Nieder]

Felipe Contreras wrote:

> Perhaps it's time to avoid the __ prefix for public interfaces;
> otherwise how would people know they are public?

I'm happy as long as all the identifiers the script exports start with
an underscore, as that is the usual convention for completion scripts.
Within that namespace, if people want to use ___git or _git__ for
private things or something, I don't mind.

If __git_ps1 is moved to a separate script library, it seems fine for
it to provide a git_ps1 identifier with no leading underscores when it
is not being implicitly included by the completion script.

Jonathan

Re: [PATCH v2] completion: add new git_complete helper

[SZEDER Gábor]

Hi,

On Sun, Apr 15, 2012 at 04:37:18PM -0500, Jonathan Nieder wrote:
> Felipe Contreras wrote:
> 
> > This simplifies the completions, and makes it easier to define aliases:
> >
> >  git_complete gf git_fetch
> 
> Sounds neat.  Unfortunately users could already be using a function
> with some other purpose named git_complete in their .profile and this
> would override it.  The completion script has so far stuck to a
> limited namespace:
> 
> 	_git_*	(completion functions)
> 	__git_*	(everything else, including public interfaces like __git_ps1)

Don't forget the oddball __gitdir() function ;)

> A name like __git_complete should work, presumably.

And foo_wrap() should also fit into those namespaces.


Best,
Gábor

Re: [PATCH v2] completion: add new git_complete helper

[Felipe Contreras]

2012/4/17 SZEDER Gábor <szeder@ira.uka.de>:
> On Sun, Apr 15, 2012 at 04:37:18PM -0500, Jonathan Nieder wrote:
>> Felipe Contreras wrote:

>> A name like __git_complete should work, presumably.
>
> And foo_wrap() should also fit into those namespaces.

Yeah, I don't have a problem with that, just forgot about it.

But git_complete I think is different.

-- 
Felipe Contreras

Re: [PATCH v2] completion: add new git_complete helper

[Junio C Hamano]

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

> 2012/4/17 SZEDER Gábor <szeder@ira.uka.de>:
>> On Sun, Apr 15, 2012 at 04:37:18PM -0500, Jonathan Nieder wrote:
>>> Felipe Contreras wrote:
>
>>> A name like __git_complete should work, presumably.
>>
>> And foo_wrap() should also fit into those namespaces.
>
> Yeah, I don't have a problem with that, just forgot about it.
>
> But git_complete I think is different.

Is git_complete something the user types interactively, or is it meant to
be used in their .bashrc to help them complete arguments to their custom
scripts that take arguments similar to Git Porcelains?

Re: [PATCH v2] completion: add new git_complete helper

[Felipe Contreras]

On Tue, Apr 17, 2012 at 6:50 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> 2012/4/17 SZEDER Gábor <szeder@ira.uka.de>:
>>> On Sun, Apr 15, 2012 at 04:37:18PM -0500, Jonathan Nieder wrote:
>>>> Felipe Contreras wrote:
>>
>>>> A name like __git_complete should work, presumably.
>>>
>>> And foo_wrap() should also fit into those namespaces.
>>
>> Yeah, I don't have a problem with that, just forgot about it.
>>
>> But git_complete I think is different.
>
> Is git_complete something the user types interactively, or is it meant to
> be used in their .bashrc to help them complete arguments to their custom
> scripts that take arguments similar to Git Porcelains?

It's meant for their .bashrc, but can be used interactively, exactly
like 'complete'. You can type 'complete -o bashdefault -o default -o
nospace -F _git git' in the command like, but that would rarely
happen.

-- 
Felipe Contreras

Re: [PATCH v2] completion: add new git_complete helper

[Junio C Hamano]

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

> On Tue, Apr 17, 2012 at 6:50 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Felipe Contreras <felipe.contreras@gmail.com> writes:
> ...
>>>> And foo_wrap() should also fit into those namespaces.
>>>
>>> Yeah, I don't have a problem with that, just forgot about it.
>>>
>>> But git_complete I think is different.
>>
>> Is git_complete something the user types interactively, or is it meant to
>> be used in their .bashrc to help them complete arguments to their custom
>> scripts that take arguments similar to Git Porcelains?
>
> It's meant for their .bashrc, but can be used interactively, exactly
> like 'complete'. You can type 'complete -o bashdefault -o default -o
> nospace -F _git git' in the command like, but that would rarely
> happen.

OK, then I do not think "as a public interface it looks somewhat ugly"
(which I happen to think, and I am guessing that you agree with) matters.
It looks to me that it would be sane to follow the convention to avoid
accidental name clashes with userspace names by naming it "__git_complete"
in that case.

Re: [PATCH v2] completion: add new git_complete helper

[Felipe Contreras]

On Tue, Apr 17, 2012 at 9:15 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> On Tue, Apr 17, 2012 at 6:50 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>> ...
>>>>> And foo_wrap() should also fit into those namespaces.
>>>>
>>>> Yeah, I don't have a problem with that, just forgot about it.
>>>>
>>>> But git_complete I think is different.
>>>
>>> Is git_complete something the user types interactively, or is it meant to
>>> be used in their .bashrc to help them complete arguments to their custom
>>> scripts that take arguments similar to Git Porcelains?
>>
>> It's meant for their .bashrc, but can be used interactively, exactly
>> like 'complete'. You can type 'complete -o bashdefault -o default -o
>> nospace -F _git git' in the command like, but that would rarely
>> happen.
>
> OK, then I do not think "as a public interface it looks somewhat ugly"
> (which I happen to think, and I am guessing that you agree with) matters.
> It looks to me that it would be sane to follow the convention

But that convention is for *private* functions.

> to avoid
> accidental name clashes with userspace names by naming it "__git_complete"
> in that case.

What if there are no clashes?

Are you saying that even if there are no real clashes, only
hypothetical ones, you would still prefer __git_complete?

How are people going to distinguish between public and private functions?

-- 
Felipe Contreras

Re: [PATCH v2] completion: add new git_complete helper

[Jonathan Nieder]

Felipe Contreras wrote:

> How are people going to distinguish between public and private functions?

There are only two public functions.

They will be the ones provided without an __ (git_ps1 and
git_complete) when you source the git-prompt-and-completion-helpers
library in your .bashrc.  Neither will be exposed automatically from
the completion script, though synonyms with leading __ to avoid
namespace clashes will be.

Sensible?
Jonathan

Re: [PATCH v2] completion: add new git_complete helper

[SZEDER Gábor]

On Tue, Apr 17, 2012 at 11:53:08PM +0300, Felipe Contreras wrote:
> Are you saying that even if there are no real clashes, only
> hypothetical ones, you would still prefer __git_complete?

Can you ascertain that there are no real clashes?

> How are people going to distinguish between public and private functions?

By reading the documentation, perhaps?  It was not a problem for
__git_ps1(), so I guess it won't be a problem for __git_complete()
either.  Assuming that it will be documented eventually, of course.


Best,
Gábor

Re: [PATCH v2] completion: add new git_complete helper

[Felipe Contreras]

2012/4/18 SZEDER Gábor <szeder@ira.uka.de>:
> On Tue, Apr 17, 2012 at 11:53:08PM +0300, Felipe Contreras wrote:
>> Are you saying that even if there are no real clashes, only
>> hypothetical ones, you would still prefer __git_complete?
>
> Can you ascertain that there are no real clashes?

After having this code for a certain amount of time, yeah:

if ! type git_complete >/dev/null 2>&1; then
git_complete ()
{
       echo "WARNING: This function is not meant for public use; the name" \
               "might change. Use __git_complete for now."
       __git_complete "$@"
}
else
type git_complete | grep -q "WARNING" ||
echo "git_complete is already being used, please notify
git@vger.kernel.org, to" \
       "avoid overriding this function in the future."
fi

>> How are people going to distinguish between public and private functions?
>
> By reading the documentation, perhaps?  It was not a problem for
> __git_ps1(), so I guess it won't be a problem for __git_complete()
> either.  Assuming that it will be documented eventually, of course.

I guess that would do it. I would still prefer to have a different
convention for public functions. I feel we are being paranoid here.

Cheers.

-- 
Felipe Contreras

Re: [PATCH v2] completion: add new git_complete helper

[SZEDER Gábor]

On Tue, Apr 17, 2012 at 08:50:01AM -0700, Junio C Hamano wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
> 
> > 2012/4/17 SZEDER Gábor <szeder@ira.uka.de>:
> >> On Sun, Apr 15, 2012 at 04:37:18PM -0500, Jonathan Nieder wrote:
> >>> Felipe Contreras wrote:
> >
> >>> A name like __git_complete should work, presumably.
> >>
> >> And foo_wrap() should also fit into those namespaces.
> >
> > Yeah, I don't have a problem with that, just forgot about it.
> >
> > But git_complete I think is different.
> 
> Is git_complete something the user types interactively, or is it meant to
> be used in their .bashrc to help them complete arguments to their custom
> scripts that take arguments similar to Git Porcelains?

Primarily for .bashrc, however, not for their custom scripts but to be
able to use completion for their one-word git aliases, e.g. after an

  alias gf='git fetch'

they want to do

  gf o<TAB>

and get 'origin' completed.

Here's a relevant thread:

  http://thread.gmane.org/gmane.comp.version-control.git/185184/


Best,
Gábor