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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 20, 2012, 07:58 UTC
Message-ID: <7vvcn2lyw6.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1329571973-20796-1-git-send-email-philip@foolip.org>

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