{"thread":{"id":"41328","subject":"[PATCH 14/15] git-am.sh: replace using expr for arithmetic operations with the equivalent shell builtin","startedAt":"2016-02-04T10:20:58Z","lastAt":"2016-02-05T12:50:45Z","messageCount":9,"participants":["Elia Pinto","Johannes Schindelin","Junio C Hamano","Eric Sunshine","Stefan Beller"],"isPatch":true,"patchVersion":1,"patchTotal":15},"messages":[{"id":"277415","messageId":"1454581259-57095-1-git-send-email-gitter.spiros@gmail.com","threadId":"41328","inReplyTo":null,"subject":"[PATCH 14/15] git-am.sh: replace using expr for arithmetic operations with the equivalent shell builtin","fromName":"Elia Pinto","fromEmail":"gitter.spiros@gmail.com","sentAt":"2016-02-04T10:20:58Z","receivedAt":"2016-02-04T10:20:58Z","isPatch":true,"sender":{"key":"gitter.spiros@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158490?v=4"},"body":"expr is considered generally antiquated. It is best to use for arithmetic operations\nthe shell $((..)).\n\nTo quote POSIX:\n\n\"The expr utility has a rather difficult syntax [...] In many cases, the arithmetic\nand string features provided as part of the shell command language are easier to use\nthan their equivalents in expr. Newly written scripts should avoid expr in favor of\nthe new features within the shell.\"\n\nSigned-off-by: Elia Pinto \n---\n git-am.sh | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/git-am.sh b/git-am.sh\nindex ee61a77..4f8148e 100755\n--- a/git-am.sh\n+++ b/git-am.sh\n@@ -298,7 +298,7 @@ split_patches () {\n \t\tthis=0\n \t\tfor stgit in \"$@\"\n \t\tdo\n-\t\t\tthis=$(expr \"$this\" + 1)\n+\t\t\tthis=$(( \"$this\" + 1 ))\n \t\t\tmsgnum=$(printf \"%0${prec}d\" $this)\n \t\t\t# Perl version of StGIT parse_patch. The first nonemptyline\n \t\t\t# not starting with Author, From or Date is the\n@@ -656,14 +656,14 @@ last=$(cat \"$dotest/last\")\n this=$(cat \"$dotest/next\")\n if test \"$skip\" = t\n then\n-\tthis=$(expr \"$this\" + 1)\n+\tthis=$(( \"$this\" + 1 ))\n \tresume=\n fi\n \n while test \"$this\" -le \"$last\"\n do\n \tmsgnum=$(printf \"%0${prec}d\" $this)\n-\tnext=$(expr \"$this\" + 1)\n+\tnext=$(( \"$this\" + 1 ))\n \ttest -f \"$dotest/$msgnum\" || {\n \t\tresume=\n \t\tgo_next\n-- \n2.5.0\n"},{"id":"277416","messageId":"1454581259-57095-2-git-send-email-gitter.spiros@gmail.com","threadId":"41328","inReplyTo":"1454581259-57095-1-git-send-email-gitter.spiros@gmail.com","subject":"[PATCH 15/15] t4032-diff-inter-hunk-context.sh: replace using expr for arithmetic operations with the equivalent shell builtin","fromName":"Elia Pinto","fromEmail":"gitter.spiros@gmail.com","sentAt":"2016-02-04T10:20:59Z","receivedAt":"2016-02-04T10:20:59Z","isPatch":true,"sender":{"key":"gitter.spiros@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158490?v=4"},"body":"expr is considered generally antiquated. It is best to use for arithmetic operations\nthe shell $((..)).\n\nTo quote POSIX:\n\n \"The expr utility has a rather difficult syntax [...] In many cases, the arithmetic\n and string features provided as part of the shell command language are easier to use\n than their equivalents in expr. Newly written scripts should avoid expr in favor of\n the new features within the shell.\"\n\nSigned-off-by: Elia Pinto \n---\n t/t4032-diff-inter-hunk-context.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t4032-diff-inter-hunk-context.sh b/t/t4032-diff-inter-hunk-context.sh\nindex e4e3e28..0b0b60f 100755\n--- a/t/t4032-diff-inter-hunk-context.sh\n+++ b/t/t4032-diff-inter-hunk-context.sh\n@@ -10,7 +10,7 @@ f() {\n \twhile test $i -le $2\n \tdo\n \t\techo $i\n-\t\ti=$(expr $i + 1)\n+\t\ti=$(( $i + 1 ))\n \tdone\n \techo $3\n }\n-- \n2.5.0\n"},{"id":"282596","messageId":"alpine.DEB.2.20.1602041204430.2964@virtualbox","threadId":"41328","inReplyTo":"1454581259-57095-1-git-send-email-gitter.spiros@gmail.com","subject":"Re: [PATCH 14/15] git-am.sh: replace using expr for arithmetic operations with the equivalent shell builtin","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-02-04T11:14:27Z","receivedAt":"2016-02-04T11:14:27Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Elia,\n\nOn Thu, 4 Feb 2016, Elia Pinto wrote:\n\n> -\t\t\tthis=$(expr \"$this\" + 1)\n> +\t\t\tthis=$(( \"$this\" + 1 ))\n\nWhy the funny spaces? We do not do that anywhere in the existing code\nexcept in three places (2x filter-branch, 1x rebase--interactive, all\nthree *not* my fault) and in some tests.\n\nAlso, I am *pretty* certain that the quotes break this code:\n\n\tme@work MINGW64 /usr/src/git (master)\n\t$ this=1\n\n\tme@work MINGW64 /usr/src/git (master)\n\t$ this=$(( \"$this\" + 1 ))\n\tbash: \"1\" + 1 : syntax error: operand expected (error token is \"\"1\" + 1 \")\n\nWhereas if you do *not* add the superfluous spaces and quotes, it works:\n\n\tme@work MINGW64 /usr/src/git (master)\n\t$ this=1\n\n\tme@work MINGW64 /usr/src/git (master)\n\t$ this=$(($this+1))\n\n\tme@work MINGW64 /usr/src/git (master)\n\t$ echo $this\n\t2\n\nMaybe this is only a problem with Bash 4.3.42 in MSYS2, but I do not think\nso.\n\n*Clicketyclick*\n\nNope. It also happens in Ubuntu's Bash 4.3.42:\n\n\tme@ubuntu-vm ~/git (master)\n\t$ this=1\n\n\tme@ubuntu-vm ~/git (master)\n\t$ this=$(( \"$this\" + 1 ))\n\tbash: \"1\" + 1 : syntax error: operand expected (error token is \"\"1\" + 1 \")\n\n\tme@ubuntu-vm ~/git (master)\n\t$ bash --version\n\tGNU bash, version 4.3.42(1)-release (x86_64-pc-linux-gnu)\n\tCopyright (C) 2013 Free Software Foundation, Inc.\n\tLicense GPLv3+: GNU GPL version 3 or later\n\t\n\n\tThis is free software; you are free to change and redistribute it.\n\tThere is NO WARRANTY, to the extent permitted by law.\n\n... which makes me wonder in which environment you tested this?\n\nCiao,\nDscho\n"},{"id":"277421","messageId":"CA+EOSB=ky8E7J0Fa26DjgT=5-X9N09=4kRbGvWZo5K7PhwS7hg@mail.gmail.com","threadId":"41328","inReplyTo":"alpine.DEB.2.20.1602041204430.2964@virtualbox","subject":"Re: [PATCH 14/15] git-am.sh: replace using expr for arithmetic operations with the equivalent shell builtin","fromName":"Elia Pinto","fromEmail":"gitter.spiros@gmail.com","sentAt":"2016-02-04T11:53:27Z","receivedAt":"2016-02-04T11:53:27Z","isPatch":true,"sender":{"key":"gitter.spiros@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158490?v=4"},"body":"2016-02-04 12:14 GMT+01:00 Johannes Schindelin :\n> Hi Elia,\n>\n> On Thu, 4 Feb 2016, Elia Pinto wrote:\n>\n>> - this=$(expr \"$this\" + 1)\n>> + this=$(( \"$this\" + 1 ))\n>\n> Why the funny spaces? We do not do that anywhere in the existing code\n> except in three places (2x filter-branch, 1x rebase--interactive, all\n> three *not* my fault) and in some tests.\n>\n> Also, I am *pretty* certain that the quotes break this code:\n>\n> me@work MINGW64 /usr/src/git (master)\n> $ this=1\n>\n> me@work MINGW64 /usr/src/git (master)\n> $ this=$(( \"$this\" + 1 ))\n> bash: \"1\" + 1 : syntax error: operand expected (error token is \"\"1\" + 1 \")\n>\n> Whereas if you do *not* add the superfluous spaces and quotes, it works:\n>\n> me@work MINGW64 /usr/src/git (master)\n> $ this=1\n>\n> me@work MINGW64 /usr/src/git (master)\nThanks for noticing.\n\nYou are right. I ran the test but did not notice mistakes, my fault.\n\nI will resend. Thanks again.\n\nBest\n\n> $ this=$(($this+1))\n>\n> me@work MINGW64 /usr/src/git (master)\n> $ echo $this\n> 2\n>\n> Maybe this is only a problem with Bash 4.3.42 in MSYS2, but I do not think\n> so.\n>\n> *Clicketyclick*\n>\n> Nope. It also happens in Ubuntu's Bash 4.3.42:\n>\n> me@ubuntu-vm ~/git (master)\n> $ this=1\n>\n> me@ubuntu-vm ~/git (master)\n> $ this=$(( \"$this\" + 1 ))\n> bash: \"1\" + 1 : syntax error: operand expected (error token is \"\"1\" + 1 \")\n>\n> me@ubuntu-vm ~/git (master)\n> $ bash --version\n> GNU bash, version 4.3.42(1)-release (x86_64-pc-linux-gnu)\n> Copyright (C) 2013 Free Software Foundation, Inc.\n> License GPLv3+: GNU GPL version 3 or later\n> \n>\n> This is free software; you are free to change and redistribute it.\n> There is NO WARRANTY, to the extent permitted by law.\n>\n> ... which makes me wonder in which environment you tested this?\n>\n> Ciao,\n> Dscho\n"},{"id":"277422","messageId":"alpine.DEB.2.20.1602041258400.2964@virtualbox","threadId":"41328","inReplyTo":"CA+EOSB=ky8E7J0Fa26DjgT=5-X9N09=4kRbGvWZo5K7PhwS7hg@mail.gmail.com","subject":"Re: [PATCH 14/15] git-am.sh: replace using expr for arithmetic operations with the equivalent shell builtin","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-02-04T12:01:21Z","receivedAt":"2016-02-04T12:01:21Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Elia,\n\nOn Thu, 4 Feb 2016, Elia Pinto wrote:\n\n> 2016-02-04 12:14 GMT+01:00 Johannes Schindelin :\n> > Hi Elia,\n> >\n> > On Thu, 4 Feb 2016, Elia Pinto wrote:\n> >\n> >> - this=$(expr \"$this\" + 1)\n> >> + this=$(( \"$this\" + 1 ))\n> >\n> > Why the funny spaces? We do not do that anywhere in the existing code\n> > except in three places (2x filter-branch, 1x rebase--interactive, all\n> > three *not* my fault) and in some tests.\n\nI actually noticed more places now, and will send a quick fixer-upper\npatch series in a few seconds, that we can hopefully apply before your\nseries?\n\nThanks,\nDscho\n"},{"id":"277449","messageId":"xmqqoabwi83q.fsf@gitster.mtv.corp.google.com","threadId":"41328","inReplyTo":"1454581259-57095-1-git-send-email-gitter.spiros@gmail.com","subject":"Re: [PATCH 14/15] git-am.sh: replace using expr for arithmetic operations with the equivalent shell builtin","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-04T19:33:13Z","receivedAt":"2016-02-04T19:33:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"As pointed out already, quoting of \"$this\" inside the arithmetic\nexpansion would not work very well, so [14/15] needs fixing.\n\nI do not see 01/15 thru 13/15 here, by the way. Is it just me?\n"},{"id":"277450","messageId":"CAPig+cRuNzQR_-dRQqV+KOqEaXhuTNB2RxX6rkXKzembbgWyKw@mail.gmail.com","threadId":"41328","inReplyTo":"xmqqoabwi83q.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 14/15] git-am.sh: replace using expr for arithmetic operations with the equivalent shell builtin","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-02-04T19:39:49Z","receivedAt":"2016-02-04T19:39:49Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Feb 4, 2016 at 2:33 PM, Junio C Hamano wrote:\n> As pointed out already, quoting of \"$this\" inside the arithmetic\n> expansion would not work very well, so [14/15] needs fixing.\n>\n> I do not see 01/15 thru 13/15 here, by the way. Is it just me?\n\nI didn't receive them either, and they don't seem to be on gmane...\n"},{"id":"277453","messageId":"CAGZ79kYYYwkX2NWN6SsW-CWWxOd-C87VqfzSFAhFfgmMf4795w@mail.gmail.com","threadId":"41328","inReplyTo":"1454581259-57095-1-git-send-email-gitter.spiros@gmail.com","subject":"Re: [PATCH 14/15] git-am.sh: replace using expr for arithmetic operations with the equivalent shell builtin","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-02-04T19:46:44Z","receivedAt":"2016-02-04T19:46:44Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On which version did you base your patches?\n\ngit-am.sh is no more since 2015-08-04,\n(it was moved to contrib/examples/git-am.sh as part of the rewrite of am in C)\n"},{"id":"277538","messageId":"CA+EOSBk=hPs6oLeDT6w-qgwogOq8eSt6Qzs_M3Uwjs87popMpQ@mail.gmail.com","threadId":"41328","inReplyTo":"xmqqoabwi83q.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 14/15] git-am.sh: replace using expr for arithmetic operations with the equivalent shell builtin","fromName":"Elia Pinto","fromEmail":"gitter.spiros@gmail.com","sentAt":"2016-02-05T12:50:45Z","receivedAt":"2016-02-05T12:50:45Z","isPatch":true,"sender":{"key":"gitter.spiros@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158490?v=4"},"body":"2016-02-04 20:33 GMT+01:00 Junio C Hamano :\n> As pointed out already, quoting of \"$this\" inside the arithmetic\n> expansion would not work very well, so [14/15] needs fixing.\n>\n> I do not see 01/15 thru 13/15 here, by the way. Is it just me?\n\nExcuse me, everyone. Yesterday was a bad day. I did a bit of confusion :=(.\nBut this is not a patch series, yes. The patch is for git-am in\ncontrib: so it is not important.\n\nThank you\n>\n"}]}