{"thread":{"id":"41331","subject":"[PATCH 0/3] Fix $((...)) coding style","startedAt":"2016-02-04T12:01:39Z","lastAt":"2016-02-04T19:43:58Z","messageCount":13,"participants":["Johannes Schindelin","John Keeping","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"277423","messageId":"cover.1454587284.git.johannes.schindelin@gmx.de","threadId":"41331","inReplyTo":null,"subject":"[PATCH 0/3] Fix $((...)) coding style","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-02-04T12:01:39Z","receivedAt":"2016-02-04T12:01:39Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"I noticed through a nearby patch series that was submitted by Elia that\nsome of the $((...)) expressions introduced in scripts that I introduced\nto Git's source code did not match the existing code's convention:\npreviously these expressions did not contain any spaces, now *some* do.\n\nThis patch series tries to clean that up quickly before even more code\nhas to decide which one of the disagreeing coding conventions to use.\n\nNote: For the sake of getting this patch series out, I skipped t/ and\ncontrib/. I do not care much about the latter, but t/ should probably be\nfixed, too.\n\n\nJohannes Schindelin (3):\n filter-branch: fix style of $((...) construct\n rebase--interactive: adjust the coding style of $((...))\n rebase --merge: adjust $((...)) coding style\n\n git-filter-branch.sh | 8 ++++----\n git-rebase--interactive.sh | 4 ++--\n git-rebase--merge.sh | 8 ++++----\n 3 files changed, 10 insertions(+), 10 deletions(-)\n\n-- \n2.7.0.windows.1.7.g55a05c8\n"},{"id":"282598","messageId":"98c202fa82ce99367da1d616ca02cc7b48d1e04e.1454587284.git.johannes.schindelin@gmx.de","threadId":"41331","inReplyTo":"cover.1454587284.git.johannes.schindelin@gmx.de","subject":"[PATCH 1/3] filter-branch: fix style of $((...) construct","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-02-04T12:02:00Z","receivedAt":"2016-02-04T12:02:00Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"In 6a9d16a (filter-branch: add passed/remaining seconds on progress,\n2015-09-07) we introduced a new style to write $((...) expressions: with\nspaces in the arithmetic expression. We actually introduced *two* new\nstyles, even: some of the $((...)) expressions had spaces after the two\nopening and before the two closing parentheses, but others did not.\n\nThere had been an arithmetic expression in that script, though, and it\nmatched the coding style of the rest of Git's source code: $((...))\nexpressions simply do not contain spaces.\n\nLet's adjust the style to the previously existing code.\n\nCc: Gabor Bernat \nSigned-off-by: Johannes Schindelin \n---\n git-filter-branch.sh | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/git-filter-branch.sh b/git-filter-branch.sh\nindex 86b2ff1..1067785 100755\n--- a/git-filter-branch.sh\n+++ b/git-filter-branch.sh\n@@ -283,13 +283,13 @@ report_progress ()\n \t\tcount=$git_filter_branch__commit_count\n \n \t\tnow=$(date +%s)\n-\t\telapsed=$(($now - $start_timestamp))\n-\t\tremaining=$(( ($commits - $count) * $elapsed / $count ))\n+\t\telapsed=$(($now-$start_timestamp))\n+\t\tremaining=$((($commits-$count)*$elapsed/$count))\n \t\tif test $elapsed -gt 0\n \t\tthen\n-\t\t\tnext_sample_at=$(( ($elapsed + 1) * $count / $elapsed ))\n+\t\t\tnext_sample_at=$((($elapsed+1)*$count/$elapsed))\n \t\telse\n-\t\t\tnext_sample_at=$(($next_sample_at + 1))\n+\t\t\tnext_sample_at=$(($next_sample_at+1))\n \t\tfi\n \t\tprogress=\" ($elapsed seconds passed, remaining $remaining predicted)\"\n \tfi\n-- \n2.7.0.windows.1.7.g55a05c8\n"},{"id":"277424","messageId":"dbd8ee3b1de55b7414c91bf203be2aaa77b78822.1454587284.git.johannes.schindelin@gmx.de","threadId":"41331","inReplyTo":"cover.1454587284.git.johannes.schindelin@gmx.de","subject":"[PATCH 2/3] rebase--interactive: adjust the coding style of $((...))","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-02-04T12:02:30Z","receivedAt":"2016-02-04T12:02:30Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"In 180bad3 (rebase -i: respect core.commentchar, 2013-02-11) we changed\nthe coding style of an arithmetic expression to introduce spaces. The\nexisting $((...)) expression did not use that convention, though, and\nneither was it the habit of the other shell scripts in Git's source code.\n\nLater, 1db168e (rebase-i: loosen over-eager check_bad_cmd check,\n2015-10-01) repeated the same coding style, again not matching the rest of\nthe source code's style.\n\nLet's make the coding style consistent again.\n\nCc: John Keeping \nCc: Matthieu Moy \nSigned-off-by: Johannes Schindelin \n---\n git-rebase--interactive.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex c0cfe88..3e2bd1b 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -122,7 +122,7 @@ mark_action_done () {\n \tmv -f \"$todo\".new \"$todo\"\n \tnew_count=$(git stripspace --strip-comments <\"$done\" | wc -l)\n \techo $new_count >\"$msgnum\"\n-\ttotal=$(($new_count + $(git stripspace --strip-comments <\"$todo\" | wc -l)))\n+\ttotal=$(($new_count+$(git stripspace --strip-comments <\"$todo\" | wc -l)))\n \techo $total >\"$end\"\n \tif test \"$last_count\" != \"$new_count\"\n \tthen\n@@ -895,7 +895,7 @@ check_bad_cmd_and_sha () {\n \tlineno=0\n \twhile read -r command rest\n \tdo\n-\t\tlineno=$(( $lineno + 1 ))\n+\t\tlineno=$(($lineno+1))\n \t\tcase $command in\n \t\t\"$comment_char\"*|''|noop|x|exec)\n \t\t\t# Doesn't expect a SHA-1\n-- \n2.7.0.windows.1.7.g55a05c8\n"},{"id":"277425","messageId":"6c243cf13271ed285409063e501ec67c19df767d.1454587284.git.johannes.schindelin@gmx.de","threadId":"41331","inReplyTo":"cover.1454587284.git.johannes.schindelin@gmx.de","subject":"[PATCH 3/3] rebase --merge: adjust $((...)) coding style","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-02-04T12:03:00Z","receivedAt":"2016-02-04T12:03:00Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"In 58634db (rebase: Allow merge strategies to be used when rebasing,\n2006-06-21) we introduced $((...)) expressions containing spaces,\ndisagreeing with Git's existing source code's convention.\n\nNaturally, 0bb733c (Use branch names in 'git-rebase -m' conflict hunks.,\n2006-12-28) repeated the convention set forth in git-rebase--merge.sh.\n\nLet's make the coding style consistent again.\n\nCc: Eric Wong \nCc: Shawn O. Pearce \nSigned-off-by: Johannes Schindelin \n---\n git-rebase--merge.sh | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/git-rebase--merge.sh b/git-rebase--merge.sh\nindex 2cc2a6d..a9bd39a 100644\n--- a/git-rebase--merge.sh\n+++ b/git-rebase--merge.sh\n@@ -48,7 +48,7 @@ continue_merge () {\n \tGIT_PAGER='' git log --format=%s -1 \"$cmt\"\n \n \t# onto the next patch:\n-\tmsgnum=$(($msgnum + 1))\n+\tmsgnum=$(($msgnum+1))\n \techo \"$msgnum\" >\"$state_dir/msgnum\"\n }\n \n@@ -59,7 +59,7 @@ call_merge () {\n \techo \"$cmt\" > \"$state_dir/current\"\n \thd=$(git rev-parse --verify HEAD)\n \tcmt_name=$(git symbolic-ref HEAD 2> /dev/null || echo HEAD)\n-\teval GITHEAD_$cmt='\"${cmt_name##refs/heads/}~$(($end - $msgnum))\"'\n+\teval GITHEAD_$cmt='\"${cmt_name##refs/heads/}~$(($end-$msgnum))\"'\n \teval GITHEAD_$hd='$onto_name'\n \texport GITHEAD_$cmt GITHEAD_$hd\n \tif test -n \"$GIT_QUIET\"\n@@ -126,7 +126,7 @@ continue)\n skip)\n \tread_state\n \tgit rerere clear\n-\tmsgnum=$(($msgnum + 1))\n+\tmsgnum=$(($msgnum+1))\n \twhile test \"$msgnum\" -le \"$end\"\n \tdo\n \t\tcall_merge \"$msgnum\"\n@@ -144,7 +144,7 @@ write_basic_state\n msgnum=0\n for cmt in $(git rev-list --reverse --no-merges \"$revisions\")\n do\n-\tmsgnum=$(($msgnum + 1))\n+\tmsgnum=$(($msgnum+1))\n \techo \"$cmt\" > \"$state_dir/cmt.$msgnum\"\n done\n \n-- \n2.7.0.windows.1.7.g55a05c8\n"},{"id":"277426","messageId":"20160204121437.GF29880@serenity.lan","threadId":"41331","inReplyTo":"cover.1454587284.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH 0/3] Fix $((...)) coding style","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2016-02-04T12:14:37Z","receivedAt":"2016-02-04T12:14:37Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Thu, Feb 04, 2016 at 01:01:39PM +0100, Johannes Schindelin wrote:\n> I noticed through a nearby patch series that was submitted by Elia that\n> some of the $((...)) expressions introduced in scripts that I introduced\n> to Git's source code did not match the existing code's convention:\n> previously these expressions did not contain any spaces, now *some* do.\n> \n> This patch series tries to clean that up quickly before even more code\n> has to decide which one of the disagreeing coding conventions to use.\n> \n> Note: For the sake of getting this patch series out, I skipped t/ and\n> contrib/. I do not care much about the latter, but t/ should probably be\n> fixed, too.\n\nShould this be going this other way (i.e. standardising on having the\nspaces)?\n\nThe current state (excluding contrib/ and t/) seems to favour spaces:\n\n$ git grep '\\$((' -- ':/' ':!t/' ':!contrib/'\nDocumentation/CodingGuidelines: - We use Arithmetic Expansion $(( ... )).\nDocumentation/CodingGuidelines: of them, as some shells do not grok $((x)) while accepting $(($x))\ngenerate-cmdlist.sh: n=$(($n+1))\ngit-filter-branch.sh: elapsed=$(($now - $start_timestamp))\ngit-filter-branch.sh: remaining=$(( ($commits - $count) * $elapsed / $count ))\ngit-filter-branch.sh: next_sample_at=$(( ($elapsed + 1) * $count / $elapsed ))\ngit-filter-branch.sh: next_sample_at=$(($next_sample_at + 1))\ngit-filter-branch.sh: git_filter_branch__commit_count=$(($git_filter_branch__commit_count+1))\ngit-rebase--interactive.sh: total=$(($new_count + $(git stripspace --strip-comments <\"$todo\" | wc -l)))\ngit-rebase--interactive.sh: count=$(($(sed -n \\\ngit-rebase--interactive.sh: lineno=$(( $lineno + 1 ))\ngit-rebase--merge.sh: msgnum=$(($msgnum + 1))\ngit-rebase--merge.sh: eval GITHEAD_$cmt='\"${cmt_name##refs/heads/}~$(($end - $msgnum))\"'\ngit-rebase--merge.sh: msgnum=$(($msgnum + 1))\ngit-rebase--merge.sh: msgnum=$(($msgnum + 1))\ngit-submodule.sh: n=$(($1 + 0)) 2>/dev/null && test \"$n\" = \"$1\"\ngit-submodule.sh: total_commits=\" ($(($total_commits + 0)))\"\n\nI make that 3 without spaces (including the git-rebase--interactive.sh\ncase that wraps) and 12 that do have spaces around operators. Using\nspaces around operators also matches our C coding style.\n"},{"id":"277427","messageId":"alpine.DEB.2.20.1602041334450.2964@virtualbox","threadId":"41331","inReplyTo":"20160204121437.GF29880@serenity.lan","subject":"Re: [PATCH 0/3] Fix $((...)) coding style","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-02-04T12:38:51Z","receivedAt":"2016-02-04T12:38:51Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi John,\n\nOn Thu, 4 Feb 2016, John Keeping wrote:\n\n> Using spaces around operators also matches our C coding style.\n\nWell, by that reasoning you should go the whole nine yards and write\n\n\tlineno = $(( $lineno + 1 ))\n\nExcept you can't. Because shell code is inherently not like C code.\n\nWhat I found particularly interesting about 180bad3 (rebase -i: respect\ncore.commentchar, 2013-02-11) was that it *snuck in* that coding style: it\n*changed* the existing one (without rationale in the commit message, too).\n\nCiao,\nJohannes\n"},{"id":"277428","messageId":"20160204130111.GG29880@serenity.lan","threadId":"41331","inReplyTo":"alpine.DEB.2.20.1602041334450.2964@virtualbox","subject":"Re: [PATCH 0/3] Fix $((...)) coding style","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2016-02-04T13:01:11Z","receivedAt":"2016-02-04T13:01:11Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Thu, Feb 04, 2016 at 01:38:51PM +0100, Johannes Schindelin wrote:\n> On Thu, 4 Feb 2016, John Keeping wrote:\n> \n> > Using spaces around operators also matches our C coding style.\n> \n> Well, by that reasoning you should go the whole nine yards and write\n> \n> \tlineno = $(( $lineno + 1 ))\n> \n> Except you can't. Because shell code is inherently not like C code.\n\nThat isn't my main argument, although I do think the (presumed)\nrationale for using spaces in C to improve usability applies here as\nwell, even if the confines of the language don't let us go as far as in\nC.\n\nI'm not actually sure whether spaces inside the enclosing $(( and )) are\nhelpful, we're much less consistent about that than about spaces around\nbinary operators. Having looked at t/ as well now, we really do seem to\nfavour using spaces around the binary operators so I'm further convinced\nthis series is switching the wrong cases.\n\n> What I found particularly interesting about 180bad3 (rebase -i: respect\n> core.commentchar, 2013-02-11) was that it *snuck in* that coding style: it\n> *changed* the existing one (without rationale in the commit message, too).\n\nThe last version of that patch I can find in the archive [1] doesn't add\nthe spaces, so I think they must have been part of Junio's fixup\ndiscusses in the following messages. Although I don't think the\nhistoric context is useful in deciding which direction to go in the\nfuture.\n\n[1] http://article.gmane.org/gmane.comp.version-control.git/216103\n"},{"id":"277429","messageId":"alpine.DEB.2.20.1602041411520.2964@virtualbox","threadId":"41331","inReplyTo":"20160204130111.GG29880@serenity.lan","subject":"Re: [PATCH 0/3] Fix $((...)) coding style","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-02-04T13:13:47Z","receivedAt":"2016-02-04T13:13:47Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi John,\n\nOn Thu, 4 Feb 2016, John Keeping wrote:\n\n> Although I don't think the historic context is useful in deciding which\n> direction to go in the future.\n\nBeing a maintainer, I find that argument particularly hard to defend.\n\nBut sure, you go ahead and prepare a patch series that turns everything\naround, adding spaces around those operators.\n\nWhatever the outcome, the inconsistency must be fixed. I just submitted my\nproposed solution.\n\nCiao,\nJohannes\n"},{"id":"277431","messageId":"20160204140609.GH29880@serenity.lan","threadId":"41331","inReplyTo":"alpine.DEB.2.20.1602041411520.2964@virtualbox","subject":"Re: [PATCH 0/3] Fix $((...)) coding style","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2016-02-04T14:06:09Z","receivedAt":"2016-02-04T14:06:09Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Thu, Feb 04, 2016 at 02:13:47PM +0100, Johannes Schindelin wrote:\n> On Thu, 4 Feb 2016, John Keeping wrote:\n> \n> > Although I don't think the historic context is useful in deciding which\n> > direction to go in the future.\n> \n> Being a maintainer, I find that argument particularly hard to defend.\n\nI worded that badly, what I wanted to say is that how we got here is\nless interesting than where we are. From a quick bit of grep'ing it\nlooks to me like where we are is in favour of adding spaces around\nbinary operators inside $(( )) constructs based on the majority of the\nuses in the code as it currently stands.\n\n> But sure, you go ahead and prepare a patch series that turns everything\n> around, adding spaces around those operators.\n> \n> Whatever the outcome, the inconsistency must be fixed.\n\nI disagree. Unless there are other changes in the same area, the noise\nisn't worth it.\n\nHowever, I do think we need to agree on a policy so that new code can be\nconsistent. This should then be documented in CodingGuidelines.\n"},{"id":"277432","messageId":"alpine.DEB.2.20.1602041619430.2964@virtualbox","threadId":"41331","inReplyTo":"20160204140609.GH29880@serenity.lan","subject":"Re: [PATCH 0/3] Fix $((...)) coding style","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-02-04T15:27:49Z","receivedAt":"2016-02-04T15:27:49Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi John,\n\nOn Thu, 4 Feb 2016, John Keeping wrote:\n\n> On Thu, Feb 04, 2016 at 02:13:47PM +0100, Johannes Schindelin wrote:\n> > Whatever the outcome, the inconsistency must be fixed.\n> \n> I disagree. Unless there are other changes in the same area, the noise\n> isn't worth it.\n> \n> However, I do think we need to agree on a policy so that new code can be\n> consistent. This should then be documented in CodingGuidelines.\n\nIf you want to enforce it in the future, how can you possibly be against\nfixing the inconsistency in the existing code? Sorry, I am really unable\nto understand this.\n\nAlso, we *did* document the policy in the CodingGuidelines:\n\n\tAs for more concrete guidelines, just imitate the existing code\n\nSo. There you are. By that token, my patch series was the correct thing to\ndo because the first arithmetic expression introduced into a shell script\nin Git's source code had no spaces.\n\nAt this point I am a bit fed up with the discussion because by now it\nfeels to me more like a Slashdot thread than a technical discussion with a\ndirection.\n\nAs far as technical direction goes, here is the summary of what I have to\nsay: I still think that the changes I submitted are good because they seem\nto me to reinstate the coding style with which we started out.\n\nAnd that's really all I have to say about this matter.\n\nCiao,\nJohannes\n"},{"id":"277434","messageId":"20160204155316.GI29880@serenity.lan","threadId":"41331","inReplyTo":"alpine.DEB.2.20.1602041619430.2964@virtualbox","subject":"Re: [PATCH 0/3] Fix $((...)) coding style","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2016-02-04T15:53:16Z","receivedAt":"2016-02-04T15:53:16Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Thu, Feb 04, 2016 at 04:27:49PM +0100, Johannes Schindelin wrote:\n> On Thu, 4 Feb 2016, John Keeping wrote:\n> \n> > On Thu, Feb 04, 2016 at 02:13:47PM +0100, Johannes Schindelin wrote:\n> > > Whatever the outcome, the inconsistency must be fixed.\n> > \n> > I disagree. Unless there are other changes in the same area, the noise\n> > isn't worth it.\n> > \n> > However, I do think we need to agree on a policy so that new code can be\n> > consistent. This should then be documented in CodingGuidelines.\n> \n> If you want to enforce it in the future, how can you possibly be against\n> fixing the inconsistency in the existing code? Sorry, I am really unable\n> to understand this.\n\nI avoided quoting CodingGuidelines in my previous message, but it says:\n\n - Fixing style violations while working on a real change as a\n preparatory clean-up step is good, but otherwise avoid useless code\n churn for the sake of conforming to the style.\n\n \"Once it _is_ in the tree, it's not really worth the patch noise to\n go and fix it up.\"\n Cf. http://article.gmane.org/gmane.linux.kernel/943020\n\n> Also, we *did* document the policy in the CodingGuidelines:\n> \n> \tAs for more concrete guidelines, just imitate the existing code\n> \n> So. There you are. By that token, my patch series was the correct thing to\n> do because the first arithmetic expression introduced into a shell script\n> in Git's source code had no spaces.\n\nThis is the first point I made. To take one example, in\ngit-filter-branch.sh there are five occurrences of the sequence \"$((\";\nyour patch changes four of them to remove spaces. If we standardise on\nhaving spaces only one needs to change:\n\n$ git grep -F '$((' origin/master -- git-filter-branch.sh\norigin/master:git-filter-branch.sh: elapsed=$(($now - $start_timestamp))\norigin/master:git-filter-branch.sh: remaining=$(( ($commits - $count) * $elapsed / $count ))\norigin/master:git-filter-branch.sh: next_sample_at=$(( ($elapsed + 1) * $count / $elapsed ))\norigin/master:git-filter-branch.sh: next_sample_at=$(($next_sample_at + 1))\norigin/master:git-filter-branch.sh: git_filter_branch__commit_count=$(($git_filter_branch__commit_count+1))\n\nI chose git-filter-branch.sh simply because it was touched by this patch\nset but it is not an outlier in this regard. Some crude statistics\nacross all of git.git:\n\n# No space after plus\n$ git grep -F '$((' | grep '\\+[\\$0-9]' | wc -l\n34\n\n# With space after plus\n$ git grep -F '$((' | grep '\\+ [\\$0-9]' | wc -l\n96\n"},{"id":"277451","messageId":"xmqq1t8si7my.fsf@gitster.mtv.corp.google.com","threadId":"41331","inReplyTo":"20160204155316.GI29880@serenity.lan","subject":"Re: [PATCH 0/3] Fix $((...)) coding style","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-04T19:43:17Z","receivedAt":"2016-02-04T19:43:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"John Keeping writes:\n\n> I avoided quoting CodingGuidelines in my previous message, but it says:\n>\n> - Fixing style violations while working on a real change as a\n> preparatory clean-up step is good, but otherwise avoid useless code\n> churn for the sake of conforming to the style.\n>\n> \"Once it _is_ in the tree, it's not really worth the patch noise to\n> go and fix it up.\"\n> Cf. http://article.gmane.org/gmane.linux.kernel/943020\n>\n>> Also, we *did* document the policy in the CodingGuidelines:\n>> \n>> \tAs for more concrete guidelines, just imitate the existing code\n>\n> This is the first point I made. To take one example, in\n> git-filter-branch.sh there are five occurrences of the sequence \"$((\";\n> your patch changes four of them to remove spaces. If we standardise on\n> having spaces only one needs to change:\n\nI agree that our codebase seems to have a moderately strong\npreference for having SP around binary operators, i.e.\n\n\t$term1 * $term2 + $term3\n\nover not having one:\n\n\t$term1*$term2+$term3\n\nand I think that is OK to declare it as our style, primarily because\nthat is how we encourage to write our expressions in C, as you\nmentioned earlier.\n\nOne thing I was wondering about the $(( ... )) syntax while reading\nthis thread was about the SP around the expression, i.e.\n\n\tvar=$(( $term1 * $term2 + $term3 ))\n\nvs\n\n\tvar=$(($term1 * $term2 + $term3))\n\nI personally do not have strong preference between the two, but I\nhave a vague impression that we preferred the former because\nsomebody in the past gave us a good explanation why we should.\n\n\"git grep\" however seems to tell us that we are not clearly decided\nbetween the two, so I probably am misremembering it and there is no\npreference either way.\n\nIn any case, it is good to catch a patch that mixes style changes\nand other things during a review, and it also is good to clean up\nthe style before you start working on a specific part of the code\nto make gradual improvement without stepping on others' toes.\n\nThanks.\n"},{"id":"277452","messageId":"xmqqtwlogt1d.fsf@gitster.mtv.corp.google.com","threadId":"41331","inReplyTo":"20160204155316.GI29880@serenity.lan","subject":"Re: [PATCH 0/3] Fix $((...)) coding style","fromName":"Junio C Hamano","fromEmail":"junio@pobox.com","sentAt":"2016-02-04T19:43:58Z","receivedAt":"2016-02-04T19:43:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> One thing I was wondering about the $(( ... )) syntax while reading\n> this thread was about the SP around the expression, i.e.\n>\n> \tvar=$(( $term1 * $term2 + $term3 ))\n>\n> vs\n>\n> \tvar=$(($term1 * $term2 + $term3))\n>\n> I personally do not have strong preference between the two, but I\n> have a vague impression that we preferred the former because\n> somebody in the past gave us a good explanation why we should.\n>\n> \"git grep\" however seems to tell us that we are not clearly decided\n> between the two, so I probably am misremembering it and there is no\n> preference either way.\n\nOne thing that is somewhat related is that we would want to avoid\nwriting things like this by mistake:\n\n\tvar=$((cmd1 && cmd2) | cmd3)\n\nwhich is not meant to be an arithmetic expansion, but is a command\nsubstitution that happens to have a subshell at the head of the\npipeline. I think bash gets this right, but some shells (e.g. dash)\ntries to parse this as an arithmetic expansion upon seeing \"$((\" and\nfails to parse it, waiting forever until it sees the matching \"))\".\nSo we write it like this to make it safer:\n\n\tvar=$( (cmd1 && cmd2) | cmd3 )\n\nPerhaps having a SP after $(( of a real arithmetic expression, i.e.\n\n\tvar=$(( ... anything ... ))\n\nmakes it easier to tell these two apart? I dunno.\n"}]}