[PATCH libgpiod v2 0/4] tools: tests: fix a few issues in bash scripts

[PATCH libgpiod v2 0/4] tools: tests: fix a few issues in bash scripts

[Bartosz Golaszewski]

Fix a few issues with tools tests reported by Andy.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
Changes in v2:
- Use double quotes around $@ to prevent globbing but allow variable expansion
- Link to v1: https://lore.kernel.org/r/20240524-fix-bash-tests-v1-0-1397c73073a6@linaro.org

---
Bartosz Golaszewski (4):
      tools: tests: use tabs for indentation consistently
      tools: tests: use "$@" instead of $*
      tools: tests: remove unneeded ';' in while loops
      tools: tests: remove dependency on grep

 tools/gpio-tools-test.bash | 33 ++++++++++++++++-----------------
 1 file changed, 16 insertions(+), 17 deletions(-)
---
base-commit: 27fe10150f6d5fa78d1a1ef1e922dc8395d1154d
change-id: 20240524-fix-bash-tests-545bf26447b3

Best regards,
-- 
Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

[PATCH libgpiod v2 1/4] tools: tests: use tabs for indentation consistently

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Replace all spaces used for indentation with tabs.

Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 tools/gpio-tools-test.bash | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/tools/gpio-tools-test.bash b/tools/gpio-tools-test.bash
index b55c5eb..abb2f5d 100755
--- a/tools/gpio-tools-test.bash
+++ b/tools/gpio-tools-test.bash
@@ -203,9 +203,9 @@ dut_run_redirect() {
 
 dut_read_redirect() {
 	output=$(<$SHUNIT_TMPDIR/$DUT_OUTPUT)
-        local ORIG_IFS="$IFS"
-        IFS=$'\n' lines=($output)
-        IFS="$ORIG_IFS"
+	local ORIG_IFS="$IFS"
+	IFS=$'\n' lines=($output)
+	IFS="$ORIG_IFS"
 }
 
 dut_read() {
@@ -269,12 +269,12 @@ dut_wait() {
 }
 
 dut_cleanup() {
-        if [ -n "$DUT_PID" ]
-        then
+	if [ -n "$DUT_PID" ]
+	then
 		kill -SIGTERM $DUT_PID 2> /dev/null
 		wait $DUT_PID || false
-        fi
-        rm -f $SHUNIT_TMPDIR/$DUT_OUTPUT
+	fi
+	rm -f $SHUNIT_TMPDIR/$DUT_OUTPUT
 }
 
 tearDown() {

-- 
2.43.0

[PATCH libgpiod v2 2/4] tools: tests: use "$@" instead of $*

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

$@ does not break up quoted arguments which is what we want in all cases
in the bash test-suite. Use it instead of $*. While at it: prevent
globbing with double quotes but allow variable expansion.

Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 tools/gpio-tools-test.bash | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/tools/gpio-tools-test.bash b/tools/gpio-tools-test.bash
index abb2f5d..dde26b4 100755
--- a/tools/gpio-tools-test.bash
+++ b/tools/gpio-tools-test.bash
@@ -27,10 +27,10 @@ GPIOSIM_APP_NAME="gpio-tools-test"
 MIN_KERNEL_VERSION="5.17.4"
 MIN_SHUNIT_VERSION="2.1.8"
 
-# Run the command in $* and fail the test if the command succeeds.
+# Run the command in $@ and fail the test if the command succeeds.
 assert_fail() {
-	$* || return 0
-	fail " '$*': command did not fail as expected"
+	"$@" || return 0
+	fail " '$@': command did not fail as expected"
 }
 
 # Check if the string in $2 matches against the pattern in $1.
@@ -71,7 +71,7 @@ gpiosim_chip() {
 
 	mkdir -p $BANKPATH
 
-	for ARG in $*
+	for ARG in "$@"
 	do
 		local KEY=$(echo $ARG | cut -d"=" -f1)
 		local VAL=$(echo $ARG | cut -d"=" -f2)
@@ -253,7 +253,7 @@ dut_regex_match() {
 }
 
 dut_write() {
-	echo $* >&${COPROC[1]}
+	echo "$@" >&${COPROC[1]}
 }
 
 dut_kill() {
@@ -283,7 +283,7 @@ tearDown() {
 }
 
 request_release_line() {
-	$SOURCE_DIR/gpioget -c $* >/dev/null
+	$SOURCE_DIR/gpioget -c "$@" >/dev/null
 }
 
 #

-- 
2.43.0

[PATCH libgpiod v2 3/4] tools: tests: remove unneeded ';' in while loops

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

We're already breaking the line between while and do so there's no need
for the ';' character.

Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 tools/gpio-tools-test.bash | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/gpio-tools-test.bash b/tools/gpio-tools-test.bash
index dde26b4..ff4dea6 100755
--- a/tools/gpio-tools-test.bash
+++ b/tools/gpio-tools-test.bash
@@ -211,7 +211,7 @@ dut_read_redirect() {
 dut_read() {
 	local LINE
 	lines=()
-	while read -t 0.2 -u ${COPROC[0]} LINE;
+	while read -t 0.2 -u ${COPROC[0]} LINE
 	do
 		if [ -n "$DUT_FIRST_CHAR" ]
 		then
@@ -232,7 +232,7 @@ dut_flush() {
 	lines=()
 	output=
 	unset DUT_FIRST_CHAR
-	while read -t 0 -u ${COPROC[0]} JUNK;
+	while read -t 0 -u ${COPROC[0]} JUNK
 	do
 		read -t 0.1 -u ${COPROC[0]} JUNK || true
 	done

-- 
2.43.0

[PATCH libgpiod v2 4/4] tools: tests: remove dependency on grep

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

We only use grep in one place where we can use the -regex option of find
instead. Switch to using find --regex and drop grep entirely from the
tools test-suite.

Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 tools/gpio-tools-test.bash | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/tools/gpio-tools-test.bash b/tools/gpio-tools-test.bash
index ff4dea6..61bfa8f 100755
--- a/tools/gpio-tools-test.bash
+++ b/tools/gpio-tools-test.bash
@@ -162,7 +162,7 @@ gpiosim_cleanup() {
 		ls $BANKPATH/line* > /dev/null 2>&1
 		if [ "$?" = "0" ]
 		then
-			for LINE in $(find $BANKPATH/ | grep -E "line[0-9]+$")
+			for LINE in $(find $BANKPATH/ -regex ".*line[0-9]+$")
 			do
 				test -e $LINE/hog && rmdir $LINE/hog
 				rmdir $LINE
@@ -3078,7 +3078,6 @@ check_prog() {
 check_prog shunit2
 check_prog modprobe
 check_prog timeout
-check_prog grep
 
 # Check if we're running a kernel at the required version or later
 check_kernel $MIN_KERNEL_VERSION

-- 
2.43.0

Re: [PATCH libgpiod v2 2/4] tools: tests: use "$@" instead of $*

[Kent Gibson]

On Mon, May 27, 2024 at 02:02:34PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> $@ does not break up quoted arguments which is what we want in all cases
> in the bash test-suite. Use it instead of $*. While at it: prevent
> globbing with double quotes but allow variable expansion.
>
> Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
>  tools/gpio-tools-test.bash | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/tools/gpio-tools-test.bash b/tools/gpio-tools-test.bash
> index abb2f5d..dde26b4 100755
> --- a/tools/gpio-tools-test.bash
> +++ b/tools/gpio-tools-test.bash
> @@ -27,10 +27,10 @@ GPIOSIM_APP_NAME="gpio-tools-test"
>  MIN_KERNEL_VERSION="5.17.4"
>  MIN_SHUNIT_VERSION="2.1.8"
>
> -# Run the command in $* and fail the test if the command succeeds.
> +# Run the command in $@ and fail the test if the command succeeds.
>  assert_fail() {
> -	$* || return 0
> -	fail " '$*': command did not fail as expected"
> +	"$@" || return 0
> +	fail " '$@': command did not fail as expected"
>  }
>

Ironically, shellcheck doesn't like the '$@' in the fail string[1], so you
should use $* there.

It also doesn't like looping on find results in patch 4[2], though that
is not related to your change, so leave it and I'll fix it later?

Cheers,
Kent.

[1] https://www.shellcheck.net/wiki/SC2145
[2] https://www.shellcheck.net/wiki/SC2044

Re: [PATCH libgpiod v2 2/4] tools: tests: use "$@" instead of $*

[Bartosz Golaszewski]

On Mon, May 27, 2024 at 2:44 PM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Mon, May 27, 2024 at 02:02:34PM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > $@ does not break up quoted arguments which is what we want in all cases
> > in the bash test-suite. Use it instead of $*. While at it: prevent
> > globbing with double quotes but allow variable expansion.
> >
> > Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > ---
> >  tools/gpio-tools-test.bash | 12 ++++++------
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/tools/gpio-tools-test.bash b/tools/gpio-tools-test.bash
> > index abb2f5d..dde26b4 100755
> > --- a/tools/gpio-tools-test.bash
> > +++ b/tools/gpio-tools-test.bash
> > @@ -27,10 +27,10 @@ GPIOSIM_APP_NAME="gpio-tools-test"
> >  MIN_KERNEL_VERSION="5.17.4"
> >  MIN_SHUNIT_VERSION="2.1.8"
> >
> > -# Run the command in $* and fail the test if the command succeeds.
> > +# Run the command in $@ and fail the test if the command succeeds.
> >  assert_fail() {
> > -     $* || return 0
> > -     fail " '$*': command did not fail as expected"
> > +     "$@" || return 0
> > +     fail " '$@': command did not fail as expected"
> >  }
> >
>
> Ironically, shellcheck doesn't like the '$@' in the fail string[1], so you
> should use $* there.
>
> It also doesn't like looping on find results in patch 4[2], though that
> is not related to your change, so leave it and I'll fix it later?
>

What does it want here? This looks correct to me? Should we do "$(find...)"?

Bart

> Cheers,
> Kent.
>
> [1] https://www.shellcheck.net/wiki/SC2145
> [2] https://www.shellcheck.net/wiki/SC2044

Re: [PATCH libgpiod v2 2/4] tools: tests: use "$@" instead of $*

[Kent Gibson]

On Mon, May 27, 2024 at 02:51:52PM +0200, Bartosz Golaszewski wrote:
> On Mon, May 27, 2024 at 2:44 PM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > On Mon, May 27, 2024 at 02:02:34PM +0200, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > >
> > > $@ does not break up quoted arguments which is what we want in all cases
> > > in the bash test-suite. Use it instead of $*. While at it: prevent
> > > globbing with double quotes but allow variable expansion.
> > >
> > > Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > > ---
> > >  tools/gpio-tools-test.bash | 12 ++++++------
> > >  1 file changed, 6 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/tools/gpio-tools-test.bash b/tools/gpio-tools-test.bash
> > > index abb2f5d..dde26b4 100755
> > > --- a/tools/gpio-tools-test.bash
> > > +++ b/tools/gpio-tools-test.bash
> > > @@ -27,10 +27,10 @@ GPIOSIM_APP_NAME="gpio-tools-test"
> > >  MIN_KERNEL_VERSION="5.17.4"
> > >  MIN_SHUNIT_VERSION="2.1.8"
> > >
> > > -# Run the command in $* and fail the test if the command succeeds.
> > > +# Run the command in $@ and fail the test if the command succeeds.
> > >  assert_fail() {
> > > -     $* || return 0
> > > -     fail " '$*': command did not fail as expected"
> > > +     "$@" || return 0
> > > +     fail " '$@': command did not fail as expected"
> > >  }
> > >
> >
> > Ironically, shellcheck doesn't like the '$@' in the fail string[1], so you
> > should use $* there.
> >
> > It also doesn't like looping on find results in patch 4[2], though that
> > is not related to your change, so leave it and I'll fix it later?
> >
>
> What does it want here? This looks correct to me? Should we do "$(find...)"?
>

Refer to the referenced link - it is worried about filenames containing
whitespace.
Not sure what the best option is here - I am only just looking into it...

Kent.

> Bart
>
> > Cheers,
> > Kent.
> >
> > [1] https://www.shellcheck.net/wiki/SC2145
> > [2] https://www.shellcheck.net/wiki/SC2044

Re: [PATCH libgpiod v2 2/4] tools: tests: use "$@" instead of $*

[Kent Gibson]

On Mon, May 27, 2024 at 08:57:32PM +0800, Kent Gibson wrote:
> On Mon, May 27, 2024 at 02:51:52PM +0200, Bartosz Golaszewski wrote:
> > On Mon, May 27, 2024 at 2:44 PM Kent Gibson <warthog618@gmail.com> wrote:
> > >
> > > On Mon, May 27, 2024 at 02:02:34PM +0200, Bartosz Golaszewski wrote:
> > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > > >
> > > > $@ does not break up quoted arguments which is what we want in all cases
> > > > in the bash test-suite. Use it instead of $*. While at it: prevent
> > > > globbing with double quotes but allow variable expansion.
> > > >
> > > > Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > > > ---
> > > >  tools/gpio-tools-test.bash | 12 ++++++------
> > > >  1 file changed, 6 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/tools/gpio-tools-test.bash b/tools/gpio-tools-test.bash
> > > > index abb2f5d..dde26b4 100755
> > > > --- a/tools/gpio-tools-test.bash
> > > > +++ b/tools/gpio-tools-test.bash
> > > > @@ -27,10 +27,10 @@ GPIOSIM_APP_NAME="gpio-tools-test"
> > > >  MIN_KERNEL_VERSION="5.17.4"
> > > >  MIN_SHUNIT_VERSION="2.1.8"
> > > >
> > > > -# Run the command in $* and fail the test if the command succeeds.
> > > > +# Run the command in $@ and fail the test if the command succeeds.
> > > >  assert_fail() {
> > > > -     $* || return 0
> > > > -     fail " '$*': command did not fail as expected"
> > > > +     "$@" || return 0
> > > > +     fail " '$@': command did not fail as expected"
> > > >  }
> > > >
> > >
> > > Ironically, shellcheck doesn't like the '$@' in the fail string[1], so you
> > > should use $* there.
> > >
> > > It also doesn't like looping on find results in patch 4[2], though that
> > > is not related to your change, so leave it and I'll fix it later?
> > >
> >
> > What does it want here? This looks correct to me? Should we do "$(find...)"?
> >
>
> Refer to the referenced link - it is worried about filenames containing
> whitespace.
> Not sure what the best option is here - I am only just looking into it...
>

How about using this for the cleanup:

		echo 0 > "$DEVPATH/live"
		find "$DEVPATH" -type d -name hog -exec rmdir '{}' '+'
		find "$DEVPATH" -type d -name "line*" -exec rmdir '{}' '+'
		find "$DEVPATH" -type d -name "bank*" -exec rmdir '{}' '+'
		rmdir "$DEVPATH"

It is a bit less subtle, but that works for me.

Cheers,
Kent.

Re: [PATCH libgpiod v2 2/4] tools: tests: use "$@" instead of $*

[Bartosz Golaszewski]

On Mon, May 27, 2024 at 3:20 PM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Mon, May 27, 2024 at 08:57:32PM +0800, Kent Gibson wrote:
> > On Mon, May 27, 2024 at 02:51:52PM +0200, Bartosz Golaszewski wrote:
> > > On Mon, May 27, 2024 at 2:44 PM Kent Gibson <warthog618@gmail.com> wrote:
> > > >
> > > > On Mon, May 27, 2024 at 02:02:34PM +0200, Bartosz Golaszewski wrote:
> > > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > > > >
> > > > > $@ does not break up quoted arguments which is what we want in all cases
> > > > > in the bash test-suite. Use it instead of $*. While at it: prevent
> > > > > globbing with double quotes but allow variable expansion.
> > > > >
> > > > > Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > > > > ---
> > > > >  tools/gpio-tools-test.bash | 12 ++++++------
> > > > >  1 file changed, 6 insertions(+), 6 deletions(-)
> > > > >
> > > > > diff --git a/tools/gpio-tools-test.bash b/tools/gpio-tools-test.bash
> > > > > index abb2f5d..dde26b4 100755
> > > > > --- a/tools/gpio-tools-test.bash
> > > > > +++ b/tools/gpio-tools-test.bash
> > > > > @@ -27,10 +27,10 @@ GPIOSIM_APP_NAME="gpio-tools-test"
> > > > >  MIN_KERNEL_VERSION="5.17.4"
> > > > >  MIN_SHUNIT_VERSION="2.1.8"
> > > > >
> > > > > -# Run the command in $* and fail the test if the command succeeds.
> > > > > +# Run the command in $@ and fail the test if the command succeeds.
> > > > >  assert_fail() {
> > > > > -     $* || return 0
> > > > > -     fail " '$*': command did not fail as expected"
> > > > > +     "$@" || return 0
> > > > > +     fail " '$@': command did not fail as expected"
> > > > >  }
> > > > >
> > > >
> > > > Ironically, shellcheck doesn't like the '$@' in the fail string[1], so you
> > > > should use $* there.
> > > >
> > > > It also doesn't like looping on find results in patch 4[2], though that
> > > > is not related to your change, so leave it and I'll fix it later?
> > > >
> > >
> > > What does it want here? This looks correct to me? Should we do "$(find...)"?
> > >
> >
> > Refer to the referenced link - it is worried about filenames containing
> > whitespace.
> > Not sure what the best option is here - I am only just looking into it...
> >
>
> How about using this for the cleanup:
>
>                 echo 0 > "$DEVPATH/live"
>                 find "$DEVPATH" -type d -name hog -exec rmdir '{}' '+'
>                 find "$DEVPATH" -type d -name "line*" -exec rmdir '{}' '+'
>                 find "$DEVPATH" -type d -name "bank*" -exec rmdir '{}' '+'
>                 rmdir "$DEVPATH"
>
> It is a bit less subtle, but that works for me.
>

Looks good and works fine. I'll use it, thanks a lot!

Bart

Re: [PATCH libgpiod v2 4/4] tools: tests: remove dependency on grep

[Andy Shevchenko]

On Mon, May 27, 2024 at 02:02:36PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> We only use grep in one place where we can use the -regex option of find
> instead. Switch to using find --regex and drop grep entirely from the

-regex

> tools test-suite.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH libgpiod v2 2/4] tools: tests: use "$@" instead of $*

[Andy Shevchenko]

On Mon, May 27, 2024 at 08:44:20PM +0800, Kent Gibson wrote:
> On Mon, May 27, 2024 at 02:02:34PM +0200, Bartosz Golaszewski wrote:

...

> >  assert_fail() {
> > -	$* || return 0
> > -	fail " '$*': command did not fail as expected"
> > +	"$@" || return 0
> > +	fail " '$@': command did not fail as expected"
> >  }
> 
> Ironically, shellcheck doesn't like the '$@' in the fail string[1], so you
> should use $* there.

But why does it do like this?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH libgpiod v2 2/4] tools: tests: use "$@" instead of $*

[Andy Shevchenko]

On Mon, May 27, 2024 at 08:44:20PM +0800, Kent Gibson wrote:
> On Mon, May 27, 2024 at 02:02:34PM +0200, Bartosz Golaszewski wrote:

...

> It also doesn't like looping on find results in patch 4[2], though that
> is not related to your change, so leave it and I'll fix it later?

Does it really mean _to fix_ rather than _to "fix"_? I mean how do we know that
shellcheck is 100% correct tool and has no bugs?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH libgpiod v2 2/4] tools: tests: use "$@" instead of $*

[Kent Gibson]

On Mon, May 27, 2024 at 07:17:37PM +0300, Andy Shevchenko wrote:
> On Mon, May 27, 2024 at 08:44:20PM +0800, Kent Gibson wrote:
> > On Mon, May 27, 2024 at 02:02:34PM +0200, Bartosz Golaszewski wrote:
>
> ...
>
> > >  assert_fail() {
> > > -	$* || return 0
> > > -	fail " '$*': command did not fail as expected"
> > > +	"$@" || return 0
> > > +	fail " '$@': command did not fail as expected"
> > >  }
> >
> > Ironically, shellcheck doesn't like the '$@' in the fail string[1], so you
> > should use $* there.
>
> But why does it do like this?
>

Read the link[1].

Because $@ is an array being used to build a string, and that may not
work the way you expect.  In this case $* is clearer as that has already
been concatenated.

Cheers,
Kent.

[1] https://www.shellcheck.net/wiki/SC2145

Re: [PATCH libgpiod v2 2/4] tools: tests: use "$@" instead of $*

[Kent Gibson]

On Mon, May 27, 2024 at 07:20:01PM +0300, Andy Shevchenko wrote:
> On Mon, May 27, 2024 at 08:44:20PM +0800, Kent Gibson wrote:
> > On Mon, May 27, 2024 at 02:02:34PM +0200, Bartosz Golaszewski wrote:
>
> ...
>
> > It also doesn't like looping on find results in patch 4[2], though that
> > is not related to your change, so leave it and I'll fix it later?
>
> Does it really mean _to fix_ rather than _to "fix"_? I mean how do we know that
> shellcheck is 100% correct tool and has no bugs?
>

How do we know anything?

In this case you can read the description of the faults, which I had linked,
and see if that makes sense to you.  And we test the fixed code to ensure
it still works as intended.

I'm not claiming shellcheck is fool-proof, or 100% correct, or 100% complete,
but it is more available and repeatable than Andy's Eyeballs.
And if we do find bugs in it we can always fix those too.
As I stated earlier, if you have a better metric to use then I'm more than
happy to compare, but so far shellcheck seems a reasonable option to me.

Cheers,
Kent.

Re: [PATCH libgpiod v2 2/4] tools: tests: use "$@" instead of $*

[Andy Shevchenko]

On Tue, May 28, 2024 at 07:39:10AM +0800, Kent Gibson wrote:
> On Mon, May 27, 2024 at 07:17:37PM +0300, Andy Shevchenko wrote:
> > On Mon, May 27, 2024 at 08:44:20PM +0800, Kent Gibson wrote:
> > > On Mon, May 27, 2024 at 02:02:34PM +0200, Bartosz Golaszewski wrote:

...

> > > >  assert_fail() {
> > > > -	$* || return 0
> > > > -	fail " '$*': command did not fail as expected"
> > > > +	"$@" || return 0
> > > > +	fail " '$@': command did not fail as expected"
> > > >  }
> > >
> > > Ironically, shellcheck doesn't like the '$@' in the fail string[1], so you
> > > should use $* there.
> >
> > But why does it do like this?
> 
> Read the link[1].

Okay, this is only for some debug / error messages. Still if one wants to have
clear understanding on what has been passed to some function, $* is not a
correct option. Also note the single quotes, shouldn't that protect from the
arguments loss?

> Because $@ is an array being used to build a string, and that may not
> work the way you expect.

I think it's the opposite, $* works in a way I do not expect :-)

> In this case $* is clearer as that has already
> been concatenated.

...loosing information about which word refers to which argument, yes.

> [1] https://www.shellcheck.net/wiki/SC2145

TL;DR: I consider this is still a bug in shellcheck. But if you rely on the
tool as on the ruleset carved in stone, I will not die. Just a remark to
myself "even honourable tools may also be broken".

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH libgpiod v2 2/4] tools: tests: use "$@" instead of $*

[Andy Shevchenko]

On Tue, May 28, 2024 at 07:54:26AM +0800, Kent Gibson wrote:
> On Mon, May 27, 2024 at 07:20:01PM +0300, Andy Shevchenko wrote:
> > On Mon, May 27, 2024 at 08:44:20PM +0800, Kent Gibson wrote:
> > > On Mon, May 27, 2024 at 02:02:34PM +0200, Bartosz Golaszewski wrote:

...

> > > It also doesn't like looping on find results in patch 4[2], though that
> > > is not related to your change, so leave it and I'll fix it later?
> >
> > Does it really mean _to fix_ rather than _to "fix"_? I mean how do we know that
> > shellcheck is 100% correct tool and has no bugs?
> 
> How do we know anything?
> 
> In this case you can read the description of the faults, which I had linked,
> and see if that makes sense to you.  And we test the fixed code to ensure
> it still works as intended.
> 
> I'm not claiming shellcheck is fool-proof, or 100% correct, or 100% complete,
> but it is more available and repeatable than Andy's Eyeballs.
> And if we do find bugs in it we can always fix those too.

Sure, any tool has its own limitations. Esp. Andy's Eyeballs!

> As I stated earlier, if you have a better metric to use then I'm more than
> happy to compare, but so far shellcheck seems a reasonable option to me.

No problem!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH libgpiod v2 2/4] tools: tests: use "$@" instead of $*

[Kent Gibson]

On Wed, May 29, 2024 at 04:08:49PM +0300, Andy Shevchenko wrote:
> On Tue, May 28, 2024 at 07:39:10AM +0800, Kent Gibson wrote:
> > On Mon, May 27, 2024 at 07:17:37PM +0300, Andy Shevchenko wrote:
> > > On Mon, May 27, 2024 at 08:44:20PM +0800, Kent Gibson wrote:
> > > > On Mon, May 27, 2024 at 02:02:34PM +0200, Bartosz Golaszewski wrote:
>
> ...
>
> > > > >  assert_fail() {
> > > > > -	$* || return 0
> > > > > -	fail " '$*': command did not fail as expected"
> > > > > +	"$@" || return 0
> > > > > +	fail " '$@': command did not fail as expected"
> > > > >  }
> > > >
> > > > Ironically, shellcheck doesn't like the '$@' in the fail string[1], so you
> > > > should use $* there.
> > >
> > > But why does it do like this?
> >
> > Read the link[1].
>
> Okay, this is only for some debug / error messages. Still if one wants to have
> clear understanding on what has been passed to some function, $* is not a
> correct option. Also note the single quotes, shouldn't that protect from the
> arguments loss?
>

That's right - I was only referring to this particular case where a
string is being constructed.  Wasn't that clear?

The single quotes are within double quotes, so aren't they just part of
the text in this context?

> > Because $@ is an array being used to build a string, and that may not
> > work the way you expect.
>
> I think it's the opposite, $* works in a way I do not expect :-)
>

When passing arguments, sure.  Not when constructing strings.

> > In this case $* is clearer as that has already
> > been concatenated.
>
> ...loosing information about which word refers to which argument, yes.
>

It is building a string, so arguments are irrelevant.


> > [1] https://www.shellcheck.net/wiki/SC2145
>
> TL;DR: I consider this is still a bug in shellcheck. But if you rely on the
> tool as on the ruleset carved in stone, I will not die. Just a remark to
> myself "even honourable tools may also be broken".
>

If you think it is a bug then raise it with shellcheck.
I think you are conflating cases, and I agree with shellcheck on this one.

Cheers,
Kent.

Re: [PATCH libgpiod v2 2/4] tools: tests: use "$@" instead of $*

[Andy Shevchenko]

On Wed, May 29, 2024 at 09:18:47PM +0800, Kent Gibson wrote:
> On Wed, May 29, 2024 at 04:08:49PM +0300, Andy Shevchenko wrote:
> > On Tue, May 28, 2024 at 07:39:10AM +0800, Kent Gibson wrote:
> > > On Mon, May 27, 2024 at 07:17:37PM +0300, Andy Shevchenko wrote:
> > > > On Mon, May 27, 2024 at 08:44:20PM +0800, Kent Gibson wrote:
> > > > > On Mon, May 27, 2024 at 02:02:34PM +0200, Bartosz Golaszewski wrote:

...

> > > > > >  assert_fail() {
> > > > > > -	$* || return 0
> > > > > > -	fail " '$*': command did not fail as expected"
> > > > > > +	"$@" || return 0
> > > > > > +	fail " '$@': command did not fail as expected"
> > > > > >  }
> > > > >
> > > > > Ironically, shellcheck doesn't like the '$@' in the fail string[1], so you
> > > > > should use $* there.
> > > >
> > > > But why does it do like this?
> > >
> > > Read the link[1].
> >
> > Okay, this is only for some debug / error messages. Still if one wants to have
> > clear understanding on what has been passed to some function, $* is not a
> > correct option. Also note the single quotes, shouldn't that protect from the
> > arguments loss?
> 
> That's right - I was only referring to this particular case where a
> string is being constructed.  Wasn't that clear?

I meant that if you want to have this knowledge in the debug / error message,
you will fail with $*, that's why I consider shellcheck is incorrect.

Ex.

I have

	foo bar "baz bar2"

and I want

	"ERROR: 'foo bar "baz bar2"' failed"

type of message.

AFAIU this is not what shellcheck wants. It want me to mange this to

	"ERROR: 'foo bar baz bar2' failed"

Thanks, but no thanks to shellcheck.

> The single quotes are within double quotes, so aren't they just part of
> the text in this context?

I don't remember by heard the shell expansion rules. I presumable that it
might affect the inner argument on the recursive expansion.

> > > Because $@ is an array being used to build a string, and that may not
> > > work the way you expect.
> >
> > I think it's the opposite, $* works in a way I do not expect :-)
> 
> When passing arguments, sure.

> Not when constructing strings.

Why not? This is pure puzzle to me why anybody wants the mangled string.

> > > In this case $* is clearer as that has already
> > > been concatenated.
> >
> > ...loosing information about which word refers to which argument, yes.
> 
> It is building a string, so arguments are irrelevant.

See above why I think it's relevant.

> > > [1] https://www.shellcheck.net/wiki/SC2145
> >
> > TL;DR: I consider this is still a bug in shellcheck. But if you rely on the
> > tool as on the ruleset carved in stone, I will not die. Just a remark to
> > myself "even honourable tools may also be broken".
> 
> If you think it is a bug then raise it with shellcheck.
> I think you are conflating cases, and I agree with shellcheck on this one.

Okay.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH libgpiod v2 2/4] tools: tests: use "$@" instead of $*

[Kent Gibson]

On Wed, May 29, 2024 at 04:27:00PM +0300, Andy Shevchenko wrote:
> On Wed, May 29, 2024 at 09:18:47PM +0800, Kent Gibson wrote:
> > On Wed, May 29, 2024 at 04:08:49PM +0300, Andy Shevchenko wrote:
> > > On Tue, May 28, 2024 at 07:39:10AM +0800, Kent Gibson wrote:
> > > > On Mon, May 27, 2024 at 07:17:37PM +0300, Andy Shevchenko wrote:
> > > > > On Mon, May 27, 2024 at 08:44:20PM +0800, Kent Gibson wrote:
> > > > > > On Mon, May 27, 2024 at 02:02:34PM +0200, Bartosz Golaszewski wrote:
>
> ...
>
> > > > > > >  assert_fail() {
> > > > > > > -	$* || return 0
> > > > > > > -	fail " '$*': command did not fail as expected"
> > > > > > > +	"$@" || return 0
> > > > > > > +	fail " '$@': command did not fail as expected"
> > > > > > >  }
> > > > > >
> > > > > > Ironically, shellcheck doesn't like the '$@' in the fail string[1], so you
> > > > > > should use $* there.
> > > > >
> > > > > But why does it do like this?
> > > >
> > > > Read the link[1].
> > >
> > > Okay, this is only for some debug / error messages. Still if one wants to have
> > > clear understanding on what has been passed to some function, $* is not a
> > > correct option. Also note the single quotes, shouldn't that protect from the
> > > arguments loss?
> >
> > That's right - I was only referring to this particular case where a
> > string is being constructed.  Wasn't that clear?
>
> I meant that if you want to have this knowledge in the debug / error message,
> you will fail with $*, that's why I consider shellcheck is incorrect.
>
> Ex.
>
> I have
>
> 	foo bar "baz bar2"
>
> and I want
>
> 	"ERROR: 'foo bar "baz bar2"' failed"
>
> type of message.
>

Fair point, but $@ doesn't give you that either:

boo() {
	echo "star '$*'"
	echo "hash '$@'"
}

boo foo bar "baz bar2"

gives:

star 'foo bar baz bar2'
hash 'foo bar baz bar2'

Is there any form that gives you the format you want?

Cheers,
Kent.

Re: [PATCH libgpiod v2 2/4] tools: tests: use "$@" instead of $*

[Andy Shevchenko]

On Wed, May 29, 2024 at 09:44:40PM +0800, Kent Gibson wrote:
> On Wed, May 29, 2024 at 04:27:00PM +0300, Andy Shevchenko wrote:
> > On Wed, May 29, 2024 at 09:18:47PM +0800, Kent Gibson wrote:
> > > On Wed, May 29, 2024 at 04:08:49PM +0300, Andy Shevchenko wrote:
> > > > On Tue, May 28, 2024 at 07:39:10AM +0800, Kent Gibson wrote:
> > > > > On Mon, May 27, 2024 at 07:17:37PM +0300, Andy Shevchenko wrote:
> > > > > > On Mon, May 27, 2024 at 08:44:20PM +0800, Kent Gibson wrote:
> > > > > > > On Mon, May 27, 2024 at 02:02:34PM +0200, Bartosz Golaszewski wrote:

...

> > > > > > > >  assert_fail() {
> > > > > > > > -	$* || return 0
> > > > > > > > -	fail " '$*': command did not fail as expected"
> > > > > > > > +	"$@" || return 0
> > > > > > > > +	fail " '$@': command did not fail as expected"
> > > > > > > >  }
> > > > > > >
> > > > > > > Ironically, shellcheck doesn't like the '$@' in the fail string[1], so you
> > > > > > > should use $* there.
> > > > > >
> > > > > > But why does it do like this?
> > > > >
> > > > > Read the link[1].
> > > >
> > > > Okay, this is only for some debug / error messages. Still if one wants to have
> > > > clear understanding on what has been passed to some function, $* is not a
> > > > correct option. Also note the single quotes, shouldn't that protect from the
> > > > arguments loss?
> > >
> > > That's right - I was only referring to this particular case where a
> > > string is being constructed.  Wasn't that clear?
> >
> > I meant that if you want to have this knowledge in the debug / error message,
> > you will fail with $*, that's why I consider shellcheck is incorrect.
> >
> > Ex.
> >
> > I have
> >
> > 	foo bar "baz bar2"
> >
> > and I want
> >
> > 	"ERROR: 'foo bar "baz bar2"' failed"
> >
> > type of message.
> >
> 
> Fair point, but $@ doesn't give you that either:
> 
> boo() {
> 	echo "star '$*'"
> 	echo "hash '$@'"
> }
> 
> boo foo bar "baz bar2"
> 
> gives:
> 
> star 'foo bar baz bar2'
> hash 'foo bar baz bar2'

Oh, this is unfortunate. It seems entire model with quotation depends on the
commands, printf makes it different, print -r -- makes it better, though, if
one uses non-space IFS for it.

> Is there any form that gives you the format you want?

Yes, but it requires an iteration over arguments, roughly something like below
(which is not yet what I want, but closer):

	for a in "$@"; do
		echo -n '"$a" ' # echo -n seems not portable IIRC
	done
	echo

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH libgpiod v2 2/4] tools: tests: use "$@" instead of $*

[Kent Gibson]

On Wed, May 29, 2024 at 05:33:00PM +0300, Andy Shevchenko wrote:
> On Wed, May 29, 2024 at 09:44:40PM +0800, Kent Gibson wrote:
> > On Wed, May 29, 2024 at 04:27:00PM +0300, Andy Shevchenko wrote:
> > > On Wed, May 29, 2024 at 09:18:47PM +0800, Kent Gibson wrote:
> > > > On Wed, May 29, 2024 at 04:08:49PM +0300, Andy Shevchenko wrote:
> > > > > On Tue, May 28, 2024 at 07:39:10AM +0800, Kent Gibson wrote:
> > > > > > On Mon, May 27, 2024 at 07:17:37PM +0300, Andy Shevchenko wrote:
> > > > > > > On Mon, May 27, 2024 at 08:44:20PM +0800, Kent Gibson wrote:
> > > > > > > > On Mon, May 27, 2024 at 02:02:34PM +0200, Bartosz Golaszewski wrote:
>
> ...
>
> > >
> >
> > Fair point, but $@ doesn't give you that either:
> >
> > boo() {
> > 	echo "star '$*'"
> > 	echo "hash '$@'"
> > }
> >
> > boo foo bar "baz bar2"
> >
> > gives:
> >
> > star 'foo bar baz bar2'
> > hash 'foo bar baz bar2'
>
> Oh, this is unfortunate. It seems entire model with quotation depends on the
> commands, printf makes it different, print -r -- makes it better, though, if
> one uses non-space IFS for it.
>
> > Is there any form that gives you the format you want?
>
> Yes, but it requires an iteration over arguments, roughly something like below
> (which is not yet what I want, but closer):
>
> 	for a in "$@"; do
> 		echo -n '"$a" ' # echo -n seems not portable IIRC
> 	done
> 	echo
>

Ok, we're heading into the weeds here.
The issue isn't that shellcheck is wrong, it is that the error message
is not formatted the way you would like, and fixing that requires
writing a function to perform that formatting as bash can't do it out of
the box.  That isn't a huge issue, as we currently don't have any
parameters containing whitespace, but it is something that might want to
be addressed at some point.

Cheers,
Kent.

Re: [PATCH libgpiod v2 2/4] tools: tests: use "$@" instead of $*

[Andy Shevchenko]

On Thu, May 30, 2024 at 08:22:02AM +0800, Kent Gibson wrote:
> On Wed, May 29, 2024 at 05:33:00PM +0300, Andy Shevchenko wrote:
> > On Wed, May 29, 2024 at 09:44:40PM +0800, Kent Gibson wrote:
> > > On Wed, May 29, 2024 at 04:27:00PM +0300, Andy Shevchenko wrote:
> > > > On Wed, May 29, 2024 at 09:18:47PM +0800, Kent Gibson wrote:
> > > > > On Wed, May 29, 2024 at 04:08:49PM +0300, Andy Shevchenko wrote:
> > > > > > On Tue, May 28, 2024 at 07:39:10AM +0800, Kent Gibson wrote:
> > > > > > > On Mon, May 27, 2024 at 07:17:37PM +0300, Andy Shevchenko wrote:
> > > > > > > > On Mon, May 27, 2024 at 08:44:20PM +0800, Kent Gibson wrote:
> > > > > > > > > On Mon, May 27, 2024 at 02:02:34PM +0200, Bartosz Golaszewski wrote:

...

> > > >
> > >
> > > Fair point, but $@ doesn't give you that either:
> > >
> > > boo() {
> > > 	echo "star '$*'"
> > > 	echo "hash '$@'"
> > > }
> > >
> > > boo foo bar "baz bar2"
> > >
> > > gives:
> > >
> > > star 'foo bar baz bar2'
> > > hash 'foo bar baz bar2'
> >
> > Oh, this is unfortunate. It seems entire model with quotation depends on the
> > commands, printf makes it different, print -r -- makes it better, though, if
> > one uses non-space IFS for it.
> >
> > > Is there any form that gives you the format you want?
> >
> > Yes, but it requires an iteration over arguments, roughly something like below
> > (which is not yet what I want, but closer):
> >
> > 	for a in "$@"; do
> > 		echo -n '"$a" ' # echo -n seems not portable IIRC
> > 	done
> > 	echo
> >
> 
> Ok, we're heading into the weeds here.
> The issue isn't that shellcheck is wrong, it is that the error message
> is not formatted the way you would like, and fixing that requires
> writing a function to perform that formatting as bash can't do it out of
> the box.  That isn't a huge issue, as we currently don't have any
> parameters containing whitespace, but it is something that might want to
> be addressed at some point.

Yeah, I have no objection now for using $* in the strings for reporting.

-- 
With Best Regards,
Andy Shevchenko