SZEDER Gábor [off-list ref] writes:
Instead of having a huge hard-coded list of command-completion
function pairs (in _git()), the completion script will figure out
which completion function to call based on the command's name. That
is, when completing the options of 'git foo', the main completion
script will check whether the function '_git_foo' is declared, and if
declared, it will invoke that function to perform the completion. If
such a function is not declared, it will fall back to complete file
names. So, users will only need to provide this '_git_foo' completion
function in a separate file, source that file, and it will be used the
next time they press TAB after 'git foo '.
I think the basic idea is sound, but I have a minor issue with the names.
Admittedly, we have already taken over _git_foo (and "_git") namespace,
and anybody who uses bash with the completion support cannot write their
own shell function with these names for purposes that are unrelated to
completion, so in that sense, the patch is not introducing a new problem,
but making it a documented interface and casting it in stone will make the
namespace contamination issue harder to rectify later.
So if we were to go in the direction as the patch proposes (which I think
is a good idea), we might want to rename them to __git_completion_foo or
something that is less likely to collide with whatever names users might
want to use. It is my understanding that the only published interface so
far is __git_ps1.
Hi,
On Fri, Jan 29, 2010 at 12:32:12PM -0800, Junio C Hamano wrote:
Admittedly, we have already taken over _git_foo (and "_git") namespace,
and anybody who uses bash with the completion support cannot write their
own shell function with these names for purposes that are unrelated to
completion,
Actually, the "_" namespace is taken over by bash completion in
general, so writing shell functions starting with "_" is probably
not a good idea anyway. E.g. to see all non-completion-related shell
functions you can do a "declare -F |grep -v ' _'", but if you name
shell functions not related to completion as _git_foo(), then this
will no longer work.
so in that sense, the patch is not introducing a new problem,
but making it a documented interface and casting it in stone will make the
namespace contamination issue harder to rectify later.
So if we were to go in the direction as the patch proposes (which I think
is a good idea), we might want to rename them to __git_completion_foo or
something that is less likely to collide with whatever names users might
want to use. It is my understanding that the only published interface so
far is __git_ps1.
I would say that __git_ps1() is the only interface that is advertised
as being public. If someone is unsatisfied with the completion
script, because he wanted completion for a custom git command or for a
frequently used plumbing command, then I bet he just reused existing
functions, e.g. when he needed refs, he just used __git_refs(), or
when he needed git log's options, he used _git_log(). I did that,
probably others too. If we were to rename completion functions, these
people's setup will break (although they will likely get merge
conflicts caused by this patch anyway). On the other hand: should we
really care that much about such users, who use non-pulic interfaces
from contrib/ ?
Having said all that, I don't really care either way. If you or Shawn
would prefer to have the completion functions renamed, I will do a
s/this/that/ preparation patch for the series. BTW, Mercurial's
completion script uses _hg_cmd_foo() for hg commands and
_hg_ext_bar() for extensions, so we might as well be a bit consistent,
and call our completion functions _git_cmd_foo().
Best,
Gábor
SZEDER Gábor [off-list ref] writes:
quoted
so in that sense, the patch is not introducing a new problem,
but making it a documented interface and casting it in stone will make the
namespace contamination issue harder to rectify later.
I won't quote the first paragraph where you are repeating what I said,
while sounding as if you were disagreeing with me.
quoted
So if we were to ...
... It is my understanding that the only published interface so
far is __git_ps1.
I would say that __git_ps1() is the only interface that is advertised
as being public. ...
... If we were to rename completion functions, these
people's setup will break (although they will likely get merge
conflicts caused by this patch anyway). On the other hand: should we
really care that much about such users, who use non-pulic interfaces
from contrib/ ?
I see we are in agreement in the first half of your paragraph; my answer
to the question in the latter half is:
- we shouldn't care about people who already used unpublished interface
in contrib/ so far; _but_
- because we will be advertising it as a way to override and enhance
completion to define your own shell functions, the naming _will_ become
part of published interface---what we decide _now_ will matter.
That is why I wanted people to at least think about renaming _git_frotz to
something less generic. The name tells us that it is a helper shell
function about the "git frotz" command, but it does not say what aspect of
"git frotz" it is meant to help, i.e. completion. _git_complete_frotz or
a variant of such would not have that problem, and will keep the door open
for future shell helpers that are about different aspect "xxx" that is
unrelated to completion---they can then name theirs _git_xxx_frotz.
... BTW, Mercurial's
completion script uses _hg_cmd_foo() for hg commands and
_hg_ext_bar() for extensions, so we might as well be a bit consistent,
and call our completion functions _git_cmd_foo().
In Hg's context it might make sense to name a function _hg_cmd_foo vs
_hg_ext_bar iff the end users need to be very aware of the distinction
between commands and extensions, but for us I think "git_cmd_foo" is
probably the most meaningless rename, as it doesn't add any extra
information (we know 'git foo' is a command already without 'cmd').
Junio C Hamano [off-list ref] wrote:
quoted
... BTW, Mercurial's
completion script uses _hg_cmd_foo() for hg commands and
_hg_ext_bar() for extensions, so we might as well be a bit consistent,
and call our completion functions _git_cmd_foo().
In Hg's context it might make sense to name a function _hg_cmd_foo vs
_hg_ext_bar iff the end users need to be very aware of the distinction
between commands and extensions, but for us I think "git_cmd_foo" is
probably the most meaningless rename, as it doesn't add any extra
information (we know 'git foo' is a command already without 'cmd').
I agree. _git_cmd_foo is pointless.
But I would be ok with _git_completion_foo for the completion
function of git foo. As Junio pointed out, better to do it now
before users start to really build their own extension library on
top of the package.
--
Shawn.