[PATCH] Speedup bash completion loading

[PATCH] Speedup bash completion loading

[Kirill Smelkov]

On my slow laptop (P3 700MHz), system-wide bash completions take too
much time to load (> 1s), and significant fraction of this time is spent
loading git-completion.bash:

    $ time bash -c '. git-completion.bash'  # before this patch

    real    0m0.317s
    user    0m0.250s
    sys     0m0.060s

I've tracked down that the most time is spent warming up merge_strategy,
all_command & porcelain_command caches.

Since git is not used in each and every interactive xterm, I think it
would be perfectly ok to load completion support with cold caches, and
then load needed thing lazily.

As __git_merge_stratiegies(), __git_all_commands() &
__git_porcelain_command() already cache their results, we can safely
remove associated cache initialization code and be done with it:

    $ time bash -c '. git-completion.bash'  # after this patch

    real    0m0.069s
    user    0m0.050s
    sys     0m0.020s

Signed-off-by: Kirill Smelkov <kirr@mns.spb.ru>
---
 contrib/completion/git-completion.bash |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 2c2a0d4..4c09d41 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -340,7 +340,6 @@ __git_merge_strategies ()
 	}'
 }
 __git_merge_strategylist=
-__git_merge_strategylist=$(__git_merge_strategies 2>/dev/null)
 
 __git_complete_file ()
 {
@@ -505,7 +504,6 @@ __git_all_commands ()
 	done
 }
 __git_all_commandlist=
-__git_all_commandlist="$(__git_all_commands 2>/dev/null)"
 
 __git_porcelain_commands ()
 {
@@ -596,7 +594,6 @@ __git_porcelain_commands ()
 	done
 }
 __git_porcelain_commandlist=
-__git_porcelain_commandlist="$(__git_porcelain_commands 2>/dev/null)"
 
 __git_aliases ()
 {
-- 
1.6.5.rc2.17.gdbc1b

Re: [PATCH] Speedup bash completion loading

[Shawn O. Pearce]

Kirill Smelkov <kirr@mns.spb.ru> wrote:
> I've tracked down that the most time is spent warming up merge_strategy,
> all_command & porcelain_command caches.

Nak.

The problem is, during completion when we modify the value the
change doesn't persist beyond the current completion invocation.
Thus there is no value in the cache, so every completion attempt
which needs the list has to rerun the command to compute it.

 
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 2c2a0d4..4c09d41 100755
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -340,7 +340,6 @@ __git_merge_strategies ()
>  	}'
>  }
>  __git_merge_strategylist=
> -__git_merge_strategylist=$(__git_merge_strategies 2>/dev/null)
>  
>  __git_complete_file ()
>  {
> @@ -505,7 +504,6 @@ __git_all_commands ()
>  	done
>  }
>  __git_all_commandlist=
> -__git_all_commandlist="$(__git_all_commands 2>/dev/null)"
>  
>  __git_porcelain_commands ()
>  {
> @@ -596,7 +594,6 @@ __git_porcelain_commands ()
>  	done
>  }
>  __git_porcelain_commandlist=
> -__git_porcelain_commandlist="$(__git_porcelain_commands 2>/dev/null)"
>  
>  __git_aliases ()
>  {
> -- 
> 1.6.5.rc2.17.gdbc1b
> 

-- 
Shawn.

Re: [PATCH] Speedup bash completion loading

[Kirill Smelkov]

On Mon, Oct 05, 2009 at 08:25:04AM -0700, Shawn O. Pearce wrote:
> Kirill Smelkov <kirr@mns.spb.ru> wrote:
> > I've tracked down that the most time is spent warming up merge_strategy,
> > all_command & porcelain_command caches.
> 
> Nak.
> 
> The problem is, during completion when we modify the value the
> change doesn't persist beyond the current completion invocation.
> Thus there is no value in the cache, so every completion attempt
> which needs the list has to rerun the command to compute it.

Yes, my mistake.

To avoid spawning subshell on $(__git_porcelain_commands) I could come up
with "caching" at the upper level, as e.g.

@@ -2107,7 +2111,10 @@ _git ()
                        --help
                        "
                        ;;
-               *)     __gitcomp "$(__git_porcelain_commands) $(__git_aliases)" ;;
+               *)     __gitcomp "
+                       ${__git_porcelain_commandlist:=$(__git_porcelain_commands)}
+                       $(__git_aliases)
+                       " ;;


but that's ugly and not maintainable since e.g. __git_all_commands() is used in
several places.


The other possibility is to pregenerate all these lists at git build time, but
are we going to do it for things under contrib/ ?


This could have easy solution if bash provided analog of $() to call a function
and obtain either its stdout or return value without spawning subshell, but up
to now I could not find such a construct in bash's man.

I have to go, but I'll try to come up with something practical in several days.


Thanks for spotting it,
Kirill

Re: [PATCH] Speedup bash completion loading

[Ted Pavlic]

> The other possibility is to pregenerate all these lists at git build time, but
> are we going to do it for things under contrib/ ?

IIRC, that's what StGIT does with its completion script. As a
consequence, StGIT completion is substantially faster than git
completion.

--Ted

-- 
Ted Pavlic <ted@tedpavlic.com>

Re: [PATCH] Speedup bash completion loading

[Kirill Smelkov]

On Mon, Oct 05, 2009 at 08:25:04AM -0700, Shawn O. Pearce wrote:
> Kirill Smelkov <kirr@mns.spb.ru> wrote:
> > I've tracked down that the most time is spent warming up merge_strategy,
> > all_command & porcelain_command caches.
> 
> Nak.
> 
> The problem is, during completion when we modify the value the
> change doesn't persist beyond the current completion invocation.
> Thus there is no value in the cache, so every completion attempt
> which needs the list has to rerun the command to compute it.

> > Yes, my mistake.
> > 
> > To avoid spawning subshell on $(__git_porcelain_commands) I could come up
> > with "caching" at the upper level, as e.g.
> > 
> > @@ -2107,7 +2111,10 @@ _git ()
> >                         --help
> >                         "
> >                         ;;
> > -               *)     __gitcomp "$(__git_porcelain_commands) $(__git_aliases)" ;;
> > +               *)     __gitcomp "
> > +                       ${__git_porcelain_commandlist:=$(__git_porcelain_commands)}
> > +                       $(__git_aliases)
> > +                       " ;;
> > 
> > 
> > but that's ugly and not maintainable since e.g. __git_all_commands() is used in
> > several places.

On Mon, Oct 05, 2009 at 03:18:48PM -0400, Ted Pavlic wrote:
> > The other possibility is to pregenerate all these lists at git build time, but
> > are we going to do it for things under contrib/ ?
> 
> IIRC, that's what StGIT does with its completion script. As a
> consequence, StGIT completion is substantially faster than git
> completion.

Ok, it turned out that in bash, functions can't return values except
through their stdout, but then we are forever with spawning
subprocesses. So I've decided to go the pregenerate-it way.


[ While at it, I've also done chmod -x to git-completion.bash -- this
  script is never executed, only sourced, so it does not need to be
  executable. If needed I could split it in separate patch ]



---- 8< ----

>From 4178dcc2bd1005e26e069fe32a88b9a5b164ced6 Mon Sep 17 00:00:00 2001
From: Kirill Smelkov <kirr@mns.spb.ru>
Date: Mon, 5 Oct 2009 13:36:15 +0400
Subject: [PATCH v2] Speedup bash completion loading

On my slow laptop (P3 700MHz), system-wide bash completions take too
much time to load (> 1s), and significant fraction of this time is spent
loading git-completion.bash:

    $ time bash -c '. git-completion.bash'  # before this patch

    real    0m0.317s
    user    0m0.250s
    sys     0m0.060s

I've tracked down that the most time is spent warming up merge_strategy,
all_command & porcelain_command caches.

Initially I thought that since git is not used in each and every
interactive xterm, it would be perfectly ok to load completion support
with cold caches, and then load needed thing lazily.

But for me this strategy turned out to be difficult to implement in
simple and maintainable way -- bash does not provide a way to return values
from inside functions, so one will have to use e.g.

    ${__git_all_commandlist:=$(__git_all_commands)}

everywhere in place where $(__git_all_commands) we used before, so as
also Ted Pavlic suggested let's pregenerate everything at build time so
that we have nothing to compute at runtime when git-completion.bash
script is loaded.

The result is that loading completion is significantly faster now:

    $ time bash -c '. git-completion.bash'  # after this patch

    real    0m0.068s
    user    0m0.060s
    sys     0m0.010s

Cc: Ted Pavlic <ted@tedpavlic.com>
Signed-off-by: Kirill Smelkov <kirr@mns.spb.ru>
---
 contrib/completion/.gitignore                      |    1 +
 contrib/completion/Makefile                        |   11 ++
 contrib/completion/git-completion.bash.generate    |  128 ++++++++++++++++
 ...{git-completion.bash => git-completion.bash.in} |  154 ++------------------
 4 files changed, 155 insertions(+), 139 deletions(-)
 create mode 100644 contrib/completion/.gitignore
 create mode 100644 contrib/completion/Makefile
 create mode 100755 contrib/completion/git-completion.bash.generate
 rename contrib/completion/{git-completion.bash => git-completion.bash.in} (90%)
 mode change 100755 => 100644

diff --git a/contrib/completion/.gitignore b/contrib/completion/.gitignore
new file mode 100644
index 0000000..578e6a8
--- /dev/null
+++ b/contrib/completion/.gitignore
@@ -0,0 +1 @@
+git-completion.bash
diff --git a/contrib/completion/Makefile b/contrib/completion/Makefile
new file mode 100644
index 0000000..a0fbb66
--- /dev/null
+++ b/contrib/completion/Makefile
@@ -0,0 +1,11 @@
+all	: git-completion.bash
+
+
+git-completion.bash: git-completion.bash.in git-completion.bash.generate
+	# Generate completions for binaries we have just built
+	PATH="$(shell pwd)/..:$$PATH" ./git-completion.bash.generate
+
+
+clean:
+	rm -f git-completion.bash
+
diff --git a/contrib/completion/git-completion.bash.generate b/contrib/completion/git-completion.bash.generate
new file mode 100755
index 0000000..fa998dc
--- /dev/null
+++ b/contrib/completion/git-completion.bash.generate
@@ -0,0 +1,128 @@
+#!/bin/bash
+#
+# Generate bash completion for git.
+#
+# Precompute everything that can be known in advance at build time, so that
+# actual bash completion script is loaded faster.
+
+__git_merge_strategies ()
+{
+	git merge -s help 2>&1 |
+	sed -n -e '/[Aa]vailable strategies are: /,/^$/{
+		s/\.$//
+		s/.*://
+		s/^[ 	]*//
+		s/[ 	]*$//
+		p
+	}'
+}
+
+__git_all_commands ()
+{
+	local i IFS=" "$'\n'
+	for i in $(git help -a|egrep '^ ')
+	do
+		case $i in
+		*--*)             : helper pattern;;
+		*) echo $i;;
+		esac
+	done
+}
+
+
+__git_porcelain_commands ()
+{
+	local i IFS=" "$'\n'
+	for i in "help" $(__git_all_commands)
+	do
+		case $i in
+		*--*)             : helper pattern;;
+		applymbox)        : ask gittus;;
+		applypatch)       : ask gittus;;
+		archimport)       : import;;
+		cat-file)         : plumbing;;
+		check-attr)       : plumbing;;
+		check-ref-format) : plumbing;;
+		checkout-index)   : plumbing;;
+		commit-tree)      : plumbing;;
+		count-objects)    : infrequent;;
+		cvsexportcommit)  : export;;
+		cvsimport)        : import;;
+		cvsserver)        : daemon;;
+		daemon)           : daemon;;
+		diff-files)       : plumbing;;
+		diff-index)       : plumbing;;
+		diff-tree)        : plumbing;;
+		fast-import)      : import;;
+		fast-export)      : export;;
+		fsck-objects)     : plumbing;;
+		fetch-pack)       : plumbing;;
+		fmt-merge-msg)    : plumbing;;
+		for-each-ref)     : plumbing;;
+		hash-object)      : plumbing;;
+		http-*)           : transport;;
+		index-pack)       : plumbing;;
+		init-db)          : deprecated;;
+		local-fetch)      : plumbing;;
+		lost-found)       : infrequent;;
+		ls-files)         : plumbing;;
+		ls-remote)        : plumbing;;
+		ls-tree)          : plumbing;;
+		mailinfo)         : plumbing;;
+		mailsplit)        : plumbing;;
+		merge-*)          : plumbing;;
+		mktree)           : plumbing;;
+		mktag)            : plumbing;;
+		pack-objects)     : plumbing;;
+		pack-redundant)   : plumbing;;
+		pack-refs)        : plumbing;;
+		parse-remote)     : plumbing;;
+		patch-id)         : plumbing;;
+		peek-remote)      : plumbing;;
+		prune)            : plumbing;;
+		prune-packed)     : plumbing;;
+		quiltimport)      : import;;
+		read-tree)        : plumbing;;
+		receive-pack)     : plumbing;;
+		reflog)           : plumbing;;
+		repo-config)      : deprecated;;
+		rerere)           : plumbing;;
+		rev-list)         : plumbing;;
+		rev-parse)        : plumbing;;
+		runstatus)        : plumbing;;
+		sh-setup)         : internal;;
+		shell)            : daemon;;
+		show-ref)         : plumbing;;
+		send-pack)        : plumbing;;
+		show-index)       : plumbing;;
+		ssh-*)            : transport;;
+		stripspace)       : plumbing;;
+		symbolic-ref)     : plumbing;;
+		tar-tree)         : deprecated;;
+		unpack-file)      : plumbing;;
+		unpack-objects)   : plumbing;;
+		update-index)     : plumbing;;
+		update-ref)       : plumbing;;
+		update-server-info) : daemon;;
+		upload-archive)   : plumbing;;
+		upload-pack)      : plumbing;;
+		write-tree)       : plumbing;;
+		var)              : infrequent;;
+		verify-pack)      : infrequent;;
+		verify-tag)       : plumbing;;
+		*) echo $i;;
+		esac
+	done
+}
+
+
+__git_merge_strategylist=$(__git_merge_strategies | tr '\n' ' ')
+__git_all_commandlist="$(__git_all_commands | tr '\n' ' ')"
+__git_porcelain_commandlist="$(__git_porcelain_commands | tr '\n' ' ')"
+
+
+sed -e "s/__GIT_MERGE_STRATEGYLIST/\"$__git_merge_strategylist\"/"	\
+    -e "s/__GIT_ALL_COMMANDLIST/\"$__git_all_commandlist\"/"	\
+    -e "s/__GIT_PORCELAIN_COMMANDLIST/\"$__git_porcelain_commandlist\"/"	\
+    git-completion.bash.in > git-completion.bash
+
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash.in
old mode 100755
new mode 100644
similarity index 90%
rename from contrib/completion/git-completion.bash
rename to contrib/completion/git-completion.bash.in
index 88b1b3c..cf1b5fd
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash.in
@@ -21,13 +21,7 @@
 #    2) Added the following line to your .bashrc:
 #        source ~/.git-completion.sh
 #
-#    3) You may want to make sure the git executable is available
-#       in your PATH before this script is sourced, as some caching
-#       is performed while the script loads.  If git isn't found
-#       at source time then all lookups will be done on demand,
-#       which may be slightly slower.
-#
-#    4) Consider changing your PS1 to also show the current branch:
+#    3) Consider changing your PS1 to also show the current branch:
 #        PS1='[\u@\h \W$(__git_ps1 " (%s)")]\$ '
 #
 #       The argument to __git_ps1 will be displayed only if you
@@ -84,6 +78,14 @@ __gitdir ()
 	fi
 }
 
+
+# pregenerated stuff (to save load time)
+__git_merge_strategylist=__GIT_MERGE_STRATEGYLIST
+__git_all_commandlist=__GIT_ALL_COMMANDLIST
+__git_porcelain_commandlist=__GIT_PORCELAIN_COMMANDLIST
+
+
+
 # __git_ps1 accepts 0 or 1 arguments (i.e., format string)
 # returns text to add to bash PS1 prompt (includes branch name)
 __git_ps1 ()
@@ -324,23 +326,6 @@ __git_remotes ()
 	done
 }
 
-__git_merge_strategies ()
-{
-	if [ -n "${__git_merge_strategylist-}" ]; then
-		echo "$__git_merge_strategylist"
-		return
-	fi
-	git merge -s help 2>&1 |
-	sed -n -e '/[Aa]vailable strategies are: /,/^$/{
-		s/\.$//
-		s/.*://
-		s/^[ 	]*//
-		s/[ 	]*$//
-		p
-	}'
-}
-__git_merge_strategylist=
-__git_merge_strategylist=$(__git_merge_strategies 2>/dev/null)
 
 __git_complete_file ()
 {
@@ -476,128 +461,19 @@ __git_complete_strategy ()
 {
 	case "${COMP_WORDS[COMP_CWORD-1]}" in
 	-s|--strategy)
-		__gitcomp "$(__git_merge_strategies)"
+		__gitcomp "$__git_merge_strategylist"
 		return 0
 	esac
 	local cur="${COMP_WORDS[COMP_CWORD]}"
 	case "$cur" in
 	--strategy=*)
-		__gitcomp "$(__git_merge_strategies)" "" "${cur##--strategy=}"
+		__gitcomp "$__git_merge_strategylist" "" "${cur##--strategy=}"
 		return 0
 		;;
 	esac
 	return 1
 }
 
-__git_all_commands ()
-{
-	if [ -n "${__git_all_commandlist-}" ]; then
-		echo "$__git_all_commandlist"
-		return
-	fi
-	local i IFS=" "$'\n'
-	for i in $(git help -a|egrep '^ ')
-	do
-		case $i in
-		*--*)             : helper pattern;;
-		*) echo $i;;
-		esac
-	done
-}
-__git_all_commandlist=
-__git_all_commandlist="$(__git_all_commands 2>/dev/null)"
-
-__git_porcelain_commands ()
-{
-	if [ -n "${__git_porcelain_commandlist-}" ]; then
-		echo "$__git_porcelain_commandlist"
-		return
-	fi
-	local i IFS=" "$'\n'
-	for i in "help" $(__git_all_commands)
-	do
-		case $i in
-		*--*)             : helper pattern;;
-		applymbox)        : ask gittus;;
-		applypatch)       : ask gittus;;
-		archimport)       : import;;
-		cat-file)         : plumbing;;
-		check-attr)       : plumbing;;
-		check-ref-format) : plumbing;;
-		checkout-index)   : plumbing;;
-		commit-tree)      : plumbing;;
-		count-objects)    : infrequent;;
-		cvsexportcommit)  : export;;
-		cvsimport)        : import;;
-		cvsserver)        : daemon;;
-		daemon)           : daemon;;
-		diff-files)       : plumbing;;
-		diff-index)       : plumbing;;
-		diff-tree)        : plumbing;;
-		fast-import)      : import;;
-		fast-export)      : export;;
-		fsck-objects)     : plumbing;;
-		fetch-pack)       : plumbing;;
-		fmt-merge-msg)    : plumbing;;
-		for-each-ref)     : plumbing;;
-		hash-object)      : plumbing;;
-		http-*)           : transport;;
-		index-pack)       : plumbing;;
-		init-db)          : deprecated;;
-		local-fetch)      : plumbing;;
-		lost-found)       : infrequent;;
-		ls-files)         : plumbing;;
-		ls-remote)        : plumbing;;
-		ls-tree)          : plumbing;;
-		mailinfo)         : plumbing;;
-		mailsplit)        : plumbing;;
-		merge-*)          : plumbing;;
-		mktree)           : plumbing;;
-		mktag)            : plumbing;;
-		pack-objects)     : plumbing;;
-		pack-redundant)   : plumbing;;
-		pack-refs)        : plumbing;;
-		parse-remote)     : plumbing;;
-		patch-id)         : plumbing;;
-		peek-remote)      : plumbing;;
-		prune)            : plumbing;;
-		prune-packed)     : plumbing;;
-		quiltimport)      : import;;
-		read-tree)        : plumbing;;
-		receive-pack)     : plumbing;;
-		reflog)           : plumbing;;
-		repo-config)      : deprecated;;
-		rerere)           : plumbing;;
-		rev-list)         : plumbing;;
-		rev-parse)        : plumbing;;
-		runstatus)        : plumbing;;
-		sh-setup)         : internal;;
-		shell)            : daemon;;
-		show-ref)         : plumbing;;
-		send-pack)        : plumbing;;
-		show-index)       : plumbing;;
-		ssh-*)            : transport;;
-		stripspace)       : plumbing;;
-		symbolic-ref)     : plumbing;;
-		tar-tree)         : deprecated;;
-		unpack-file)      : plumbing;;
-		unpack-objects)   : plumbing;;
-		update-index)     : plumbing;;
-		update-ref)       : plumbing;;
-		update-server-info) : daemon;;
-		upload-archive)   : plumbing;;
-		upload-pack)      : plumbing;;
-		write-tree)       : plumbing;;
-		var)              : infrequent;;
-		verify-pack)      : infrequent;;
-		verify-tag)       : plumbing;;
-		*) echo $i;;
-		esac
-	done
-}
-__git_porcelain_commandlist=
-__git_porcelain_commandlist="$(__git_porcelain_commands 2>/dev/null)"
-
 __git_aliases ()
 {
 	local i IFS=$'\n'
@@ -1077,7 +953,7 @@ _git_help ()
 		return
 		;;
 	esac
-	__gitcomp "$(__git_all_commands)
+	__gitcomp "$__git_all_commandlist
 		attributes cli core-tutorial cvs-migration
 		diffcore gitk glossary hooks ignore modules
 		repository-layout tutorial tutorial-2
@@ -1423,7 +1299,7 @@ _git_config ()
 		return
 		;;
 	pull.twohead|pull.octopus)
-		__gitcomp "$(__git_merge_strategies)"
+		__gitcomp "$__git_merge_strategylist"
 		return
 		;;
 	color.branch|color.diff|color.interactive|\
@@ -1524,7 +1400,7 @@ _git_config ()
 	pager.*)
 		local pfx="${cur%.*}."
 		cur="${cur#*.}"
-		__gitcomp "$(__git_all_commands)" "$pfx" "$cur"
+		__gitcomp "$__git_all_commandlist" "$pfx" "$cur"
 		return
 		;;
 	remote.*.*)
@@ -2116,7 +1992,7 @@ _git ()
 			--help
 			"
 			;;
-		*)     __gitcomp "$(__git_porcelain_commands) $(__git_aliases)" ;;
+		*)     __gitcomp "$__git_porcelain_commandlist $(__git_aliases)" ;;
 		esac
 		return
 	fi
-- 
1.6.5.rc2.18.g84f98.dirty

Re: [PATCH] Speedup bash completion loading

[Shawn O. Pearce]

Kirill Smelkov <kirr@mns.spb.ru> wrote:
> diff --git a/contrib/completion/Makefile b/contrib/completion/Makefile
> new file mode 100644
> index 0000000..a0fbb66
> --- /dev/null
> +++ b/contrib/completion/Makefile
> @@ -0,0 +1,11 @@
> +all	: git-completion.bash
> +
> +
> +git-completion.bash: git-completion.bash.in git-completion.bash.generate
> +	# Generate completions for binaries we have just built
> +	PATH="$(shell pwd)/..:$$PATH" ./git-completion.bash.generate

Is only one .. enough?  Isn't that putting us into the contrib
directory, and therefore not finding the 'git' we just compiled?

I'm also concerned that git-completion.bash.generate requires
bash to compile the completion for bash.  IMHO, if we are building
this code at compile time we shouldn't assume bash is available.
What if this is a sandboxed build environment using another shell
and /bin/bash isn't installed?

I think the git-completion.bash.generate code needs to be a bit
more sh agnostic than the completion routines themselves are.

> +# pregenerated stuff (to save load time)
> +__git_merge_strategylist=__GIT_MERGE_STRATEGYLIST
> +__git_all_commandlist=__GIT_ALL_COMMANDLIST
> +__git_porcelain_commandlist=__GIT_PORCELAIN_COMMANDLIST

This also makes testing the completion a bit more difficult, now
we have to build it before we can load it, making the testing cycle
actually be:

  make && . git-completion.bash

We probably should place a quick comment here to remind folks that
they need to build the script in order to test it properly.

-- 
Shawn.

Re: [PATCH] Speedup bash completion loading

[Kirill Smelkov]

On Thu, Oct 08, 2009 at 08:02:06AM -0700, Shawn O. Pearce wrote:
> Kirill Smelkov <kirr@mns.spb.ru> wrote:
> > diff --git a/contrib/completion/Makefile b/contrib/completion/Makefile
> > new file mode 100644
> > index 0000000..a0fbb66
> > --- /dev/null
> > +++ b/contrib/completion/Makefile
> > @@ -0,0 +1,11 @@
> > +all	: git-completion.bash
> > +
> > +
> > +git-completion.bash: git-completion.bash.in git-completion.bash.generate
> > +	# Generate completions for binaries we have just built
> > +	PATH="$(shell pwd)/..:$$PATH" ./git-completion.bash.generate
> 
> Is only one .. enough?  Isn't that putting us into the contrib
> directory, and therefore not finding the 'git' we just compiled?
> 
> I'm also concerned that git-completion.bash.generate requires
> bash to compile the completion for bash.  IMHO, if we are building
> this code at compile time we shouldn't assume bash is available.
> What if this is a sandboxed build environment using another shell
> and /bin/bash isn't installed?
> 
> I think the git-completion.bash.generate code needs to be a bit
> more sh agnostic than the completion routines themselves are.
> 
> > +# pregenerated stuff (to save load time)
> > +__git_merge_strategylist=__GIT_MERGE_STRATEGYLIST
> > +__git_all_commandlist=__GIT_ALL_COMMANDLIST
> > +__git_porcelain_commandlist=__GIT_PORCELAIN_COMMANDLIST
> 
> This also makes testing the completion a bit more difficult, now
> we have to build it before we can load it, making the testing cycle
> actually be:
> 
>   make && . git-completion.bash
> 
> We probably should place a quick comment here to remind folks that
> they need to build the script in order to test it properly.

Agree with everything and thanks. Will reroll tommorow.

Кирилл

Re: [PATCH] Speedup bash completion loading

[Kirill Smelkov]

On Thu, Oct 08, 2009 at 08:02:06AM -0700, Shawn O. Pearce wrote:
> Kirill Smelkov <kirr@mns.spb.ru> wrote:
> > diff --git a/contrib/completion/Makefile b/contrib/completion/Makefile
> > new file mode 100644
> > index 0000000..a0fbb66
> > --- /dev/null
> > +++ b/contrib/completion/Makefile
> > @@ -0,0 +1,11 @@
> > +all	: git-completion.bash
> > +
> > +
> > +git-completion.bash: git-completion.bash.in git-completion.bash.generate
> > +	# Generate completions for binaries we have just built
> > +	PATH="$(shell pwd)/..:$$PATH" ./git-completion.bash.generate
> 
> Is only one .. enough?  Isn't that putting us into the contrib
> directory, and therefore not finding the 'git' we just compiled?

Fixed, thanks.

> I'm also concerned that git-completion.bash.generate requires
> bash to compile the completion for bash.  IMHO, if we are building
> this code at compile time we shouldn't assume bash is available.
> What if this is a sandboxed build environment using another shell
> and /bin/bash isn't installed?
> 
> I think the git-completion.bash.generate code needs to be a bit
> more sh agnostic than the completion routines themselves are.

I've reworked it not to depend on bash in 2nd patch.

> > +# pregenerated stuff (to save load time)
> > +__git_merge_strategylist=__GIT_MERGE_STRATEGYLIST
> > +__git_all_commandlist=__GIT_ALL_COMMANDLIST
> > +__git_porcelain_commandlist=__GIT_PORCELAIN_COMMANDLIST
> 
> This also makes testing the completion a bit more difficult, now
> we have to build it before we can load it, making the testing cycle
> actually be:
> 
>   make && . git-completion.bash
> 
> We probably should place a quick comment here to remind folks that
> they need to build the script in order to test it properly.

I've added some sort of protection, so that git-completion.bash.in can't
be sourced at all. Is it ok?

(interdiff for patch 1)

diff --git a/contrib/completion/Makefile b/contrib/completion/Makefile
index a0fbb66..90aa225 100644
--- a/contrib/completion/Makefile
+++ b/contrib/completion/Makefile
@@ -3,7 +3,7 @@ all	: git-completion.bash
 
 git-completion.bash: git-completion.bash.in git-completion.bash.generate
 	# Generate completions for binaries we have just built
-	PATH="$(shell pwd)/..:$$PATH" ./git-completion.bash.generate
+	PATH="$(shell pwd)/../..:$$PATH" ./git-completion.bash.generate
 
 
 clean:
diff --git a/contrib/completion/git-completion.bash.in b/contrib/completion/git-completion.bash.in
index cf1b5fd..e1ab612 100644
--- a/contrib/completion/git-completion.bash.in
+++ b/contrib/completion/git-completion.bash.in
@@ -54,6 +54,21 @@
 #       git@vger.kernel.org
 #
 
+
+# pregenerated stuff (to save load time)
+__git_merge_strategylist=__GIT_MERGE_STRATEGYLIST
+__git_all_commandlist=__GIT_ALL_COMMANDLIST
+__git_porcelain_commandlist=__GIT_PORCELAIN_COMMANDLIST
+
+# remind folks that git-completion.bash.in can't be sourced
+case "$__git_merge_strategylist" in
+__GIT*)
+	echo "E: git-completion.bash.in can't be sourced"
+	return 1 ;;
+esac
+
+
+
 case "$COMP_WORDBREAKS" in
 *:*) : great ;;
 *)   COMP_WORDBREAKS="$COMP_WORDBREAKS:"
@@ -79,12 +94,6 @@ __gitdir ()
 }
 
 
-# pregenerated stuff (to save load time)
-__git_merge_strategylist=__GIT_MERGE_STRATEGYLIST
-__git_all_commandlist=__GIT_ALL_COMMANDLIST
-__git_porcelain_commandlist=__GIT_PORCELAIN_COMMANDLIST
-
-
 
 # __git_ps1 accepts 0 or 1 arguments (i.e., format string)
 # returns text to add to bash PS1 prompt (includes branch name)



Here are new patches themselves:


---- 8< ----

From: Kirill Smelkov <kirr@mns.spb.ru>
Date: Mon, 5 Oct 2009 13:36:15 +0400
Subject: [PATCH v3 1/2] Speedup bash completion loading

On my slow laptop (P3 700MHz), system-wide bash completions take too
much time to load (> 1s), and significant fraction of this time is spent
loading git-completion.bash:

    $ time bash -c '. git-completion.bash'  # before this patch

    real    0m0.317s
    user    0m0.250s
    sys     0m0.060s

I've tracked down that the most time is spent warming up merge_strategy,
all_command & porcelain_command caches.

Initially I thought that since git is not used in each and every
interactive xterm, it would be perfectly ok to load completion support
with cold caches, and then load needed thing lazily.

But for me this strategy turned out to be difficult to implement in
simple and maintainable way -- bash does not provide a way to return values
from inside functions, so one will have to use e.g.

    ${__git_all_commandlist:=$(__git_all_commands)}

everywhere in place where $(__git_all_commands) we used before, so as
also Ted Pavlic suggested let's pregenerate everything at build time so
that we have nothing to compute at runtime when git-completion.bash
script is loaded.

The result is that loading completion is significantly faster now:

    $ time bash -c '. git-completion.bash'  # after this patch

    real    0m0.068s
    user    0m0.060s
    sys     0m0.010s

Cc: Ted Pavlic <ted@tedpavlic.com>
Signed-off-by: Kirill Smelkov <kirr@mns.spb.ru>
---
 contrib/completion/.gitignore                      |    1 +
 contrib/completion/Makefile                        |   11 ++
 contrib/completion/git-completion.bash.generate    |  128 ++++++++++++++++
 ...{git-completion.bash => git-completion.bash.in} |  161 +++-----------------
 4 files changed, 162 insertions(+), 139 deletions(-)
 create mode 100644 contrib/completion/.gitignore
 create mode 100644 contrib/completion/Makefile
 create mode 100755 contrib/completion/git-completion.bash.generate
 rename contrib/completion/{git-completion.bash => git-completion.bash.in} (90%)
 mode change 100755 => 100644

diff --git a/contrib/completion/.gitignore b/contrib/completion/.gitignore
new file mode 100644
index 0000000..578e6a8
--- /dev/null
+++ b/contrib/completion/.gitignore
@@ -0,0 +1 @@
+git-completion.bash
diff --git a/contrib/completion/Makefile b/contrib/completion/Makefile
new file mode 100644
index 0000000..90aa225
--- /dev/null
+++ b/contrib/completion/Makefile
@@ -0,0 +1,11 @@
+all	: git-completion.bash
+
+
+git-completion.bash: git-completion.bash.in git-completion.bash.generate
+	# Generate completions for binaries we have just built
+	PATH="$(shell pwd)/../..:$$PATH" ./git-completion.bash.generate
+
+
+clean:
+	rm -f git-completion.bash
+
diff --git a/contrib/completion/git-completion.bash.generate b/contrib/completion/git-completion.bash.generate
new file mode 100755
index 0000000..fa998dc
--- /dev/null
+++ b/contrib/completion/git-completion.bash.generate
@@ -0,0 +1,128 @@
+#!/bin/bash
+#
+# Generate bash completion for git.
+#
+# Precompute everything that can be known in advance at build time, so that
+# actual bash completion script is loaded faster.
+
+__git_merge_strategies ()
+{
+	git merge -s help 2>&1 |
+	sed -n -e '/[Aa]vailable strategies are: /,/^$/{
+		s/\.$//
+		s/.*://
+		s/^[ 	]*//
+		s/[ 	]*$//
+		p
+	}'
+}
+
+__git_all_commands ()
+{
+	local i IFS=" "$'\n'
+	for i in $(git help -a|egrep '^ ')
+	do
+		case $i in
+		*--*)             : helper pattern;;
+		*) echo $i;;
+		esac
+	done
+}
+
+
+__git_porcelain_commands ()
+{
+	local i IFS=" "$'\n'
+	for i in "help" $(__git_all_commands)
+	do
+		case $i in
+		*--*)             : helper pattern;;
+		applymbox)        : ask gittus;;
+		applypatch)       : ask gittus;;
+		archimport)       : import;;
+		cat-file)         : plumbing;;
+		check-attr)       : plumbing;;
+		check-ref-format) : plumbing;;
+		checkout-index)   : plumbing;;
+		commit-tree)      : plumbing;;
+		count-objects)    : infrequent;;
+		cvsexportcommit)  : export;;
+		cvsimport)        : import;;
+		cvsserver)        : daemon;;
+		daemon)           : daemon;;
+		diff-files)       : plumbing;;
+		diff-index)       : plumbing;;
+		diff-tree)        : plumbing;;
+		fast-import)      : import;;
+		fast-export)      : export;;
+		fsck-objects)     : plumbing;;
+		fetch-pack)       : plumbing;;
+		fmt-merge-msg)    : plumbing;;
+		for-each-ref)     : plumbing;;
+		hash-object)      : plumbing;;
+		http-*)           : transport;;
+		index-pack)       : plumbing;;
+		init-db)          : deprecated;;
+		local-fetch)      : plumbing;;
+		lost-found)       : infrequent;;
+		ls-files)         : plumbing;;
+		ls-remote)        : plumbing;;
+		ls-tree)          : plumbing;;
+		mailinfo)         : plumbing;;
+		mailsplit)        : plumbing;;
+		merge-*)          : plumbing;;
+		mktree)           : plumbing;;
+		mktag)            : plumbing;;
+		pack-objects)     : plumbing;;
+		pack-redundant)   : plumbing;;
+		pack-refs)        : plumbing;;
+		parse-remote)     : plumbing;;
+		patch-id)         : plumbing;;
+		peek-remote)      : plumbing;;
+		prune)            : plumbing;;
+		prune-packed)     : plumbing;;
+		quiltimport)      : import;;
+		read-tree)        : plumbing;;
+		receive-pack)     : plumbing;;
+		reflog)           : plumbing;;
+		repo-config)      : deprecated;;
+		rerere)           : plumbing;;
+		rev-list)         : plumbing;;
+		rev-parse)        : plumbing;;
+		runstatus)        : plumbing;;
+		sh-setup)         : internal;;
+		shell)            : daemon;;
+		show-ref)         : plumbing;;
+		send-pack)        : plumbing;;
+		show-index)       : plumbing;;
+		ssh-*)            : transport;;
+		stripspace)       : plumbing;;
+		symbolic-ref)     : plumbing;;
+		tar-tree)         : deprecated;;
+		unpack-file)      : plumbing;;
+		unpack-objects)   : plumbing;;
+		update-index)     : plumbing;;
+		update-ref)       : plumbing;;
+		update-server-info) : daemon;;
+		upload-archive)   : plumbing;;
+		upload-pack)      : plumbing;;
+		write-tree)       : plumbing;;
+		var)              : infrequent;;
+		verify-pack)      : infrequent;;
+		verify-tag)       : plumbing;;
+		*) echo $i;;
+		esac
+	done
+}
+
+
+__git_merge_strategylist=$(__git_merge_strategies | tr '\n' ' ')
+__git_all_commandlist="$(__git_all_commands | tr '\n' ' ')"
+__git_porcelain_commandlist="$(__git_porcelain_commands | tr '\n' ' ')"
+
+
+sed -e "s/__GIT_MERGE_STRATEGYLIST/\"$__git_merge_strategylist\"/"	\
+    -e "s/__GIT_ALL_COMMANDLIST/\"$__git_all_commandlist\"/"	\
+    -e "s/__GIT_PORCELAIN_COMMANDLIST/\"$__git_porcelain_commandlist\"/"	\
+    git-completion.bash.in > git-completion.bash
+
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash.in
old mode 100755
new mode 100644
similarity index 90%
rename from contrib/completion/git-completion.bash
rename to contrib/completion/git-completion.bash.in
index 88b1b3c..67d03c3
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash.in
@@ -21,13 +21,7 @@
 #    2) Added the following line to your .bashrc:
 #        source ~/.git-completion.sh
 #
-#    3) You may want to make sure the git executable is available
-#       in your PATH before this script is sourced, as some caching
-#       is performed while the script loads.  If git isn't found
-#       at source time then all lookups will be done on demand,
-#       which may be slightly slower.
-#
-#    4) Consider changing your PS1 to also show the current branch:
+#    3) Consider changing your PS1 to also show the current branch:
 #        PS1='[\u@\h \W$(__git_ps1 " (%s)")]\$ '
 #
 #       The argument to __git_ps1 will be displayed only if you
@@ -60,6 +54,21 @@
 #       git@vger.kernel.org
 #
 
+
+# pregenerated stuff (to save load time)
+__git_merge_strategylist=__GIT_MERGE_STRATEGYLIST
+__git_all_commandlist=__GIT_ALL_COMMANDLIST
+__git_porcelain_commandlist=__GIT_PORCELAIN_COMMANDLIST
+
+# remind folks that git-completion.bash.in can't be sourced
+case "$__git_merge_strategylist" in
+__GIT*)
+	echo "E: git-completion.bash.in can't be sourced"
+	return 1 ;;
+esac
+
+
+
 case "$COMP_WORDBREAKS" in
 *:*) : great ;;
 *)   COMP_WORDBREAKS="$COMP_WORDBREAKS:"
@@ -324,23 +333,6 @@ __git_remotes ()
 	done
 }
 
-__git_merge_strategies ()
-{
-	if [ -n "${__git_merge_strategylist-}" ]; then
-		echo "$__git_merge_strategylist"
-		return
-	fi
-	git merge -s help 2>&1 |
-	sed -n -e '/[Aa]vailable strategies are: /,/^$/{
-		s/\.$//
-		s/.*://
-		s/^[ 	]*//
-		s/[ 	]*$//
-		p
-	}'
-}
-__git_merge_strategylist=
-__git_merge_strategylist=$(__git_merge_strategies 2>/dev/null)
 
 __git_complete_file ()
 {
@@ -476,128 +468,19 @@ __git_complete_strategy ()
 {
 	case "${COMP_WORDS[COMP_CWORD-1]}" in
 	-s|--strategy)
-		__gitcomp "$(__git_merge_strategies)"
+		__gitcomp "$__git_merge_strategylist"
 		return 0
 	esac
 	local cur="${COMP_WORDS[COMP_CWORD]}"
 	case "$cur" in
 	--strategy=*)
-		__gitcomp "$(__git_merge_strategies)" "" "${cur##--strategy=}"
+		__gitcomp "$__git_merge_strategylist" "" "${cur##--strategy=}"
 		return 0
 		;;
 	esac
 	return 1
 }
 
-__git_all_commands ()
-{
-	if [ -n "${__git_all_commandlist-}" ]; then
-		echo "$__git_all_commandlist"
-		return
-	fi
-	local i IFS=" "$'\n'
-	for i in $(git help -a|egrep '^ ')
-	do
-		case $i in
-		*--*)             : helper pattern;;
-		*) echo $i;;
-		esac
-	done
-}
-__git_all_commandlist=
-__git_all_commandlist="$(__git_all_commands 2>/dev/null)"
-
-__git_porcelain_commands ()
-{
-	if [ -n "${__git_porcelain_commandlist-}" ]; then
-		echo "$__git_porcelain_commandlist"
-		return
-	fi
-	local i IFS=" "$'\n'
-	for i in "help" $(__git_all_commands)
-	do
-		case $i in
-		*--*)             : helper pattern;;
-		applymbox)        : ask gittus;;
-		applypatch)       : ask gittus;;
-		archimport)       : import;;
-		cat-file)         : plumbing;;
-		check-attr)       : plumbing;;
-		check-ref-format) : plumbing;;
-		checkout-index)   : plumbing;;
-		commit-tree)      : plumbing;;
-		count-objects)    : infrequent;;
-		cvsexportcommit)  : export;;
-		cvsimport)        : import;;
-		cvsserver)        : daemon;;
-		daemon)           : daemon;;
-		diff-files)       : plumbing;;
-		diff-index)       : plumbing;;
-		diff-tree)        : plumbing;;
-		fast-import)      : import;;
-		fast-export)      : export;;
-		fsck-objects)     : plumbing;;
-		fetch-pack)       : plumbing;;
-		fmt-merge-msg)    : plumbing;;
-		for-each-ref)     : plumbing;;
-		hash-object)      : plumbing;;
-		http-*)           : transport;;
-		index-pack)       : plumbing;;
-		init-db)          : deprecated;;
-		local-fetch)      : plumbing;;
-		lost-found)       : infrequent;;
-		ls-files)         : plumbing;;
-		ls-remote)        : plumbing;;
-		ls-tree)          : plumbing;;
-		mailinfo)         : plumbing;;
-		mailsplit)        : plumbing;;
-		merge-*)          : plumbing;;
-		mktree)           : plumbing;;
-		mktag)            : plumbing;;
-		pack-objects)     : plumbing;;
-		pack-redundant)   : plumbing;;
-		pack-refs)        : plumbing;;
-		parse-remote)     : plumbing;;
-		patch-id)         : plumbing;;
-		peek-remote)      : plumbing;;
-		prune)            : plumbing;;
-		prune-packed)     : plumbing;;
-		quiltimport)      : import;;
-		read-tree)        : plumbing;;
-		receive-pack)     : plumbing;;
-		reflog)           : plumbing;;
-		repo-config)      : deprecated;;
-		rerere)           : plumbing;;
-		rev-list)         : plumbing;;
-		rev-parse)        : plumbing;;
-		runstatus)        : plumbing;;
-		sh-setup)         : internal;;
-		shell)            : daemon;;
-		show-ref)         : plumbing;;
-		send-pack)        : plumbing;;
-		show-index)       : plumbing;;
-		ssh-*)            : transport;;
-		stripspace)       : plumbing;;
-		symbolic-ref)     : plumbing;;
-		tar-tree)         : deprecated;;
-		unpack-file)      : plumbing;;
-		unpack-objects)   : plumbing;;
-		update-index)     : plumbing;;
-		update-ref)       : plumbing;;
-		update-server-info) : daemon;;
-		upload-archive)   : plumbing;;
-		upload-pack)      : plumbing;;
-		write-tree)       : plumbing;;
-		var)              : infrequent;;
-		verify-pack)      : infrequent;;
-		verify-tag)       : plumbing;;
-		*) echo $i;;
-		esac
-	done
-}
-__git_porcelain_commandlist=
-__git_porcelain_commandlist="$(__git_porcelain_commands 2>/dev/null)"
-
 __git_aliases ()
 {
 	local i IFS=$'\n'
@@ -1077,7 +960,7 @@ _git_help ()
 		return
 		;;
 	esac
-	__gitcomp "$(__git_all_commands)
+	__gitcomp "$__git_all_commandlist
 		attributes cli core-tutorial cvs-migration
 		diffcore gitk glossary hooks ignore modules
 		repository-layout tutorial tutorial-2
@@ -1423,7 +1306,7 @@ _git_config ()
 		return
 		;;
 	pull.twohead|pull.octopus)
-		__gitcomp "$(__git_merge_strategies)"
+		__gitcomp "$__git_merge_strategylist"
 		return
 		;;
 	color.branch|color.diff|color.interactive|\
@@ -1524,7 +1407,7 @@ _git_config ()
 	pager.*)
 		local pfx="${cur%.*}."
 		cur="${cur#*.}"
-		__gitcomp "$(__git_all_commands)" "$pfx" "$cur"
+		__gitcomp "$__git_all_commandlist" "$pfx" "$cur"
 		return
 		;;
 	remote.*.*)
@@ -2116,7 +1999,7 @@ _git ()
 			--help
 			"
 			;;
-		*)     __gitcomp "$(__git_porcelain_commands) $(__git_aliases)" ;;
+		*)     __gitcomp "$__git_porcelain_commandlist $(__git_aliases)" ;;
 		esac
 		return
 	fi
-- 
1.6.5.rc2.18.g84f98.dirty



From: Kirill Smelkov <kirr@mns.spb.ru>
Date: Fri, 9 Oct 2009 12:45:30 +0400
Subject: [PATCH 2/2] bash: make git-completion.bash.generate bash agnostic

We've just moved some code from git-completion.bash into
git-completion.bash.generate, but as Shawn O. Pearce notes, this code is
now used at compile time, so we shouldn't assume bash is avalable.

In more details: we used IFS=" "$'\n' which works in bash, but not e.g.
in dash, but it turns out that we can avoid setting IFS at all, look:

    $ ./git help -a | egrep '^ '
               [-p|--paginate|--no-pager]
               [--bare] [--git-dir=GIT_DIR] [--work-tree=GIT_WORK_TREE]
               [--help] COMMAND [ARGS]
      add                    get-tar-commit-id      rebase
      add--interactive       grep                   rebase--interactive
      am                     gui                    receive-pack
      ...

First, there are some unneedded lines, with e.g. [--bare], and this does
produce noise, since e.g. in bash

    $ for i in "[--bare]"; do echo $i ; done
    a b

so we kill it though grepping more explicitely:

    $ ./git help -a | egrep '^  [^ ]'
      add                    get-tar-commit-id      rebase
      add--interactive       grep                   rebase--interactive
      am                     gui                    receive-pack
      ...

And then, plain "for in in $(git help -a|egrep '^ [^ ]')" works in both
bash and dash, which was the goal.

Signed-off-by: Kirill Smelkov <kirr@mns.spb.ru>
---
 contrib/completion/git-completion.bash.generate |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/contrib/completion/git-completion.bash.generate b/contrib/completion/git-completion.bash.generate
index fa998dc..04460eb 100755
--- a/contrib/completion/git-completion.bash.generate
+++ b/contrib/completion/git-completion.bash.generate
@@ -1,4 +1,4 @@
-#!/bin/bash
+#!/bin/sh
 #
 # Generate bash completion for git.
 #
@@ -19,8 +19,8 @@ __git_merge_strategies ()
 
 __git_all_commands ()
 {
-	local i IFS=" "$'\n'
-	for i in $(git help -a|egrep '^ ')
+	local i
+	for i in $(git help -a|egrep '^  [^ ]')
 	do
 		case $i in
 		*--*)             : helper pattern;;
@@ -32,7 +32,7 @@ __git_all_commands ()
 
 __git_porcelain_commands ()
 {
-	local i IFS=" "$'\n'
+	local i
 	for i in "help" $(__git_all_commands)
 	do
 		case $i in
-- 
1.6.5.rc2.18.g84f98.dirty

Re: [PATCH] Speedup bash completion loading

[Shawn O. Pearce]

Kirill Smelkov <kirr@mns.spb.ru> wrote:
> On Thu, Oct 08, 2009 at 08:02:06AM -0700, Shawn O. Pearce wrote:
> > We probably should place a quick comment here to remind folks that
> > they need to build the script in order to test it properly.
> 
> I've added some sort of protection, so that git-completion.bash.in can't
> be sourced at all. Is it ok?

Yes, looks fine.

> Subject: [PATCH 2/2] bash: make git-completion.bash.generate bash agnostic

Squash this into the first patch and add the egrep change made by
Stephen Boyd "[PATCH 1/2] completion: fix completion of git <TAB><TAB>".
 
-- 
Shawn.

Re: [PATCH] Speedup bash completion loading

[Kirill Smelkov]

On Fri, Oct 09, 2009 at 07:46:06AM -0700, Shawn O. Pearce wrote:
> Kirill Smelkov <kirr@mns.spb.ru> wrote:
> > On Thu, Oct 08, 2009 at 08:02:06AM -0700, Shawn O. Pearce wrote:
> > > We probably should place a quick comment here to remind folks that
> > > they need to build the script in order to test it properly.
> > 
> > I've added some sort of protection, so that git-completion.bash.in can't
> > be sourced at all. Is it ok?
> 
> Yes, looks fine.

Thanks.

> > Subject: [PATCH 2/2] bash: make git-completion.bash.generate bash agnostic
> 
> Squash this into the first patch and add the egrep change made by
> Stephen Boyd "[PATCH 1/2] completion: fix completion of git <TAB><TAB>".

While it is technically easy to do so, isn't this a bad idea to squash
semantic changes and fixes into something which should be as close as
code movement? Especially Stephen's fix?

Sure you are the maintainer, but maybe let's apply this 3 patches as
separate ones? I don't insist - this is just my 1коп.

Kirill

Re: [PATCH] Speedup bash completion loading

[Shawn O. Pearce]

Kirill Smelkov <kirr@mns.spb.ru> wrote:
> On Fri, Oct 09, 2009 at 07:46:06AM -0700, Shawn O. Pearce wrote:
> > 
> > Squash this into the first patch and add the egrep change made by
> > Stephen Boyd "[PATCH 1/2] completion: fix completion of git <TAB><TAB>".
> 
> While it is technically easy to do so, isn't this a bad idea to squash
> semantic changes and fixes into something which should be as close as
> code movement?

The ejection forces the code to build with /bin/bash, unless you
apply the trivial fixes related to IFS to make it work with dash.
As they are trivial the tiny semantic change during the code move
is just cleaner.

> Especially Stephen's fix?

I expect Junio to apply his patch first, and then yours.  Since he
is changing a region you are moving, you need to account for it in
your patch, otherwise we have a conflict and/or a regression.
 
-- 
Shawn.

Re: [PATCH] Speedup bash completion loading

[Kirill Smelkov]

On Fri, Oct 09, 2009 at 08:39:36AM -0700, Shawn O. Pearce wrote:
> Kirill Smelkov <kirr@mns.spb.ru> wrote:
> > On Fri, Oct 09, 2009 at 07:46:06AM -0700, Shawn O. Pearce wrote:
> > > 
> > > Squash this into the first patch and add the egrep change made by
> > > Stephen Boyd "[PATCH 1/2] completion: fix completion of git <TAB><TAB>".
> > 
> > While it is technically easy to do so, isn't this a bad idea to squash
> > semantic changes and fixes into something which should be as close as
> > code movement?
> 
> The ejection forces the code to build with /bin/bash, unless you
> apply the trivial fixes related to IFS to make it work with dash.
> As they are trivial the tiny semantic change during the code move
> is just cleaner.
> 
> > Especially Stephen's fix?
> 
> I expect Junio to apply his patch first, and then yours.  Since he
> is changing a region you are moving, you need to account for it in
> your patch, otherwise we have a conflict and/or a regression.

Now I see, OK. Here is the patch which should be applied on top of
Stephen's 1/2:

---- 8< ----

From: Kirill Smelkov <kirr@mns.spb.ru>
Date: Mon, 5 Oct 2009 13:36:15 +0400
Subject: [PATCH v4] Speedup bash completion loading

On my slow laptop (P3 700MHz), system-wide bash completions take too
much time to load (> 1s), and significant fraction of this time is spent
loading git-completion.bash:

    $ time bash -c '. git-completion.bash'  # before this patch

    real    0m0.317s
    user    0m0.250s
    sys     0m0.060s

I've tracked down that the most time is spent warming up merge_strategy,
all_command & porcelain_command caches.

Initially I thought that since git is not used in each and every
interactive xterm, it would be perfectly ok to load completion support
with cold caches, and then load needed thing lazily.

But for me this strategy turned out to be difficult to implement in
simple and maintainable way -- bash does not provide a way to return values
from inside functions, so one will have to use e.g.

    ${__git_all_commandlist:=$(__git_all_commands)}

everywhere in place where $(__git_all_commands) we used before, so as
also Ted Pavlic suggested let's pregenerate everything at build time so
that we have nothing to compute at runtime when git-completion.bash
script is loaded.

The result is that loading completion is significantly faster now:

    $ time bash -c '. git-completion.bash'  # after this patch

    real    0m0.068s
    user    0m0.060s
    sys     0m0.010s

NOTE
----

As Shawn O. Pearce suggested, git-completion.bash.generate code is now
used at compile time, so we shouldn't assume bash is avalable there.

Hence, IFS tweak (not needed in the first place) in __git_all_commands()
was removed, for compile-time code to be bashism free.

Cc: Ted Pavlic <ted@tedpavlic.com>
Signed-off-by: Kirill Smelkov <kirr@mns.spb.ru>
---
 contrib/completion/.gitignore                      |    1 +
 contrib/completion/Makefile                        |   11 ++
 contrib/completion/git-completion.bash.generate    |  128 ++++++++++++++++
 ...{git-completion.bash => git-completion.bash.in} |  161 +++-----------------
 4 files changed, 162 insertions(+), 139 deletions(-)
 create mode 100644 contrib/completion/.gitignore
 create mode 100644 contrib/completion/Makefile
 create mode 100755 contrib/completion/git-completion.bash.generate
 rename contrib/completion/{git-completion.bash => git-completion.bash.in} (90%)
 mode change 100755 => 100644

diff --git a/contrib/completion/.gitignore b/contrib/completion/.gitignore
new file mode 100644
index 0000000..578e6a8
--- /dev/null
+++ b/contrib/completion/.gitignore
@@ -0,0 +1 @@
+git-completion.bash
diff --git a/contrib/completion/Makefile b/contrib/completion/Makefile
new file mode 100644
index 0000000..90aa225
--- /dev/null
+++ b/contrib/completion/Makefile
@@ -0,0 +1,11 @@
+all	: git-completion.bash
+
+
+git-completion.bash: git-completion.bash.in git-completion.bash.generate
+	# Generate completions for binaries we have just built
+	PATH="$(shell pwd)/../..:$$PATH" ./git-completion.bash.generate
+
+
+clean:
+	rm -f git-completion.bash
+
diff --git a/contrib/completion/git-completion.bash.generate b/contrib/completion/git-completion.bash.generate
new file mode 100755
index 0000000..ff64202
--- /dev/null
+++ b/contrib/completion/git-completion.bash.generate
@@ -0,0 +1,128 @@
+#!/bin/sh
+#
+# Generate bash completion for git.
+#
+# Precompute everything that can be known in advance at build time, so that
+# actual bash completion script is loaded faster.
+
+__git_merge_strategies ()
+{
+	git merge -s help 2>&1 |
+	sed -n -e '/[Aa]vailable strategies are: /,/^$/{
+		s/\.$//
+		s/.*://
+		s/^[ 	]*//
+		s/[ 	]*$//
+		p
+	}'
+}
+
+__git_all_commands ()
+{
+	local i
+	for i in $(git help -a|egrep '^  [a-zA-Z0-9]')
+	do
+		case $i in
+		*--*)             : helper pattern;;
+		*) echo $i;;
+		esac
+	done
+}
+
+
+__git_porcelain_commands ()
+{
+	local i
+	for i in "help" $(__git_all_commands)
+	do
+		case $i in
+		*--*)             : helper pattern;;
+		applymbox)        : ask gittus;;
+		applypatch)       : ask gittus;;
+		archimport)       : import;;
+		cat-file)         : plumbing;;
+		check-attr)       : plumbing;;
+		check-ref-format) : plumbing;;
+		checkout-index)   : plumbing;;
+		commit-tree)      : plumbing;;
+		count-objects)    : infrequent;;
+		cvsexportcommit)  : export;;
+		cvsimport)        : import;;
+		cvsserver)        : daemon;;
+		daemon)           : daemon;;
+		diff-files)       : plumbing;;
+		diff-index)       : plumbing;;
+		diff-tree)        : plumbing;;
+		fast-import)      : import;;
+		fast-export)      : export;;
+		fsck-objects)     : plumbing;;
+		fetch-pack)       : plumbing;;
+		fmt-merge-msg)    : plumbing;;
+		for-each-ref)     : plumbing;;
+		hash-object)      : plumbing;;
+		http-*)           : transport;;
+		index-pack)       : plumbing;;
+		init-db)          : deprecated;;
+		local-fetch)      : plumbing;;
+		lost-found)       : infrequent;;
+		ls-files)         : plumbing;;
+		ls-remote)        : plumbing;;
+		ls-tree)          : plumbing;;
+		mailinfo)         : plumbing;;
+		mailsplit)        : plumbing;;
+		merge-*)          : plumbing;;
+		mktree)           : plumbing;;
+		mktag)            : plumbing;;
+		pack-objects)     : plumbing;;
+		pack-redundant)   : plumbing;;
+		pack-refs)        : plumbing;;
+		parse-remote)     : plumbing;;
+		patch-id)         : plumbing;;
+		peek-remote)      : plumbing;;
+		prune)            : plumbing;;
+		prune-packed)     : plumbing;;
+		quiltimport)      : import;;
+		read-tree)        : plumbing;;
+		receive-pack)     : plumbing;;
+		reflog)           : plumbing;;
+		repo-config)      : deprecated;;
+		rerere)           : plumbing;;
+		rev-list)         : plumbing;;
+		rev-parse)        : plumbing;;
+		runstatus)        : plumbing;;
+		sh-setup)         : internal;;
+		shell)            : daemon;;
+		show-ref)         : plumbing;;
+		send-pack)        : plumbing;;
+		show-index)       : plumbing;;
+		ssh-*)            : transport;;
+		stripspace)       : plumbing;;
+		symbolic-ref)     : plumbing;;
+		tar-tree)         : deprecated;;
+		unpack-file)      : plumbing;;
+		unpack-objects)   : plumbing;;
+		update-index)     : plumbing;;
+		update-ref)       : plumbing;;
+		update-server-info) : daemon;;
+		upload-archive)   : plumbing;;
+		upload-pack)      : plumbing;;
+		write-tree)       : plumbing;;
+		var)              : infrequent;;
+		verify-pack)      : infrequent;;
+		verify-tag)       : plumbing;;
+		*) echo $i;;
+		esac
+	done
+}
+
+
+__git_merge_strategylist=$(__git_merge_strategies | tr '\n' ' ')
+__git_all_commandlist="$(__git_all_commands | tr '\n' ' ')"
+__git_porcelain_commandlist="$(__git_porcelain_commands | tr '\n' ' ')"
+
+
+sed -e "s/__GIT_MERGE_STRATEGYLIST/\"$__git_merge_strategylist\"/"	\
+    -e "s/__GIT_ALL_COMMANDLIST/\"$__git_all_commandlist\"/"	\
+    -e "s/__GIT_PORCELAIN_COMMANDLIST/\"$__git_porcelain_commandlist\"/"	\
+    git-completion.bash.in > git-completion.bash
+
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash.in
old mode 100755
new mode 100644
similarity index 90%
rename from contrib/completion/git-completion.bash
rename to contrib/completion/git-completion.bash.in
index 652a47c..67d03c3
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash.in
@@ -21,13 +21,7 @@
 #    2) Added the following line to your .bashrc:
 #        source ~/.git-completion.sh
 #
-#    3) You may want to make sure the git executable is available
-#       in your PATH before this script is sourced, as some caching
-#       is performed while the script loads.  If git isn't found
-#       at source time then all lookups will be done on demand,
-#       which may be slightly slower.
-#
-#    4) Consider changing your PS1 to also show the current branch:
+#    3) Consider changing your PS1 to also show the current branch:
 #        PS1='[\u@\h \W$(__git_ps1 " (%s)")]\$ '
 #
 #       The argument to __git_ps1 will be displayed only if you
@@ -60,6 +54,21 @@
 #       git@vger.kernel.org
 #
 
+
+# pregenerated stuff (to save load time)
+__git_merge_strategylist=__GIT_MERGE_STRATEGYLIST
+__git_all_commandlist=__GIT_ALL_COMMANDLIST
+__git_porcelain_commandlist=__GIT_PORCELAIN_COMMANDLIST
+
+# remind folks that git-completion.bash.in can't be sourced
+case "$__git_merge_strategylist" in
+__GIT*)
+	echo "E: git-completion.bash.in can't be sourced"
+	return 1 ;;
+esac
+
+
+
 case "$COMP_WORDBREAKS" in
 *:*) : great ;;
 *)   COMP_WORDBREAKS="$COMP_WORDBREAKS:"
@@ -324,23 +333,6 @@ __git_remotes ()
 	done
 }
 
-__git_merge_strategies ()
-{
-	if [ -n "${__git_merge_strategylist-}" ]; then
-		echo "$__git_merge_strategylist"
-		return
-	fi
-	git merge -s help 2>&1 |
-	sed -n -e '/[Aa]vailable strategies are: /,/^$/{
-		s/\.$//
-		s/.*://
-		s/^[ 	]*//
-		s/[ 	]*$//
-		p
-	}'
-}
-__git_merge_strategylist=
-__git_merge_strategylist=$(__git_merge_strategies 2>/dev/null)
 
 __git_complete_file ()
 {
@@ -476,128 +468,19 @@ __git_complete_strategy ()
 {
 	case "${COMP_WORDS[COMP_CWORD-1]}" in
 	-s|--strategy)
-		__gitcomp "$(__git_merge_strategies)"
+		__gitcomp "$__git_merge_strategylist"
 		return 0
 	esac
 	local cur="${COMP_WORDS[COMP_CWORD]}"
 	case "$cur" in
 	--strategy=*)
-		__gitcomp "$(__git_merge_strategies)" "" "${cur##--strategy=}"
+		__gitcomp "$__git_merge_strategylist" "" "${cur##--strategy=}"
 		return 0
 		;;
 	esac
 	return 1
 }
 
-__git_all_commands ()
-{
-	if [ -n "${__git_all_commandlist-}" ]; then
-		echo "$__git_all_commandlist"
-		return
-	fi
-	local i IFS=" "$'\n'
-	for i in $(git help -a|egrep '^  [a-zA-Z0-9]')
-	do
-		case $i in
-		*--*)             : helper pattern;;
-		*) echo $i;;
-		esac
-	done
-}
-__git_all_commandlist=
-__git_all_commandlist="$(__git_all_commands 2>/dev/null)"
-
-__git_porcelain_commands ()
-{
-	if [ -n "${__git_porcelain_commandlist-}" ]; then
-		echo "$__git_porcelain_commandlist"
-		return
-	fi
-	local i IFS=" "$'\n'
-	for i in "help" $(__git_all_commands)
-	do
-		case $i in
-		*--*)             : helper pattern;;
-		applymbox)        : ask gittus;;
-		applypatch)       : ask gittus;;
-		archimport)       : import;;
-		cat-file)         : plumbing;;
-		check-attr)       : plumbing;;
-		check-ref-format) : plumbing;;
-		checkout-index)   : plumbing;;
-		commit-tree)      : plumbing;;
-		count-objects)    : infrequent;;
-		cvsexportcommit)  : export;;
-		cvsimport)        : import;;
-		cvsserver)        : daemon;;
-		daemon)           : daemon;;
-		diff-files)       : plumbing;;
-		diff-index)       : plumbing;;
-		diff-tree)        : plumbing;;
-		fast-import)      : import;;
-		fast-export)      : export;;
-		fsck-objects)     : plumbing;;
-		fetch-pack)       : plumbing;;
-		fmt-merge-msg)    : plumbing;;
-		for-each-ref)     : plumbing;;
-		hash-object)      : plumbing;;
-		http-*)           : transport;;
-		index-pack)       : plumbing;;
-		init-db)          : deprecated;;
-		local-fetch)      : plumbing;;
-		lost-found)       : infrequent;;
-		ls-files)         : plumbing;;
-		ls-remote)        : plumbing;;
-		ls-tree)          : plumbing;;
-		mailinfo)         : plumbing;;
-		mailsplit)        : plumbing;;
-		merge-*)          : plumbing;;
-		mktree)           : plumbing;;
-		mktag)            : plumbing;;
-		pack-objects)     : plumbing;;
-		pack-redundant)   : plumbing;;
-		pack-refs)        : plumbing;;
-		parse-remote)     : plumbing;;
-		patch-id)         : plumbing;;
-		peek-remote)      : plumbing;;
-		prune)            : plumbing;;
-		prune-packed)     : plumbing;;
-		quiltimport)      : import;;
-		read-tree)        : plumbing;;
-		receive-pack)     : plumbing;;
-		reflog)           : plumbing;;
-		repo-config)      : deprecated;;
-		rerere)           : plumbing;;
-		rev-list)         : plumbing;;
-		rev-parse)        : plumbing;;
-		runstatus)        : plumbing;;
-		sh-setup)         : internal;;
-		shell)            : daemon;;
-		show-ref)         : plumbing;;
-		send-pack)        : plumbing;;
-		show-index)       : plumbing;;
-		ssh-*)            : transport;;
-		stripspace)       : plumbing;;
-		symbolic-ref)     : plumbing;;
-		tar-tree)         : deprecated;;
-		unpack-file)      : plumbing;;
-		unpack-objects)   : plumbing;;
-		update-index)     : plumbing;;
-		update-ref)       : plumbing;;
-		update-server-info) : daemon;;
-		upload-archive)   : plumbing;;
-		upload-pack)      : plumbing;;
-		write-tree)       : plumbing;;
-		var)              : infrequent;;
-		verify-pack)      : infrequent;;
-		verify-tag)       : plumbing;;
-		*) echo $i;;
-		esac
-	done
-}
-__git_porcelain_commandlist=
-__git_porcelain_commandlist="$(__git_porcelain_commands 2>/dev/null)"
-
 __git_aliases ()
 {
 	local i IFS=$'\n'
@@ -1077,7 +960,7 @@ _git_help ()
 		return
 		;;
 	esac
-	__gitcomp "$(__git_all_commands)
+	__gitcomp "$__git_all_commandlist
 		attributes cli core-tutorial cvs-migration
 		diffcore gitk glossary hooks ignore modules
 		repository-layout tutorial tutorial-2
@@ -1423,7 +1306,7 @@ _git_config ()
 		return
 		;;
 	pull.twohead|pull.octopus)
-		__gitcomp "$(__git_merge_strategies)"
+		__gitcomp "$__git_merge_strategylist"
 		return
 		;;
 	color.branch|color.diff|color.interactive|\
@@ -1524,7 +1407,7 @@ _git_config ()
 	pager.*)
 		local pfx="${cur%.*}."
 		cur="${cur#*.}"
-		__gitcomp "$(__git_all_commands)" "$pfx" "$cur"
+		__gitcomp "$__git_all_commandlist" "$pfx" "$cur"
 		return
 		;;
 	remote.*.*)
@@ -2116,7 +1999,7 @@ _git ()
 			--help
 			"
 			;;
-		*)     __gitcomp "$(__git_porcelain_commands) $(__git_aliases)" ;;
+		*)     __gitcomp "$__git_porcelain_commandlist $(__git_aliases)" ;;
 		esac
 		return
 	fi
-- 
1.6.5.rc2.18.g84f98.dirty

Re: [PATCH] Speedup bash completion loading

[Shawn O. Pearce]

Kirill Smelkov <kirr@mns.spb.ru> wrote:
> Now I see, OK. Here is the patch which should be applied on top of
> Stephen's 1/2:
> 
> ---- 8< ----
> From: Kirill Smelkov <kirr@mns.spb.ru>
> Date: Mon, 5 Oct 2009 13:36:15 +0400
> Subject: [PATCH v4] Speedup bash completion loading

Acked-by: Shawn O. Pearce <spearce@spearce.org>
 
>  contrib/completion/.gitignore                      |    1 +
>  contrib/completion/Makefile                        |   11 ++
>  contrib/completion/git-completion.bash.generate    |  128 ++++++++++++++++
>  ...{git-completion.bash => git-completion.bash.in} |  161 +++-----------------
>  4 files changed, 162 insertions(+), 139 deletions(-)
>  create mode 100644 contrib/completion/.gitignore
>  create mode 100644 contrib/completion/Makefile
>  create mode 100755 contrib/completion/git-completion.bash.generate
>  rename contrib/completion/{git-completion.bash => git-completion.bash.in} (90%)
>  mode change 100755 => 100644

-- 
Shawn.

Re: [PATCH] Speedup bash completion loading

[Junio C Hamano]

Thanks for this, and all the other acks and patches.