Re: [PATCH 0/3] Fix $((...)) coding style

[John Keeping <john@keeping.me.uk>]

On Thu, Feb 04, 2016 at 04:27:49PM +0100, Johannes Schindelin wrote:
> On Thu, 4 Feb 2016, John Keeping wrote:
> 
> > On Thu, Feb 04, 2016 at 02:13:47PM +0100, Johannes Schindelin wrote:
> > > Whatever the outcome, the inconsistency must be fixed.
> > 
> > I disagree.  Unless there are other changes in the same area, the noise
> > isn't worth it.
> > 
> > However, I do think we need to agree on a policy so that new code can be
> > consistent.  This should then be documented in CodingGuidelines.
> 
> If you want to enforce it in the future, how can you possibly be against
> fixing the inconsistency in the existing code? Sorry, I am really unable
> to understand this.

I avoided quoting CodingGuidelines in my previous message, but it says:

 - Fixing style violations while working on a real change as a
   preparatory clean-up step is good, but otherwise avoid useless code
   churn for the sake of conforming to the style.

   "Once it _is_ in the tree, it's not really worth the patch noise to
   go and fix it up."
   Cf. http://article.gmane.org/gmane.linux.kernel/943020

> Also, we *did* document the policy in the CodingGuidelines:
> 
> 	As for more concrete guidelines, just imitate the existing code
> 
> So. There you are. By that token, my patch series was the correct thing to
> do because the first arithmetic expression introduced into a shell script
> in Git's source code had no spaces.

This is the first point I made.  To take one example, in
git-filter-branch.sh there are five occurrences of the sequence "$((";
your patch changes four of them to remove spaces.  If we standardise on
having spaces only one needs to change:

$ git grep -F '$((' origin/master -- git-filter-branch.sh
origin/master:git-filter-branch.sh:             elapsed=$(($now - $start_timestamp))
origin/master:git-filter-branch.sh:             remaining=$(( ($commits - $count) * $elapsed / $count ))
origin/master:git-filter-branch.sh:                     next_sample_at=$(( ($elapsed + 1) * $count / $elapsed ))
origin/master:git-filter-branch.sh:                     next_sample_at=$(($next_sample_at + 1))
origin/master:git-filter-branch.sh:     git_filter_branch__commit_count=$(($git_filter_branch__commit_count+1))

I chose git-filter-branch.sh simply because it was touched by this patch
set but it is not an outlier in this regard.  Some crude statistics
across all of git.git:

# No space after plus
$ git grep -F '$((' | grep '\+[\$0-9]' | wc -l
34

# With space after plus
$ git grep -F '$((' | grep '\+ [\$0-9]' | wc -l
96