threads / patch / 31894

v6, 3 partscompletion: refactor and zsh wrapper

Subject: [PATCH v6 0/3] completion: refactor and zsh wrapper

## tl;dr

8 messages between Oct 22, 2012 and Oct 31, 2012. Diffs are folded; open one to read it.

replies: 7people: 2as markdown or json

Felipe Contreras· Oct 22, 2012, 01:45 UTC · lore
Hi,

Here's a bit of reorganition. I'm introducing a new __gitcompadd helper that is useful to wrapp all changes to COMPREPLY, but first, lets get rid of unnecessary assignments as SZEDER suggested.

The zsh wrapper is now very very simple.
Since v5:
 
 * Get rid of unnecessary COMPREPLY assignments
Felipe Contreras (3):
  completion: get rid of empty COMPREPLY assignments
  completion: add new __gitcompadd helper
  completion: add new zsh completion
 contrib/completion/git-completion.bash | 41 ++++++-----------------------
 contrib/completion/git-completion.zsh  | 48 ++++++++++++++++++++++++++++++++++
 2 files changed, 56 insertions(+), 33 deletions(-)
 create mode 100644 contrib/completion/git-completion.zsh
-- 
1.8.0
Felipe Contreras· Oct 22, 2012, 01:45 UTC · re: Felipe Contreras · lore

[PATCH v6 1/3] completion: get rid of empty COMPREPLY assignments

There's no functional reason for those, the only purpose they are supposed to serve is to say "we don't provide any words here", but even for that it's not used consitently.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 contrib/completion/git-completion.bash | 28 ----------------------------
 1 file changed, 28 deletions(-)
Show changes to contrib/completion/git-completion.bash +0 −28
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index be800e0..7bdd6a8 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -238,7 +238,6 @@ __gitcomp ()
 
 	case "$cur_" in
 	--*=)
-		COMPREPLY=()
 		;;
 	*)
 		local IFS=$'\n'
@@ -486,7 +485,6 @@ __git_complete_remote_or_refspec ()
 			case "$cmd" in
 			push) no_complete_refspec=1 ;;
 			fetch)
-				COMPREPLY=()
 				return
 				;;
 			*) ;;
@@ -502,7 +500,6 @@ __git_complete_remote_or_refspec ()
 		return
 	fi
 	if [ $no_complete_refspec = 1 ]; then
-		COMPREPLY=()
 		return
 	fi
 	[ "$remote" = "." ] && remote=
@@ -776,7 +773,6 @@ _git_am ()
 			"
 		return
 	esac
-	COMPREPLY=()
 }
 
 _git_apply ()
@@ -796,7 +792,6 @@ _git_apply ()
 			"
 		return
 	esac
-	COMPREPLY=()
 }
 
 _git_add ()
@@ -811,7 +806,6 @@ _git_add ()
 			"
 		return
 	esac
-	COMPREPLY=()
 }
 
 _git_archive ()
@@ -856,7 +850,6 @@ _git_bisect ()
 		__gitcomp_nl "$(__git_refs)"
 		;;
 	*)
-		COMPREPLY=()
 		;;
 	esac
 }
@@ -969,7 +962,6 @@ _git_clean ()
 		return
 		;;
 	esac
-	COMPREPLY=()
 }
 
 _git_clone ()
@@ -993,7 +985,6 @@ _git_clone ()
 		return
 		;;
 	esac
-	COMPREPLY=()
 }
 
 _git_commit ()
@@ -1027,7 +1018,6 @@ _git_commit ()
 			"
 		return
 	esac
-	COMPREPLY=()
 }
 
 _git_describe ()
@@ -1158,7 +1148,6 @@ _git_fsck ()
 		return
 		;;
 	esac
-	COMPREPLY=()
 }
 
 _git_gc ()
@@ -1169,7 +1158,6 @@ _git_gc ()
 		return
 		;;
 	esac
-	COMPREPLY=()
 }
 
 _git_gitk ()
@@ -1246,7 +1234,6 @@ _git_init ()
 		return
 		;;
 	esac
-	COMPREPLY=()
 }
 
 _git_ls_files ()
@@ -1265,7 +1252,6 @@ _git_ls_files ()
 		return
 		;;
 	esac
-	COMPREPLY=()
 }
 
 _git_ls_remote ()
@@ -1381,7 +1367,6 @@ _git_mergetool ()
 		return
 		;;
 	esac
-	COMPREPLY=()
 }
 
 _git_merge_base ()
@@ -1397,7 +1382,6 @@ _git_mv ()
 		return
 		;;
 	esac
-	COMPREPLY=()
 }
 
 _git_name_rev ()
@@ -1567,7 +1551,6 @@ _git_send_email ()
 		return
 		;;
 	esac
-	COMPREPLY=()
 }
 
 _git_stage ()
@@ -1680,7 +1663,6 @@ _git_config ()
 		return
 		;;
 	*.*)
-		COMPREPLY=()
 		return
 		;;
 	esac
@@ -2060,7 +2042,6 @@ _git_remote ()
 		__gitcomp "$c"
 		;;
 	*)
-		COMPREPLY=()
 		;;
 	esac
 }
@@ -2104,7 +2085,6 @@ _git_rm ()
 		return
 		;;
 	esac
-	COMPREPLY=()
 }
 
 _git_shortlog ()
@@ -2173,8 +2153,6 @@ _git_stash ()
 		*)
 			if [ -z "$(__git_find_on_cmdline "$save_opts")" ]; then
 				__gitcomp "$subcommands"
-			else
-				COMPREPLY=()
 			fi
 			;;
 		esac
@@ -2187,14 +2165,12 @@ _git_stash ()
 			__gitcomp "--index --quiet"
 			;;
 		show,--*|drop,--*|branch,--*)
-			COMPREPLY=()
 			;;
 		show,*|apply,*|drop,*|pop,*|branch,*)
 			__gitcomp_nl "$(git --git-dir="$(__gitdir)" stash list \
 					| sed -n -e 's/:.*//p')"
 			;;
 		*)
-			COMPREPLY=()
 			;;
 		esac
 	fi
@@ -2311,7 +2287,6 @@ _git_svn ()
 			__gitcomp "--revision= --parent"
 			;;
 		*)
-			COMPREPLY=()
 			;;
 		esac
 	fi
@@ -2336,13 +2311,10 @@ _git_tag ()
 
 	case "$prev" in
 	-m|-F)
-		COMPREPLY=()
 		;;
 	-*|tag)
 		if [ $f = 1 ]; then
 			__gitcomp_nl "$(__git_tags)"
-		else
-			COMPREPLY=()
 		fi
 		;;
 	*)
-- 
1.8.0
Felipe Contreras· Oct 22, 2012, 01:45 UTC · re: Felipe Contreras · lore

[PATCH v6 2/3] completion: add new __gitcompadd helper

The idea is to never touch the COMPREPLY variable directly.

This allows other completion systems override __gitcompadd, and do something different instead.

Also, this allows the simplification of the completion tests (separate patch).

There should be no functional changes.
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 contrib/completion/git-completion.bash | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)
Show changes to contrib/completion/git-completion.bash +8 −5
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 7bdd6a8..975ae13 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -225,6 +225,11 @@ _get_comp_words_by_ref ()
 fi
 fi
 
+__gitcompadd ()
+{
+	COMPREPLY=($(compgen -W "$1" -P "$2" -S "$4" -- "$3"))
+}
+
 # Generates completion reply with compgen, appending a space to possible
 # completion words, if necessary.
 # It accepts 1 to 4 arguments:
@@ -241,9 +246,7 @@ __gitcomp ()
 		;;
 	*)
 		local IFS=$'\n'
-		COMPREPLY=($(compgen -P "${2-}" \
-			-W "$(__gitcomp_1 "${1-}" "${4-}")" \
-			-- "$cur_"))
+		__gitcompadd "$(__gitcomp_1 "${1-}" "${4-}")" "${2-}" "$cur_" ""
 		;;
 	esac
 }
@@ -260,7 +263,7 @@ __gitcomp ()
 __gitcomp_nl ()
 {
 	local IFS=$'\n'
-	COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$1" -- "${3-$cur}"))
+	__gitcompadd "$1" "${2-}" "${3-$cur}" "${4- }"
 }
 
 __git_heads ()
@@ -1603,7 +1606,7 @@ _git_config ()
 		local remote="${prev#remote.}"
 		remote="${remote%.fetch}"
 		if [ -z "$cur" ]; then
-			COMPREPLY=("refs/heads/")
+			__gitcompadd "refs/heads/"
 			return
 		fi
 		__gitcomp_nl "$(__git_refs_remotes "$remote")"
-- 
1.8.0
SZEDER Gábor· Oct 30, 2012, 22:58 UTC · re: Felipe Contreras · lore

Re: [PATCH v6 2/3] completion: add new __gitcompadd helper

On Mon, Oct 22, 2012 at 03:45:41AM +0200, Felipe Contreras wrote:
Show 7 quoted lines
> The idea is to never touch the COMPREPLY variable directly.
> 
> This allows other completion systems override __gitcompadd, and do
> something different instead.
> 
> Also, this allows the simplification of the completion tests (separate
> patch).

This doesn't apply anymore, does it? The mentioned simplification is done in the other series.

Felipe Contreras· Oct 30, 2012, 23:05 UTC · re: SZEDER Gábor · lore

Re: [PATCH v6 2/3] completion: add new __gitcompadd helper

On Tue, Oct 30, 2012 at 11:58 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:
Show 11 quoted lines
> On Mon, Oct 22, 2012 at 03:45:41AM +0200, Felipe Contreras wrote:
>> The idea is to never touch the COMPREPLY variable directly.
>>
>> This allows other completion systems override __gitcompadd, and do
>> something different instead.
>>
>> Also, this allows the simplification of the completion tests (separate
>> patch).
>
> This doesn't apply anymore, does it?  The mentioned simplification is
> done in the other series.

Yeah, but you mentioned you didn't like all the COMPREPLY=() changes and it might be time to get rid of them.

So this series supersedes that one.
-- 
Felipe Contreras
SZEDER Gábor· Oct 31, 2012, 14:15 UTC · re: Felipe Contreras · lore

Re: [PATCH v6 2/3] completion: add new __gitcompadd helper

On Wed, Oct 31, 2012 at 12:05:34AM +0100, Felipe Contreras wrote:
Show 17 quoted lines
> On Tue, Oct 30, 2012 at 11:58 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:
> > On Mon, Oct 22, 2012 at 03:45:41AM +0200, Felipe Contreras wrote:
> >> The idea is to never touch the COMPREPLY variable directly.
> >>
> >> This allows other completion systems override __gitcompadd, and do
> >> something different instead.
> >>
> >> Also, this allows the simplification of the completion tests (separate
> >> patch).
> >
> > This doesn't apply anymore, does it?  The mentioned simplification is
> > done in the other series.
> 
> Yeah, but you mentioned you didn't like all the COMPREPLY=() changes
> and it might be time to get rid of them.
> 
> So this series supersedes that one.

COMPREPLY=() has nothing to do with it. My point is that there is no "separate patch" that performs the alleged simplification made possible by this patch, therefore that sentence should have been removed from the log message for the resubmission.

Felipe Contreras· Oct 22, 2012, 01:45 UTC · re: Felipe Contreras · lore

[PATCH v6 3/3] completion: add new zsh completion

It seems there's always issues with zsh's bash completion emulation. I've tried to fix as many as I could and most of the fixes are already in the latest version of zsh, but still, there are issues.

There is no point in going through all that pain; the emulation is easy to achieve, and this patch works better than zsh's emulation.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
v5:
 * Even more simplification by using __gitcompadd
v4:
 * Simplification updates for the latest bash completion
v3:
 * Simplification
 * Avoid COMPREPLY; call compadd directly
 * Fix _get_comp_words_by_ref
 contrib/completion/git-completion.zsh | 48 +++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)
 create mode 100644 contrib/completion/git-completion.zsh
Show changes to contrib/completion/git-completion.zsh +48 −0
diff --git a/contrib/completion/git-completion.zsh b/contrib/completion/git-completion.zsh
new file mode 100644
index 0000000..ae9c532
--- /dev/null
+++ b/contrib/completion/git-completion.zsh
@@ -0,0 +1,48 @@
+#compdef git gitk
+
+# zsh completion wrapper for git
+#
+# You need git's bash completion script installed somewhere, by default on the
+# same directory as this script.
+#
+# If your script is on ~/.git-completion.sh instead, you can configure it on
+# your ~/.zshrc:
+#
+#  zstyle ':completion:*:*:git:*' script ~/.git-completion.sh
+#
+# The recommended way to install this script is to copy to
+# '~/.zsh/completion/_git', and then add the following to your ~/.zshrc file:
+#
+#  fpath=(~/.zsh/completion $fpath)
+
+complete ()
+{
+	# do nothing
+	return 0
+}
+
+zstyle -s ":completion:*:*:git:*" script script
+test -z "$script" && script="$(dirname ${funcsourcetrace[1]%:*})"/git-completion.bash
+ZSH_VERSION='' . "$script"
+
+__gitcompadd ()
+{
+	compadd -Q -S "$4" -P "${(M)cur#*[=:]}" -p "$2" -- ${=1} && _ret=0
+}
+
+_git ()
+{
+	local _ret=1
+	() {
+		emulate -L ksh
+		local cur cword prev
+		cur=${words[CURRENT-1]}
+		prev=${words[CURRENT-2]}
+		let cword=CURRENT-1
+		__${service}_main
+	}
+	let _ret && _default -S '' && _ret=0
+	return _ret
+}
+
+_git
-- 
1.8.0
Felipe Contreras· Oct 30, 2012, 16:15 UTC · re: Felipe Contreras · lore

Re: [PATCH v6 0/3] completion: refactor and zsh wrapper

Hi,

On Mon, Oct 22, 2012 at 3:45 AM, Felipe Contreras <felipe.contreras@gmail.com> wrote:

Show 5 quoted lines
> Here's a bit of reorganition. I'm introducing a new __gitcompadd helper that is
> useful to wrapp all changes to COMPREPLY, but first, lets get rid of
> unnecessary assignments as SZEDER suggested.
>
> The zsh wrapper is now very very simple.

Junio, Jeff, just to let you know, this is an updated version of the new zsh wrapper patch series with the feedback from SZEDER. I see the old version is in pu, and hasn't been updated.

Cheers.
Show 8 quoted lines
> Since v5:
>
>  * Get rid of unnecessary COMPREPLY assignments
>
> Felipe Contreras (3):
>   completion: get rid of empty COMPREPLY assignments
>   completion: add new __gitcompadd helper
>   completion: add new zsh completion
-- 
Felipe Contreras

← back to recent threads