threads / patch / 29661

patchcompletion: remote set-* <name> and <branch>

Subject: [PATCH] completion: remote set-* <name> and <branch>

## tl;dr

14 messages between Feb 18, 2012 and Feb 22, 2012. Diffs are folded; open one to read it.

replies: 13people: 4as markdown or json

Philip Jägenstedt· Feb 18, 2012, 13:32 UTC · lore

Complete <name> only for set-url. For set-branches and set-head, complete <name> and <branch> over the network, like e.g. git pull already does.

Signed-off-by: Philip Jägenstedt <philip@foolip.org>
---
 contrib/completion/git-completion.bash |   12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)
Show changes to contrib/completion/git-completion.bash +9 −3
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 1505cff..8e7abb6 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -738,6 +738,9 @@ __git_complete_remote_or_refspec ()
 {
 	local cur_="$cur" cmd="${words[1]}"
 	local i c=2 remote="" pfx="" lhs=1 no_complete_refspec=0
+	if [ "$cmd" = "remote" ]; then
+		c=$((++c))
+	fi
 	while [ $c -lt $cword ]; do
 		i="${words[c]}"
 		case "$i" in
@@ -788,7 +791,7 @@ __git_complete_remote_or_refspec ()
 			__gitcomp_nl "$(__git_refs)" "$pfx" "$cur_"
 		fi
 		;;
-	pull)
+	pull|remote)
 		if [ $lhs = 1 ]; then
 			__gitcomp_nl "$(__git_refs "$remote")" "$pfx" "$cur_"
 		else
@@ -2289,7 +2292,7 @@ _git_config ()
 
 _git_remote ()
 {
-	local subcommands="add rename rm show prune update set-head"
+	local subcommands="add rename rm set-head set-branches set-url show prune update"
 	local subcommand="$(__git_find_on_cmdline "$subcommands")"
 	if [ -z "$subcommand" ]; then
 		__gitcomp "$subcommands"
@@ -2297,9 +2300,12 @@ _git_remote ()
 	fi
 
 	case "$subcommand" in
-	rename|rm|show|prune)
+	rename|rm|set-url|show|prune)
 		__gitcomp_nl "$(__git_remotes)"
 		;;
+	set-head|set-branches)
+		__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
-- 
1.7.9.1.245.g4cbe6
Junio C Hamano· Feb 20, 2012, 07:58 UTC · re: Philip Jägenstedt · lore

Re: [PATCH] completion: remote set-* <name> and <branch>

Philip Jägenstedt <philip@foolip.org> writes:
Show 5 quoted lines
> Complete <name> only for set-url. For set-branches and
> set-head, complete <name> and <branch> over the network,
> like e.g. git pull already does.
>
> Signed-off-by: Philip Jägenstedt <philip@foolip.org>
You addressed your patch to Shawn, who originally wrote this, but
"git shortlog -n -s --no-merges --since=9.months pu contrib/completion"

indicates that he no longer is involved in enhancing this script, and it has seen actions primarily from three people I Cc'ed this message to.

Show 14 quoted lines
>  contrib/completion/git-completion.bash |   12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 1505cff..8e7abb6 100755
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -738,6 +738,9 @@ __git_complete_remote_or_refspec ()
>  {
>  	local cur_="$cur" cmd="${words[1]}"
>  	local i c=2 remote="" pfx="" lhs=1 no_complete_refspec=0
> +	if [ "$cmd" = "remote" ]; then
> +		c=$((++c))
> +	fi

I don't know about others, but auto-incrementing a variable and assigning the result to the same variable, while not wrong at all, hurts my brain.

	c=$(($c + 1))

is far more readable and does not suggest there is any funky magic involved. Also it is a good habit to get into not to omit $ from variables inside arithmetic substitution, even though bash allows it and this script is meant to be consumed only by shells that understand this bash-ism.

I do not know offhand if zsh groks it, but the point is that you do not have to worry about it if you write "$(($c+1))" instead of "$((c+1))".

I'll let the area experts to comment on the remainder of the patch.
Thanks.
Show 32 quoted lines
> @@ -788,7 +791,7 @@ __git_complete_remote_or_refspec ()
>  			__gitcomp_nl "$(__git_refs)" "$pfx" "$cur_"
>  		fi
>  		;;
> -	pull)
> +	pull|remote)
>  		if [ $lhs = 1 ]; then
>  			__gitcomp_nl "$(__git_refs "$remote")" "$pfx" "$cur_"
>  		else
> @@ -2289,7 +2292,7 @@ _git_config ()
>  
>  _git_remote ()
>  {
> -	local subcommands="add rename rm show prune update set-head"
> +	local subcommands="add rename rm set-head set-branches set-url show prune update"
>  	local subcommand="$(__git_find_on_cmdline "$subcommands")"
>  	if [ -z "$subcommand" ]; then
>  		__gitcomp "$subcommands"
> @@ -2297,9 +2300,12 @@ _git_remote ()
>  	fi
>  
>  	case "$subcommand" in
> -	rename|rm|show|prune)
> +	rename|rm|set-url|show|prune)
>  		__gitcomp_nl "$(__git_remotes)"
>  		;;
> +	set-head|set-branches)
> +		__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
Philip Jägenstedt· Feb 21, 2012, 21:29 UTC · re: Junio C Hamano · lore

Re: [PATCH] completion: remote set-* <name> and <branch>

On Mon, Feb 20, 2012 at 08:58, Junio C Hamano <gitster@pobox.com> wrote:
Show 14 quoted lines
> Philip Jägenstedt <philip@foolip.org> writes:
>
>> Complete <name> only for set-url. For set-branches and
>> set-head, complete <name> and <branch> over the network,
>> like e.g. git pull already does.
>>
>> Signed-off-by: Philip Jägenstedt <philip@foolip.org>
>
> You addressed your patch to Shawn, who originally wrote this, but
>
> "git shortlog -n -s --no-merges --since=9.months pu contrib/completion"
>
> indicates that he no longer is involved in enhancing this script, and it
> has seen actions primarily from three people I Cc'ed this message to.

Thanks. Perhaps git-completion.bash should not say "Send all patches to the current maintainer" and simply defer to SubmittingPatches?

Show 28 quoted lines
>>  contrib/completion/git-completion.bash |   12 +++++++++---
>>  1 file changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
>> index 1505cff..8e7abb6 100755
>> --- a/contrib/completion/git-completion.bash
>> +++ b/contrib/completion/git-completion.bash
>> @@ -738,6 +738,9 @@ __git_complete_remote_or_refspec ()
>>  {
>>       local cur_="$cur" cmd="${words[1]}"
>>       local i c=2 remote="" pfx="" lhs=1 no_complete_refspec=0
>> +     if [ "$cmd" = "remote" ]; then
>> +             c=$((++c))
>> +     fi
>
> I don't know about others, but auto-incrementing a variable and assigning
> the result to the same variable, while not wrong at all, hurts my brain.
>
>        c=$(($c + 1))
>
> is far more readable and does not suggest there is any funky magic
> involved.  Also it is a good habit to get into not to omit $ from
> variables inside arithmetic substitution, even though bash allows it and
> this script is meant to be consumed only by shells that understand this
> bash-ism.
>
> I do not know offhand if zsh groks it, but the point is that you do not
> have to worry about it if you write "$(($c+1))" instead of "$((c+1))".

CodingGuidlines suggests to follow local convention, which was "c=$((++c))". This file also uses "++n", "i++" and "((i++))". I will send a v2 patch that normalizes these, open to discussion of course.

-- 
Philip Jägenstedt
Philip Jägenstedt· Feb 21, 2012, 21:54 UTC · re: Philip Jägenstedt · lore

[PATCH v2] completion: remote set-* <name> and <branch>

Complete <name> only for set-url. For set-branches and set-head, complete <name> and <branch> over the network, like e.g. git pull already does.

The style used for incrementing and decrementing variables was fairly inconsistenty and was normalized to use ++x, or ((++x)) in contexts where the former would otherwise be interpreted as a command. This is a bash-ism, but for obvious reasons this script is already bash-specific.

Finally, remove out-of-date documentation for how to submit patches and (silently) defer to Documentation/SubmittingPatches like all other code.

Signed-off-by: Philip Jägenstedt <philip@foolip.org>
---
 contrib/completion/git-completion.bash |   42 +++++++++++++------------------
 1 files changed, 18 insertions(+), 24 deletions(-)
Show changes to contrib/completion/git-completion.bash +18 −24
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 1505cff..9dac084 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -60,18 +60,6 @@
 #       per-repository basis by setting the bash.showUpstream config
 #       variable.
 #
-#
-# To submit patches:
-#
-#    *) Read Documentation/SubmittingPatches
-#    *) Send all patches to the current maintainer:
-#
-#       "Shawn O. Pearce" <spearce@spearce.org>
-#
-#    *) Always CC the Git mailing list:
-#
-#       git@vger.kernel.org
-#
 
 if [[ -n ${ZSH_VERSION-} ]]; then
 	autoload -U +X bashcompinit && bashcompinit
@@ -395,7 +383,7 @@ __git_reassemble_comp_words_by_ref()
 	fi
 	# List of word completion separators has shrunk;
 	# re-assemble words to complete.
-	for ((i=0, j=0; i < ${#COMP_WORDS[@]}; i++, j++)); do
+	for ((i=0, j=0; i < ${#COMP_WORDS[@]}; ++i, ++j)); do
 		# Append each nonempty word consisting of just
 		# word separator characters to the current word.
 		first=t
@@ -408,7 +396,7 @@ __git_reassemble_comp_words_by_ref()
 			# Attach to the previous token,
 			# unless the previous token is the command name.
 			if [ $j -ge 2 ] && [ -n "$first" ]; then
-				((j--))
+				((--j))
 			fi
 			first=
 			words_[$j]=${words_[j]}${COMP_WORDS[i]}
@@ -416,7 +404,7 @@ __git_reassemble_comp_words_by_ref()
 				cword_=$j
 			fi
 			if (($i < ${#COMP_WORDS[@]} - 1)); then
-				((i++))
+				((++i))
 			else
 				# Done.
 				return
@@ -738,6 +726,9 @@ __git_complete_remote_or_refspec ()
 {
 	local cur_="$cur" cmd="${words[1]}"
 	local i c=2 remote="" pfx="" lhs=1 no_complete_refspec=0
+	if [ "$cmd" = "remote" ]; then
+		((++c))
+	fi
 	while [ $c -lt $cword ]; do
 		i="${words[c]}"
 		case "$i" in
@@ -755,7 +746,7 @@ __git_complete_remote_or_refspec ()
 		-*) ;;
 		*) remote="$i"; break ;;
 		esac
-		c=$((++c))
+		((++c))
 	done
 	if [ -z "$remote" ]; then
 		__gitcomp_nl "$(__git_remotes)"
@@ -788,7 +779,7 @@ __git_complete_remote_or_refspec ()
 			__gitcomp_nl "$(__git_refs)" "$pfx" "$cur_"
 		fi
 		;;
-	pull)
+	pull|remote)
 		if [ $lhs = 1 ]; then
 			__gitcomp_nl "$(__git_refs "$remote")" "$pfx" "$cur_"
 		else
@@ -995,7 +986,7 @@ __git_find_on_cmdline ()
 				return
 			fi
 		done
-		c=$((++c))
+		((++c))
 	done
 }
 
@@ -1006,7 +997,7 @@ __git_has_doubledash ()
 		if [ "--" = "${words[c]}" ]; then
 			return 0
 		fi
-		c=$((++c))
+		((++c))
 	done
 	return 1
 }
@@ -1129,7 +1120,7 @@ _git_branch ()
 		-d|-m)	only_local_ref="y" ;;
 		-r)	has_r="y" ;;
 		esac
-		c=$((++c))
+		((++c))
 	done
 
 	case "$cur" in
@@ -2289,7 +2280,7 @@ _git_config ()
 
 _git_remote ()
 {
-	local subcommands="add rename rm show prune update set-head"
+	local subcommands="add rename rm set-head set-branches set-url show prune update"
 	local subcommand="$(__git_find_on_cmdline "$subcommands")"
 	if [ -z "$subcommand" ]; then
 		__gitcomp "$subcommands"
@@ -2297,9 +2288,12 @@ _git_remote ()
 	fi
 
 	case "$subcommand" in
-	rename|rm|show|prune)
+	rename|rm|set-url|show|prune)
 		__gitcomp_nl "$(__git_remotes)"
 		;;
+	set-head|set-branches)
+		__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
@@ -2580,7 +2574,7 @@ _git_tag ()
 			f=1
 			;;
 		esac
-		c=$((++c))
+		((++c))
 	done
 
 	case "$prev" in
@@ -2633,7 +2627,7 @@ _git ()
 		--help) command="help"; break ;;
 		*) command="$i"; break ;;
 		esac
-		c=$((++c))
+		((++c))
 	done
 
 	if [ -z "$command" ]; then
-- 
1.7.5.4
SZEDER Gábor· Feb 22, 2012, 00:17 UTC · re: Philip Jägenstedt · lore

Re: [PATCH v2] completion: remote set-* <name> and <branch>

Hi,
On Tue, Feb 21, 2012 at 10:54:34PM +0100, Philip Jägenstedt wrote:
Show 29 quoted lines
> Complete <name> only for set-url. For set-branches and
> set-head, complete <name> and <branch> over the network,
> like e.g. git pull already does.
> 
> The style used for incrementing and decrementing variables was fairly
> inconsistenty and was normalized to use ++x, or ((++x)) in contexts
> where the former would otherwise be interpreted as a command. This is a
> bash-ism, but for obvious reasons this script is already bash-specific.
> 
> Finally, remove out-of-date documentation for how to submit patches and
> (silently) defer to Documentation/SubmittingPatches like all other code.
> 
> Signed-off-by: Philip Jägenstedt <philip@foolip.org>
> ---
>  contrib/completion/git-completion.bash |   42 +++++++++++++------------------
>  1 files changed, 18 insertions(+), 24 deletions(-)
> 
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 1505cff..9dac084 100755
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -60,18 +60,6 @@
>  #       per-repository basis by setting the bash.showUpstream config
>  #       variable.
>  #
> -#
> -# To submit patches:
> -#
> -#    *) Read Documentation/SubmittingPatches

Um, well... Did you actually read it? I mean the part that talks about making separate commits for logically separate changes? ;) You clearly squashed three separate changes into a single patch.

Your proposed changes to __git_complete_remote_or_refspec() and __git_remote() make sense, modulo the increment/decrement part. I don't have strong preference either way, but please submit those changes in separate patches. Otherwise if ever bisect points to this commit during a hunt for a 'git remote' completion bug, we'll need to think about whether the increment style change or the functional change is causing the trouble.

Thanks, Gábor

 
Philip Jägenstedt· Feb 22, 2012, 08:58 UTC · re: Philip Jägenstedt · lore

[PATCH 1/4] completion: remote set-* <name> and <branch>

Complete <name> only for set-url. For set-branches and set-head, complete <name> and <branch> over the network, like e.g. git pull already does.

Signed-off-by: Philip Jägenstedt <philip@foolip.org>
---
 contrib/completion/git-completion.bash |   12 +++++++++---
 1 files changed, 9 insertions(+), 3 deletions(-)
Show changes to contrib/completion/git-completion.bash +9 −3
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index ab24310..c63a408 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -738,6 +738,9 @@ __git_complete_remote_or_refspec ()
 {
 	local cur_="$cur" cmd="${words[1]}"
 	local i c=2 remote="" pfx="" lhs=1 no_complete_refspec=0
+	if [ "$cmd" = "remote" ]; then
+		c=$((++c))
+	fi
 	while [ $c -lt $cword ]; do
 		i="${words[c]}"
 		case "$i" in
@@ -788,7 +791,7 @@ __git_complete_remote_or_refspec ()
 			__gitcomp_nl "$(__git_refs)" "$pfx" "$cur_"
 		fi
 		;;
-	pull)
+	pull|remote)
 		if [ $lhs = 1 ]; then
 			__gitcomp_nl "$(__git_refs "$remote")" "$pfx" "$cur_"
 		else
@@ -2289,7 +2292,7 @@ _git_config ()
 
 _git_remote ()
 {
-	local subcommands="add rename rm show prune update set-head"
+	local subcommands="add rename rm set-head set-branches set-url show prune update"
 	local subcommand="$(__git_find_on_cmdline "$subcommands")"
 	if [ -z "$subcommand" ]; then
 		__gitcomp "$subcommands"
@@ -2297,9 +2300,12 @@ _git_remote ()
 	fi
 
 	case "$subcommand" in
-	rename|rm|show|prune)
+	rename|rm|set-url|show|prune)
 		__gitcomp_nl "$(__git_remotes)"
 		;;
+	set-head|set-branches)
+		__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
-- 
1.7.5.4
Philip Jägenstedt· Feb 22, 2012, 08:58 UTC · re: Philip Jägenstedt · lore

[PATCH 2/4] completion: normalize increment/decrement style

The style used for incrementing and decrementing variables was fairly inconsistenty and was normalized to use x++, or ((x++)) in contexts where the former would otherwise be interpreted as a command. This is a bash-ism, but for obvious reasons this script is already bash-specific.

Signed-off-by: Philip Jägenstedt <philip@foolip.org>
---
 contrib/completion/git-completion.bash |   22 ++++++++++------------
 1 files changed, 10 insertions(+), 12 deletions(-)
Show changes to contrib/completion/git-completion.bash +10 −12
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index c63a408..1903bc9 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -149,7 +149,7 @@ __git_ps1_show_upstream ()
 			svn_upstream=${svn_upstream[ ${#svn_upstream[@]} - 2 ]}
 			svn_upstream=${svn_upstream%@*}
 			local n_stop="${#svn_remote[@]}"
-			for ((n=1; n <= n_stop; ++n)); do
+			for ((n=1; n <= n_stop; n++)); do
 				svn_upstream=${svn_upstream#${svn_remote[$n]}}
 			done
 
@@ -178,10 +178,8 @@ __git_ps1_show_upstream ()
 			for commit in $commits
 			do
 				case "$commit" in
-				"<"*) let ++behind
-					;;
-				*)    let ++ahead
-					;;
+				"<"*) ((behind++)) ;;
+				*)    ((ahead++))  ;;
 				esac
 			done
 			count="$behind	$ahead"
@@ -739,7 +737,7 @@ __git_complete_remote_or_refspec ()
 	local cur_="$cur" cmd="${words[1]}"
 	local i c=2 remote="" pfx="" lhs=1 no_complete_refspec=0
 	if [ "$cmd" = "remote" ]; then
-		c=$((++c))
+		((c++))
 	fi
 	while [ $c -lt $cword ]; do
 		i="${words[c]}"
@@ -758,7 +756,7 @@ __git_complete_remote_or_refspec ()
 		-*) ;;
 		*) remote="$i"; break ;;
 		esac
-		c=$((++c))
+		((c++))
 	done
 	if [ -z "$remote" ]; then
 		__gitcomp_nl "$(__git_remotes)"
@@ -998,7 +996,7 @@ __git_find_on_cmdline ()
 				return
 			fi
 		done
-		c=$((++c))
+		((c++))
 	done
 }
 
@@ -1009,7 +1007,7 @@ __git_has_doubledash ()
 		if [ "--" = "${words[c]}" ]; then
 			return 0
 		fi
-		c=$((++c))
+		((c++))
 	done
 	return 1
 }
@@ -1132,7 +1130,7 @@ _git_branch ()
 		-d|-m)	only_local_ref="y" ;;
 		-r)	has_r="y" ;;
 		esac
-		c=$((++c))
+		((c++))
 	done
 
 	case "$cur" in
@@ -2586,7 +2584,7 @@ _git_tag ()
 			f=1
 			;;
 		esac
-		c=$((++c))
+		((c++))
 	done
 
 	case "$prev" in
@@ -2639,7 +2637,7 @@ _git ()
 		--help) command="help"; break ;;
 		*) command="$i"; break ;;
 		esac
-		c=$((++c))
+		((c++))
 	done
 
 	if [ -z "$command" ]; then
-- 
1.7.5.4
Junio C Hamano· Feb 22, 2012, 22:05 UTC · re: Philip Jägenstedt · lore

Re: [PATCH 2/4] completion: normalize increment/decrement style

Philip Jägenstedt <philip@foolip.org> writes:
Show 6 quoted lines
> The style used for incrementing and decrementing variables was fairly
> inconsistenty and was normalized to use x++, or ((x++)) in contexts
> where the former would otherwise be interpreted as a command. This is a
> bash-ism, but for obvious reasons this script is already bash-specific.
>
> Signed-off-by: Philip Jägenstedt <philip@foolip.org>

I am not sure if the kind of changes seen in the following hunks are necessary but I do not care too deeply about it. Assuming that the change does not make the emulation by zsh unhappy, will apply.

Please stop me if somebody has issues with this rewrite.
Thanks.
Show 26 quoted lines
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index c63a408..1903bc9 100755
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -178,10 +178,8 @@ __git_ps1_show_upstream ()
>  			for commit in $commits
>  			do
>  				case "$commit" in
> -				"<"*) let ++behind
> -					;;
> -				*)    let ++ahead
> -					;;
> +				"<"*) ((behind++)) ;;
> +				*)    ((ahead++))  ;;
>  				esac
>  			done
>  			count="$behind	$ahead"
> @@ -739,7 +737,7 @@ __git_complete_remote_or_refspec ()
>  	local cur_="$cur" cmd="${words[1]}"
>  	local i c=2 remote="" pfx="" lhs=1 no_complete_refspec=0
>  	if [ "$cmd" = "remote" ]; then
> -		c=$((++c))
> +		((c++))
>  	fi
>  	while [ $c -lt $cword ]; do
>  		i="${words[c]}"
Philip Jägenstedt· Feb 22, 2012, 08:58 UTC · re: Philip Jägenstedt · lore

[PATCH 3/4] completion: remove stale "to submit patches" documentation

It was out-of-sync with the reality of who works on this script. Defer (silently) to Documentation/SubmittingPatches like all other code.

Signed-off-by: Philip Jägenstedt <philip@foolip.org>
---
 contrib/completion/git-completion.bash |   12 ------------
 1 files changed, 0 insertions(+), 12 deletions(-)
Show changes to contrib/completion/git-completion.bash +0 −12
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 1903bc9..48237c8 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -60,18 +60,6 @@
 #       per-repository basis by setting the bash.showUpstream config
 #       variable.
 #
-#
-# To submit patches:
-#
-#    *) Read Documentation/SubmittingPatches
-#    *) Send all patches to the current maintainer:
-#
-#       "Shawn O. Pearce" <spearce@spearce.org>
-#
-#    *) Always CC the Git mailing list:
-#
-#       git@vger.kernel.org
-#
 
 if [[ -n ${ZSH_VERSION-} ]]; then
 	autoload -U +X bashcompinit && bashcompinit
-- 
1.7.5.4
Philip Jägenstedt· Feb 22, 2012, 08:58 UTC · re: Philip Jägenstedt · lore

[PATCH 4/4] completion: use tabs for indentation

CodingGuidlines confidently declares "We use tabs for indentation." It would be a shame if it were caught lying.

Signed-off-by: Philip Jägenstedt <philip@foolip.org>
---
 contrib/completion/git-completion.bash |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)
Show changes to contrib/completion/git-completion.bash +4 −4
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 48237c8..03f0b8c 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -284,13 +284,13 @@ __git_ps1 ()
 				fi
 			fi
 			if [ -n "${GIT_PS1_SHOWSTASHSTATE-}" ]; then
-			        git rev-parse --verify refs/stash >/dev/null 2>&1 && s="$"
+				git rev-parse --verify refs/stash >/dev/null 2>&1 && s="$"
 			fi
 
 			if [ -n "${GIT_PS1_SHOWUNTRACKEDFILES-}" ]; then
-			   if [ -n "$(git ls-files --others --exclude-standard)" ]; then
-			      u="%"
-			   fi
+				if [ -n "$(git ls-files --others --exclude-standard)" ]; then
+					u="%"
+				fi
 			fi
 
 			if [ -n "${GIT_PS1_SHOWUPSTREAM-}" ]; then
-- 
1.7.5.4
Thomas Rast· Feb 22, 2012, 09:52 UTC · re: Philip Jägenstedt · lore

Re: [PATCH v3] completion: remote set-* <name> and <branch>

Philip Jägenstedt <philip@foolip.org> writes:
> Hope this works better. Is it possible to use format-patch or send-email 
> to get subjects like [PATCH 1/4 v3], as opposed to what I am sending?

'git format-patch --subject-prefix="PATCH v3"' gives you [PATCH v3 1/4] which I think is what everyone does around here...

-- 
Thomas Rast
trast@{inf,student}.ethz.ch
Junio C Hamano· Feb 21, 2012, 22:23 UTC · re: Philip Jägenstedt · lore

Re: [PATCH] completion: remote set-* <name> and <branch>

Philip Jägenstedt <philip@foolip.org> writes:
> Thanks. Perhaps git-completion.bash should not say "Send all patches
> to the current maintainer" and simply defer to SubmittingPatches?
I see you did this in your follow-up patch.  Thanks.
Show 24 quoted lines
>>>  {
>>>       local cur_="$cur" cmd="${words[1]}"
>>>       local i c=2 remote="" pfx="" lhs=1 no_complete_refspec=0
>>> +     if [ "$cmd" = "remote" ]; then
>>> +             c=$((++c))
>>> +     fi
>>
>> I don't know about others, but auto-incrementing a variable and assigning
>> the result to the same variable, while not wrong at all, hurts my brain.
>>
>>        c=$(($c + 1))
>>
>> is far more readable and does not suggest there is any funky magic
>> involved.  Also it is a good habit to get into not to omit $ from
>> variables inside arithmetic substitution, even though bash allows it and
>> this script is meant to be consumed only by shells that understand this
>> bash-ism.
>>
>> I do not know offhand if zsh groks it, but the point is that you do not
>> have to worry about it if you write "$(($c+1))" instead of "$((c+1))".
>
> CodingGuidlines suggests to follow local convention, which was
> "c=$((++c))". This file also uses "++n", "i++" and "((i++))". I will
> send a v2 patch that normalizes these, open to discussion of course.

OK, it was my mistake to suggest $c++ in this file; it liberally uses dollar-less variables, and I agree that it is a good idea to stick to that local convention.

But I think you went too far in your follow-up patch to make the increment and decrement uniform.

"i++" is so much easier on the eye unless you must use "++i" in order to use the value of the incremented "i" in an expression, and the changes to turn existing instances of free-standing "i++" to "++i" done only for the side effect of incrementing the variables look totally backwards.

Although I do not deeply care. Just leaving the new one as you originally wrote, i.e.

	c=$((++c))
would have been easier to review for the area experts, I would think.
SZEDER Gábor· Feb 22, 2012, 00:03 UTC · re: Junio C Hamano · lore

Re: [PATCH] completion: remote set-* <name> and <branch>

On Tue, Feb 21, 2012 at 02:23:11PM -0800, Junio C Hamano wrote:
Show 20 quoted lines
> >>>  {
> >>>       local cur_="$cur" cmd="${words[1]}"
> >>>       local i c=2 remote="" pfx="" lhs=1 no_complete_refspec=0
> >>> +     if [ "$cmd" = "remote" ]; then
> >>> +             c=$((++c))
> >>> +     fi
> >>
> >> I don't know about others, but auto-incrementing a variable and assigning
> >> the result to the same variable, while not wrong at all, hurts my brain.
> >>
> >>        c=$(($c + 1))
> >>
> >> is far more readable and does not suggest there is any funky magic
> >> involved.  Also it is a good habit to get into not to omit $ from
> >> variables inside arithmetic substitution, even though bash allows it and
> >> this script is meant to be consumed only by shells that understand this
> >> bash-ism.
> >>
> >> I do not know offhand if zsh groks it, but the point is that you do not
> >> have to worry about it if you write "$(($c+1))" instead of "$((c+1))".

This c=$((++c)) style increment is used in several completion functions, even in the main entry function _git() itself. If zsh didn't grok it, then completion wouldn't work at all on zsh, because the while loop in _git() would be an endless loop.

Show 22 quoted lines
> > CodingGuidlines suggests to follow local convention, which was
> > "c=$((++c))". This file also uses "++n", "i++" and "((i++))". I will
> > send a v2 patch that normalizes these, open to discussion of course.
> 
> OK, it was my mistake to suggest $c++ in this file; it liberally uses
> dollar-less variables, and I agree that it is a good idea to stick to that
> local convention.
> 
> But I think you went too far in your follow-up patch to make the increment
> and decrement uniform.
> 
> "i++" is so much easier on the eye unless you must use "++i" in order to
> use the value of the incremented "i" in an expression, and the changes to
> turn existing instances of free-standing "i++" to "++i" done only for the
> side effect of incrementing the variables look totally backwards.
> 
> Although I do not deeply care.  Just leaving the new one as you originally
> wrote, i.e.
> 
> 	c=$((++c))
> 
> would have been easier to review for the area experts, I would think.
The issue of this weird increment came up before
  http://thread.gmane.org/gmane.comp.version-control.git/87650/focus=87806

and the conclusion back then was that we kept the existing style. But otherwise I agree, it hurts my brain, too.

Best, Gábor

← back to recent threads