threads / patch / 39289

patch, 2 partscompletion: simplify query for config variables

Subject: [PATCH 2/2] completion: simplify query for config variables

## tl;dr

4 messages between May 10, 2015 and May 12, 2015. Diffs are folded; open one to read it.

replies: 3people: 2as markdown or json

SZEDER Gábor· May 10, 2015, 12:50 UTC · lore

[PATCH 1/2] completion: add a helper function to get config variables

Currently there are a few completion functions that perform similar 'git config' queries and filtering to get config variable names: the completion of pretty aliases, aliases, and remote groups for 'git remote update'.

Unify those 'git config' queries in a helper function to eliminate code duplication.

Though the helper functions to get pretty aliases and alieses are reduced to mere one-liner wrappers around the newly added function, keep these helpers still, because users' completion functions out there might depend on them. And they keep their callers a tad easier to read, too.

Add tests for the pretty alias and alias helper to show that they work as before; not for the remote groups query, though, because that's not extracted into a helper function and it's not worth the effort to do so for a sole callsite.

Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
---
 contrib/completion/git-completion.bash | 34 ++++++++++++++--------------------
 t/t9902-completion.sh                  | 22 ++++++++++++++++++++++
 2 files changed, 36 insertions(+), 20 deletions(-)
Show changes to 2 files +36 −20

contrib/completion/git-completion.bash, t/t9902-completion.sh

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 5944c824ab..6973620857 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -740,30 +740,29 @@ __git_compute_porcelain_commands ()
 	__git_porcelain_commands=$(__git_list_porcelain_commands)
 }
 
-__git_pretty_aliases ()
+# Lists all set config variables starting with the given section prefix,
+# with the prefix removed.
+__git_get_config_variables ()
 {
-	local i IFS=$'\n'
-	for i in $(git --git-dir="$(__gitdir)" config --get-regexp "pretty\..*" 2>/dev/null); do
+	local section="$1" i IFS=$'\n'
+	for i in $(git --git-dir="$(__gitdir)" config --get-regexp "$section\..*" 2>/dev/null); do
 		case "$i" in
-		pretty.*)
-			i="${i#pretty.}"
+		$section.*)
+			i="${i#$section.}"
 			echo "${i/ */}"
 			;;
 		esac
 	done
 }
 
+__git_pretty_aliases ()
+{
+	__git_get_config_variables "pretty"
+}
+
 __git_aliases ()
 {
-	local i IFS=$'\n'
-	for i in $(git --git-dir="$(__gitdir)" config --get-regexp "alias\..*" 2>/dev/null); do
-		case "$i" in
-		alias.*)
-			i="${i#alias.}"
-			echo "${i/ */}"
-			;;
-		esac
-	done
+	__git_get_config_variables "alias"
 }
 
 # __git_aliased_command requires 1 argument
@@ -2260,12 +2259,7 @@ _git_remote ()
 		__git_complete_remote_or_refspec
 		;;
 	update)
-		local i c='' IFS=$'\n'
-		for i in $(git --git-dir="$(__gitdir)" config --get-regexp "remotes\..*" 2>/dev/null); do
-			i="${i#remotes.}"
-			c="$c ${i/ */}"
-		done
-		__gitcomp "$c"
+		__gitcomp "$(__git_get_config_variables "remotes")"
 		;;
 	*)
 		;;
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 4a14a5892e..07f2478c9b 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -370,6 +370,28 @@ test_expect_success '__git_remotes - list remotes from $GIT_DIR/remotes and from
 	test_cmp expect actual
 '
 
+test_expect_success '__git_pretty_aliases' '
+	cat >expect <<-EOF &&
+	author
+	hash
+	EOF
+	test_config pretty.author "%an %ae" &&
+	test_config pretty.hash %H &&
+	__git_pretty_aliases >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '__git_aliases' '
+	cat >expect <<-EOF &&
+	ci
+	co
+	EOF
+	test_config alias.ci commit &&
+	test_config alias.co checkout &&
+	__git_aliases >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'basic' '
 	run_completion "git " &&
 	# built-in
-- 
1.9.5.msysgit.0
SZEDER Gábor· May 10, 2015, 12:50 UTC · re: SZEDER Gábor · lore

To get the name of all config variables in a given section we perform a 'git config --get-regex' query for all config variables containing the name of that section, and then filter its output through a case statement to throw away those that though contain but don't start with the given section.

Modify the regex to match only at the beginning, so the case statement becomes unnecessary and we can get rid of it. Add a test to check that a match in the middle doesn't fool us.

Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
---
 contrib/completion/git-completion.bash | 10 +++-------
 t/t9902-completion.sh                  | 12 ++++++++++++
 2 files changed, 15 insertions(+), 7 deletions(-)
Show changes to 2 files +15 −7

contrib/completion/git-completion.bash, t/t9902-completion.sh

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 6973620857..63f960b33d 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -745,13 +745,9 @@ __git_compute_porcelain_commands ()
 __git_get_config_variables ()
 {
 	local section="$1" i IFS=$'\n'
-	for i in $(git --git-dir="$(__gitdir)" config --get-regexp "$section\..*" 2>/dev/null); do
-		case "$i" in
-		$section.*)
-			i="${i#$section.}"
-			echo "${i/ */}"
-			;;
-		esac
+	for i in $(git --git-dir="$(__gitdir)" config --get-regexp "^$section\..*" 2>/dev/null); do
+		i="${i#$section.}"
+		echo "${i/ */}"
 	done
 }
 
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 07f2478c9b..2ba62fbc17 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -370,6 +370,18 @@ test_expect_success '__git_remotes - list remotes from $GIT_DIR/remotes and from
 	test_cmp expect actual
 '
 
+test_expect_success '__git_get_config_variables' '
+	cat >expect <<-EOF &&
+	name-1
+	name-2
+	EOF
+	test_config interesting.name-1 good &&
+	test_config interesting.name-2 good &&
+	test_config subsection.interesting.name-3 bad &&
+	__git_get_config_variables interesting >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success '__git_pretty_aliases' '
 	cat >expect <<-EOF &&
 	author
-- 
1.9.5.msysgit.0
SZEDER Gábor· May 10, 2015, 13:16 UTC · re: SZEDER Gábor · lore

Re: [PATCH 1/2] completion: add a helper function to get config variables

Quoting SZEDER Gábor <szeder@ira.uka.de>:
Show 31 quoted lines
> Currently there are a few completion functions that perform similar 'git
> config' queries and filtering to get config variable names: the completion
> of pretty aliases, aliases, and remote groups for 'git remote update'.
>
> Unify those 'git config' queries in a helper function to eliminate code
> duplication.
>
> Though the helper functions to get pretty aliases and alieses are reduced
> to mere one-liner wrappers around the newly added function, keep these
> helpers still, because users' completion functions out there might depend
> on them.  And they keep their callers a tad easier to read, too.
>
> Add tests for the pretty alias and alias helper to show that they work
> as before; not for the remote groups query, though, because that's not
> extracted into a helper function and it's not worth the effort to do so
> for a sole callsite.
>
> Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
> ---
> @@ -2260,12 +2259,7 @@ _git_remote ()
>  		__git_complete_remote_or_refspec
>  		;;
>  	update)
> -		local i c='' IFS=$'\n'
> -		for i in $(git --git-dir="$(__gitdir)" config --get-regexp  
> "remotes\..*" 2>/dev/null); do
> -			i="${i#remotes.}"
> -			c="$c ${i/ */}"
> -		done
> -		__gitcomp "$c"
> +		__gitcomp "$(__git_get_config_variables "remotes")"

I just noticed that this query for remote groups does lack that filtering case statement eliminated in the next patch and the regex is not limited to the beginning of the config variable name. So, let's suppose we have a branch named 'remotes', for which we add a branch description (and no remote groups, for symplicity's sake):

   $ git config branch.remotes.description uh-oh
   $ git remote update <TAB>
This will complete to:
   $ git remote update branch.remotes.description
and that's obviously bad.
This series fixes this.

Best, Gábor

Junio C Hamano· May 12, 2015, 22:14 UTC · re: SZEDER Gábor · lore

Re: [PATCH 1/2] completion: add a helper function to get config variables

SZEDER Gábor <szeder@ira.uka.de> writes:
Show 6 quoted lines
> +	for i in $(git --git-dir="$(__gitdir)" config --get-regexp "$section\..*" 2>/dev/null); do
>  		case "$i" in
> -		pretty.*)
> -			i="${i#pretty.}"
> +		$section.*)
> +			i="${i#$section.}"

The case arm treats $section as a shell glob, --get-regexp treats it as a regex fragment and then the shell expansion uses it as a literal.

The current set of callers give simple single-token like pretty, remote, etc. to it so this is safe, but you may want to give a comment to the function to help future generation, perhaps?

Other than that, looks quite straight-forward.
Thanks.

← back to recent threads