| Message ID | 20180222204331.10802-2-lukeshu@lukeshu.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Misc touchup | expand |
On 02/22/2018 03:43 PM, Luke Shumaker wrote: > From: Luke Shumaker <lukeshu@parabola.nu> > > 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?
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?
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...
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 <multipart/mixed (7bit)>] > [1.1 <text/plain; utf-8 (quoted-printable)>] > 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.
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() {
From: Luke Shumaker <lukeshu@parabola.nu> 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(-)