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

From: John Keeping <john@keeping.me.uk>
Date: Feb 4, 2016, 15:53 UTC
Message-ID: <20160204155316.GI29880@serenity.lan>
In-Reply-To: <alpine.DEB.2.20.1602041619430.2964@virtualbox>

On Thu, Feb 04, 2016 at 04:27:49PM +0100, Johannes Schindelin wrote:

Show 14 quoted lines

> 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

Show 7 quoted lines

> 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