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

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

[Johannes Schindelin]

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(-)

-- 
2.7.0.windows.1.7.g55a05c8

[PATCH 1/3] filter-branch: fix style of $((...) construct

[Johannes Schindelin]

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(-)

diff --git a/git-filter-branch.sh b/git-filter-branch.sh
index 86b2ff1..1067785 100755
--- a/git-filter-branch.sh
+++ b/git-filter-branch.sh
@@ -283,13 +283,13 @@ report_progress ()
 		count=$git_filter_branch__commit_count
 
 		now=$(date +%s)
-		elapsed=$(($now - $start_timestamp))
-		remaining=$(( ($commits - $count) * $elapsed / $count ))
+		elapsed=$(($now-$start_timestamp))
+		remaining=$((($commits-$count)*$elapsed/$count))
 		if test $elapsed -gt 0
 		then
-			next_sample_at=$(( ($elapsed + 1) * $count / $elapsed ))
+			next_sample_at=$((($elapsed+1)*$count/$elapsed))
 		else
-			next_sample_at=$(($next_sample_at + 1))
+			next_sample_at=$(($next_sample_at+1))
 		fi
 		progress=" ($elapsed seconds passed, remaining $remaining predicted)"
 	fi
-- 
2.7.0.windows.1.7.g55a05c8

[PATCH 2/3] rebase--interactive: adjust the coding style of $((...))

[Johannes Schindelin]

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(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index c0cfe88..3e2bd1b 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -122,7 +122,7 @@ mark_action_done () {
 	mv -f "$todo".new "$todo"
 	new_count=$(git stripspace --strip-comments <"$done" | wc -l)
 	echo $new_count >"$msgnum"
-	total=$(($new_count + $(git stripspace --strip-comments <"$todo" | wc -l)))
+	total=$(($new_count+$(git stripspace --strip-comments <"$todo" | wc -l)))
 	echo $total >"$end"
 	if test "$last_count" != "$new_count"
 	then
@@ -895,7 +895,7 @@ check_bad_cmd_and_sha () {
 	lineno=0
 	while read -r command rest
 	do
-		lineno=$(( $lineno + 1 ))
+		lineno=$(($lineno+1))
 		case $command in
 		"$comment_char"*|''|noop|x|exec)
 			# Doesn't expect a SHA-1
-- 
2.7.0.windows.1.7.g55a05c8

[PATCH 3/3] rebase --merge: adjust $((...)) coding style

[Johannes Schindelin]

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(-)

diff --git a/git-rebase--merge.sh b/git-rebase--merge.sh
index 2cc2a6d..a9bd39a 100644
--- a/git-rebase--merge.sh
+++ b/git-rebase--merge.sh
@@ -48,7 +48,7 @@ continue_merge () {
 	GIT_PAGER='' git log --format=%s -1 "$cmt"
 
 	# onto the next patch:
-	msgnum=$(($msgnum + 1))
+	msgnum=$(($msgnum+1))
 	echo "$msgnum" >"$state_dir/msgnum"
 }
 
@@ -59,7 +59,7 @@ call_merge () {
 	echo "$cmt" > "$state_dir/current"
 	hd=$(git rev-parse --verify HEAD)
 	cmt_name=$(git symbolic-ref HEAD 2> /dev/null || echo HEAD)
-	eval GITHEAD_$cmt='"${cmt_name##refs/heads/}~$(($end - $msgnum))"'
+	eval GITHEAD_$cmt='"${cmt_name##refs/heads/}~$(($end-$msgnum))"'
 	eval GITHEAD_$hd='$onto_name'
 	export GITHEAD_$cmt GITHEAD_$hd
 	if test -n "$GIT_QUIET"
@@ -126,7 +126,7 @@ continue)
 skip)
 	read_state
 	git rerere clear
-	msgnum=$(($msgnum + 1))
+	msgnum=$(($msgnum+1))
 	while test "$msgnum" -le "$end"
 	do
 		call_merge "$msgnum"
@@ -144,7 +144,7 @@ write_basic_state
 msgnum=0
 for cmt in $(git rev-list --reverse --no-merges "$revisions")
 do
-	msgnum=$(($msgnum + 1))
+	msgnum=$(($msgnum+1))
 	echo "$cmt" > "$state_dir/cmt.$msgnum"
 done
 
-- 
2.7.0.windows.1.7.g55a05c8

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

[John Keeping]

On Thu, Feb 04, 2016 at 01:01:39PM +0100, Johannes Schindelin wrote:
> 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.

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.

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

[Johannes Schindelin]

Hi John,

On Thu, 4 Feb 2016, John Keeping wrote:

> Using spaces around operators also matches our C coding style.

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

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

[John Keeping]

On Thu, Feb 04, 2016 at 01:38:51PM +0100, Johannes Schindelin wrote:
> On Thu, 4 Feb 2016, John Keeping wrote:
> 
> > Using spaces around operators also matches our C coding style.
> 
> 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.

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.

> 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).

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

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

[Johannes Schindelin]

Hi John,

On Thu, 4 Feb 2016, John Keeping wrote:

> Although I don't think the historic context is useful in deciding which
> direction to go in the future.

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

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

[John Keeping]

On Thu, Feb 04, 2016 at 02:13:47PM +0100, Johannes Schindelin wrote:
> On Thu, 4 Feb 2016, John Keeping wrote:
> 
> > Although I don't think the historic context is useful in deciding which
> > direction to go in the future.
> 
> Being a maintainer, I find that argument particularly hard to defend.

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.

> 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 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.

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

[Johannes Schindelin]

Hi John,

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.

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

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

[John Keeping]

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

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

[Junio C Hamano]

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

> 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
>
> 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:

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.

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

[Junio C Hamano]

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

> 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.

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.