Re: [arch-projects] [dbscripts] [PATCH 1/3] test: common.bash:__getCheckSum: Don't rely on IFS

2018-02-22 Thread Luke Shumaker
On Thu, 22 Feb 2018 19:27:04 -0500,
Eli Schwartz wrote:
> 
> [1 Re: [arch-projects] [dbscripts] [PATCH 1/3] test: 
> common.bash:__getCheckSum: Don't rely on IFS ]
> [1.1  ]
> On 02/22/2018 06:43 PM, Luke Shumaker wrote:
> > On Thu, 22 Feb 2018 16:43:36 -0500,
> > Eli Schwartz wrote:
> >>>  __getCheckSum() {
> >>> - local result=($(sha1sum $1))
> >>> - echo ${result[0]}
> >>> + local result
> >>> + result="$(sha1sum "$1")"
> >>> + echo "${result%% *}"
> >>
> >> Why are you moving over to declaring the variable and assigning it on
> >> different lines?
> > 
> > Because shellcheck complains about it, so it's a habit I've gotten in
> > to :) Even in cases where it doesn't really make a difference.
> > 
> > https://github.com/koalaman/shellcheck/wiki/SC2155
> > 
> > However, BATS does run the test suite with `set -e`, so splitting it
> > does mean that BATS will now detect errors from sha1sum.  We don't
> > really expect that to happen, but if BATS will give us error checking
> > on it for free, why not?
> 
> Then the commit message should say so...

Honestly, I didn't even think about it.  Like I said, I've just made
it a habit.

-- 
Happy hacking,
~ Luke Shumaker


Re: [arch-projects] [dbscripts] [PATCH 1/3] test: common.bash:__getCheckSum: Don't rely on IFS

2018-02-22 Thread Eli Schwartz via arch-projects
On 02/22/2018 06:43 PM, Luke Shumaker wrote:
> On Thu, 22 Feb 2018 16:43:36 -0500,
> Eli Schwartz wrote:
>>>  __getCheckSum() {
>>> -   local result=($(sha1sum $1))
>>> -   echo ${result[0]}
>>> +   local result
>>> +   result="$(sha1sum "$1")"
>>> +   echo "${result%% *}"
>>
>> Why are you moving over to declaring the variable and assigning it on
>> different lines?
> 
> Because shellcheck complains about it, so it's a habit I've gotten in
> to :) Even in cases where it doesn't really make a difference.
> 
> https://github.com/koalaman/shellcheck/wiki/SC2155
> 
> However, BATS does run the test suite with `set -e`, so splitting it
> does mean that BATS will now detect errors from sha1sum.  We don't
> really expect that to happen, but if BATS will give us error checking
> on it for free, why not?

Then the commit message should say so...

-- 
Eli Schwartz
Bug Wrangler and Trusted User



signature.asc
Description: OpenPGP digital signature


Re: [arch-projects] [dbscripts] [PATCH 1/3] test: common.bash:__getCheckSum: Don't rely on IFS

2018-02-22 Thread Luke Shumaker
On Thu, 22 Feb 2018 16:43:36 -0500,
Eli Schwartz wrote:
> >  __getCheckSum() {
> > -   local result=($(sha1sum $1))
> > -   echo ${result[0]}
> > +   local result
> > +   result="$(sha1sum "$1")"
> > +   echo "${result%% *}"
> 
> Why are you moving over to declaring the variable and assigning it on
> different lines?

Because shellcheck complains about it, so it's a habit I've gotten in
to :) Even in cases where it doesn't really make a difference.

https://github.com/koalaman/shellcheck/wiki/SC2155

However, BATS does run the test suite with `set -e`, so splitting it
does mean that BATS will now detect errors from sha1sum.  We don't
really expect that to happen, but if BATS will give us error checking
on it for free, why not?

-- 
Happy hacking,
~ Luke Shumaker


Re: [arch-projects] [dbscripts] [PATCH 1/3] test: common.bash:__getCheckSum: Don't rely on IFS

2018-02-22 Thread Eli Schwartz via arch-projects
On 02/22/2018 03:43 PM, Luke Shumaker wrote:
> From: Luke Shumaker 
> 
> I managed to stumble across a bug in BATS where the run() function
> screwed with the global IFS.  The bug has been fixed in git, but isn't
> in a release yet.
> 
> https://github.com/sstephenson/bats/issues/89
> 
> Anyway, this bug breaks __getCheckSum().  Fortunately, we have avoided
> tripping it so far because luck has it that we never call
> __getCheckSum() after run() in the same test.
> 
> So, there's nothing actually broken here, but it makes me nervous.  So
> go ahead and modify __getCheckSum to not rely on IFS.
> ---
>  test/lib/common.bash | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/test/lib/common.bash b/test/lib/common.bash
> index 568a541..5411641 100644
> --- a/test/lib/common.bash
> +++ b/test/lib/common.bash
> @@ -9,8 +9,9 @@ __updatePKGBUILD() {
>  }
>  
>  __getCheckSum() {
> - local result=($(sha1sum $1))
> - echo ${result[0]}
> + local result
> + result="$(sha1sum "$1")"
> + echo "${result%% *}"

Why are you moving over to declaring the variable and assigning it on
different lines?

-- 
Eli Schwartz
Bug Wrangler and Trusted User



signature.asc
Description: OpenPGP digital signature


[arch-projects] [dbscripts] [PATCH 1/3] test: common.bash:__getCheckSum: Don't rely on IFS

2018-02-22 Thread Luke Shumaker
From: Luke Shumaker 

I managed to stumble across a bug in BATS where the run() function
screwed with the global IFS.  The bug has been fixed in git, but isn't
in a release yet.

https://github.com/sstephenson/bats/issues/89

Anyway, this bug breaks __getCheckSum().  Fortunately, we have avoided
tripping it so far because luck has it that we never call
__getCheckSum() after run() in the same test.

So, there's nothing actually broken here, but it makes me nervous.  So
go ahead and modify __getCheckSum to not rely on IFS.
---
 test/lib/common.bash | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/test/lib/common.bash b/test/lib/common.bash
index 568a541..5411641 100644
--- a/test/lib/common.bash
+++ b/test/lib/common.bash
@@ -9,8 +9,9 @@ __updatePKGBUILD() {
 }
 
 __getCheckSum() {
-   local result=($(sha1sum $1))
-   echo ${result[0]}
+   local result
+   result="$(sha1sum "$1")"
+   echo "${result%% *}"
 }
 
 __buildPackage() {
-- 
2.16.1