threads / patch / 21362

patch, 2 partspre-generated completion fix and RFC

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

## tl;dr

12 messages between Oct 26, 2009 and Oct 28, 2009. Diffs are folded; open one to read it.

replies: 11people: 4as markdown or json

Stephen Boyd· Oct 26, 2009, 20:31 UTC · lore

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(-)
Stephen Boyd· Oct 26, 2009, 20:31 UTC · re: Stephen Boyd · lore

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

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(-)
Show changes to contrib/completion/git-completion.bash.generate +1 −0
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
Stephen Boyd· Oct 26, 2009, 20:31 UTC · re: Stephen Boyd · lore

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

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(-)
Show changes to contrib/completion/git-completion.bash.in +123 −9
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
Junio C Hamano· Oct 26, 2009, 23:59 UTC · re: Stephen Boyd · lore

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

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

Clemens Buchacher· Oct 27, 2009, 00:33 UTC · re: Junio C Hamano · lore

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

On Mon, Oct 26, 2009 at 04:59:18PM -0700, Junio C Hamano wrote:
Show 7 quoted lines
> 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
Junio C Hamano· Oct 27, 2009, 00:38 UTC · re: Clemens Buchacher · lore

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

Clemens Buchacher <drizzd@aon.at> writes:
Show 12 quoted lines
> 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.

Clemens Buchacher· Oct 28, 2009, 08:19 UTC · re: Junio C Hamano · lore

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

On Mon, Oct 26, 2009 at 05:38:10PM -0700, Junio C Hamano wrote:
Show 17 quoted lines
> 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
Stephen Boyd· Oct 27, 2009, 02:49 UTC · re: Junio C Hamano · lore

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

Junio C Hamano wrote:
Show 5 quoted lines
>
> 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?

Show 13 quoted lines
>
> 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.

Shawn O. Pearce· Oct 27, 2009, 18:52 UTC · re: Stephen Boyd · lore

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

Stephen Boyd <bebarino@gmail.com> wrote:
Show 14 quoted lines
> 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.
Stephen Boyd· Oct 28, 2009, 01:43 UTC · re: Shawn O. Pearce · lore

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

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<----
Show changes to contrib/completion/git-completion.bash +21 −9
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 ()
 {
Junio C Hamano· Oct 28, 2009, 07:18 UTC · re: Stephen Boyd · lore

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

Stephen Boyd <bebarino@gmail.com> writes:
Show 12 quoted lines
> 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.
Stephen Boyd· Oct 28, 2009, 07:29 UTC · re: Junio C Hamano · lore

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

Junio C Hamano wrote:
Show 15 quoted lines
> 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?

← back to recent threads