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

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

[Stephen Boyd]

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

[PATCH 1/2] completion: ignore custom merge strategies when pre-generating

[Stephen Boyd]

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 <bebarino@gmail.com>
---
 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

[PATCH/RFC 2/2] completion: allow use without compiling

[Stephen Boyd]

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 <bebarino@gmail.com>
---

 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

Re: [PATCH/RFC 2/2] completion: allow use without compiling

[Junio C Hamano]

Stephen Boyd <bebarino@gmail.com> 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 <bebarino@gmail.com>
> ---
>
>  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.

Re: [PATCH/RFC 2/2] completion: allow use without compiling

[Clemens Buchacher]

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

> Stephen Boyd <bebarino@gmail.com> writes:
> 
> >  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

Re: [PATCH/RFC 2/2] completion: allow use without compiling

[Junio C Hamano]

Clemens Buchacher <drizzd@aon.at> writes:

> On Mon, Oct 26, 2009 at 04:59:18PM -0700, Junio C Hamano wrote:
>
>> Stephen Boyd <bebarino@gmail.com> writes:
>> 
>> >  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.

That defeats the whole point of my suggestion, as you would be overwriting
the tracked file.

Re: [PATCH/RFC 2/2] completion: allow use without compiling

[Clemens Buchacher]

On Mon, Oct 26, 2009 at 05:38:10PM -0700, Junio C Hamano wrote:
> Clemens Buchacher <drizzd@aon.at> writes:
> 
> > On Mon, Oct 26, 2009 at 04:59:18PM -0700, Junio C Hamano wrote:
> >
> >> Stephen Boyd <bebarino@gmail.com> writes:
> >> 
> >> >  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.
> 
> That defeats the whole point of my suggestion, as you would be overwriting
> the tracked file.

Ok, not in-place then. What I meant is something like this.

git-completion.bash.in:

 completion script with placeholders for command list generation code and
 static command list

git-command-list.sh:

 bash-agnostic command list generation script

git-completion.bash.generate:

 run git-command-list.sh and replace static command list placeholder in
 git-completion.bash.in, also insert command list generation code into
 git-completion.bash.in, write result to git-completion.bash

Whether or not the command list should be loaded dynamically can be decided
on a per-user basis using a configuration option.

A downside of this approach is that even if we do not need the static
command list, we still need to generate the completion script. I therefore
recommend we make this part of the normal build process.

Alternatively, we could source the command list code. But it's inconvenient
to copy two completion scripts and it will probably cause confusion among
users.

Clemens

Re: [PATCH/RFC 2/2] completion: allow use without compiling

[Stephen Boyd]

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.

Re: [PATCH/RFC 2/2] completion: allow use without compiling

[Shawn O. Pearce]

Stephen Boyd <bebarino@gmail.com> 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 <bebarino@gmail.com>
> ---
> 
>  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.

Re: [PATCH/RFC 2/2] completion: allow use without compiling

[Stephen Boyd]

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 ()
 {

Re: [PATCH/RFC 2/2] completion: allow use without compiling

[Junio C Hamano]

Stephen Boyd <bebarino@gmail.com> 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.

Re: [PATCH/RFC 2/2] completion: allow use without compiling

[Stephen Boyd]

Junio C Hamano wrote:
> Stephen Boyd <bebarino@gmail.com> 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.

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?