Re: [PATCH v2 1/1] completion: load completion file for external subcommand

["SZEDER Gábor" <szeder.dev@gmail.com>]

On Thu, Apr 19, 2018 at 9:07 PM, Florian Gamböck <mail@floga.de> wrote:
> On 2018-04-18 21:51, SZEDER Gábor wrote:
>>
>> On Tue, Apr 10, 2018 at 10:28 PM, Florian Gamböck <mail@floga.de> wrote:
>>>
>>> Adding external subcommands to Git is as easy as to put an executable
>>> file git-foo into PATH. Packaging such subcommands for a Linux distribution
>>> can be achieved by unpacking the executable into /usr/bin of the user's
>>> system. Adding system-wide completion scripts for new subcommands, however,
>>> can be a bit tricky.
>>>
>>> Since bash-completion started to use dynamical loading of completion
>>> scripts since v1.90 (preview of v2.0),
>>
>>
>> I believe the main bash-completion repository can be found at:
>>
>>  https://github.com/scop/bash-completion.git
>>
>> This repository still contains the branch 'dynamic-loading'; for the
>> record it points to 3b029892f6f9db3b7210a7f66d636be3e5ec5fa2.
>>
>> Two commits on that branch are worth mentioning:
>>
>>   20c05b43 (Load completions in separate files dynamically, get rid of
>>             have()., 2011-10-12)
>>   5baebf81 (Add _xfunc for loading and calling functions on demand,
>>             use it in apt-get, cvsps, rsync, and sshfs., 2011-10-13)
>
>
> Nice, thanks for the pointers!
>
>>> (...)
>>>
>>> I think the easiest method is to use a function that is defined by
>>> bash-completion v2.0+, namely __load_completion.
>>
>>
>> This is wrong, __load_completion() was introduced in cad3abfc
>> (__load_completion: New function, use in _completion_loader and _xfunc,
>> 2015-07-15), and the first release tag containg it is '2.2' from 2016-03-03.
>
>
> Dang, I thought it was introduced at the same time. Sorry for that. I guess,
> 2016 is a bit too young to take it for granted then?
>
>> The release tags '1.90' and '2.0' are from 2011-11-03 and 2012-06-17,
>> respectively.  This leaves a couple of years long hole where completions
>> were already loaded dynamically but there was no __load_completion()
>> function.
>>
>> Would it be possible to use _xfunc() instead to plug that hole?  It seems
>> the be tricky, because that function not only sources but also _calls_ the
>> completion function.
>
>
> But isn't this exactly what we want?

No, that's definitely not what we want.

The bash-completion project can get away with it, because they only
use their _xfunc() to source a file they themselves ship and to call a
completion function they know that that file contains.

We, however, would like to load a file that might not exist and to
call a function that might not be defined.  Git has a lot of plumbing
commands with neither a corresponding _git_plumbing_cmd() completion
function in our completion script nor a corresponding
'git-plumbing-cmd' file that could be sourced dynamically to provide
that function.  The same applies to any 'git-foo' command in the
user's $PATH (the user's own scripts or various git-related tools,
e.g. Git LFS).

So if someone tries e.g. 'git diff-index <TAB>' to complete files in
the current directory, then it would result in an error message like

  _git_diff_index: command not found

Furthermore, earlier versions of _xfunc(), I think until the
introduction  of __load_completion(), tried to source the file given
as parameter without checking its existence beforehand, so on whatever
LTS I happen to be currently using I would also get an error like

  bash: /usr/share/bash-completion/completions/git-diff-index: No such
file or directory

Finally, since all this is running in the user's interactive shell,
Bash will even run the 'command_not_found_handle', if it's set (and
most Linux distros do set it in their default configuration (OK, maybe
not most, but at least some of the more popular do)), which may or may
not have any suggestions, but at the very least it takes quite a while
to scan through its database.


> Lucky us, we can replace the whole
> if-fi block with a simpler:
>
>    _xfunc git-$command $completion_func 2>/dev/null && return
>
> If _xfunc is not defined -- as in, bashcomp is not installed / loaded --
> then the return will not get called and the original completion will
> continue:
>
>    declare -f $completion_func >/dev/null 2>/dev/null &&
>        $completion_func && return
>
> Since this would be redundant, we could define a fall-back for _xfunc like
> so:
>
>    declare -f _xfunc || _xfunc() {
>        declare -f $completion_func >/dev/null 2>/dev/null &&
>            $completion_func && return
>    }
>
> This way, we retain the "old" behavior and get dynamic loading if bashcomp
> is available. The actual call to get the completions would just be _xfunc
> like in my first example above.
>
> What do you think?
>
> --
> Regards
>
> Florian