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

Hi Jonathan,


On Thu, Oct 14, 2010 at 12:15:07PM -0500, Jonathan Nieder wrote:

SZEDER Gábor wrote:

quoted

Currently there are three completion functions that perform similar
queries to 'git config' to get config variable names.

Good point.

quoted

--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash

@@ -451,10 +451,7 @@ __git_remotes ()
 		echo ${i#$d/remotes/}
 	done
 	[ "$ngoff" ] && shopt -u nullglob
-	for i in $(git --git-dir="$d" config --get-regexp 'remote\..*\.url' 2>/dev/null); do
-		i="${i#remote.}"
-		echo "${i/.url*/}"
-	done
+	__git_get_config_variables "remote" "url"
 }

Ok, so __git_get_config_variables $category $var means something like

	git config --get-regexp '$category[.].*[.]$var' |
	cut -d. -f2

Almost.  Considering the current invocations of
__git_get_config_variables() introduced in this patch, yes, they do
the same.  But "cut -d. -f2" will behave differently when $category
contains a dot, or when neither $category nor $var contain a dot, but
the config variable contains more than two (does git have any such
config variables?).

quoted

@@ -750,14 +747,16 @@ __git_compute_porcelain_commands ()
 	: ${__git_porcelain_commands:=$(__git_list_porcelain_commands)}
 }
 
-__git_aliases ()
+# returns all config variables within a given section with an optional
+# suffix, with both the section name and the suffix removed
+__git_get_config_variables ()
 {
-	local i IFS=$'\n'
-	for i in $(git --git-dir="$(__gitdir)" config --get-regexp "alias\..*" 2>/dev/null); do
+	local section="$1" suffix="${2-}" i IFS=$'\n'
+	for i in $(git --git-dir="$(__gitdir)" config --get-regexp "$section\..*${suffix:+\.$suffix}" 2>/dev/null); do

Would it be possible to shorten this line?  e.g.

	for i in $(
		git --git-dir="$(__gitdir)" ...
	); do

or

	while read -r setting
	do
		...
	done < <(
		git --git-dir="$(__gitdir)" ...
	)

or

	local ... IFS=$'\n'
	set -- $(git ... )
	for i do
		...
	done

Well, yes, of course.  But the original line was already too long, and
neither of your proposals in itself would make it short enough to fit
80 characters.  Besides, the latter two changes the loop itself, not
just what the body of the loop does and what it is looping on.

Maybe we could just split the line in two in the middle, like 

	for i in $(git --git-dir="$(__gitdir)" config --get-regexp \
		$section\..*${suffix:+\.$suffix}" 2>/dev/null); do

Still doesn't look pretty, but maybe a bit better.


Best,
Gábor