Re: [PATCH] tests/test-xalloc-die.sh: Use $EXEEXT.

[Jim Meyering]

Eric Blake wrote:
> According to Jim Meyering on 2/13/2010 4:51 AM:
>> Review/comments appreciated.

Thanks for the prompt review!

>> +# Given a directory name, DIR, if every entry in it that matches *.exe
>> +# contains only the specified bytes (see the case stmt below), then print
>> +# a space-separated list of those names and return 0.  Otherwise, don't
>> +# print anything and return 1.  Naming constraints apply also to DIR.
>> +find_exe_basenames_()
>> +{
>> +  feb_dir_=$1
>> +  feb_fail_=0
>> +  feb_result_=
>> +  feb_sp_=
>> +  for feb_file_ in $feb_dir_/*.exe dummy; do
>
> I don't think there are any shells that misbehave if you omit dummy.  I
> know there are shells the misbehave on 'for f in $xyz' if xyz expands to
> nothing, but globbing is different than expansion, and either a word will
> be present, or (for shells with nullglob support) the shell is
> sufficiently powerful enough to handle a glob that expands to nothing.
> This is particularly true since you already assume the shell is modern
> enough to support $().

Removing unnecessary code is always good.

>> +    case $feb_file_ in
>> +      dummy) continue;;
>> +      *[^-a-zA-Z/0-9_.+]*) feb_fail_=1; break;;
>
> For negated character classes in shell case statements, POSIX says to use
> [!a-z], not [^a-z].

I've made that change, too.
Does it matter in practice?

>> +      *) feb_file_=$(echo $feb_file_ | sed "s,^$feb_dir_/,,;"'s/\.exe$//')
>
> Given the fact that you are assuming the shell supports $(), shouldn't we
> also be relying on XSI manipulations, like ${feb_file_%.exe} rather than
> forking subshells and sed processes?
...
That's certainly more efficient.
Since it's less readable to me, I've added a comment.

> Should the error message use 'exe_shim' rather than 'exe-shim', for easier
> searching for the shell function when inspecting why the message was printed?

Sure.

Here's a change on top of the original patch.

>From 5a3195334cf78e4bd60905193b29f920831c9d71 Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Sat, 13 Feb 2010 15:51:37 +0100
Subject: [PATCH] * tests/init.sh (find_exe_basenames_): Remove unnecessary use 
of

"dummy" in a for loop.
Use '!', not '^' to select the complement of a character set used
in a "case" statement.
Use shell variable manipulation, a la ${...%.exe}, rather than sed.
Suggestions from Eric Blake.
---
 ChangeLog     |    9 +++++++++
 tests/init.sh |   10 +++++-----
 2 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index c382d58..0644236 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,12 @@
+2010-02-13  Jim Meyering  <address@hidden>
+
+       * tests/init.sh (find_exe_basenames_): Remove unnecessary use of
+       "dummy" in a for loop.
+       Use '!', not '^' to select the complement of a character set used
+       in a "case" statement.
+       Use shell variable manipulation, a la ${...%.exe}, rather than sed.
+       Suggestions from Eric Blake.
+
 2010-02-10  Jim Meyering  <address@hidden>

        maint.mk: prohibit inclusion of "ignore-value.h" without_use
diff --git a/tests/init.sh b/tests/init.sh
index c584b55..fc2796a 100644
--- a/tests/init.sh
+++ b/tests/init.sh
@@ -102,11 +102,11 @@ find_exe_basenames_()
   feb_fail_=0
   feb_result_=
   feb_sp_=
-  for feb_file_ in $feb_dir_/*.exe dummy; do
+  for feb_file_ in $feb_dir_/*.exe; do
     case $feb_file_ in
-      dummy) continue;;
-      *[^-a-zA-Z/0-9_.+]*) feb_fail_=1; break;;
-      *) feb_file_=$(echo $feb_file_ | sed "s,^$feb_dir_/,,;"'s/\.exe$//')
+      *[!-a-zA-Z/0-9_.+]*) feb_fail_=1; break;;
+      *) # Remove leading file name components as well as the .exe suffix.
+         feb_file_=${${feb_file_##*/}%.exe}
         feb_result_="$feb_result_$feb_sp_$feb_file_";;
     esac
     feb_sp_=' '
@@ -129,7 +129,7 @@ create_exe_shim_functions_()
   esac

   base_names_=$(find_exe_basenames_ $1) \
-    || { echo "$0 (exe-shim): skipping directory: $1" 1>&2; return 1; }
+    || { echo "$0 (exe_shim): skipping directory: $1" 1>&2; return 1; }

   if test -n "$base_names_"; then
     for base_ in $base_names_; do
--
1.7.0.169.g57c99