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

From: Philip Jägenstedt <philip@foolip.org>
Date: Feb 21, 2012, 21:29 UTC
Message-ID: <CAKHWUkZjQYnf=LwS_RC-E_7gV73AzbWjxs33E5-FL25s6_qX4g@mail.gmail.com>
In-Reply-To: <7vvcn2lyw6.fsf@alter.siamese.dyndns.org>

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