Re: [PATCH] completion: remote set-* <name> and <branch>
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Feb 21, 2012, 22:23 UTC
- Message-ID
- <7v8vjvg71s.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <CAKHWUkZjQYnf=LwS_RC-E_7gV73AzbWjxs33E5-FL25s6_qX4g@mail.gmail.com>
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.