[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] tests/test-xalloc-die.sh: Use $EXEEXT.
From: |
Jim Meyering |
Subject: |
Re: [PATCH] tests/test-xalloc-die.sh: Use $EXEEXT. |
Date: |
Sat, 13 Feb 2010 15:55:41 +0100 |
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