patch, 3 partsFix $((...)) coding style

I noticed through a nearby patch series that was submitted by Elia that some of the $((...)) expressions introduced in scripts that I introduced to Git's source code did not match the existing code's convention: previously these expressions did not contain any spaces, now *some* do.

This patch series tries to clean that up quickly before even more code has to decide which one of the disagreeing coding conventions to use.

Note: For the sake of getting this patch series out, I skipped t/ and
contrib/. I do not care much about the latter, but t/ should probably be
fixed, too.

Johannes Schindelin (3):
  filter-branch: fix style of $((...) construct
  rebase--interactive: adjust the coding style of $((...))
  rebase --merge: adjust $((...)) coding style

 git-filter-branch.sh       | 8 ++++----
 git-rebase--interactive.sh | 4 ++--
 git-rebase--merge.sh       | 8 ++++----
 3 files changed, 10 insertions(+), 10 deletions(-)

In 6a9d16a (filter-branch: add passed/remaining seconds on progress, 2015-09-07) we introduced a new style to write $((...) expressions: with spaces in the arithmetic expression. We actually introduced *two* new styles, even: some of the $((...)) expressions had spaces after the two opening and before the two closing parentheses, but others did not.

There had been an arithmetic expression in that script, though, and it matched the coding style of the rest of Git's source code: $((...)) expressions simply do not contain spaces.

Let's adjust the style to the previously existing code.

Cc: Gabor Bernat <gabor.bernat@gravityrd.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 git-filter-branch.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

In 180bad3 (rebase -i: respect core.commentchar, 2013-02-11) we changed the coding style of an arithmetic expression to introduce spaces. The existing $((...)) expression did not use that convention, though, and neither was it the habit of the other shell scripts in Git's source code.

Later, 1db168e (rebase-i: loosen over-eager check_bad_cmd check, 2015-10-01) repeated the same coding style, again not matching the rest of the source code's style.

Let's make the coding style consistent again.

Cc: John Keeping <john@keeping.me.uk>
Cc: Matthieu Moy <Matthieu.Moy@imag.fr>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 git-rebase--interactive.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

In 58634db (rebase: Allow merge strategies to be used when rebasing, 2006-06-21) we introduced $((...)) expressions containing spaces, disagreeing with Git's existing source code's convention.

Naturally, 0bb733c (Use branch names in 'git-rebase -m' conflict hunks., 2006-12-28) repeated the convention set forth in git-rebase--merge.sh.

Let's make the coding style consistent again.

Cc: Eric Wong <normalperson@yhbt.net>
Cc: Shawn O. Pearce <spearce@spearce.org>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 git-rebase--merge.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

On Thu, Feb 04, 2016 at 01:01:39PM +0100, Johannes Schindelin wrote:

Should this be going this other way (i.e. standardising on having the spaces)?

The current state (excluding contrib/ and t/) seems to favour spaces:

$ git grep '\$((' -- ':/' ':!t/' ':!contrib/' Documentation/CodingGuidelines: - We use Arithmetic Expansion $(( ... )). Documentation/CodingGuidelines: of them, as some shells do not grok $((x)) while accepting $(($x)) generate-cmdlist.sh: n=$(($n+1)) git-filter-branch.sh: elapsed=$(($now - $start_timestamp)) git-filter-branch.sh: remaining=$(( ($commits - $count) * $elapsed / $count )) git-filter-branch.sh: next_sample_at=$(( ($elapsed + 1) * $count / $elapsed )) git-filter-branch.sh: next_sample_at=$(($next_sample_at + 1)) git-filter-branch.sh: git_filter_branch__commit_count=$(($git_filter_branch__commit_count+1)) git-rebase--interactive.sh: total=$(($new_count + $(git stripspace --strip-comments <"$todo" | wc -l))) git-rebase--interactive.sh: count=$(($(sed -n \ git-rebase--interactive.sh: lineno=$(( $lineno + 1 )) git-rebase--merge.sh: msgnum=$(($msgnum + 1)) git-rebase--merge.sh: eval GITHEAD_$cmt='"${cmt_name##refs/heads/}~$(($end - $msgnum))"' git-rebase--merge.sh: msgnum=$(($msgnum + 1)) git-rebase--merge.sh: msgnum=$(($msgnum + 1)) git-submodule.sh: n=$(($1 + 0)) 2>/dev/null && test "$n" = "$1" git-submodule.sh: total_commits=" ($(($total_commits + 0)))"

I make that 3 without spaces (including the git-rebase--interactive.sh case that wraps) and 12 that do have spaces around operators. Using spaces around operators also matches our C coding style.

Hi John,

On Thu, 4 Feb 2016, John Keeping wrote:

Well, by that reasoning you should go the whole nine yards and write

	lineno = $(( $lineno + 1 ))

Except you can't. Because shell code is inherently not like C code.

What I found particularly interesting about 180bad3 (rebase -i: respect core.commentchar, 2013-02-11) was that it *snuck in* that coding style: it *changed* the existing one (without rationale in the commit message, too).

Ciao, Johannes

On Thu, Feb 04, 2016 at 01:38:51PM +0100, Johannes Schindelin wrote:

That isn't my main argument, although I do think the (presumed) rationale for using spaces in C to improve usability applies here as well, even if the confines of the language don't let us go as far as in C.

I'm not actually sure whether spaces inside the enclosing $(( and )) are helpful, we're much less consistent about that than about spaces around binary operators. Having looked at t/ as well now, we really do seem to favour using spaces around the binary operators so I'm further convinced this series is switching the wrong cases.

The last version of that patch I can find in the archive [1] doesn't add the spaces, so I think they must have been part of Junio's fixup discusses in the following messages. Although I don't think the historic context is useful in deciding which direction to go in the future.

[1] http://article.gmane.org/gmane.comp.version-control.git/216103

Hi John,

On Thu, 4 Feb 2016, John Keeping wrote:

Being a maintainer, I find that argument particularly hard to defend.

But sure, you go ahead and prepare a patch series that turns everything around, adding spaces around those operators.

Whatever the outcome, the inconsistency must be fixed. I just submitted my proposed solution.

Ciao, Johannes

On Thu, Feb 04, 2016 at 02:13:47PM +0100, Johannes Schindelin wrote:

I worded that badly, what I wanted to say is that how we got here is less interesting than where we are. From a quick bit of grep'ing it looks to me like where we are is in favour of adding spaces around binary operators inside $(( )) constructs based on the majority of the uses in the code as it currently stands.

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.

Hi John,

On Thu, 4 Feb 2016, John Keeping wrote:

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.

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.

At this point I am a bit fed up with the discussion because by now it feels to me more like a Slashdot thread than a technical discussion with a direction.

As far as technical direction goes, here is the summary of what I have to say: I still think that the changes I submitted are good because they seem to me to reinstate the coding style with which we started out.

And that's really all I have to say about this matter.

Ciao, Johannes

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

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

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

John Keeping <john@keeping.me.uk> writes:

I agree that our codebase seems to have a moderately strong preference for having SP around binary operators, i.e.

	$term1 * $term2 + $term3

over not having one:

	$term1*$term2+$term3

and I think that is OK to declare it as our style, primarily because that is how we encourage to write our expressions in C, as you mentioned earlier.

One thing I was wondering about the $(( ... )) syntax while reading this thread was about the SP around the expression, i.e.

	var=$(( $term1 * $term2 + $term3 ))

vs

	var=$(($term1 * $term2 + $term3))

I personally do not have strong preference between the two, but I have a vague impression that we preferred the former because somebody in the past gave us a good explanation why we should.

"git grep" however seems to tell us that we are not clearly decided between the two, so I probably am misremembering it and there is no preference either way.

In any case, it is good to catch a patch that mixes style changes and other things during a review, and it also is good to clean up the style before you start working on a specific part of the code to make gradual improvement without stepping on others' toes.

Thanks.

Junio C Hamano <gitster@pobox.com> writes:

One thing that is somewhat related is that we would want to avoid writing things like this by mistake:

	var=$((cmd1 && cmd2) | cmd3)

which is not meant to be an arithmetic expansion, but is a command substitution that happens to have a subshell at the head of the pipeline. I think bash gets this right, but some shells (e.g. dash) tries to parse this as an arithmetic expansion upon seeing "$((" and fails to parse it, waiting forever until it sees the matching "))". So we write it like this to make it safer:

	var=$( (cmd1 && cmd2) | cmd3 )

Perhaps having a SP after $(( of a real arithmetic expression, i.e.

	var=$(( ... anything ... ))

makes it easier to tell these two apart?  I dunno.