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

["SZEDER Gábor" <szeder@ira.uka.de>]

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