[RFC PATCH 0/1] completion: Dynamic completion loading

[RFC PATCH 0/1] completion: Dynamic completion loading

[Florian Gamböck]

In this small patch I want to introduce a way to dynamically load completion 
scripts for external subcommands.

A few years ago, you would put a completion script (which defines a Bash 
function _git_foo for a custom git subcommand foo) into 
/etc/bash_completion.d, or save it somewhere in your $HOME and source it 
manually in your .bashrc.

Starting with bash-completion v2.0 (or, to be absolutely exact, the preview 
versions started at v1.90), completion scripts are not sourced at bash startup 
anymore. Rather, when typing in a command and trigger completion by pressing 
the TAB key, the new bash-completion's main script looks for a script with the 
same name as the typed command in the completion directory, sources it, and 
provides the completions defined in this script.

However, that only works for top level commands. After a completion script has 
been found, the logic is delegated to this script. This means, that the 
completion of subcommands must be handled by the corresponding completion 
script.

As it is now, git is perfectly able to call custom completion functions, iff 
they are already defined when calling the git completion. With my patch, git 
uses an already defined loader function of bash-completion (or continues 
silently if this function is not found), loads an external completion script, 
and provides its completions.

An example for an external completion script would be 
/usr/share/bash-completion/completions/git-foo for a git subcommand foo.

Florian Gamböck (1):
  completion: Load completion file for external subcommand

 contrib/completion/git-completion.bash | 8 ++++++++
 1 file changed, 8 insertions(+)

-- 
2.16.1

[RFC PATCH 1/1] completion: Load completion file for external subcommand

[Florian Gamböck]

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 somewhere around v2.0, it is no longer sufficient to drop a
completion script of a subcommand into the standard completions path,
/usr/share/bash-completion/completions, since this script will not be
loaded if called as a git subcommand.

For example, look at https://bugs.gentoo.org/544722. To give a short
summary: The popular git-flow subcommand provides a completion script,
which gets installed as /usr/share/bash-completion/completions/git-flow.

If you now type into a Bash shell:

    git flow <TAB>

You will not get any completions, because bash-completion only loads
completions for git and git has no idea that git-flow is defined in
another file. You have to load this script manually or trigger the
dynamic loader with:

    git-flow <TAB> # Please notice the dash instead of whitespace

This will not complete anything either, because it only defines a Bash
function, without generating completions. But now the correct completion
script has been loaded and the first command can use the completions.

So, the goal is now to teach the git completion script to consider the
possibility of external completion scripts for subcommands, but of
course without breaking current workflows.

I think the easiest method is to use a function that is defined by
bash-completion v2.0+, namely __load_completion. It will take care of
loading the correct script if present. Afterwards, the git completion
script behaves as usual.

This way we can leverage bash-completion's dynamic loading for git
subcommands and make it easier for developers to distribute custom
completion scripts.

Signed-off-by: Florian Gamböck <mail@floga.de>
---
 contrib/completion/git-completion.bash | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index b09c8a236..e6114822c 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -3096,12 +3096,20 @@ __git_main ()
 	fi
 
 	local completion_func="_git_${command//-/_}"
+	if ! declare -f $completion_func >/dev/null 2>/dev/null; then
+		declare -f __load_completion >/dev/null 2>/dev/null &&
+			__load_completion "git-$command"
+	fi
 	declare -f $completion_func >/dev/null 2>/dev/null && $completion_func && return
 
 	local expansion=$(__git_aliased_command "$command")
 	if [ -n "$expansion" ]; then
 		words[1]=$expansion
 		completion_func="_git_${expansion//-/_}"
+		if ! declare -f $completion_func >/dev/null 2>/dev/null; then
+			declare -f __load_completion >/dev/null 2>/dev/null &&
+				__load_completion "git-$expansion"
+		fi
 		declare -f $completion_func >/dev/null 2>/dev/null && $completion_func
 	fi
 }
-- 
2.16.1

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

[Florian Gamböck]

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 somewhere around v2.0, it is no longer sufficient to drop a
completion script of a subcommand into the standard completions path,
/usr/share/bash-completion/completions, since this script will not be
loaded if called as a git subcommand.

For example, look at https://bugs.gentoo.org/544722. To give a short
summary: The popular git-flow subcommand provides a completion script,
which gets installed as /usr/share/bash-completion/completions/git-flow.

If you now type into a Bash shell:

    git flow <TAB>

You will not get any completions, because bash-completion only loads
completions for git and git has no idea that git-flow is defined in
another file. You have to load this script manually or trigger the
dynamic loader with:

    git-flow <TAB> # Please notice the dash instead of whitespace

This will not complete anything either, because it only defines a Bash
function, without generating completions. But now the correct completion
script has been loaded and the first command can use the completions.

So, the goal is now to teach the git completion script to consider the
possibility of external completion scripts for subcommands, but of
course without breaking current workflows.

I think the easiest method is to use a function that is defined by
bash-completion v2.0+, namely __load_completion. It will take care of
loading the correct script if present. Afterwards, the git completion
script behaves as usual.

This way we can leverage bash-completion's dynamic loading for git
subcommands and make it easier for developers to distribute custom
completion scripts.

Signed-off-by: Florian Gamböck <mail@floga.de>
---
 contrib/completion/git-completion.bash | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index b09c8a236..e6114822c 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -3096,12 +3096,20 @@ __git_main ()
 	fi
 
 	local completion_func="_git_${command//-/_}"
+	if ! declare -f $completion_func >/dev/null 2>/dev/null; then
+		declare -f __load_completion >/dev/null 2>/dev/null &&
+			__load_completion "git-$command"
+	fi
 	declare -f $completion_func >/dev/null 2>/dev/null && $completion_func && return
 
 	local expansion=$(__git_aliased_command "$command")
 	if [ -n "$expansion" ]; then
 		words[1]=$expansion
 		completion_func="_git_${expansion//-/_}"
+		if ! declare -f $completion_func >/dev/null 2>/dev/null; then
+			declare -f __load_completion >/dev/null 2>/dev/null &&
+				__load_completion "git-$expansion"
+		fi
 		declare -f $completion_func >/dev/null 2>/dev/null && $completion_func
 	fi
 }
-- 
2.16.1

Re: [RFC PATCH 1/1] completion: Load completion file for external subcommand

[Florian Gamböck]

Ah, sorry, please ignore this one.

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

[Junio C Hamano]

Florian Gamböck <mail@floga.de> writes:

> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index b09c8a236..e6114822c 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -3096,12 +3096,20 @@ __git_main ()
>  	fi
>  

Sorry if I am asking something obvious, as I am not fluent in
bash-isms, but

>  	local completion_func="_git_${command//-/_}"
> +	if ! declare -f $completion_func >/dev/null 2>/dev/null; then
> +		declare -f __load_completion >/dev/null 2>/dev/null &&
> +			__load_completion "git-$command"

wouldn't the above be easier to read if it were

	if ! declare ... $completion_func ... && declare -f __load_completion
	then
		__load_completion "git-$command"
	fi

or is there a reason why it is better to &&-chain the check for
__load_completion with its use?  Same comment applies to the other
hunk.

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

[Florian Gamböck]

On 2018-04-09 07:59, Junio C Hamano wrote:
> >  	local completion_func="_git_${command//-/_}"
> > +	if ! declare -f $completion_func >/dev/null 2>/dev/null; then
> > +		declare -f __load_completion >/dev/null 2>/dev/null &&
> > +			__load_completion "git-$command"
> 
> wouldn't the above be easier to read if it were
> 
> 	if ! declare ... $completion_func ... && declare -f __load_completion
> 	then
> 		__load_completion "git-$command"
> 	fi
> 
> or is there a reason why it is better to &&-chain the check for
> __load_completion with its use?  Same comment applies to the other
> hunk.

Good point. I could go even further and ditch the if-construct:

    ! declare -f $completion_func && declare -f __load_completion &&
        __load_completion "git-$command"

I originally intended to do a if-else-construct, which I re-thought
halfway through. I will change that in the next iteration.

Thank you!

-- 
Regards

Florian

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

[Junio C Hamano]

Florian Gamböck <mail@floga.de> writes:

> Good point. I could go even further and ditch the if-construct:
>
>    ! declare -f $completion_func && declare -f __load_completion &&
>        __load_completion "git-$command"

I personally find that a lot harder to read than if/then/fi.

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

[Florian Gamböck]

On 2018-04-09 18:36, Junio C Hamano wrote:
> Florian Gamböck <mail@floga.de> writes:
> 
> > Good point. I could go even further and ditch the if-construct:
> >
> >    ! declare -f $completion_func && declare -f __load_completion &&
> >        __load_completion "git-$command"
> 
> I personally find that a lot harder to read than if/then/fi.

Then I mis-understood you the first time. It sounded a bit as if you
wanted to avoid if-fi. After all, the rest of this code block also uses
quite long &&-chains.

Then we are back at the first question:

> >  	local completion_func="_git_${command//-/_}"
> > +	if ! declare -f $completion_func >/dev/null 2>/dev/null; then
> > +		declare -f __load_completion >/dev/null 2>/dev/null &&
> > +			__load_completion "git-$command"
> 
> wouldn't the above be easier to read if it were
> 
> 	if ! declare ... $completion_func ... && declare -f __load_completion
> 	then
> 		__load_completion "git-$command"
> 	fi
> 
> or is there a reason why it is better to &&-chain the check for
> __load_completion with its use?

As for readability, I would prefer my first approach then with the
following reasoning: Checking the existence of a function and actually
calling it can be seen as a unit. Either the function does not exist or
you call it. You could even create a function like "call_if_exists",
that does exactly this. Either way, at the end of the line you are
smarter than before. As for the if-statement, this describes the reason
why you even want to load an external file. If the function in question
($completion_func) already exists, we do not want to load it again. If
you chain the statement with the existence check of __load_completion,
you make it look like those two functions are somehow related, which is
not the case.

To put it in words: "If $completion_func does not already exist, then
load another file (if you know how to do that)." versus "If
$completion_func does not yet exist and you know how to load another
file, then load another file." The difference is subtle, but I think the
first sentence better describes the intention.

Does my reasoning make sense? I mean, the result will be exactly the
same, we are clearly only talking about readability here.

Re: [RFC PATCH 1/1] completion: Load completion file for external subcommand

[Stefan Beller]

On Sun, Apr 8, 2018 at 11:26 AM, 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 somewhere around v2.0, it is no longer sufficient to drop a
> completion script of a subcommand into the standard completions path,
> /usr/share/bash-completion/completions, since this script will not be
> loaded if called as a git subcommand.

Also v1.90 here? (hint from the cover letter, please be exact)

If Gits own completion script would be broken up by subcommand,
would that also deliver an improvement in performance?

Thanks,
Stefan

Re: [RFC PATCH 1/1] completion: Load completion file for external subcommand

[Stefan Beller]

On Sun, Apr 8, 2018 at 11:26 AM, 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 somewhere around v2.0, it is no longer sufficient to drop a
> completion script of a subcommand into the standard completions path,
> /usr/share/bash-completion/completions, since this script will not be
> loaded if called as a git subcommand.

Also v1.90 here? (hint from the cover letter, please be exact)

If Gits own completion script would be broken up by subcommand,
would that also deliver an improvement in performance?

Thanks,
Stefan

Re: [RFC PATCH 1/1] completion: Load completion file for external subcommand

[Florian Gamböck]

On 2018-04-09 11:26, Stefan Beller wrote:
>> Since bash-completion started to use dynamical loading of completion 
>> scripts somewhere around v2.0, it is no longer sufficient to drop a 
>> completion script of a subcommand into the standard completions path, 
>> /usr/share/bash-completion/completions, since this script will not be 
>> loaded if called as a git subcommand.
>
> Also v1.90 here? (hint from the cover letter, please be exact)

Yes, it started at 1.90. I will reword the commit message and be more 
exact in the next iteration.

> If Gits own completion script would be broken up by subcommand, would 
> that also deliver an improvement in performance?

As it is now, the completion script is quite big. On a system with 
limited resources, the initial loading time can be long and the memory 
footprint is big, given that most users will just use a few commands. If 
you just use the "commit" subcommand, the first loading of the two 
(smaller) scripts will be slightly longer the first time (but not as 
long as with one big script, I think), but the footprint will be 
drastically lower. The whole script is 56kB big (without comments), 
after radically removing everything which is not connected to 
_git_commit, it is only 11kB.

So to answer your question: Yes. My first intuition is, that by 
splitting the completion script and loading the sub-scripts dynamically, 
it will improve in terms of speed and overall memory footprint, at least 
for the average user that does not fire up all possible git commands.

-- 
Regards

Florian

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

[Junio C Hamano]

Florian Gamböck <mail@floga.de> writes:

> On 2018-04-09 18:36, Junio C Hamano wrote:
>> Florian Gamböck <mail@floga.de> writes:
>>
>> > Good point. I could go even further and ditch the if-construct:
>> >
>> >    ! declare -f $completion_func && declare -f __load_completion &&
>> >        __load_completion "git-$command"
>>
>> I personally find that a lot harder to read than if/then/fi.
> ...
> Does my reasoning make sense?

Not at all.  But ...

> I mean, the result will be exactly the same, we are clearly only
> talking about readability here.

... I agree, and I also think the "readability" is not absolute
anyway.

FWIW, personally I'd find "if $completion_func does not yet exist
and a way to dynload stuff exists, then use that way to load it from
an external file" the most natural way to express what you are
doing.

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

[Florian Gamböck]

On 2018-04-10 07:06, Junio C Hamano wrote:
> Florian Gamböck <mail@floga.de> writes:
>> Does my reasoning make sense?
>
> Not at all.

:-( That actually hurt a bit.

> But ...
>
>> I mean, the result will be exactly the same, we are clearly only 
>> talking about readability here.
>
> ... I agree, and I also think the "readability" is not absolute 
> anyway.
>
> FWIW, personally I'd find "if $completion_func does not yet exist and 
> a way to dynload stuff exists, then use that way to load it from an 
> external file" the most natural way to express what you are doing.

Then let's go with that. Maybe I am overthinking things here and 
re-wording doesn't hurt.

I'll send a new patch version later this day, including the commit 
message re-wording suggestion from Stefan.

Thank you for your feedback!

-- 
Regards

Florian

Re: [RFC PATCH 1/1] completion: Load completion file for external subcommand

[SZEDER Gábor]

On Mon, Apr 9, 2018 at 9:49 PM, Florian Gamböck <ml@floga.de> wrote:
> On 2018-04-09 11:26, Stefan Beller wrote:
>> If Gits own completion script would be broken up by subcommand, would that
>> also deliver an improvement in performance?
>
>
> As it is now, the completion script is quite big. On a system with limited
> resources, the initial loading time can be long and the memory footprint is
> big, given that most users will just use a few commands. If you just use the
> "commit" subcommand, the first loading of the two (smaller) scripts will be
> slightly longer the first time (but not as long as with one big script, I
> think), but the footprint will be drastically lower. The whole script is
> 56kB big (without comments), after radically removing everything which is
> not connected to _git_commit, it is only 11kB.
>
> So to answer your question: Yes. My first intuition is, that by splitting
> the completion script and loading the sub-scripts dynamically, it will
> improve in terms of speed and overall memory footprint, at least for the
> average user that does not fire up all possible git commands.

While "most users" might indeed only use a couple of git commands,
they don't use systems so limited in resources where 56k vs. 11k makes
a non-negligible difference.  A system, where such a small difference
is significant, must be so thight on resources that the user will most
likely have issues running git anyway.

I don't know how much memory Bash uses to store completion scripts,
and it appears to be using a memory pool or something like our
ALLOC_GROW, making measuring its memory footprint with simple means
unusably inaccurate.  Anyway, here are some numbers:

  # Baseline, bash does nothing:
  $ command time -f %M bash -c ''
  2924

  # Loading a <10k script:
  $ wc -c ./git-sh-setup.sh
  9313 ./git-sh-setup.sh
  command time -f %M bash -c '. ./git-sh-setup.sh'
  3724

  # Loading our completion script; the increase in max memory
  # footprint is clearly not proportional to the size of the loaded
  # script:
  $ wc -c ./contrib/completion/git-completion.bash
  69413 ./contrib/completion/git-completion.bash
  $ command time -f %M bash -c '. ./contrib/completion/git-completion.bash'
  4092

  # The main bash-completion script, though slightly smaller than ours,
  # appears to be requiring much more memory:
  $ wc -c /usr/share/bash-completion/bash_completion
  67661 /usr/share/bash-completion/bash_completion
  $ command time -f %M bash -c './usr/share/bash-completion/bash_completion'
  5952

  # Loading both the main bash-completion script and our completion
  # script, which is where that memory pool/ALLOC_GROW-like thingy really
  # kicks in, as there is no real max memory footprint increase:
  $ command time -f %M bash -c '. /usr/share/bash-completion/bash_completion
                                . ./contrib/completion/git-completion.bash'
  5964


OTOH, our completion script has a couple of nice properties that we
should keep working:

  - A user can simply run '. /path/to/git-completion.bash', and then
    completion for git commands will work.  Even without
    bash-completion package, even in 'bash --norc'.

    If we were to split up our completion script, we would also have
    to roll our own __git_load_completion() funcion.

  - A user building Git from source doesn't have to install the
    completion script, it can be sourced from anywhere.  And if the
    user chooses to install it somewhere, only a single file has to be
    copied.

    Currently we have completion functions for 55 git commands, which
    would mean 56 files to install if the completion script were split
    up.

  - Sourcing 'git-completion.bash' brings in all the latest and
    greatest.

    If it were split up, then sourcing it would only update the common
    functions, but not the completion functions of individual git
    commands.  So we would have to take extra steps to delete those
    command-specific completion functions upon sourcing the completion
    script.  However, we should be extra careful to delete only those
    completion functions that were source by the completion script,
    because users might have defined such functions in their
    '~/.bashrc'...


I don't think it's worth it.