git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/9] ref completion optimizations, fixes, and cleanups
@ 2011-10-08 14:54 SZEDER Gábor
  2011-10-08 14:54 ` [PATCH 1/9] completion: document __gitcomp() SZEDER Gábor
                   ` (8 more replies)
  0 siblings, 9 replies; 16+ messages in thread
From: SZEDER Gábor @ 2011-10-08 14:54 UTC (permalink / raw)
  To: git; +Cc: Shawn O. Pearce, Jonathan Nieder, Junio C Hamano,
	SZEDER Gábor

Hi,

This series aims to improve the completion of refs & co.

This one is the most important in the series; it takes some shortcuts
to make completing large number of refs faster (it's also faster for
git.git, but it's unnoticeable).

  [2/9] completion: optimize refs completion

The following three make __git_refs() handle local and remote
repositories more consistently, and also clean up the remote-handling
code part of __git_refs().  They likely make things a bit faster, but
since the code path usually involves network communication I didn't
run any benchmarks.

  [3/9] completion: make refs completion consistent for local and remote
          repos
  [4/9] completion: improve ls-remote output filtering in __git_refs()
  [5/9] completion: support full refs from remote repositories

The following two do similar cleanups in __git_refs_remotes() than 3/9
and 4/9 in __git_refs().

  [6/9] completion: query only refs/heads/ in __git_refs_remotes()
  [7/9] completion: improve ls-remote output filtering in
          __git_refs_remotes()

A silly while-at-it optimization; the delay eliminated by this one was
annoying when testing 6/9 and 7/9.

  [8/9] completion: fast initial completion for config 'remote.*.fetch'
          value

And finally remove some bitrotted code.

  [9/9] completion: remove broken dead code from __git_heads() and
          __git_tags()


This series is meant to be applied on the merge of master and 77653abd
(completion: commit --fixup and --squash, 2011-10-06) from pu, and the
patch in

  Message-ID: <20111008010634.GB11561@goldbirke>
  (http://article.gmane.org/gmane.comp.version-control.git/183131)

from last night applied.  There will be two easily fixable conflicts
when applied directly on top of current master.


Best,
Gábor


SZEDER Gábor (9):
  completion: document __gitcomp()
  completion: optimize refs completion
  completion: make refs completion consistent for local and remote
    repos
  completion: improve ls-remote output filtering in __git_refs()
  completion: support full refs from remote repositories
  completion: query only refs/heads/ in __git_refs_remotes()
  completion: improve ls-remote output filtering in
    __git_refs_remotes()
  completion: fast initial completion for config 'remote.*.fetch' value
  completion: remove broken dead code from __git_heads() and
    __git_tags()

 contrib/completion/git-completion.bash |  200 +++++++++++++++++---------------
 1 files changed, 109 insertions(+), 91 deletions(-)

-- 
1.7.7.187.ga41de

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 1/9] completion: document __gitcomp()
  2011-10-08 14:54 [PATCH 0/9] ref completion optimizations, fixes, and cleanups SZEDER Gábor
@ 2011-10-08 14:54 ` SZEDER Gábor
  2011-10-08 14:54 ` [PATCH 2/9] completion: optimize refs completion SZEDER Gábor
                   ` (7 subsequent siblings)
  8 siblings, 0 replies; 16+ messages in thread
From: SZEDER Gábor @ 2011-10-08 14:54 UTC (permalink / raw)
  To: git; +Cc: Shawn O. Pearce, Jonathan Nieder, Junio C Hamano,
	SZEDER Gábor

I always forget which argument is which, and got tired of figuring it
out over and over again.

Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
---
 contrib/completion/git-completion.bash |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index b36f9e70..c0fb6e15 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -485,8 +485,13 @@ _get_comp_words_by_ref ()
 fi
 fi
 
-# __gitcomp accepts 1, 2, 3, or 4 arguments
-# generates completion reply with compgen
+# Generates completion reply with compgen, appending a space to possible
+# completion words, if necessary.
+# It accepts 1 to 4 arguments:
+# 1: List of possible completion words.
+# 2: A prefix to be added to each possible completion word (optional).
+# 3: Generate possible completion matches for this word (optional).
+# 4: A suffix to be appended to each possible completion word (optional).
 __gitcomp ()
 {
 	local cur_="$cur"
-- 
1.7.7.187.ga41de

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 2/9] completion: optimize refs completion
  2011-10-08 14:54 [PATCH 0/9] ref completion optimizations, fixes, and cleanups SZEDER Gábor
  2011-10-08 14:54 ` [PATCH 1/9] completion: document __gitcomp() SZEDER Gábor
@ 2011-10-08 14:54 ` SZEDER Gábor
  2011-10-13  0:50   ` Junio C Hamano
  2011-10-14 12:16   ` SZEDER Gábor
  2011-10-08 14:54 ` [PATCH 3/9] completion: make refs completion consistent for local and remote repos SZEDER Gábor
                   ` (6 subsequent siblings)
  8 siblings, 2 replies; 16+ messages in thread
From: SZEDER Gábor @ 2011-10-08 14:54 UTC (permalink / raw)
  To: git; +Cc: Shawn O. Pearce, Jonathan Nieder, Junio C Hamano,
	SZEDER Gábor

After a unique command or option is completed, in most cases it is a
good thing to add a trailing a space, but sometimes it doesn't makes
sense, e.g. when the completed word is an option taking an argument
('--option=') or a configuration section ('core.').  Therefore the
completion script uses the '-o nospace' option to prevent bash from
automatically appending a space to unique completions, and it has the
__gitcomp() function to add that trailing space only when necessary.
See 72e5e989 (bash: Add space after unique command name is completed.,
2007-02-04), 78d4d6a2 (bash: Support unique completion on git-config.,
2007-02-04), and b3391775 (bash: Support unique completion when
possible., 2007-02-04).

__gitcomp() therefore iterates over all possible completion words it
got as argument, and checks each word whether a trailing space is
necessary or not.  This is ok for commands, options, etc., i.e. when
the number of words is relatively small, but can be noticeably slow
for large number of refs.  However, while options might or might not
need that trailing space, refs are always handled uniformly and always
get that trailing space (or a trailing '.' for 'git config
branch.<head>.').  Since refs listed by __git_refs() & co. are
separated by newline, this allows us some optimizations with
'compgen'.

So, add a specialized variant of __gitcomp() that only deals with
possible completion words separated by a newline and uniformly appends
the trailing space to all words using 'compgen -S' (or any other
suffix, if specified), so no iteration over all words is done.
Convert all callsites of __gitcomp() where it's called with refs, i.e.
when it gets the output of either __git_refs(), __git_heads(),
__git_tags(), __git_refs2(), __git_refs_remotes(), or the odd 'git
for-each-ref' somewhere in _git_config().  Also convert callsites
where it gets other uniformly handled newline separated word lists,
i.e. either remotes from __git_remotes(), names of set configuration
variables from __git_config_get_set_variables(), stashes, or commands
and aliases.

Here are some timing results for dealing with 10000 refs.
Before:

  $ refs="$(__git_refs ~/tmp/git/repo-with-10k-refs/)"
  $ time __gitcomp "$refs"

  real	0m1.134s
  user	0m1.060s
  sys	0m0.130s

After:

  $ time __gitcomp_nl "$refs"

  real	0m0.373s
  user	0m0.360s
  sys	0m0.020s

Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
---
 contrib/completion/git-completion.bash |  116 +++++++++++++++++++-------------
 1 files changed, 70 insertions(+), 46 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index c0fb6e15..86de0bf4 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -512,6 +512,30 @@ __gitcomp ()
 	esac
 }
 
+# Generates completion reply with compgen.
+# It accepts 1 to 4 arguments:
+# 1: List of possible completion words, separated by a single newline.
+# 2: A prefix to be added to each possible completion word (optional).
+# 3: Generate possible completion matches for this word (optional).
+# 4: A suffix to be appended to each possible completion word (optional).
+#    If omitted, a space is appended; if specified but empty, nothing is
+#    appended.
+__gitcomp_nl ()
+{
+	local s=$'\n' IFS=' '$'\t'$'\n'
+	local cur_="$cur" suffix=" "
+
+	if [ $# -gt 2 ]; then
+		cur_="$3"
+		if [ $# -gt 3 ]; then
+			suffix="$4"
+		fi
+	fi
+
+	IFS=$s
+	COMPREPLY=($(compgen -P "${2-}" -S "$suffix" -W "$1" -- "$cur_"))
+}
+
 # __git_heads accepts 0 or 1 arguments (to pass to __gitdir)
 __git_heads ()
 {
@@ -716,15 +740,15 @@ __git_complete_revlist_file ()
 	*...*)
 		pfx="${cur_%...*}..."
 		cur_="${cur_#*...}"
-		__gitcomp "$(__git_refs)" "$pfx" "$cur_"
+		__gitcomp_nl "$(__git_refs)" "$pfx" "$cur_"
 		;;
 	*..*)
 		pfx="${cur_%..*}.."
 		cur_="${cur_#*..}"
-		__gitcomp "$(__git_refs)" "$pfx" "$cur_"
+		__gitcomp_nl "$(__git_refs)" "$pfx" "$cur_"
 		;;
 	*)
-		__gitcomp "$(__git_refs)"
+		__gitcomp_nl "$(__git_refs)"
 		;;
 	esac
 }
@@ -764,7 +788,7 @@ __git_complete_remote_or_refspec ()
 		c=$((++c))
 	done
 	if [ -z "$remote" ]; then
-		__gitcomp "$(__git_remotes)"
+		__gitcomp_nl "$(__git_remotes)"
 		return
 	fi
 	if [ $no_complete_refspec = 1 ]; then
@@ -789,23 +813,23 @@ __git_complete_remote_or_refspec ()
 	case "$cmd" in
 	fetch)
 		if [ $lhs = 1 ]; then
-			__gitcomp "$(__git_refs2 "$remote")" "$pfx" "$cur_"
+			__gitcomp_nl "$(__git_refs2 "$remote")" "$pfx" "$cur_"
 		else
-			__gitcomp "$(__git_refs)" "$pfx" "$cur_"
+			__gitcomp_nl "$(__git_refs)" "$pfx" "$cur_"
 		fi
 		;;
 	pull)
 		if [ $lhs = 1 ]; then
-			__gitcomp "$(__git_refs "$remote")" "$pfx" "$cur_"
+			__gitcomp_nl "$(__git_refs "$remote")" "$pfx" "$cur_"
 		else
-			__gitcomp "$(__git_refs)" "$pfx" "$cur_"
+			__gitcomp_nl "$(__git_refs)" "$pfx" "$cur_"
 		fi
 		;;
 	push)
 		if [ $lhs = 1 ]; then
-			__gitcomp "$(__git_refs)" "$pfx" "$cur_"
+			__gitcomp_nl "$(__git_refs)" "$pfx" "$cur_"
 		else
-			__gitcomp "$(__git_refs "$remote")" "$pfx" "$cur_"
+			__gitcomp_nl "$(__git_refs "$remote")" "$pfx" "$cur_"
 		fi
 		;;
 	esac
@@ -1084,7 +1108,7 @@ _git_archive ()
 		return
 		;;
 	--remote=*)
-		__gitcomp "$(__git_remotes)" "" "${cur##--remote=}"
+		__gitcomp_nl "$(__git_remotes)" "" "${cur##--remote=}"
 		return
 		;;
 	--*)
@@ -1115,7 +1139,7 @@ _git_bisect ()
 
 	case "$subcommand" in
 	bad|good|reset|skip|start)
-		__gitcomp "$(__git_refs)"
+		__gitcomp_nl "$(__git_refs)"
 		;;
 	*)
 		COMPREPLY=()
@@ -1146,9 +1170,9 @@ _git_branch ()
 		;;
 	*)
 		if [ $only_local_ref = "y" -a $has_r = "n" ]; then
-			__gitcomp "$(__git_heads)"
+			__gitcomp_nl "$(__git_heads)"
 		else
-			__gitcomp "$(__git_refs)"
+			__gitcomp_nl "$(__git_refs)"
 		fi
 		;;
 	esac
@@ -1195,7 +1219,7 @@ _git_checkout ()
 		if [ -n "$(__git_find_on_cmdline "$flags")" ]; then
 			track=''
 		fi
-		__gitcomp "$(__git_refs '' $track)"
+		__gitcomp_nl "$(__git_refs '' $track)"
 		;;
 	esac
 }
@@ -1212,7 +1236,7 @@ _git_cherry_pick ()
 		__gitcomp "--edit --no-commit"
 		;;
 	*)
-		__gitcomp "$(__git_refs)"
+		__gitcomp_nl "$(__git_refs)"
 		;;
 	esac
 }
@@ -1266,7 +1290,7 @@ _git_commit ()
 		;;
 	--reuse-message=*|--reedit-message=*|\
 	--fixup=*|--squash=*)
-		__gitcomp "$(__git_refs)" "" "${cur#*=}"
+		__gitcomp_nl "$(__git_refs)" "" "${cur#*=}"
 		return
 		;;
 	--untracked-files=*)
@@ -1297,7 +1321,7 @@ _git_describe ()
 			"
 		return
 	esac
-	__gitcomp "$(__git_refs)"
+	__gitcomp_nl "$(__git_refs)"
 }
 
 __git_diff_common_options="--stat --numstat --shortstat --summary
@@ -1456,7 +1480,7 @@ _git_grep ()
 		;;
 	esac
 
-	__gitcomp "$(__git_refs)"
+	__gitcomp_nl "$(__git_refs)"
 }
 
 _git_help ()
@@ -1514,7 +1538,7 @@ _git_ls_files ()
 
 _git_ls_remote ()
 {
-	__gitcomp "$(__git_remotes)"
+	__gitcomp_nl "$(__git_remotes)"
 }
 
 _git_ls_tree ()
@@ -1610,7 +1634,7 @@ _git_merge ()
 		__gitcomp "$__git_merge_options"
 		return
 	esac
-	__gitcomp "$(__git_refs)"
+	__gitcomp_nl "$(__git_refs)"
 }
 
 _git_mergetool ()
@@ -1630,7 +1654,7 @@ _git_mergetool ()
 
 _git_merge_base ()
 {
-	__gitcomp "$(__git_refs)"
+	__gitcomp_nl "$(__git_refs)"
 }
 
 _git_mv ()
@@ -1661,7 +1685,7 @@ _git_notes ()
 	,*)
 		case "${words[cword-1]}" in
 		--ref)
-			__gitcomp "$(__git_refs)"
+			__gitcomp_nl "$(__git_refs)"
 			;;
 		*)
 			__gitcomp "$subcommands --ref"
@@ -1670,7 +1694,7 @@ _git_notes ()
 		;;
 	add,--reuse-message=*|append,--reuse-message=*|\
 	add,--reedit-message=*|append,--reedit-message=*)
-		__gitcomp "$(__git_refs)" "" "${cur#*=}"
+		__gitcomp_nl "$(__git_refs)" "" "${cur#*=}"
 		;;
 	add,--*|append,--*)
 		__gitcomp '--file= --message= --reedit-message=
@@ -1689,7 +1713,7 @@ _git_notes ()
 		-m|-F)
 			;;
 		*)
-			__gitcomp "$(__git_refs)"
+			__gitcomp_nl "$(__git_refs)"
 			;;
 		esac
 		;;
@@ -1717,12 +1741,12 @@ _git_push ()
 {
 	case "$prev" in
 	--repo)
-		__gitcomp "$(__git_remotes)"
+		__gitcomp_nl "$(__git_remotes)"
 		return
 	esac
 	case "$cur" in
 	--repo=*)
-		__gitcomp "$(__git_remotes)" "" "${cur##--repo=}"
+		__gitcomp_nl "$(__git_remotes)" "" "${cur##--repo=}"
 		return
 		;;
 	--*)
@@ -1760,7 +1784,7 @@ _git_rebase ()
 
 		return
 	esac
-	__gitcomp "$(__git_refs)"
+	__gitcomp_nl "$(__git_refs)"
 }
 
 _git_reflog ()
@@ -1771,7 +1795,7 @@ _git_reflog ()
 	if [ -z "$subcommand" ]; then
 		__gitcomp "$subcommands"
 	else
-		__gitcomp "$(__git_refs)"
+		__gitcomp_nl "$(__git_refs)"
 	fi
 }
 
@@ -1853,23 +1877,23 @@ _git_config ()
 {
 	case "$prev" in
 	branch.*.remote)
-		__gitcomp "$(__git_remotes)"
+		__gitcomp_nl "$(__git_remotes)"
 		return
 		;;
 	branch.*.merge)
-		__gitcomp "$(__git_refs)"
+		__gitcomp_nl "$(__git_refs)"
 		return
 		;;
 	remote.*.fetch)
 		local remote="${prev#remote.}"
 		remote="${remote%.fetch}"
-		__gitcomp "$(__git_refs_remotes "$remote")"
+		__gitcomp_nl "$(__git_refs_remotes "$remote")"
 		return
 		;;
 	remote.*.push)
 		local remote="${prev#remote.}"
 		remote="${remote%.push}"
-		__gitcomp "$(git --git-dir="$(__gitdir)" \
+		__gitcomp_nl "$(git --git-dir="$(__gitdir)" \
 			for-each-ref --format='%(refname):%(refname)' \
 			refs/heads)"
 		return
@@ -1916,7 +1940,7 @@ _git_config ()
 		return
 		;;
 	--get|--get-all|--unset|--unset-all)
-		__gitcomp "$(__git_config_get_set_variables)"
+		__gitcomp_nl "$(__git_config_get_set_variables)"
 		return
 		;;
 	*.*)
@@ -1942,7 +1966,7 @@ _git_config ()
 		;;
 	branch.*)
 		local pfx="${cur%.*}." cur_="${cur#*.}"
-		__gitcomp "$(__git_heads)" "$pfx" "$cur_" "."
+		__gitcomp_nl "$(__git_heads)" "$pfx" "$cur_" "."
 		return
 		;;
 	guitool.*.*)
@@ -1971,7 +1995,7 @@ _git_config ()
 	pager.*)
 		local pfx="${cur%.*}." cur_="${cur#*.}"
 		__git_compute_all_commands
-		__gitcomp "$__git_all_commands" "$pfx" "$cur_"
+		__gitcomp_nl "$__git_all_commands" "$pfx" "$cur_"
 		return
 		;;
 	remote.*.*)
@@ -1984,7 +2008,7 @@ _git_config ()
 		;;
 	remote.*)
 		local pfx="${cur%.*}." cur_="${cur#*.}"
-		__gitcomp "$(__git_remotes)" "$pfx" "$cur_" "."
+		__gitcomp_nl "$(__git_remotes)" "$pfx" "$cur_" "."
 		return
 		;;
 	url.*.*)
@@ -2285,7 +2309,7 @@ _git_remote ()
 
 	case "$subcommand" in
 	rename|rm|show|prune)
-		__gitcomp "$(__git_remotes)"
+		__gitcomp_nl "$(__git_remotes)"
 		;;
 	update)
 		local i c='' IFS=$'\n'
@@ -2303,7 +2327,7 @@ _git_remote ()
 
 _git_replace ()
 {
-	__gitcomp "$(__git_refs)"
+	__gitcomp_nl "$(__git_refs)"
 }
 
 _git_reset ()
@@ -2316,7 +2340,7 @@ _git_reset ()
 		return
 		;;
 	esac
-	__gitcomp "$(__git_refs)"
+	__gitcomp_nl "$(__git_refs)"
 }
 
 _git_revert ()
@@ -2327,7 +2351,7 @@ _git_revert ()
 		return
 		;;
 	esac
-	__gitcomp "$(__git_refs)"
+	__gitcomp_nl "$(__git_refs)"
 }
 
 _git_rm ()
@@ -2426,7 +2450,7 @@ _git_stash ()
 			COMPREPLY=()
 			;;
 		show,*|apply,*|drop,*|pop,*|branch,*)
-			__gitcomp "$(git --git-dir="$(__gitdir)" stash list \
+			__gitcomp_nl "$(git --git-dir="$(__gitdir)" stash list \
 					| sed -n -e 's/:.*//p')"
 			;;
 		*)
@@ -2560,7 +2584,7 @@ _git_tag ()
 		i="${words[c]}"
 		case "$i" in
 		-d|-v)
-			__gitcomp "$(__git_tags)"
+			__gitcomp_nl "$(__git_tags)"
 			return
 			;;
 		-f)
@@ -2576,13 +2600,13 @@ _git_tag ()
 		;;
 	-*|tag)
 		if [ $f = 1 ]; then
-			__gitcomp "$(__git_tags)"
+			__gitcomp_nl "$(__git_tags)"
 		else
 			COMPREPLY=()
 		fi
 		;;
 	*)
-		__gitcomp "$(__git_refs)"
+		__gitcomp_nl "$(__git_refs)"
 		;;
 	esac
 }
@@ -2635,7 +2659,7 @@ _git ()
 			"
 			;;
 		*)     __git_compute_porcelain_commands
-		       __gitcomp "$__git_porcelain_commands $(__git_aliases)" ;;
+		       __gitcomp_nl "$__git_porcelain_commands $(__git_aliases)" ;;
 		esac
 		return
 	fi
-- 
1.7.7.187.ga41de

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 3/9] completion: make refs completion consistent for local and remote repos
  2011-10-08 14:54 [PATCH 0/9] ref completion optimizations, fixes, and cleanups SZEDER Gábor
  2011-10-08 14:54 ` [PATCH 1/9] completion: document __gitcomp() SZEDER Gábor
  2011-10-08 14:54 ` [PATCH 2/9] completion: optimize refs completion SZEDER Gábor
@ 2011-10-08 14:54 ` SZEDER Gábor
  2011-10-08 14:54 ` [PATCH 4/9] completion: improve ls-remote output filtering in __git_refs() SZEDER Gábor
                   ` (5 subsequent siblings)
  8 siblings, 0 replies; 16+ messages in thread
From: SZEDER Gábor @ 2011-10-08 14:54 UTC (permalink / raw)
  To: git; +Cc: Shawn O. Pearce, Jonathan Nieder, Junio C Hamano,
	SZEDER Gábor

For a local repository the __git_refs() completion helper function
lists refs under 'refs/(tags|heads|remotes)/', plus some special refs
like HEAD and ORIG_HEAD.  For a remote repository, however, it lists
all refs.

Fix this inconsistency by specifying refs filter patterns for 'git
ls-remote' to only list refs under 'refs/(tags|heads|remotes)/'.

For now this makes it impossible to complete refs outside of
'refs/(tags|heads|remotes)/' in a remote repository, but a followup
patch will resurrect that.

Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
---
 contrib/completion/git-completion.bash |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 86de0bf4..6b5dc5cd 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -615,13 +615,11 @@ __git_refs ()
 		fi
 		return
 	fi
-	for i in $(git ls-remote "$dir" 2>/dev/null); do
+	for i in $(git ls-remote "$dir" HEAD ORIG_HEAD 'refs/tags/*' 'refs/heads/*' 'refs/remotes/*' 2>/dev/null); do
 		case "$is_hash,$i" in
 		y,*) is_hash=n ;;
 		n,*^{}) is_hash=y ;;
-		n,refs/tags/*) is_hash=y; echo "${i#refs/tags/}" ;;
-		n,refs/heads/*) is_hash=y; echo "${i#refs/heads/}" ;;
-		n,refs/remotes/*) is_hash=y; echo "${i#refs/remotes/}" ;;
+		n,refs/*) is_hash=y; echo "${i#refs/*/}" ;;
 		n,*) is_hash=y; echo "$i" ;;
 		esac
 	done
-- 
1.7.7.187.ga41de

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 4/9] completion: improve ls-remote output filtering in __git_refs()
  2011-10-08 14:54 [PATCH 0/9] ref completion optimizations, fixes, and cleanups SZEDER Gábor
                   ` (2 preceding siblings ...)
  2011-10-08 14:54 ` [PATCH 3/9] completion: make refs completion consistent for local and remote repos SZEDER Gábor
@ 2011-10-08 14:54 ` SZEDER Gábor
  2011-10-08 14:54 ` [PATCH 5/9] completion: support full refs from remote repositories SZEDER Gábor
                   ` (4 subsequent siblings)
  8 siblings, 0 replies; 16+ messages in thread
From: SZEDER Gábor @ 2011-10-08 14:54 UTC (permalink / raw)
  To: git; +Cc: Shawn O. Pearce, Jonathan Nieder, Junio C Hamano,
	SZEDER Gábor

The remote-handling part of __git_refs() has a nice for loop and state
machine case statement to iterate over all words from the output of
'git ls-remote' to identify object names and ref names.  Since each
line in the output of 'git ls-remote' consists of an object name and a
ref name, we can do more effective filtering by using a while-read
loop and letting bash's word splitting take care of object names.
This way the code is easier to understand and the loop will need only
half the number of iterations than before.

Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
---
 contrib/completion/git-completion.bash |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 6b5dc5cd..c6ab742d 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -579,7 +579,7 @@ __git_tags ()
 # by checkout for tracking branches
 __git_refs ()
 {
-	local i is_hash=y dir="$(__gitdir "${1-}")" track="${2-}"
+	local i hash dir="$(__gitdir "${1-}")" track="${2-}"
 	local format refs
 	if [ -d "$dir" ]; then
 		case "$cur" in
@@ -615,12 +615,12 @@ __git_refs ()
 		fi
 		return
 	fi
-	for i in $(git ls-remote "$dir" HEAD ORIG_HEAD 'refs/tags/*' 'refs/heads/*' 'refs/remotes/*' 2>/dev/null); do
-		case "$is_hash,$i" in
-		y,*) is_hash=n ;;
-		n,*^{}) is_hash=y ;;
-		n,refs/*) is_hash=y; echo "${i#refs/*/}" ;;
-		n,*) is_hash=y; echo "$i" ;;
+	git ls-remote "$dir" HEAD ORIG_HEAD 'refs/tags/*' 'refs/heads/*' 'refs/remotes/*' 2>/dev/null | \
+	while read hash i; do
+		case "$i" in
+		*^{}) ;;
+		refs/*) echo "${i#refs/*/}" ;;
+		*) echo "$i" ;;
 		esac
 	done
 }
-- 
1.7.7.187.ga41de

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 5/9] completion: support full refs from remote repositories
  2011-10-08 14:54 [PATCH 0/9] ref completion optimizations, fixes, and cleanups SZEDER Gábor
                   ` (3 preceding siblings ...)
  2011-10-08 14:54 ` [PATCH 4/9] completion: improve ls-remote output filtering in __git_refs() SZEDER Gábor
@ 2011-10-08 14:54 ` SZEDER Gábor
  2011-10-08 14:54 ` [PATCH 6/9] completion: query only refs/heads/ in __git_refs_remotes() SZEDER Gábor
                   ` (3 subsequent siblings)
  8 siblings, 0 replies; 16+ messages in thread
From: SZEDER Gábor @ 2011-10-08 14:54 UTC (permalink / raw)
  To: git; +Cc: Shawn O. Pearce, Jonathan Nieder, Junio C Hamano,
	SZEDER Gábor

When the __git_refs() completion helper function lists refs from a
local repository, it usually lists the refs' short name, except when
it needs to provide completion for words starting with refs, because
in that case it lists full ref names, see 608efb87 (bash: complete
full refs, 2008-11-28).

Add the same functionality to the code path dealing with remote
repositories, too.

Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
---
 contrib/completion/git-completion.bash |   29 +++++++++++++++++++++--------
 1 files changed, 21 insertions(+), 8 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index c6ab742d..a8d3597e 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -615,14 +615,27 @@ __git_refs ()
 		fi
 		return
 	fi
-	git ls-remote "$dir" HEAD ORIG_HEAD 'refs/tags/*' 'refs/heads/*' 'refs/remotes/*' 2>/dev/null | \
-	while read hash i; do
-		case "$i" in
-		*^{}) ;;
-		refs/*) echo "${i#refs/*/}" ;;
-		*) echo "$i" ;;
-		esac
-	done
+	case "$cur" in
+	refs|refs/*)
+		git ls-remote "$dir" "$cur*" 2>/dev/null | \
+		while read hash i; do
+			case "$i" in
+			*^{}) ;;
+			*) echo "$i" ;;
+			esac
+		done
+		;;
+	*)
+		git ls-remote "$dir" HEAD ORIG_HEAD 'refs/tags/*' 'refs/heads/*' 'refs/remotes/*' 2>/dev/null | \
+		while read hash i; do
+			case "$i" in
+			*^{}) ;;
+			refs/*) echo "${i#refs/*/}" ;;
+			*) echo "$i" ;;
+			esac
+		done
+		;;
+	esac
 }
 
 # __git_refs2 requires 1 argument (to pass to __git_refs)
-- 
1.7.7.187.ga41de

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 6/9] completion: query only refs/heads/ in __git_refs_remotes()
  2011-10-08 14:54 [PATCH 0/9] ref completion optimizations, fixes, and cleanups SZEDER Gábor
                   ` (4 preceding siblings ...)
  2011-10-08 14:54 ` [PATCH 5/9] completion: support full refs from remote repositories SZEDER Gábor
@ 2011-10-08 14:54 ` SZEDER Gábor
  2011-10-08 14:54 ` [PATCH 7/9] completion: improve ls-remote output filtering " SZEDER Gábor
                   ` (2 subsequent siblings)
  8 siblings, 0 replies; 16+ messages in thread
From: SZEDER Gábor @ 2011-10-08 14:54 UTC (permalink / raw)
  To: git; +Cc: Shawn O. Pearce, Jonathan Nieder, Junio C Hamano,
	SZEDER Gábor

__git_refs_remotes() is used to provide completion for refspecs to set
'remote.*.fetch' config variables for branches on the given remote.
So it's really only interested in refs under 'refs/heads/', but it
queries the remote for all its refs and then filters out all refs
outside of 'refs/heads/'.

Let 'git ls-remote' do the filtering.

Also remove the unused $cmd variable from __git_refs_remotes().

Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
---
 contrib/completion/git-completion.bash |   13 +++++--------
 1 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index a8d3597e..dc1d5e90 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -650,17 +650,14 @@ __git_refs2 ()
 # __git_refs_remotes requires 1 argument (to pass to ls-remote)
 __git_refs_remotes ()
 {
-	local cmd i is_hash=y
-	for i in $(git ls-remote "$1" 2>/dev/null); do
-		case "$is_hash,$i" in
-		n,refs/heads/*)
+	local i is_hash=y
+	for i in $(git ls-remote "$1" 'refs/heads/*' 2>/dev/null); do
+		case "$is_hash" in
+		n)
 			is_hash=y
 			echo "$i:refs/remotes/$1/${i#refs/heads/}"
 			;;
-		y,*) is_hash=n ;;
-		n,*^{}) is_hash=y ;;
-		n,refs/tags/*) is_hash=y;;
-		n,*) is_hash=y; ;;
+		y) is_hash=n ;;
 		esac
 	done
 }
-- 
1.7.7.187.ga41de

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 7/9] completion: improve ls-remote output filtering in __git_refs_remotes()
  2011-10-08 14:54 [PATCH 0/9] ref completion optimizations, fixes, and cleanups SZEDER Gábor
                   ` (5 preceding siblings ...)
  2011-10-08 14:54 ` [PATCH 6/9] completion: query only refs/heads/ in __git_refs_remotes() SZEDER Gábor
@ 2011-10-08 14:54 ` SZEDER Gábor
  2011-10-08 14:54 ` [PATCH 8/9] completion: fast initial completion for config 'remote.*.fetch' value SZEDER Gábor
  2011-10-08 14:54 ` [PATCH 9/9] completion: remove broken dead code from __git_heads() and __git_tags() SZEDER Gábor
  8 siblings, 0 replies; 16+ messages in thread
From: SZEDER Gábor @ 2011-10-08 14:54 UTC (permalink / raw)
  To: git; +Cc: Shawn O. Pearce, Jonathan Nieder, Junio C Hamano,
	SZEDER Gábor

This follows suit of a previous patch for __git_refs(): use a
while-read loop and let bash's word splitting get rid of object names
from 'git ls-remote's output.

Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
---
 contrib/completion/git-completion.bash |   13 ++++---------
 1 files changed, 4 insertions(+), 9 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index dc1d5e90..658df3a7 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -650,15 +650,10 @@ __git_refs2 ()
 # __git_refs_remotes requires 1 argument (to pass to ls-remote)
 __git_refs_remotes ()
 {
-	local i is_hash=y
-	for i in $(git ls-remote "$1" 'refs/heads/*' 2>/dev/null); do
-		case "$is_hash" in
-		n)
-			is_hash=y
-			echo "$i:refs/remotes/$1/${i#refs/heads/}"
-			;;
-		y) is_hash=n ;;
-		esac
+	local i hash
+	git ls-remote "$1" 'refs/heads/*' 2>/dev/null | \
+	while read hash i; do
+		echo "$i:refs/remotes/$1/${i#refs/heads/}"
 	done
 }
 
-- 
1.7.7.187.ga41de

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 8/9] completion: fast initial completion for config 'remote.*.fetch' value
  2011-10-08 14:54 [PATCH 0/9] ref completion optimizations, fixes, and cleanups SZEDER Gábor
                   ` (6 preceding siblings ...)
  2011-10-08 14:54 ` [PATCH 7/9] completion: improve ls-remote output filtering " SZEDER Gábor
@ 2011-10-08 14:54 ` SZEDER Gábor
  2011-10-08 14:54 ` [PATCH 9/9] completion: remove broken dead code from __git_heads() and __git_tags() SZEDER Gábor
  8 siblings, 0 replies; 16+ messages in thread
From: SZEDER Gábor @ 2011-10-08 14:54 UTC (permalink / raw)
  To: git; +Cc: Shawn O. Pearce, Jonathan Nieder, Junio C Hamano,
	SZEDER Gábor

Refspecs for branches in a remote repository start with 'refs/heads/',
so completing those refspecs with 'git config remote.origin.fetch
<TAB>' always offers 'refs/heads/' first, because that's the unique
part of the possible refspecs.  But it does so only after querying the
remote with 'git ls-remote', which can take a while when the request
goes through some slower network to a remote server.

Don't waste the user's time and offer 'refs/heads/' right away for
'git config remote.origin.fetch <TAB>'.

The reason for putting 'refs/heads/' directly into COMPREPLY instead
of using __gitcomp() is to avoid __gitcomp() adding a trailing space.

Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
---
 contrib/completion/git-completion.bash |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 658df3a7..d7151220 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1890,6 +1890,10 @@ _git_config ()
 	remote.*.fetch)
 		local remote="${prev#remote.}"
 		remote="${remote%.fetch}"
+		if [ -z "$cur" ]; then
+			COMPREPLY=("refs/heads/")
+			return
+		fi
 		__gitcomp_nl "$(__git_refs_remotes "$remote")"
 		return
 		;;
-- 
1.7.7.187.ga41de

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 9/9] completion: remove broken dead code from __git_heads() and __git_tags()
  2011-10-08 14:54 [PATCH 0/9] ref completion optimizations, fixes, and cleanups SZEDER Gábor
                   ` (7 preceding siblings ...)
  2011-10-08 14:54 ` [PATCH 8/9] completion: fast initial completion for config 'remote.*.fetch' value SZEDER Gábor
@ 2011-10-08 14:54 ` SZEDER Gábor
  8 siblings, 0 replies; 16+ messages in thread
From: SZEDER Gábor @ 2011-10-08 14:54 UTC (permalink / raw)
  To: git; +Cc: Shawn O. Pearce, Jonathan Nieder, Junio C Hamano,
	SZEDER Gábor

__git_heads() was introduced in 5de40f5 (Teach bash about
git-repo-config., 2006-11-27), and __git_tags() in 88e21dc (Teach bash
about completing arguments for git-tag, 2007-08-31).  As their name
suggests, __git_heads() is supposed to list only branches, and
__git_tags() only tags.

Since their introduction both of these functions consist of two
distinct parts.  The first part gets branches or tags, respectively,
from a local repositoty using 'git for-each-ref'.  The second part
queries a remote repository given as argument using 'git ls-remote'.

These remote-querying parts are broken in both functions since their
introduction, because they list both branches and tags from the remote
repository.  (The 'git ls-remote' query is not limited to list only
heads or tags, respectively, and the for loop filtering the query
results prints everything except dereferenced tags.)  This breakage
could be easily fixed by passing the '--heads' or '--tags' options or
appropriate refs patterns to the 'git ls-remote' invocations.

However, that no one noticed this breakage yet is probably not a
coincidence: neither of these two functions were used to query a
remote repository, the remote-querying parts were dead code already
upon thier introduction and remained dead ever since.

Since those parts of code are broken, are and were never used, stop
the bit-rotting and remove them.

Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
---
 contrib/completion/git-completion.bash |   22 ++--------------------
 1 files changed, 2 insertions(+), 20 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index d7151220..802b703d 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -536,42 +536,24 @@ __gitcomp_nl ()
 	COMPREPLY=($(compgen -P "${2-}" -S "$suffix" -W "$1" -- "$cur_"))
 }
 
-# __git_heads accepts 0 or 1 arguments (to pass to __gitdir)
 __git_heads ()
 {
-	local cmd i is_hash=y dir="$(__gitdir "${1-}")"
+	local dir="$(__gitdir)"
 	if [ -d "$dir" ]; then
 		git --git-dir="$dir" for-each-ref --format='%(refname:short)' \
 			refs/heads
 		return
 	fi
-	for i in $(git ls-remote "${1-}" 2>/dev/null); do
-		case "$is_hash,$i" in
-		y,*) is_hash=n ;;
-		n,*^{}) is_hash=y ;;
-		n,refs/heads/*) is_hash=y; echo "${i#refs/heads/}" ;;
-		n,*) is_hash=y; echo "$i" ;;
-		esac
-	done
 }
 
-# __git_tags accepts 0 or 1 arguments (to pass to __gitdir)
 __git_tags ()
 {
-	local cmd i is_hash=y dir="$(__gitdir "${1-}")"
+	local dir="$(__gitdir)"
 	if [ -d "$dir" ]; then
 		git --git-dir="$dir" for-each-ref --format='%(refname:short)' \
 			refs/tags
 		return
 	fi
-	for i in $(git ls-remote "${1-}" 2>/dev/null); do
-		case "$is_hash,$i" in
-		y,*) is_hash=n ;;
-		n,*^{}) is_hash=y ;;
-		n,refs/tags/*) is_hash=y; echo "${i#refs/tags/}" ;;
-		n,*) is_hash=y; echo "$i" ;;
-		esac
-	done
 }
 
 # __git_refs accepts 0, 1 (to pass to __gitdir), or 2 arguments
-- 
1.7.7.187.ga41de

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/9] completion: optimize refs completion
  2011-10-08 14:54 ` [PATCH 2/9] completion: optimize refs completion SZEDER Gábor
@ 2011-10-13  0:50   ` Junio C Hamano
  2011-10-13 10:40     ` SZEDER Gábor
  2011-10-14 12:16   ` SZEDER Gábor
  1 sibling, 1 reply; 16+ messages in thread
From: Junio C Hamano @ 2011-10-13  0:50 UTC (permalink / raw)
  To: SZEDER Gábor; +Cc: git, Shawn O. Pearce, Jonathan Nieder

SZEDER Gábor <szeder@ira.uka.de> writes:

> After a unique command or option is completed, in most cases it is a
> good thing to add a trailing a space, but sometimes it doesn't makes

s/makes/make/;

> __gitcomp() therefore iterates over all possible completion words it
> got as argument, and checks each word whether a trailing space is
> necessary or not.  This is ok for commands, options, etc., i.e. when
> the number of words is relatively small, but can be noticeably slow
> for large number of refs.  However, while options might or might not
> need that trailing space, refs are always handled uniformly and always
> get that trailing space (or a trailing '.' for 'git config
> branch.<head>.').
> ...
> So, add a specialized variant of __gitcomp() that only deals with
> possible completion words separated by a newline and uniformly appends
> the trailing space to all words using 'compgen -S' (or any other
> suffix, if specified), so no iteration over all words is done.

s/is done./is needed./;

I think I followed your logic (very well written ;-), but feel somewhat
dirty, as you are conflating the "These things are separated with newlines"
with "These things do not need inspection --- they all need suffix", which
has one obvious drawback --- you may find other class of words that always
want a SP after each of them but the source that generates such a class of
words may not separate the list elements with a newline.

Because a ref cannot have $IFS whitespace in its name anyway, I think you
can rename __gitcomp_nl to a name that conveys more clearly what it does
(i.e. "complete and always append suffix"), drop the IFS fiddling from the
function, and get the same optimization, no?

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/9] completion: optimize refs completion
  2011-10-13  0:50   ` Junio C Hamano
@ 2011-10-13 10:40     ` SZEDER Gábor
  2011-10-13 17:28       ` Junio C Hamano
  0 siblings, 1 reply; 16+ messages in thread
From: SZEDER Gábor @ 2011-10-13 10:40 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Shawn O. Pearce, Jonathan Nieder

On Wed, Oct 12, 2011 at 05:50:38PM -0700, Junio C Hamano wrote:
> SZEDER Gábor <szeder@ira.uka.de> writes:
> 
> > After a unique command or option is completed, in most cases it is a
> > good thing to add a trailing a space, but sometimes it doesn't makes
> 
> s/makes/make/;
> 
> > __gitcomp() therefore iterates over all possible completion words it
> > got as argument, and checks each word whether a trailing space is
> > necessary or not.  This is ok for commands, options, etc., i.e. when
> > the number of words is relatively small, but can be noticeably slow
> > for large number of refs.  However, while options might or might not
> > need that trailing space, refs are always handled uniformly and always
> > get that trailing space (or a trailing '.' for 'git config
> > branch.<head>.').
> > ...
> > So, add a specialized variant of __gitcomp() that only deals with
> > possible completion words separated by a newline and uniformly appends
> > the trailing space to all words using 'compgen -S' (or any other
> > suffix, if specified), so no iteration over all words is done.
> 
> s/is done./is needed./;
> 
> I think I followed your logic (very well written ;-)

Thanks; learned it around here ;)

> but feel somewhat
> dirty, as you are conflating the "These things are separated with newlines"
> with "These things do not need inspection --- they all need suffix", which
> has one obvious drawback --- you may find other class of words that always
> want a SP after each of them but the source that generates such a class of
> words may not separate the list elements with a newline.

Yes, there are a couple of other places where SP is uniformly needed,
for example completion of subcommands for bisect, notes, stash, etc.,
merge strategies, whitespace options, which are all separated by SP,
or help topics, which are separated by SP, TAB, and NL.  However, it
really is necessary that no SP is used to separate those words, see
below, so we can't use this optimization in these cases.  And since
the number of possible completion words in these cases is usually low,
it doesn't worth the effort to restructure those words to not use SP
separator, because it doesn't really make a performance difference
anyway.

> Because a ref cannot have $IFS whitespace in its name anyway, I think you
> can rename __gitcomp_nl to a name that conveys more clearly what it does
> (i.e. "complete and always append suffix"), drop the IFS fiddling from the
> function, and get the same optimization, no?

Unfortunately, this optimization depends on the IFS fiddling, because
we want to append a SP.  The same IFS trick is done in __gitcomp(),
too.  If we use the default IFS containing an SP and append a SP to
possible completion words by 'compgen -S " "' (or by word="$word ", as
in __gitcomp_1()), then that SP will be promply stripped off when
compgen's output is stored in the COMPREPLY array.  Using an IFS
without SP keeps those SP suffixes.  Perhaps I should've mentioned
this explicitly in the commit message, but didn't do so because one of
the referenced commit messages (72e5e989 (bash: Add space after unique
command name is completed., 2007-02-04)) already mentioned it briefly.

But when we use an IFS without SP, that also implies that we can't
pass words separated by SP to __gitcomp_nl(), because those words
won't be split at SPs anymore.  Since refs & co. are separated by NL,
it was the obvious choice for this special-purpose IFS.  So this
optimization can't work with the class of words mentioned above.  

So I thought that it's important to stress that this function can only
deal with NL separated words, hence I named it __gitcomp_nl().  But I
see your point about naming it after what it actually does, so I'm
fine with __gitcomp_add_suffix() or whatever else that indicates
"complete and always append suffix".

Will resend in a day or two, to leave some time for other suggestions.


Best,
Gábor

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/9] completion: optimize refs completion
  2011-10-13 10:40     ` SZEDER Gábor
@ 2011-10-13 17:28       ` Junio C Hamano
  0 siblings, 0 replies; 16+ messages in thread
From: Junio C Hamano @ 2011-10-13 17:28 UTC (permalink / raw)
  To: SZEDER Gábor; +Cc: git, Shawn O. Pearce, Jonathan Nieder

SZEDER Gábor <szeder@ira.uka.de> writes:

> ... If we use the default IFS containing an SP and append a SP to
> possible completion words by 'compgen -S " "' (or by word="$word ", as
> in __gitcomp_1()), then that SP will be promply stripped off when
> compgen's output is stored in the COMPREPLY array.  Using an IFS
> without SP keeps those SP suffixes.

Ahh, then I am perfectly fine with the "nl" in its name.

This is a function that unconditionally completes with trailing separator,
avoiding the cost of having to inspect each element, but the user needs to
keep in mind that the elements in the input must be separated with NL, so
ideally the name needs to convey both of these points---dropping "nl" was
a bad suggestion.

Thanks for a clear explanation.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/9] completion: optimize refs completion
  2011-10-08 14:54 ` [PATCH 2/9] completion: optimize refs completion SZEDER Gábor
  2011-10-13  0:50   ` Junio C Hamano
@ 2011-10-14 12:16   ` SZEDER Gábor
  2011-10-15 12:57     ` [PATCH 2/9 v2] " SZEDER Gábor
  1 sibling, 1 reply; 16+ messages in thread
From: SZEDER Gábor @ 2011-10-14 12:16 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Shawn O. Pearce, Jonathan Nieder

On Sat, Oct 08, 2011 at 04:54:36PM +0200, SZEDER Gábor wrote:
> So, add a specialized variant of __gitcomp() that only deals with
> possible completion words separated by a newline 

> @@ -2635,7 +2659,7 @@ _git ()
>  			"
>  			;;
>  		*)     __git_compute_porcelain_commands
> -		       __gitcomp "$__git_porcelain_commands $(__git_aliases)" ;;
> +		       __gitcomp_nl "$__git_porcelain_commands $(__git_aliases)" ;;
>  		esac
>  		return
>  	fi

Oops, this last hunk is wrong.

I made the thinko that $__git_porcelain_commands is NL-separated and
the output of __git_aliases() is NL-separated, so we can pass the two
together to the new __gitcomp_nl() function.  But of course not,
because the SP between the two joins the last command and the first
alias.

I will resend in the evening with this hunk removed and the commit
message updated.


Gábor

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 2/9 v2] completion: optimize refs completion
  2011-10-14 12:16   ` SZEDER Gábor
@ 2011-10-15 12:57     ` SZEDER Gábor
  2011-10-16  3:29       ` Junio C Hamano
  0 siblings, 1 reply; 16+ messages in thread
From: SZEDER Gábor @ 2011-10-15 12:57 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Shawn O. Pearce, Jonathan Nieder, SZEDER Gábor

After a unique command or option is completed, in most cases it is a
good thing to add a trailing a space, but sometimes it doesn't make
sense, e.g. when the completed word is an option taking an argument
('--option=') or a configuration section ('core.').  Therefore the
completion script uses the '-o nospace' option to prevent bash from
automatically appending a space to unique completions, and it has the
__gitcomp() function to add that trailing space only when necessary.
See 72e5e989 (bash: Add space after unique command name is completed.,
2007-02-04), 78d4d6a2 (bash: Support unique completion on git-config.,
2007-02-04), and b3391775 (bash: Support unique completion when
possible., 2007-02-04).

__gitcomp() therefore iterates over all possible completion words it
got as argument, and checks each word whether a trailing space is
necessary or not.  This is ok for commands, options, etc., i.e. when
the number of words is relatively small, but can be noticeably slow
for large number of refs.  However, while options might or might not
need that trailing space, refs are always handled uniformly and always
get that trailing space (or a trailing '.' for 'git config
branch.<head>.').  Since refs listed by __git_refs() & co. are
separated by newline, this allows us some optimizations with
'compgen'.

So, add a specialized variant of __gitcomp() that only deals with
possible completion words separated by a newline and uniformly appends
the trailing space to all words using 'compgen -S " "' (or any other
suffix, if specified), so no iteration over all words is needed.  But
we need to fiddle with IFS, because the default IFS containing a space
would cause the added space suffix to be stripped off when compgen's
output is stored in the COMPREPLY array.  Therefore we use only
newline as IFS, hence the requirement for the newline-separated
possible completion words.

Convert all callsites of __gitcomp() where it's called with refs, i.e.
when it gets the output of either __git_refs(), __git_heads(),
__git_tags(), __git_refs2(), __git_refs_remotes(), or the odd 'git
for-each-ref' somewhere in _git_config().  Also convert callsites
where it gets other uniformly handled newline separated word lists,
i.e. either remotes from __git_remotes(), names of set configuration
variables from __git_config_get_set_variables(), stashes, or commands.

Here are some timing results for dealing with 10000 refs.
Before:

  $ refs="$(__git_refs ~/tmp/git/repo-with-10k-refs/)"
  $ time __gitcomp "$refs"

  real	0m1.134s
  user	0m1.060s
  sys	0m0.130s

After:

  $ time __gitcomp_nl "$refs"

  real	0m0.373s
  user	0m0.360s
  sys	0m0.020s

Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
---

On Fri, Oct 14, 2011 at 02:16:09PM +0200, SZEDER Gábor wrote:
> Oops, this last hunk is wrong.

Here's the update with that buggy hunk removed.  I also updated the
comments before __gitcomp_nl() to be more explicit, and the commit
message with the IFS fiddling and the grammar errors you pointed out
earlier.

These changes don't conflict with later patches, so I resend only this
patch but not the whole series.

 contrib/completion/git-completion.bash |  115 +++++++++++++++++++------------
 1 files changed, 70 insertions(+), 45 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index c0fb6e15..daabf827 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -512,6 +512,31 @@ __gitcomp ()
 	esac
 }
 
+# Generates completion reply with compgen from newline-separated possible
+# completion words by appending a space to all of them.
+# It accepts 1 to 4 arguments:
+# 1: List of possible completion words, separated by a single newline.
+# 2: A prefix to be added to each possible completion word (optional).
+# 3: Generate possible completion matches for this word (optional).
+# 4: A suffix to be appended to each possible completion word instead of
+#    the default space (optional).  If specified but empty, nothing is
+#    appended.
+__gitcomp_nl ()
+{
+	local s=$'\n' IFS=' '$'\t'$'\n'
+	local cur_="$cur" suffix=" "
+
+	if [ $# -gt 2 ]; then
+		cur_="$3"
+		if [ $# -gt 3 ]; then
+			suffix="$4"
+		fi
+	fi
+
+	IFS=$s
+	COMPREPLY=($(compgen -P "${2-}" -S "$suffix" -W "$1" -- "$cur_"))
+}
+
 # __git_heads accepts 0 or 1 arguments (to pass to __gitdir)
 __git_heads ()
 {
@@ -716,15 +741,15 @@ __git_complete_revlist_file ()
 	*...*)
 		pfx="${cur_%...*}..."
 		cur_="${cur_#*...}"
-		__gitcomp "$(__git_refs)" "$pfx" "$cur_"
+		__gitcomp_nl "$(__git_refs)" "$pfx" "$cur_"
 		;;
 	*..*)
 		pfx="${cur_%..*}.."
 		cur_="${cur_#*..}"
-		__gitcomp "$(__git_refs)" "$pfx" "$cur_"
+		__gitcomp_nl "$(__git_refs)" "$pfx" "$cur_"
 		;;
 	*)
-		__gitcomp "$(__git_refs)"
+		__gitcomp_nl "$(__git_refs)"
 		;;
 	esac
 }
@@ -764,7 +789,7 @@ __git_complete_remote_or_refspec ()
 		c=$((++c))
 	done
 	if [ -z "$remote" ]; then
-		__gitcomp "$(__git_remotes)"
+		__gitcomp_nl "$(__git_remotes)"
 		return
 	fi
 	if [ $no_complete_refspec = 1 ]; then
@@ -789,23 +814,23 @@ __git_complete_remote_or_refspec ()
 	case "$cmd" in
 	fetch)
 		if [ $lhs = 1 ]; then
-			__gitcomp "$(__git_refs2 "$remote")" "$pfx" "$cur_"
+			__gitcomp_nl "$(__git_refs2 "$remote")" "$pfx" "$cur_"
 		else
-			__gitcomp "$(__git_refs)" "$pfx" "$cur_"
+			__gitcomp_nl "$(__git_refs)" "$pfx" "$cur_"
 		fi
 		;;
 	pull)
 		if [ $lhs = 1 ]; then
-			__gitcomp "$(__git_refs "$remote")" "$pfx" "$cur_"
+			__gitcomp_nl "$(__git_refs "$remote")" "$pfx" "$cur_"
 		else
-			__gitcomp "$(__git_refs)" "$pfx" "$cur_"
+			__gitcomp_nl "$(__git_refs)" "$pfx" "$cur_"
 		fi
 		;;
 	push)
 		if [ $lhs = 1 ]; then
-			__gitcomp "$(__git_refs)" "$pfx" "$cur_"
+			__gitcomp_nl "$(__git_refs)" "$pfx" "$cur_"
 		else
-			__gitcomp "$(__git_refs "$remote")" "$pfx" "$cur_"
+			__gitcomp_nl "$(__git_refs "$remote")" "$pfx" "$cur_"
 		fi
 		;;
 	esac
@@ -1084,7 +1109,7 @@ _git_archive ()
 		return
 		;;
 	--remote=*)
-		__gitcomp "$(__git_remotes)" "" "${cur##--remote=}"
+		__gitcomp_nl "$(__git_remotes)" "" "${cur##--remote=}"
 		return
 		;;
 	--*)
@@ -1115,7 +1140,7 @@ _git_bisect ()
 
 	case "$subcommand" in
 	bad|good|reset|skip|start)
-		__gitcomp "$(__git_refs)"
+		__gitcomp_nl "$(__git_refs)"
 		;;
 	*)
 		COMPREPLY=()
@@ -1146,9 +1171,9 @@ _git_branch ()
 		;;
 	*)
 		if [ $only_local_ref = "y" -a $has_r = "n" ]; then
-			__gitcomp "$(__git_heads)"
+			__gitcomp_nl "$(__git_heads)"
 		else
-			__gitcomp "$(__git_refs)"
+			__gitcomp_nl "$(__git_refs)"
 		fi
 		;;
 	esac
@@ -1195,7 +1220,7 @@ _git_checkout ()
 		if [ -n "$(__git_find_on_cmdline "$flags")" ]; then
 			track=''
 		fi
-		__gitcomp "$(__git_refs '' $track)"
+		__gitcomp_nl "$(__git_refs '' $track)"
 		;;
 	esac
 }
@@ -1212,7 +1237,7 @@ _git_cherry_pick ()
 		__gitcomp "--edit --no-commit"
 		;;
 	*)
-		__gitcomp "$(__git_refs)"
+		__gitcomp_nl "$(__git_refs)"
 		;;
 	esac
 }
@@ -1266,7 +1291,7 @@ _git_commit ()
 		;;
 	--reuse-message=*|--reedit-message=*|\
 	--fixup=*|--squash=*)
-		__gitcomp "$(__git_refs)" "" "${cur#*=}"
+		__gitcomp_nl "$(__git_refs)" "" "${cur#*=}"
 		return
 		;;
 	--untracked-files=*)
@@ -1297,7 +1322,7 @@ _git_describe ()
 			"
 		return
 	esac
-	__gitcomp "$(__git_refs)"
+	__gitcomp_nl "$(__git_refs)"
 }
 
 __git_diff_common_options="--stat --numstat --shortstat --summary
@@ -1456,7 +1481,7 @@ _git_grep ()
 		;;
 	esac
 
-	__gitcomp "$(__git_refs)"
+	__gitcomp_nl "$(__git_refs)"
 }
 
 _git_help ()
@@ -1514,7 +1539,7 @@ _git_ls_files ()
 
 _git_ls_remote ()
 {
-	__gitcomp "$(__git_remotes)"
+	__gitcomp_nl "$(__git_remotes)"
 }
 
 _git_ls_tree ()
@@ -1610,7 +1635,7 @@ _git_merge ()
 		__gitcomp "$__git_merge_options"
 		return
 	esac
-	__gitcomp "$(__git_refs)"
+	__gitcomp_nl "$(__git_refs)"
 }
 
 _git_mergetool ()
@@ -1630,7 +1655,7 @@ _git_mergetool ()
 
 _git_merge_base ()
 {
-	__gitcomp "$(__git_refs)"
+	__gitcomp_nl "$(__git_refs)"
 }
 
 _git_mv ()
@@ -1661,7 +1686,7 @@ _git_notes ()
 	,*)
 		case "${words[cword-1]}" in
 		--ref)
-			__gitcomp "$(__git_refs)"
+			__gitcomp_nl "$(__git_refs)"
 			;;
 		*)
 			__gitcomp "$subcommands --ref"
@@ -1670,7 +1695,7 @@ _git_notes ()
 		;;
 	add,--reuse-message=*|append,--reuse-message=*|\
 	add,--reedit-message=*|append,--reedit-message=*)
-		__gitcomp "$(__git_refs)" "" "${cur#*=}"
+		__gitcomp_nl "$(__git_refs)" "" "${cur#*=}"
 		;;
 	add,--*|append,--*)
 		__gitcomp '--file= --message= --reedit-message=
@@ -1689,7 +1714,7 @@ _git_notes ()
 		-m|-F)
 			;;
 		*)
-			__gitcomp "$(__git_refs)"
+			__gitcomp_nl "$(__git_refs)"
 			;;
 		esac
 		;;
@@ -1717,12 +1742,12 @@ _git_push ()
 {
 	case "$prev" in
 	--repo)
-		__gitcomp "$(__git_remotes)"
+		__gitcomp_nl "$(__git_remotes)"
 		return
 	esac
 	case "$cur" in
 	--repo=*)
-		__gitcomp "$(__git_remotes)" "" "${cur##--repo=}"
+		__gitcomp_nl "$(__git_remotes)" "" "${cur##--repo=}"
 		return
 		;;
 	--*)
@@ -1760,7 +1785,7 @@ _git_rebase ()
 
 		return
 	esac
-	__gitcomp "$(__git_refs)"
+	__gitcomp_nl "$(__git_refs)"
 }
 
 _git_reflog ()
@@ -1771,7 +1796,7 @@ _git_reflog ()
 	if [ -z "$subcommand" ]; then
 		__gitcomp "$subcommands"
 	else
-		__gitcomp "$(__git_refs)"
+		__gitcomp_nl "$(__git_refs)"
 	fi
 }
 
@@ -1853,23 +1878,23 @@ _git_config ()
 {
 	case "$prev" in
 	branch.*.remote)
-		__gitcomp "$(__git_remotes)"
+		__gitcomp_nl "$(__git_remotes)"
 		return
 		;;
 	branch.*.merge)
-		__gitcomp "$(__git_refs)"
+		__gitcomp_nl "$(__git_refs)"
 		return
 		;;
 	remote.*.fetch)
 		local remote="${prev#remote.}"
 		remote="${remote%.fetch}"
-		__gitcomp "$(__git_refs_remotes "$remote")"
+		__gitcomp_nl "$(__git_refs_remotes "$remote")"
 		return
 		;;
 	remote.*.push)
 		local remote="${prev#remote.}"
 		remote="${remote%.push}"
-		__gitcomp "$(git --git-dir="$(__gitdir)" \
+		__gitcomp_nl "$(git --git-dir="$(__gitdir)" \
 			for-each-ref --format='%(refname):%(refname)' \
 			refs/heads)"
 		return
@@ -1916,7 +1941,7 @@ _git_config ()
 		return
 		;;
 	--get|--get-all|--unset|--unset-all)
-		__gitcomp "$(__git_config_get_set_variables)"
+		__gitcomp_nl "$(__git_config_get_set_variables)"
 		return
 		;;
 	*.*)
@@ -1942,7 +1967,7 @@ _git_config ()
 		;;
 	branch.*)
 		local pfx="${cur%.*}." cur_="${cur#*.}"
-		__gitcomp "$(__git_heads)" "$pfx" "$cur_" "."
+		__gitcomp_nl "$(__git_heads)" "$pfx" "$cur_" "."
 		return
 		;;
 	guitool.*.*)
@@ -1971,7 +1996,7 @@ _git_config ()
 	pager.*)
 		local pfx="${cur%.*}." cur_="${cur#*.}"
 		__git_compute_all_commands
-		__gitcomp "$__git_all_commands" "$pfx" "$cur_"
+		__gitcomp_nl "$__git_all_commands" "$pfx" "$cur_"
 		return
 		;;
 	remote.*.*)
@@ -1984,7 +2009,7 @@ _git_config ()
 		;;
 	remote.*)
 		local pfx="${cur%.*}." cur_="${cur#*.}"
-		__gitcomp "$(__git_remotes)" "$pfx" "$cur_" "."
+		__gitcomp_nl "$(__git_remotes)" "$pfx" "$cur_" "."
 		return
 		;;
 	url.*.*)
@@ -2285,7 +2310,7 @@ _git_remote ()
 
 	case "$subcommand" in
 	rename|rm|show|prune)
-		__gitcomp "$(__git_remotes)"
+		__gitcomp_nl "$(__git_remotes)"
 		;;
 	update)
 		local i c='' IFS=$'\n'
@@ -2303,7 +2328,7 @@ _git_remote ()
 
 _git_replace ()
 {
-	__gitcomp "$(__git_refs)"
+	__gitcomp_nl "$(__git_refs)"
 }
 
 _git_reset ()
@@ -2316,7 +2341,7 @@ _git_reset ()
 		return
 		;;
 	esac
-	__gitcomp "$(__git_refs)"
+	__gitcomp_nl "$(__git_refs)"
 }
 
 _git_revert ()
@@ -2327,7 +2352,7 @@ _git_revert ()
 		return
 		;;
 	esac
-	__gitcomp "$(__git_refs)"
+	__gitcomp_nl "$(__git_refs)"
 }
 
 _git_rm ()
@@ -2426,7 +2451,7 @@ _git_stash ()
 			COMPREPLY=()
 			;;
 		show,*|apply,*|drop,*|pop,*|branch,*)
-			__gitcomp "$(git --git-dir="$(__gitdir)" stash list \
+			__gitcomp_nl "$(git --git-dir="$(__gitdir)" stash list \
 					| sed -n -e 's/:.*//p')"
 			;;
 		*)
@@ -2560,7 +2585,7 @@ _git_tag ()
 		i="${words[c]}"
 		case "$i" in
 		-d|-v)
-			__gitcomp "$(__git_tags)"
+			__gitcomp_nl "$(__git_tags)"
 			return
 			;;
 		-f)
@@ -2576,13 +2601,13 @@ _git_tag ()
 		;;
 	-*|tag)
 		if [ $f = 1 ]; then
-			__gitcomp "$(__git_tags)"
+			__gitcomp_nl "$(__git_tags)"
 		else
 			COMPREPLY=()
 		fi
 		;;
 	*)
-		__gitcomp "$(__git_refs)"
+		__gitcomp_nl "$(__git_refs)"
 		;;
 	esac
 }
-- 
1.7.7.197.g04a3e

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/9 v2] completion: optimize refs completion
  2011-10-15 12:57     ` [PATCH 2/9 v2] " SZEDER Gábor
@ 2011-10-16  3:29       ` Junio C Hamano
  0 siblings, 0 replies; 16+ messages in thread
From: Junio C Hamano @ 2011-10-16  3:29 UTC (permalink / raw)
  To: SZEDER Gábor; +Cc: git, Shawn O. Pearce, Jonathan Nieder

Thanks; will replace the corresponding patch in the topic branch and
rebuild.

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2011-10-16  5:13 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-10-08 14:54 [PATCH 0/9] ref completion optimizations, fixes, and cleanups SZEDER Gábor
2011-10-08 14:54 ` [PATCH 1/9] completion: document __gitcomp() SZEDER Gábor
2011-10-08 14:54 ` [PATCH 2/9] completion: optimize refs completion SZEDER Gábor
2011-10-13  0:50   ` Junio C Hamano
2011-10-13 10:40     ` SZEDER Gábor
2011-10-13 17:28       ` Junio C Hamano
2011-10-14 12:16   ` SZEDER Gábor
2011-10-15 12:57     ` [PATCH 2/9 v2] " SZEDER Gábor
2011-10-16  3:29       ` Junio C Hamano
2011-10-08 14:54 ` [PATCH 3/9] completion: make refs completion consistent for local and remote repos SZEDER Gábor
2011-10-08 14:54 ` [PATCH 4/9] completion: improve ls-remote output filtering in __git_refs() SZEDER Gábor
2011-10-08 14:54 ` [PATCH 5/9] completion: support full refs from remote repositories SZEDER Gábor
2011-10-08 14:54 ` [PATCH 6/9] completion: query only refs/heads/ in __git_refs_remotes() SZEDER Gábor
2011-10-08 14:54 ` [PATCH 7/9] completion: improve ls-remote output filtering " SZEDER Gábor
2011-10-08 14:54 ` [PATCH 8/9] completion: fast initial completion for config 'remote.*.fetch' value SZEDER Gábor
2011-10-08 14:54 ` [PATCH 9/9] completion: remove broken dead code from __git_heads() and __git_tags() SZEDER Gábor

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).