[PATCH 0/2] pre-generated completion fix and RFC

I should have looked at the actual generated completion before hastily
sending my last fix. Looks like we get the merge strategies twice in the
merge strategy list.

The second patch is a quick hack to have completion lazily load again
while still allowing pre-generated completion.

These are on top of next.

Stephen Boyd (2):
  completion: ignore custom merge strategies when pre-generating
  completion: allow use without compiling

 contrib/completion/git-completion.bash.generate |    1 +
 contrib/completion/git-completion.bash.in       |  132 +++++++++++++++++++++--
 2 files changed, 124 insertions(+), 9 deletions(-)

Since the Makefile adds the build directory to PATH, we get the merge
strategies twice. Ignore custom merge strategies which are just the
builtin merge strategies (octopus.sh, ours.sh, etc.) anyway.

Signed-off-by: Stephen Boyd <redacted>
---
 contrib/completion/git-completion.bash.generate |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/contrib/completion/git-completion.bash.generate b/contrib/completion/git-completion.bash.generate
index 6487fd5..b66254a 100755
--- a/contrib/completion/git-completion.bash.generate
+++ b/contrib/completion/git-completion.bash.generate

@@ -9,6 +9,7 @@ __git_merge_strategies ()
 {
 	git merge -s help 2>&1 |
 	sed -n -e '/[Aa]vailable strategies are: /,/^$/{
+		/^[Aa]vailable custom strategies are: / d
 		s/\.$//
 		s/.*://
 		s/^[ 	]*//

-- 
1.6.5.2.181.gd6f41

Some users don't want to compile their completion, even when the build
generated completion is 10x faster to load. For example, in my bashrc I
source the completion script directly so I can stay up to date with the
latest completion by merely pulling changes.

Do this by generating the lists dynamically when the merge strategy and
command lists still have their initial values (__GIT_MERGE_STRATEGYLIST,
__GIT_ALL_COMMANDLIST).

Signed-off-by: Stephen Boyd <redacted>
---

 This duplicates code, but I don't know of a way to re-use the dynamic
 code without sourcing a bash script and possibly breaking someone's build.

 contrib/completion/git-completion.bash.in |  132 +++++++++++++++++++++++++++--
 1 files changed, 123 insertions(+), 9 deletions(-)

diff --git a/contrib/completion/git-completion.bash.in b/contrib/completion/git-completion.bash.in
index d873b98..37d204c 100644
--- a/contrib/completion/git-completion.bash.in
+++ b/contrib/completion/git-completion.bash.in

@@ -60,15 +60,6 @@ __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:"

@@ -333,6 +324,22 @@ __git_remotes ()
 	done
 }
 
+__git_merge_strategies ()
+{
+	if ["$__git_merge_strategylist" != "__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_strategies 2>/dev/null)"
 
 __git_complete_file ()
 {

@@ -481,6 +488,113 @@ __git_complete_strategy ()
 	return 1
 }
 
+__git_all_commands ()
+{
+	if ["$__git_all_commandlist" != "__GIT_ALL_COMMANDLIST"]; then
+		echo "$__git_all_commandlist"
+		return
+	fi
+	local i
+	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_commands 2>/dev/null)"
+
+__git_porcelain_commands ()
+{
+	if ["$__git_porcelain_commandlist" != "__GIT_PORCELAIN_COMMANDLIST"]; then
+		echo "$__git_porcelain_commandlist"
+		return
+	fi
+	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_porcelain_commandlist="$(__git_porcelain_commands 2>/dev/null)"
+
 __git_aliases ()
 {
 	local i IFS=$'\n'

-- 
1.6.5.2.181.gd6f41

Stephen Boyd [off-list ref] writes:

Some users don't want to compile their completion, even when the build
generated completion is 10x faster to load. For example, in my bashrc I
source the completion script directly so I can stay up to date with the
latest completion by merely pulling changes.

Do this by generating the lists dynamically when the merge strategy and
command lists still have their initial values (__GIT_MERGE_STRATEGYLIST,
__GIT_ALL_COMMANDLIST).

Signed-off-by: Stephen Boyd <redacted>
---

 This duplicates code, but I don't know of a way to re-use the dynamic
 code without sourcing a bash script and possibly breaking someone's build.

If we are going to do this, wouldn't it make more sense to revert the
rename of the script, so that people can keep relying on the name of the
script being "git-completion.bash", _but_ make it produce a pre-compiled
form to a separate file when invoked in some particular way?

Then at the runtime:

 (0) If the script notices that it has already learned the command list
     it uses it; otherwise,

 (1) If the script notices that there is a file that contains the command
     list, it sources it; otherwise,

 (2) The script lazily builds the command list for its own use.

And at the buildtime, Makefile can run the script in "generation mode",
and install the output to where (1) above expects to see.

On Mon, Oct 26, 2009 at 04:59:18PM -0700, Junio C Hamano wrote:

Stephen Boyd [off-list ref] writes:

quoted

 This duplicates code, but I don't know of a way to re-use the dynamic
 code without sourcing a bash script and possibly breaking someone's build.

 (1) If the script notices that there is a file that contains the command
     list, it sources it; otherwise,

Or we substitute the command list in-place, so that we still have the entire
completion script in one file.

Clemens

Junio C Hamano wrote:

If we are going to do this, wouldn't it make more sense to revert the
rename of the script, so that people can keep relying on the name of the
script being "git-completion.bash", _but_ make it produce a pre-compiled
form to a separate file when invoked in some particular way?

Wouldn't relying on "git-completion.bash" to produce the pre-compiled
form cause problems if someone is running the build on a bash-less
system? I thought this issue was already raised by Shawn.

I guess we could ignore that issue now, and just say that you have to
build the pre-compiled form on systems with bash?

Then at the runtime:

 (0) If the script notices that it has already learned the command list
     it uses it; otherwise,

 (1) If the script notices that there is a file that contains the command
     list, it sources it; otherwise,

 (2) The script lazily builds the command list for its own use.

And at the buildtime, Makefile can run the script in "generation mode",
and install the output to where (1) above expects to see.

I assume you're suggesting this to ease the upgrade path for users. It
works nicely, we could just install the generated lists in the same path
(contrib/completion/) and then users would be free to copy the two files
anywhere as long as they're in the same directory. The only downside I
see is there's now two files, but that's ok with me.

Stephen Boyd [off-list ref] wrote:

Some users don't want to compile their completion, even when the build
generated completion is 10x faster to load. For example, in my bashrc I
source the completion script directly so I can stay up to date with the
latest completion by merely pulling changes.

Do this by generating the lists dynamically when the merge strategy and
command lists still have their initial values (__GIT_MERGE_STRATEGYLIST,
__GIT_ALL_COMMANDLIST).

Signed-off-by: Stephen Boyd <redacted>
---

 This duplicates code, but I don't know of a way to re-use the dynamic
 code without sourcing a bash script and possibly breaking someone's build.

NAK on code duplication, especially this much.  As Junio already
pointed out in this thread we need an approach that doesn't cause
this sort of redundant code.

I'm trying to catch up on email right now.  I have no great
suggestions on how to implement this to avoid the code duplication
and still be able to support both compile-time and on-the-fly
computation, but I do know I don't want this code twice.

-- 
Shawn.

Shawn O. Pearce wrote:

NAK on code duplication, especially this much.  As Junio already
pointed out in this thread we need an approach that doesn't cause
this sort of redundant code.

Ok. Following Junio's suggestion I think we would have to do the following:

(1) Revert the rename (git-completion.bash.in -> git-completion.bash)

(2) Add a "generation" mode to git-completion.bash.generate to generate 
the lists and output them to a file

(3) Add logic in git-completion.bash.generate to source the generated 
file if it exists

(4) Source git-completion.bash.generate in git-completion.bash to get 
the functions moved there

In the end we would have git-completion.bash sourcing 
git-completion.bash.generate which then sources the generated file. I 
assume this is slower than compiling to just one file.

Or we could just not load the caches until they're needed. This just 
delays the performance hit to completion time, but at least it speeds up 
loading the script without the need to compile and still has the benefit 
of some caching. It also allows users to keep the completion of their 
custom merge strategies and git programs in their PATH.

----8<----

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index d3fec32..8a7cde3 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash

@@ -326,10 +326,15 @@ __git_remotes ()
 
 __git_merge_strategies ()
 {
-	if [ -n "${__git_merge_strategylist-}" ]; then
+	if [ -n "${__git_merge_strategylist-}" ] || {
+		__git_merge_strategylist=$(__git_merge_strategies_gen 2>/dev/null)
+	}; then
 		echo "$__git_merge_strategylist"
-		return
 	fi
+}
+
+__git_merge_strategies_gen ()
+{
 	git merge -s help 2>&1 |
 	sed -n -e '/[Aa]vailable strategies are: /,/^$/{
 		s/\.$//

@@ -340,7 +345,6 @@ __git_merge_strategies ()
 	}'
 }
 __git_merge_strategylist=
-__git_merge_strategylist=$(__git_merge_strategies 2>/dev/null)
 
 __git_complete_file ()
 {

@@ -491,10 +495,15 @@ __git_complete_strategy ()
 
 __git_all_commands ()
 {
-	if [ -n "${__git_all_commandlist-}" ]; then
+	if [ -n "${__git_all_commandlist-}" ] || {
+		__git_all_commandlist="$(__git_all_commands_gen 2>/dev/null)"
+	}; then
 		echo "$__git_all_commandlist"
-		return
 	fi
+}
+
+__git_all_commands_gen ()
+{
 	local i IFS=" "$'\n'
 	for i in $(git help -a|egrep '^  [a-zA-Z0-9]')
 	do

@@ -505,14 +514,18 @@ __git_all_commands ()
 	done
 }
 __git_all_commandlist=
-__git_all_commandlist="$(__git_all_commands 2>/dev/null)"
 
 __git_porcelain_commands ()
 {
-	if [ -n "${__git_porcelain_commandlist-}" ]; then
+	if [ -n "${__git_porcelain_commandlist-}" ] || {
+		__git_porcelain_commandlist="$(__git_porcelain_commands_gen 2>/dev/null)"
+	}; then
 		echo "$__git_porcelain_commandlist"
-		return
 	fi
+}
+
+__git_porcelain_commands_gen ()
+{
 	local i IFS=" "$'\n'
 	for i in "help" $(__git_all_commands)
 	do

@@ -596,7 +609,6 @@ __git_porcelain_commands ()
 	done
 }
 __git_porcelain_commandlist=
-__git_porcelain_commandlist="$(__git_porcelain_commands 2>/dev/null)"
 
 __git_aliases ()
 {

Stephen Boyd [off-list ref] writes:

Ok. Following Junio's suggestion I think we would have to do the following:

(1) Revert the rename (git-completion.bash.in -> git-completion.bash)

(2) Add a "generation" mode to git-completion.bash.generate to
generate the lists and output them to a file

(3) Add logic in git-completion.bash.generate to source the generated
file if it exists

(4) Source git-completion.bash.generate in git-completion.bash to get
the functions moved there

Sorry, I do not quite see why an extra *.generate script is necessary.

Junio C Hamano wrote:

Stephen Boyd [off-list ref] writes:

quoted

Ok. Following Junio's suggestion I think we would have to do the following:

(1) Revert the rename (git-completion.bash.in -> git-completion.bash)

(2) Add a "generation" mode to git-completion.bash.generate to
generate the lists and output them to a file

(3) Add logic in git-completion.bash.generate to source the generated
file if it exists

(4) Source git-completion.bash.generate in git-completion.bash to get
the functions moved there

Sorry, I do not quite see why an extra *.generate script is necessary.

I'm still assuming the generation mode has to be sh agnostic, therefore 
requiring those functions to be in another *.generate script. Is that wrong?