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  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.

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  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

[Junio C Hamano]

Florian Gamböck  writes:

> On 2018-04-09 18:36, Junio C Hamano wrote:
>> Florian Gamböck  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-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

[Stefan Beller]

On Sun, Apr 8, 2018 at 11:26 AM, Florian Gamböck  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 18:36, Junio C Hamano wrote:

Florian Gamböck  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

[Junio C Hamano]

Florian Gamböck  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 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  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]

Ah, sorry, please ignore this one.

[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 

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  # 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 
---
 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 

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  # 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 
---
 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