Re: [PATCH 14/15] git-am.sh: replace using expr for arithmetic operations with the equivalent shell builtin

2016-02-05 Thread Elia Pinto 
2016-02-04 20:33 GMT+01:00 Junio C Hamano :
> As pointed out already, quoting of "$this" inside the arithmetic
> expansion would not work very well, so [14/15] needs fixing.
>
> I do not see 01/15 thru 13/15 here, by the way.  Is it just me?

Excuse me, everyone. Yesterday was a bad day. I did a bit of confusion :=(.
But this is not a patch series, yes. The patch is for git-am in
contrib: so it is not important.

Thank you
>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 14/15] git-am.sh: replace using expr for arithmetic operations with the equivalent shell builtin

2016-02-04 Thread Johannes Schindelin 
Hi Elia,

On Thu, 4 Feb 2016, Elia Pinto wrote:

> - this=$(expr "$this" + 1)
> + this=$(( "$this" + 1 ))

Why the funny spaces? We do not do that anywhere in the existing code
except in three places (2x filter-branch, 1x rebase--interactive, all
three *not* my fault) and in some tests.

Also, I am *pretty* certain that the quotes break this code:

me@work MINGW64 /usr/src/git (master)
$ this=1

me@work MINGW64 /usr/src/git (master)
$ this=$(( "$this" + 1 ))
bash: "1" + 1 : syntax error: operand expected (error token is ""1" + 1 
")

Whereas if you do *not* add the superfluous spaces and quotes, it works:

me@work MINGW64 /usr/src/git (master)
$ this=1

me@work MINGW64 /usr/src/git (master)
$ this=$(($this+1))

me@work MINGW64 /usr/src/git (master)
$ echo $this
2

Maybe this is only a problem with Bash 4.3.42 in MSYS2, but I do not think
so.

*Clicketyclick*

Nope. It also happens in Ubuntu's Bash 4.3.42:

me@ubuntu-vm  ~/git (master)
$ this=1

me@ubuntu-vm  ~/git (master)
$ this=$(( "$this" + 1 ))
bash: "1" + 1 : syntax error: operand expected (error token is ""1" + 1 
")

me@ubuntu-vm  ~/git (master)
$ bash --version
GNU bash, version 4.3.42(1)-release (x86_64-pc-linux-gnu)
Copyright (C) 2013 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later


This is free software; you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.

... which makes me wonder in which environment you tested this?

Ciao,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 14/15] git-am.sh: replace using expr for arithmetic operations with the equivalent shell builtin

2016-02-04 Thread Elia Pinto 
2016-02-04 12:14 GMT+01:00 Johannes Schindelin :
> Hi Elia,
>
> On Thu, 4 Feb 2016, Elia Pinto wrote:
>
>> - this=$(expr "$this" + 1)
>> + this=$(( "$this" + 1 ))
>
> Why the funny spaces? We do not do that anywhere in the existing code
> except in three places (2x filter-branch, 1x rebase--interactive, all
> three *not* my fault) and in some tests.
>
> Also, I am *pretty* certain that the quotes break this code:
>
> me@work MINGW64 /usr/src/git (master)
> $ this=1
>
> me@work MINGW64 /usr/src/git (master)
> $ this=$(( "$this" + 1 ))
> bash: "1" + 1 : syntax error: operand expected (error token is ""1" + 
> 1 ")
>
> Whereas if you do *not* add the superfluous spaces and quotes, it works:
>
> me@work MINGW64 /usr/src/git (master)
> $ this=1
>
> me@work MINGW64 /usr/src/git (master)
Thanks for noticing.

You are right. I ran the test but did not notice mistakes, my fault.

I will resend. Thanks again.

Best

> $ this=$(($this+1))
>
> me@work MINGW64 /usr/src/git (master)
> $ echo $this
> 2
>
> Maybe this is only a problem with Bash 4.3.42 in MSYS2, but I do not think
> so.
>
> *Clicketyclick*
>
> Nope. It also happens in Ubuntu's Bash 4.3.42:
>
> me@ubuntu-vm  ~/git (master)
> $ this=1
>
> me@ubuntu-vm  ~/git (master)
> $ this=$(( "$this" + 1 ))
> bash: "1" + 1 : syntax error: operand expected (error token is ""1" + 
> 1 ")
>
> me@ubuntu-vm  ~/git (master)
> $ bash --version
> GNU bash, version 4.3.42(1)-release (x86_64-pc-linux-gnu)
> Copyright (C) 2013 Free Software Foundation, Inc.
> License GPLv3+: GNU GPL version 3 or later
> 
>
> This is free software; you are free to change and redistribute it.
> There is NO WARRANTY, to the extent permitted by law.
>
> ... which makes me wonder in which environment you tested this?
>
> Ciao,
> Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 14/15] git-am.sh: replace using expr for arithmetic operations with the equivalent shell builtin

2016-02-04 Thread Johannes Schindelin 
Hi Elia,

On Thu, 4 Feb 2016, Elia Pinto wrote:

> 2016-02-04 12:14 GMT+01:00 Johannes Schindelin :
> > Hi Elia,
> >
> > On Thu, 4 Feb 2016, Elia Pinto wrote:
> >
> >> - this=$(expr "$this" + 1)
> >> + this=$(( "$this" + 1 ))
> >
> > Why the funny spaces? We do not do that anywhere in the existing code
> > except in three places (2x filter-branch, 1x rebase--interactive, all
> > three *not* my fault) and in some tests.

I actually noticed more places now, and will send a quick fixer-upper
patch series in a few seconds, that we can hopefully apply before your
series?

Thanks,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 14/15] git-am.sh: replace using expr for arithmetic operations with the equivalent shell builtin

2016-02-04 Thread Elia Pinto 
expr is considered generally antiquated. It is best to use for arithmetic 
operations
the shell $((..)).

To quote POSIX:

"The expr utility has a rather difficult syntax [...] In many cases, the 
arithmetic
and string features provided as part of the shell command language are easier 
to use
than their equivalents in expr. Newly written scripts should avoid expr in 
favor of
the new features within the shell."

Signed-off-by: Elia Pinto 
---
 git-am.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/git-am.sh b/git-am.sh
index ee61a77..4f8148e 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -298,7 +298,7 @@ split_patches () {
this=0
for stgit in "$@"
do
-   this=$(expr "$this" + 1)
+   this=$(( "$this" + 1 ))
msgnum=$(printf "%0${prec}d" $this)
# Perl version of StGIT parse_patch. The first 
nonemptyline
# not starting with Author, From or Date is the
@@ -656,14 +656,14 @@ last=$(cat "$dotest/last")
 this=$(cat "$dotest/next")
 if test "$skip" = t
 then
-   this=$(expr "$this" + 1)
+   this=$(( "$this" + 1 ))
resume=
 fi
 
 while test "$this" -le "$last"
 do
msgnum=$(printf "%0${prec}d" $this)
-   next=$(expr "$this" + 1)
+   next=$(( "$this" + 1 ))
test -f "$dotest/$msgnum" || {
resume=
go_next
-- 
2.5.0

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 14/15] git-am.sh: replace using expr for arithmetic operations with the equivalent shell builtin

2016-02-04 Thread Junio C Hamano 
As pointed out already, quoting of "$this" inside the arithmetic
expansion would not work very well, so [14/15] needs fixing.

I do not see 01/15 thru 13/15 here, by the way.  Is it just me?

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 14/15] git-am.sh: replace using expr for arithmetic operations with the equivalent shell builtin

2016-02-04 Thread Eric Sunshine 
On Thu, Feb 4, 2016 at 2:33 PM, Junio C Hamano  wrote:
> As pointed out already, quoting of "$this" inside the arithmetic
> expansion would not work very well, so [14/15] needs fixing.
>
> I do not see 01/15 thru 13/15 here, by the way.  Is it just me?

I didn't receive them either, and they don't seem to be on gmane...
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 14/15] git-am.sh: replace using expr for arithmetic operations with the equivalent shell builtin

2016-02-04 Thread Stefan Beller 
On which version did you base your patches?

git-am.sh is no more since 2015-08-04,
(it was moved to contrib/examples/git-am.sh as part of the rewrite of am in C)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html