On Tue, Feb 21, 2012 at 02:23:11PM -0800, Junio C Hamano wrote:
> >>> {
> >>> 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.
> > 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html