[PATCH 1/7] syntax-check: fix violations and implement sc_useless_quotes_in_case.
More syntax-check inspired goodness to make our shell code more
readable and maintainable. There's nothing controversial in here,
except perhaps a seemingly large amount of code churn for a
relatively small gain in each case, however the code definitely
*does* need all the help it can get to be maintainable without
severe hair loss, and even a journey of 1000 miles starts with a
single step, so I'll push the whole series in 72 hours or so as
usual.
By the end of this series, we've made some good progress in
unifying the style of new and old shell code alike, and since it's
all hooked into syntax-check, it'll take a concerted effort to
regress the aspects of style I've tackled, since make distcheck
will now fail if new violations are added in future. But, making
some of the older stuff not look like spaghetti is a much much
bigger task than these few patches are able to achieve.
Feedback welcome.
Contrary to popular belief, Bourne shell does not resplit case
expressions after expansion, so if there are no shell unquoted
shell metacharacters or whitespace, the quotes are useless.
* cfg.mk (sc_useless_quotes_in_case): New syntax-check rule to
ensure we don't reintroduce useless quoted case expressions.
* build-aux/ltmain.m4sh, m4/libtool.m4, tests/bindir.at,
tests/darwin.at, tests/defs.m4sh, tests/demo-hardcode.test,
tests/demo-nopic.test, tests/link-2.test, tests/quote.test,
tests/sysroot.at: Remove spurious quotes.
Signed-off-by: Gary V. Vaughan
---
build-aux/ltmain.m4sh| 12 ++--
cfg.mk |8
m4/libtool.m4|2 +-
tests/bindir.at |4 ++--
tests/darwin.at |2 +-
tests/defs.m4sh |2 +-
tests/demo-hardcode.test |4 ++--
tests/demo-nopic.test|6 +++---
tests/link-2.test|2 +-
tests/quote.test |8
tests/sysroot.at |2 +-
11 files changed, 30 insertions(+), 22 deletions(-)
diff --git a/build-aux/ltmain.m4sh b/build-aux/ltmain.m4sh
index 5e8fb25..ee93a21 100644
--- a/build-aux/ltmain.m4sh
+++ b/build-aux/ltmain.m4sh
@@ -461,7 +461,7 @@ func_lalib_unsafe_p ()
for lalib_p_l in 1 2 3 4
do
read lalib_p_line
- case "$lalib_p_line" in
+ case $lalib_p_line in
\#\ Generated\ by\ *$PACKAGE* ) lalib_p=yes; break;;
esac
done
@@ -568,7 +568,7 @@ func_resolve_sysroot ()
# store the result into func_replace_sysroot_result.
func_replace_sysroot ()
{
- case "$lt_sysroot:$1" in
+ case $lt_sysroot:$1 in
?*:"$lt_sysroot"*)
func_stripname "$lt_sysroot" '' "$1"
func_replace_sysroot_result="=$func_stripname_result"
@@ -3625,7 +3625,7 @@ main (int argc, char *argv[])
if (STREQ (argv[i], dumpscript_opt))
{
EOF
- case "$host" in
+ case $host in
*mingw* | *cygwin* )
# make stdout use "unix" line endings
echo " setmode(1,_O_BINARY);"
@@ -5796,7 +5796,7 @@ func_mode_link ()
if test -z "$libdir" && test "$linkmode" = prog; then
func_fatal_error "only libraries may -dlpreopen a convenience
library: \`$lib'"
fi
- case "$host" in
+ case $host in
# special handling for platforms with PE-DLLs.
*cygwin* | *mingw* | *cegcc* )
# Linker will automatically link against shared library if both
@@ -5896,7 +5896,7 @@ func_mode_link ()
# We need to hardcode the library path
if test -n "$shlibpath_var" && test -z "$avoidtemprpath" ; then
# Make sure the rpath contains only unique directories.
- case "$temp_rpath:" in
+ case $temp_rpath: in
*"$absdir:"*) ;;
*) func_append temp_rpath "$absdir:" ;;
esac
@@ -8765,7 +8765,7 @@ func_mode_uninstall ()
done
test -n "$old_library" && func_append rmfiles " $odir/$old_library"
- case "$opt_mode" in
+ case $opt_mode in
clean)
case " $library_names " in
*" $dlname "*) ;;
diff --git a/cfg.mk b/cfg.mk
index 6a1748c..4bc32a7 100644
--- a/cfg.mk
+++ b/cfg.mk
@@ -60,6 +60,14 @@ sc_trailing_blank-non-rfc3676:
halt='found trailing blank(s)' \
$(_sc_search_regexp)
+# Avoid useless quotes around case arguments such as:
+# case "$foo" in ...
+exclude_file_name_regexp--sc_useless_quotes_in_case = ^cfg.mk$$
+sc_useless_quotes_in_case:
+ @prohibit='case "[^ "]*"[ ][ ]*in' \
+ halt='found spurious quotes around case argument' \
+ $(_sc_search_regexp)
+
# List syntax-check exempted files.
exclude_file_name_regexp--sc_bindtextdomain = ^tests/.*demo[0-9]*/.*\.c$$
exclude_file_name_regexp--sc_error_message_uppercase = \
diff --git a/m4/libtool.m4 b/m4/libtool.m4
index a9e20cf..ec00e81 100644
--- a/m4/libtool.m4
++
[PATCH 4/7] syntax-check: fix violations and implement sc_prohibit_bare_basename.
* cfg.mk (sc_prohibit_bare_basename, sc_prohibit_basename_with_sed):
Make sure not to go back to using occasional `|$basename' or
`|$dirname' syntax.
* build-aux/general.m4sh, build-aux/git-hooks/commit-msg,
build-aux/ltmain.m4sh, build-aux/options-parser,
tests/fcdemo-conf.test, tests/fcdemo-shared.test,
tests/fcdemo-static.test, tests/libtoolize.at: Fix violations.
Signed-off-by: Gary V. Vaughan
---
build-aux/general.m4sh | 12 ++--
build-aux/git-hooks/commit-msg |4 ++--
build-aux/ltmain.m4sh |2 +-
build-aux/options-parser |6 +++---
cfg.mk | 11 +++
tests/fcdemo-conf.test |2 +-
tests/fcdemo-shared.test |2 +-
tests/fcdemo-static.test |2 +-
tests/libtoolize.at|4 ++--
9 files changed, 28 insertions(+), 17 deletions(-)
diff --git a/build-aux/general.m4sh b/build-aux/general.m4sh
index 1f44535..790f4e0 100644
--- a/build-aux/general.m4sh
+++ b/build-aux/general.m4sh
@@ -70,15 +70,15 @@ lt_nl='
'
IFS=" $lt_nl"
-dirname='s,/[^/]*$,,'
-basename='s,^.*/,,'
+dirname='s|/[^/]*$||'
+basename='s|^.*/||'
# func_dirname file append nondir_replacement
# Compute the dirname of FILE. If nonempty, add APPEND to the result,
# otherwise set result to NONDIR_REPLACEMENT.
func_dirname ()
{
-func_dirname_result=`$ECHO "${1}" | $SED "$dirname"`
+func_dirname_result=`$ECHO "${1}" |$SED "$dirname"`
if test "X$func_dirname_result" = "X${1}"; then
func_dirname_result=${3}
else
@@ -90,7 +90,7 @@ func_dirname ()
# func_basename file
func_basename ()
{
-func_basename_result=`$ECHO "${1}" | $SED "$basename"`
+func_basename_result=`$ECHO "${1}" |$SED "$basename"`
} # func_basename may be replaced by extended shell implementation
@@ -109,13 +109,13 @@ func_basename ()
func_dirname_and_basename ()
{
# Extract subdirectory from the argument.
-func_dirname_result=`$ECHO "${1}" | $SED -e "$dirname"`
+func_dirname_result=`$ECHO "${1}" |$SED "$dirname"`
if test "X$func_dirname_result" = "X${1}"; then
func_dirname_result=${3}
else
func_dirname_result=$func_dirname_result${2}
fi
-func_basename_result=`$ECHO "${1}" | $SED -e "$basename"`
+func_basename_result=`$ECHO "${1}" |$SED "$basename"`
} # func_dirname_and_basename may be replaced by extended shell implementation
diff --git a/build-aux/git-hooks/commit-msg b/build-aux/git-hooks/commit-msg
index f2e6108..bbd7751 100755
--- a/build-aux/git-hooks/commit-msg
+++ b/build-aux/git-hooks/commit-msg
@@ -6,13 +6,13 @@
: ${SED="sed"}
test set = ${ECHO+'set'} = set || ECHO='printf %s\n'
-basename="$SED -e "'s|^.*/||'
+basename='s|^.*/||'
nl='
'
progpath=$0
-progname=`$ECHO "$progpath" |$basename`
+progname=`$ECHO "$progpath" |$SED "$basename"`
log_file=$1
export log_file
diff --git a/build-aux/ltmain.m4sh b/build-aux/ltmain.m4sh
index 02ff034..b367ddd 100644
--- a/build-aux/ltmain.m4sh
+++ b/build-aux/ltmain.m4sh
@@ -3042,7 +3042,7 @@ func_extract_archives ()
$RM
"unfat-$$/${darwin_base_archive}-${darwin_arch}/${darwin_base_archive}"
done # $darwin_arches
## Okay now we've a bunch of thin objects, gotta fatten them up :)
- darwin_filelist=`find unfat-$$ -type f -name \*.o -print -o -name
\*.lo -print | $SED -e "$basename" | sort -u`
+ darwin_filelist=`find unfat-$$ -type f -name \*.o -print -o -name
\*.lo -print | $SED $basename | sort -u`
darwin_file=
darwin_files=
for darwin_file in $darwin_filelist; do
diff --git a/build-aux/options-parser b/build-aux/options-parser
index 4777d76..566ae43 100644
--- a/build-aux/options-parser
+++ b/build-aux/options-parser
@@ -167,8 +167,8 @@ EXIT_SKIP=77 # $? = 77 is used to indicate a skipped
test to automake.
debug_cmd=${debug_cmd-":"}
exit_cmd=:
-dirname="$SED -e "'s|/[^/]*$||'
-basename="$SED -e "'s|^.*/||'
+dirname='s|/[^/]*$||'
+basename='s|^.*/||'
nl='
'
@@ -181,7 +181,7 @@ nl='
progpath=$0
# The name of this program.
-progname=`echo "$progpath" |$basename`
+progname=`echo "$progpath" |$SED "$basename"`
## - ##
diff --git a/cfg.mk b/cfg.mk
index 3d2e2a6..8a44419 100644
--- a/cfg.mk
+++ b/cfg.mk
@@ -75,6 +75,17 @@ sc_prohibit_Xsed_without_X:
else :; \
fi || :
+# Use a consistent dirname and basename idiom.
+sc_prohibit_bare_basename:
+ @prohibit='\|[ ]*\$$(base|dir)name' \
+ halt='use `|$$SED "$$basename"'\'' instead of `|$$basename'\' \
+ $(_sc_search_regexp)
+
+sc_prohibit_basename_with_dollar_sed:
+ @prohibit='(base|dir)name="?(\$$SED|sed)[ "]' \
+ halt='use `basename='\''s|^.*/||'\'' instead of `basename="$$SED...'
\
+ $(_sc_search_regexp)
+
# Check for using `[' instead of `test'.
exclude_file_name_regexp--sc_prohibit_bracket_as_te
[PATCH 3/7] tests: migrate tests/sh.test checks to syntax-checks.
Some modernization of the legacy testsuite.
* tests/sh.test: Remove.
* Makefile.am (COMMON_TESTS): Adjust.
* cfg.mk (sc_libtool_m4_cc_basename, sc_prohibit_bracket_as_test)
(sc_prohibit_nested_quotes, sc_prohibit_set_dummy_without_shift)
(sc_prohibit_set_minus_minus, sc_prohibit_test_binary_operators)
(sc_prohibit_test_dollar, sc_prohibit_test_minus_e)
(sc_prohibit_test_unary_operators, sc_prohibit_test_X)
(sc_prohibit_Xsed_withou_X, sc_require_function_nl_brace):
Functionally identical tests to what used to be performed by
sh.test, only with coverage of all files.
* bootstrap, build-aux/edit-readme-alpha,
build-aux/extract-trace, build-aux/getopt.m4sh,
build-aux/ltmain.m4sh, configure.ac, m4/libtool.m4, m4/ltdl.m4,
tests/bindir.at, tests/configure-iface.at, tests/cwrapper.at,
tests/darwin.at, tests/defs.m4sh, tests/demo-hardcode.test,
tests/dlloader-api.at, tests/exceptions.at,
tests/getopt-m4sh.at, tests/lalib-syntax.at, tests/link-2.test,
tests/link-order2.at, tests/loadlibrary.at,
tests/lt_dladvise.at, tests/lt_dlexit.at, tests/lt_dlopen_a.at,
tests/lt_dlopenext.at, tests/need_lib_prefix.at,
tests/nonrecursive.at, tests/recursive.at, tests/resident.at,
tests/standalone.at, tests/static.at, tests/stresstest.at,
tests/subproject.at, tests/sysroot.at, tests/tagtrace.test,
tests/testsuite.at: Fix violations of the new syntax checks.
Signed-off-by: Gary V. Vaughan
---
Makefile.am |3 +-
bootstrap | 21 ---
build-aux/edit-readme-alpha |4 +-
build-aux/extract-trace |6 +-
build-aux/getopt.m4sh |2 +-
build-aux/ltmain.m4sh |2 +-
cfg.mk | 104 +
configure.ac|2 +-
m4/libtool.m4 | 50
m4/ltdl.m4 |2 +-
tests/bindir.at | 10 ++--
tests/configure-iface.at|8 +-
tests/cwrapper.at |2 +-
tests/darwin.at |2 +-
tests/defs.m4sh |4 +-
tests/demo-hardcode.test|6 +-
tests/dlloader-api.at |2 +-
tests/exceptions.at |2 +-
tests/getopt-m4sh.at|2 +-
tests/lalib-syntax.at |2 +-
tests/link-2.test |2 +-
tests/link-order2.at|8 +-
tests/loadlibrary.at|2 +-
tests/lt_dladvise.at|4 +-
tests/lt_dlexit.at |2 +-
tests/lt_dlopen_a.at|2 +-
tests/lt_dlopenext.at |4 +-
tests/need_lib_prefix.at|2 +-
tests/nonrecursive.at |4 +-
tests/recursive.at |4 +-
tests/resident.at |2 +-
tests/sh.test | 134 ---
tests/standalone.at |4 +-
tests/static.at |2 +-
tests/stresstest.at |2 +-
tests/subproject.at |4 +-
tests/sysroot.at|4 +-
tests/tagtrace.test |4 +-
tests/testsuite.at |4 +-
39 files changed, 200 insertions(+), 230 deletions(-)
delete mode 100755 tests/sh.test
diff --git a/Makefile.am b/Makefile.am
index 2c6cf81..31d286e 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -557,7 +557,7 @@ changelog = $(distdir)/ChangeLog
# date is updated to the following year.
changelog_start_date = 2011-01-01
$(changelog): FORCE
- $(AM_V_GEN)if test -d $(srcdir)/.git; then \
+ $(AM_V_GEN)if test -d '$(srcdir)/.git'; then \
$(gitlog_to_changelog) --amend=$(git_log_fix) \
--since=$(changelog_start_date) > '$@T'; \
rm -f '$@'; mv '$@T' '$@'; \
@@ -874,7 +874,6 @@ COMMON_TESTS = \
tests/nomode.test \
tests/objectlist.test \
tests/quote.test \
- tests/sh.test \
tests/suffix.test \
tests/tagtrace.test \
tests/cdemo-static.test \
diff --git a/bootstrap b/bootstrap
index 9a9e94b..4dbdf72 100755
--- a/bootstrap
+++ b/bootstrap
@@ -738,20 +738,20 @@ func_clean_unused_macros ()
# We use `ls|grep' instead of `ls *.m4' to avoid exceeding
# command line length limits in some shells.
- for file in `cd $macro_dir && ls -1 |grep '\.m4$'`; do
+ for file in `cd "$macro_dir" && ls -1 |grep '\.m4$'`; do
# Remove a macro file when aclocal.m4 does not m4_include it...
func_grep_q 'm4_include([[]'$macro_dir/$file'])' $aclocal_m4s \
-|| test ! -f $gnulib_path/m4/$file || {
+|| test ! -f "$gnulib_path/m4/$file" || {
# ...and there is an identical file in gnulib...
- if func_cmp_s $gnulib_path/m4/$file $macro_dir/$file; then
+ if func_cmp_s "$gnulib_path/m4/$file" "$macro_dir/$file"; then
# ...and it's not in the precious list (`echo' is needed
# here to squash whitespace for the match expression).
case " "`echo $gnulib_precious`" " in
*" $file "*) ;;
- *) rm -f $ma
[PATCH 7/7] syntax-check: fix violations and implement sc_prohibit_sed_s_comma.
I like to name temporary directories that I will remove shortly
with two leading commas so that they sort lexicographically at
the top of `ls' output. Now, `./configure
--prefix=`pwd`/,,inst' works again, for the first time in
several years.
* cfg.mk (sc_prohibit_sed_s_comma): Comma is too common a
character to use routinely as the separator for sed
substitutions on file paths and other variables determined by
the user, causing bugs like the one I describe above. Make sure
we don't accidentally reintroduce any comma separators in
future.
* Makefile.am, bootstrap, bootstrap.conf, build-aux/extract-trace,
build-aux/general.m4sh, build-aux/git-hooks/commit-msg,
build-aux/git-log-fix, build-aux/ltmain.m4sh, libtoolize.m4sh,
m4/libtool.m4, m4/ltdl.m4, tests/cdemo-undef.test,
tests/cmdline_wrap.at, tests/darwin.at, tests/defs.m4sh,
tests/getopt-m4sh.at, tests/install.at, tests/libtoolize.at,
tests/mdemo/Makefile.am, tests/need_lib_prefix.at,
tests/sysroot.at, tests/tagdemo-undef.test, tests/testsuite.at:
Try to use `|' as the default separator wherever possible,
otherwise something else that doesn't occur in the substitution
expression.
* NEWS: Updated.
Signed-off-by: Gary V. Vaughan
---
Makefile.am| 88
NEWS |1 +
bootstrap | 10 ++--
bootstrap.conf |6 +-
build-aux/extract-trace| 10 ++--
build-aux/general.m4sh | 16
build-aux/git-hooks/commit-msg |4 +-
build-aux/git-log-fix | 48 +++---
build-aux/ltmain.m4sh |6 +-
cfg.mk | 10 +
libtoolize.m4sh| 62 ++--
m4/libtool.m4 | 16
m4/ltdl.m4 |2 +-
tests/cdemo-undef.test |2 +-
tests/cmdline_wrap.at |2 +-
tests/darwin.at|2 +-
tests/defs.m4sh|2 +-
tests/getopt-m4sh.at |2 +-
tests/install.at |2 +-
tests/libtoolize.at| 14 +++---
tests/mdemo/Makefile.am|2 +-
tests/need_lib_prefix.at |2 +-
tests/sysroot.at |2 +-
tests/tagdemo-undef.test |2 +-
tests/testsuite.at |4 +-
25 files changed, 164 insertions(+), 153 deletions(-)
diff --git a/Makefile.am b/Makefile.am
index 31d286e..680ae3e 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -52,7 +52,7 @@ LT_M4SH = $(M4SH) -B
'$(srcdir)/$(m4sh_dir)'
lt__cd = CDPATH="$${ZSH_VERSION+.}$(PATH_SEPARATOR)" && cd
git_version_gen = '$(SHELL)' '$(aux_dir)/git-version-gen' '.tarball-version'
-rebuild = rebuild=:; revision=`$(lt__cd) $(srcdir) && $(git_version_gen) | sed
's,-.*$$,,g'`
+rebuild = rebuild=:; revision=`$(lt__cd) $(srcdir) && $(git_version_gen) | sed
's|-.*$$||g'`
# -- #
@@ -108,18 +108,18 @@ EXTRA_DIST += $(extract_trace) $(libtoolize_in)
$(libtoolize_m4sh) \
## because they must be static in distributed files, and not accidentally
## changed by configure running on the build machine.
bootstrap_edit = $(SED) \
- -e 's,@MACRO_VERSION\@,$(VERSION),g' \
- -e "s,@MACRO_REVISION\@,$$revision,g" \
- -e "s,@MACRO_SERIAL\@,$$serial,g" \
- -e 's,@PACKAGE\@,$(PACKAGE),g' \
- -e 's,@PACKAGE_BUGREPORT\@,$(PACKAGE_BUGREPORT),g' \
- -e 's,@PACKAGE_URL\@,$(PACKAGE_URL),g' \
- -e 's,@PACKAGE_NAME\@,$(PACKAGE_NAME),g' \
- -e "s,@package_revision\@,$$revision,g" \
- -e 's,@PACKAGE_STRING\@,$(PACKAGE_NAME) $(VERSION),g' \
- -e 's,@PACKAGE_TARNAME\@,$(PACKAGE),g' \
- -e 's,@PACKAGE_VERSION\@,$(VERSION),g' \
- -e 's,@VERSION\@,$(VERSION),g'
+ -e 's|@MACRO_VERSION\@|$(VERSION)|g' \
+ -e "s|@MACRO_REVISION\@|$$revision|g" \
+ -e "s|@MACRO_SERIAL\@|$$serial|g" \
+ -e 's|@PACKAGE\@|$(PACKAGE)|g' \
+ -e 's|@PACKAGE_BUGREPORT\@|$(PACKAGE_BUGREPORT)|g' \
+ -e 's|@PACKAGE_URL\@|$(PACKAGE_URL)|g' \
+ -e 's|@PACKAGE_NAME\@|$(PACKAGE_NAME)|g' \
+ -e "s|@package_revision\@|$$revision|g" \
+ -e 's|@PACKAGE_STRING\@|$(PACKAGE_NAME) $(VERSION)|g' \
+ -e 's|@PACKAGE_TARNAME\@|$(PACKAGE)|g' \
+ -e 's|@PACKAGE_VERSION\@|$(VERSION)|g' \
+ -e 's|@VERSION\@|$(VERSION)|g'
## ltmain.sh needs some additional editing to remove unsubstituted
## variable defaulting lines, because ltmain.sh never gets passed
@@ -210,8 +210,8 @@ $(lt_Makefile_am): $(ltdl_mk)
}; \
'$(SED)' -n '/^.. DO NOT REMOVE THIS LINE -- /,$$p' \
'$(ltdl_mk)' \
- |'$(SED)' -e 's,libltdl_,,; s,libltdl/,,; s,: li
Re: [PATCH 1/7] syntax-check: fix violations and implement sc_useless_quotes_in_case.
On 11/21/2011 07:47 AM, Gary V. Vaughan wrote:
> Contrary to popular belief, Bourne shell does not resplit case
> expressions after expansion, so if there are no shell unquoted
> shell metacharacters or whitespace, the quotes are useless.
> @@ -568,7 +568,7 @@ func_resolve_sysroot ()
> # store the result into func_replace_sysroot_result.
> func_replace_sysroot ()
> {
> - case "$lt_sysroot:$1" in
> + case $lt_sysroot:$1 in
>?*:"$lt_sysroot"*)
Likewise in the pattern expression; you could further change this to:
case $lt_sysroot:$1 in
?*:$lt_sysroot*)
--
Eric Blake ebl...@redhat.com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: [PATCH 3/7] tests: migrate tests/sh.test checks to syntax-checks.
Hi Gary. Just a quick nit (I haven't looked at the whole series, and not even at the whole patch in fact; sorry). On Monday 21 November 2011, Gary V wrote: > for file > do > - test -f $file || touch $file > + test -f "$file" || touch $file > What's the point of quoting file after `test -f' it it remains unquoted after `touch'? Regards, Stefano
Re: [PATCH 4/7] syntax-check: fix violations and implement sc_prohibit_bare_basename.
Hi Gary. Again, few quick nits here, probably incomplete.
On Monday 21 November 2011, Gary V wrote:
> * cfg.mk (sc_prohibit_bare_basename, sc_prohibit_basename_with_sed):
> Make sure not to go back to using occasional `|$basename' or
> `|$dirname' syntax.
> * build-aux/general.m4sh, build-aux/git-hooks/commit-msg,
> build-aux/ltmain.m4sh, build-aux/options-parser,
> tests/fcdemo-conf.test, tests/fcdemo-shared.test,
> tests/fcdemo-static.test, tests/libtoolize.at: Fix violations.
>
> Signed-off-by: Gary V. Vaughan
>
> diff --git a/build-aux/general.m4sh b/build-aux/general.m4sh
> index 1f44535..790f4e0 100644
> --- a/build-aux/general.m4sh
> +++ b/build-aux/general.m4sh
> @@ -70,15 +70,15 @@ lt_nl='
> '
> IFS=" $lt_nl"
>
> -dirname='s,/[^/]*$,,'
> -basename='s,^.*/,,'
> +dirname='s|/[^/]*$||'
> +basename='s|^.*/||'
>
What's the point of this change? If it's only stylistic, shouldn't it
be done in a separate patch?
> # func_dirname file append nondir_replacement
> # Compute the dirname of FILE. If nonempty, add APPEND to the result,
> # otherwise set result to NONDIR_REPLACEMENT.
> func_dirname ()
> {
> -func_dirname_result=`$ECHO "${1}" | $SED "$dirname"`
> +func_dirname_result=`$ECHO "${1}" |$SED "$dirname"`
>
Ditto, and for other similar changes as well.
> diff --git a/build-aux/ltmain.m4sh b/build-aux/ltmain.m4sh
> index 02ff034..b367ddd 100644
> --- a/build-aux/ltmain.m4sh
> +++ b/build-aux/ltmain.m4sh
> @@ -3042,7 +3042,7 @@ func_extract_archives ()
> $RM
> "unfat-$$/${darwin_base_archive}-${darwin_arch}/${darwin_base_archive}"
> done # $darwin_arches
> ## Okay now we've a bunch of thin objects, gotta fatten them up
> :)
> - darwin_filelist=`find unfat-$$ -type f ... -print | $SED -e
> "$basename" | sort -u`
> + darwin_filelist=`find unfat-$$ -type f ... -print | $SED $basename
> | sort -u`
>
Why this quote removal? It seems gratuitous -- even dangerous,
since `$basename' contains shell wildcards (`*' at least).
Regards,
Stefano
Re: [PATCH 5/7] syntax-check: fix violations and implement sc_useless_braces_in_variable_derefs.
On 11/21/2011 07:47 AM, Gary V. Vaughan wrote:
> Until now, libtool sources have used braced variable names
> seemingly at random! Almost always the braces are just noise, so
> remove all the unnecessary ones.
> * cfg.mk (sc_useless_braces_in_variable_derefs): New syntax
> check rule to ensure we only reintroduce braced variable
> dereferences if they are followed by a valid variable name
> character.
> build-aux/general.m4sh, build-aux/git-hooks/commit-msg,
> build-aux/ltmain.m4sh, build-aux/options-parser, configure.ac,
> libltdl/configure.ac, m4/libtool.m4, m4/ltdl.m4,
> m4/ltoptions.m4, tests/defs.m4sh, tests/demo-nopic.test,
> tests/depdemo/configure.ac, tests/flags.at, tests/link.test,
> tests/objectlist.test, tests/quote.test, tests/static.at: Remove
> spurious braces.
>
> +++ b/build-aux/ltmain.m4sh
> @@ -152,7 +152,7 @@ exec_cmd=
> # Append VALUE to the end of shell variable VAR.
> func_append ()
> {
> -eval "${1}=\$${1}\${2}"
> +eval "$1=\$$1\$2"
Not necessarily correct. One of the reasons for using ${1} in any m4
code that comprises the m4_define() definition of a macro is that $1 is
replaced by an argument by m4 in expanding the macro, while ${1} is
preserved unchanged through m4 expansion to be saved for the resulting
shell code. I fear that your global search-and-replace may have been
too eager in m4-related files, but haven't read it closely to check for
sure; even if it didn't, the stylistic convention of ${1} for shell
expansion vs. $1 for m4 expansion made the file slightly easier to maintain.
--
Eric Blake ebl...@redhat.com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: [PATCH 6/7] syntax-check: fix violations and implement sc_prohibit_test_const_follows_var.
On 11/21/2011 07:47 AM, Gary V. Vaughan wrote: > To safely use a non-literal first argument to `test', you must > always prepend a literal non-`-' character, but often the second > operand is a constant that doesn't begin with a `-' already, so > always use `test a = "$b"' instead of noisy `test "X$b" = Xa'. Not true. test a = "$b" is just as likely to trigger improper evaluation in buggy test(1) implementations as: test "$b" = a If you cannot guarantee the contents of "$b", then you MUST prefix both sides of the comparison with x or X. Conversely, if you CAN guarantee the contents of "$b" (for example, if you did b=$?, then you KNOW that b is a numeric tring with no problematic characters), then you might as well use the more idiomatic comparison of variable to constant. -- Eric Blake ebl...@redhat.com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
Re: [PATCH 6/7] syntax-check: fix violations and implement sc_prohibit_test_const_follows_var.
Hi Gary. Few more random nits... On Monday 21 November 2011, Gary V wrote: > To safely use a non-literal first argument to `test', you must > always prepend a literal non-`-' character, but often the second > operand is a constant that doesn't begin with a `-' already, so > always use `test a = "$b"' instead of noisy `test "X$b" = Xa'. > This seems "back-bending" to me, and slightly unclear to read. Also, it goes against the (unofficial) conventions of autoconf, which is to use either `test "x$b" = xa' or `test "x$b" = Xa'. Also ... > # Bootstrap this package from checked-out sources. > # Written by Gary V. Vaughan, 2010 > @@ -1760,7 +1760,7 @@ func_ifcontains () >;; > esac > > -test "$_G_status" -eq 0 || exit $_G_status > +test 0 -eq $_G_status || exit $_G_status > } ... changes like this seems overly paranoid, in case $_G_status is expected (as I surmise it is) to be a non-negative integer. And if this assumption stps to hold dur to a bug in your code, you are going to be bitten by much worse problem anyway: # $shell is either Solaris 1 0or AT&T ksh, Solaris 10 XPG4 sh, or # zsh 4.3.12. $ $shell -c 'exit t; echo foo'; echo status = $? status = 0 Regards, Stefano
Re: [PATCH 7/7] syntax-check: fix violations and implement sc_prohibit_sed_s_comma.
On 11/21/2011 07:47 AM, Gary V. Vaughan wrote: > I like to name temporary directories that I will remove shortly > with two leading commas so that they sort lexicographically at > the top of `ls' output. Now, `./configure > --prefix=`pwd`/,,inst' works again, for the first time in > several years. > Try to use `|' as the default separator wherever possible, > otherwise something else that doesn't occur in the substitution > expression. I'm in favor of this one. In particular, one of the reasons that autoconf documents | as superior to , is that | has to be shell-quoted to be used, while , does not, which means a user is more likely to have a filename containing comma than they are to have a filename containing a pipe. -- Eric Blake ebl...@redhat.com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
Re: [PATCH 2/7] syntax-check: fix violations and implement sc_useless_quotes_in_assignment.
Gary V. Vaughan wrote: [SNIP] diff --git a/bootstrap.conf b/bootstrap.conf index 6f0f0c3..c3491b5 100644 --- a/bootstrap.conf +++ b/bootstrap.conf @@ -77,13 +77,13 @@ gnulib_modules=' # Extra gnulib files that are not in modules, which override files of # the same name installed by other bootstrap tools. -gnulib_non_module_files="$gnulib_non_module_files"' +gnulib_non_module_files=$gnulib_non_module_files' doc/COPYINGv2 doc/fdl.texi [SNIP] hmm, origin is only with end apostrophe . Next one is # Extend the existing usage message -usage_message="$usage_message"' +usage_message=$usage_message' . May be exist more places. I did not check rest of the file. Roumen
Re: [PATCH 2/7] syntax-check: fix violations and implement sc_useless_quotes_in_assignment.
On 11/21/2011 01:59 PM, Roumen Petrov wrote: > Gary V. Vaughan wrote: > [SNIP] >> diff --git a/bootstrap.conf b/bootstrap.conf >> index 6f0f0c3..c3491b5 100644 >> --- a/bootstrap.conf >> +++ b/bootstrap.conf >> @@ -77,13 +77,13 @@ gnulib_modules=' >> >> # Extra gnulib files that are not in modules, which override files of >> # the same name installed by other bootstrap tools. >> -gnulib_non_module_files="$gnulib_non_module_files"' >> +gnulib_non_module_files=$gnulib_non_module_files' >> doc/COPYINGv2 >> doc/fdl.texi >> > [SNIP] > hmm, origin is only with end apostrophe . That's not a problem. It's merely changing: var="$expanded"'literal' to the equivalent var=$expanded'literal' where the literal portion includes newline characters. -- Eric Blake ebl...@redhat.com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
[PATCH 8/7] syntax-check: fix violations and implement sc_useless_quotes_in_case_branch.
Hi Eric,
On 21 Nov 2011, at 23:15, Eric Blake wrote:
> On 11/21/2011 07:47 AM, Gary V. Vaughan wrote:
>> Contrary to popular belief, Bourne shell does not resplit case
>> expressions after expansion, so if there are no shell unquoted
>> shell metacharacters or whitespace, the quotes are useless.
>> @@ -568,7 +568,7 @@ func_resolve_sysroot ()
>> # store the result into func_replace_sysroot_result.
>> func_replace_sysroot ()
>> {
>> - case "$lt_sysroot:$1" in
>> + case $lt_sysroot:$1 in
>> ?*:"$lt_sysroot"*)
>
> Likewise in the pattern expression; you could further change this to:
>
> case $lt_sysroot:$1 in
> ?*:$lt_sysroot*)
Good call, although narrowing the search down to eliminate false positives
is a lot trickier in this case!
I'm adding the following changeset to this series. Thanks!
Contrary to popular belief, Bourne shell does not resplit case
branch expressions after expansion, so if there are no unquoted
shell metacharacters or whitespace, the quotes are useless.
* cfg.mk (sc_useless_quotes_in_case_branch): New syntax-check
rule to ensure we don't reintroduce useless quoted case branch
expressions.
* bootstrap, build-aux/ltmain.m4sh, tests/quote.test: Remove
spurious quotes.
Signed-off-by: Gary V. Vaughan
---
bootstrap |2 +-
build-aux/ltmain.m4sh | 26 +-
cfg.mk| 16
tests/quote.test |8
4 files changed, 34 insertions(+), 18 deletions(-)
diff --git a/bootstrap b/bootstrap
index afdde66..49685f2 100755
--- a/bootstrap
+++ b/bootstrap
@@ -1782,7 +1782,7 @@ func_append_u ()
_G_delim=`expr "$2" : '\(.\)'`
case $_G_delim$_G_current_value$_G_delim in
- *"$2$_G_delim"*) ;;
+ *$2$_G_delim*) ;;
*) func_append "$@" ;;
esac
}
diff --git a/build-aux/ltmain.m4sh b/build-aux/ltmain.m4sh
index 8955838..5cd201f 100644
--- a/build-aux/ltmain.m4sh
+++ b/build-aux/ltmain.m4sh
@@ -569,7 +569,7 @@ func_resolve_sysroot ()
func_replace_sysroot ()
{
case $lt_sysroot:$1 in
- ?*:"$lt_sysroot"*)
+ ?*:$lt_sysroot*)
func_stripname "$lt_sysroot" '' "$1"
func_replace_sysroot_result='='$func_stripname_result
;;
@@ -895,7 +895,7 @@ func_to_tool_file ()
$debug_cmd
case ,$2, in
-*,"$to_tool_file_cmd",*)
+*,$to_tool_file_cmd,*)
func_to_tool_file_result=$1
;;
*)
@@ -4836,12 +4836,12 @@ func_mode_link ()
*-*-cygwin* | *-*-mingw* | *-*-pw32* | *-*-os2* | *-cegcc*)
testbindir=`$ECHO "$dir" | $SED 's*/lib$*/bin*'`
case :$dllsearchpath: in
- *":$dir:"*) ;;
+ *:$dir:*) ;;
::) dllsearchpath=$dir;;
*) func_append dllsearchpath ":$dir";;
esac
case :$dllsearchpath: in
- *":$testbindir:"*) ;;
+ *:$testbindir:*) ;;
::) dllsearchpath=$testbindir;;
*) func_append dllsearchpath ":$testbindir";;
esac
@@ -5895,7 +5895,7 @@ func_mode_link ()
if test -n "$shlibpath_var" && test -z "$avoidtemprpath" ; then
# Make sure the rpath contains only unique directories.
case $temp_rpath: in
- *"$absdir:"*) ;;
+ *$absdir:*) ;;
*) func_append temp_rpath "$absdir:" ;;
esac
fi
@@ -6122,7 +6122,7 @@ func_mode_link ()
if test -n "$add_shlibpath"; then
case :$compile_shlibpath: in
- *":$add_shlibpath:"*) ;;
+ *:$add_shlibpath:*) ;;
*) func_append compile_shlibpath "$add_shlibpath:" ;;
esac
fi
@@ -6136,7 +6136,7 @@ func_mode_link ()
test yes != "$hardcode_minus_L" &&
test yes = "$hardcode_shlibpath_var"; then
case :$finalize_shlibpath: in
- *":$libdir:"*) ;;
+ *:$libdir:*) ;;
*) func_append finalize_shlibpath "$libdir:" ;;
esac
fi
@@ -6156,7 +6156,7 @@ func_mode_link ()
add=-l$name
elif test yes = "$hardcode_shlibpath_var"; then
case :$finalize_shlibpath: in
- *":$libdir:"*) ;;
+ *:$libdir:*) ;;
*) func_append finalize_shlibpath "$libdir:" ;;
esac
add=-l$name
@@ -7307,7 +7307,7 @@ EOF
else
# Just accumulate the unique libdirs.
case
$hardcode_libdir_separator$hardcode_libdirs$hardcode_libdir_separator in
-
*"$hardcode_libdir_separator$libdir$hardcode_libdir_separator"*)
+ *$hardcode_libdir_separator$libdir$hardcode_libdir_separator*)
;;
*)
func_append hardcode_libdirs
"$hardcode_libdir_separator$libdir"
@@ -8034,7 +8034,7 @@ EOF
else
# Just accumulate the unique libdirs.
case
$hardcode_libdir_separator$hardcode_libdirs$hardcode_libdir_separator
Re: [PATCH 3/7] tests: migrate tests/sh.test checks to syntax-checks.
Hi Stefano, On 22 Nov 2011, at 02:52, Stefano Lattarini wrote: > Hi Gary. Just a quick nit (I haven't looked at the whole > series, and not even at the whole patch in fact; sorry). No apologies necessary, every little helps! Thank you. > On Monday 21 November 2011, Gary V wrote: >>for file >> do >> - test -f $file || touch $file >> + test -f "$file" || touch $file >> > What's the point of quoting file after `test -f' it it remains > unquoted after `touch'? Even though we know there is no whitespace in $file because of the for loop, there still might be other shell meta-characters in there. All uses of $file (including a bunch in the following lines) should be quoted correctly, but that is another patch. Cheers, -- Gary V. Vaughan (gary AT gnu DOT org)
Re: [PATCH 4/7] syntax-check: fix violations and implement sc_prohibit_bare_basename.
On 22 Nov 2011, at 02:59, Stefano Lattarini wrote:
> Hi Gary. Again, few quick nits here, probably incomplete.
Hi Stefano,
Thanks for taking a look again.
> On Monday 21 November 2011, Gary V wrote:
>> * cfg.mk (sc_prohibit_bare_basename, sc_prohibit_basename_with_sed):
>> Make sure not to go back to using occasional `|$basename' or
>> `|$dirname' syntax.
>> * build-aux/general.m4sh, build-aux/git-hooks/commit-msg,
>> build-aux/ltmain.m4sh, build-aux/options-parser,
>> tests/fcdemo-conf.test, tests/fcdemo-shared.test,
>> tests/fcdemo-static.test, tests/libtoolize.at: Fix violations.
>>
>> Signed-off-by: Gary V. Vaughan
>>
>> diff --git a/build-aux/general.m4sh b/build-aux/general.m4sh
>> index 1f44535..790f4e0 100644
>> --- a/build-aux/general.m4sh
>> +++ b/build-aux/general.m4sh
>> @@ -70,15 +70,15 @@ lt_nl='
>> '
>> IFS=" $lt_nl"
>>
>> -dirname='s,/[^/]*$,,'
>> -basename='s,^.*/,,'
>> +dirname='s|/[^/]*$||'
>> +basename='s|^.*/||'
>>
> What's the point of this change? If it's only stylistic, shouldn't it
> be done in a separate patch?
That leaked out of the prohibit_sed_s_comma patch later in the series, so
I've moved it back to the correct patch. Thanks.
>> # func_dirname file append nondir_replacement
>> # Compute the dirname of FILE. If nonempty, add APPEND to the result,
>> # otherwise set result to NONDIR_REPLACEMENT.
>> func_dirname ()
>> {
>> -func_dirname_result=`$ECHO "${1}" | $SED "$dirname"`
>> +func_dirname_result=`$ECHO "${1}" |$SED "$dirname"`
>>
> Ditto, and for other similar changes as well.
Not sure how that happened, maybe cut and pasting between various definitions.
Also fixed.
>> diff --git a/build-aux/ltmain.m4sh b/build-aux/ltmain.m4sh
>> index 02ff034..b367ddd 100644
>> --- a/build-aux/ltmain.m4sh
>> +++ b/build-aux/ltmain.m4sh
>> @@ -3042,7 +3042,7 @@ func_extract_archives ()
>>$RM
>> "unfat-$$/${darwin_base_archive}-${darwin_arch}/${darwin_base_archive}"
>> done # $darwin_arches
>> ## Okay now we've a bunch of thin objects, gotta fatten them up
>> :)
>> -darwin_filelist=`find unfat-$$ -type f ... -print | $SED -e
>> "$basename" | sort -u`
>> +darwin_filelist=`find unfat-$$ -type f ... -print | $SED $basename
>> | sort -u`
>>
> Why this quote removal? It seems gratuitous -- even dangerous,
> since `$basename' contains shell wildcards (`*' at least).
Yikes! Well that's embarrassing. Nicely caught, thanks.
Cheers,
--
Gary V. Vaughan (gary AT gnu DOT org)
Re: [PATCH 5/7] syntax-check: fix violations and implement sc_useless_braces_in_variable_derefs.
Hi Eric,
Thanks for the feedback.
On 22 Nov 2011, at 03:05, Eric Blake wrote:
> On 11/21/2011 07:47 AM, Gary V. Vaughan wrote:
>> Until now, libtool sources have used braced variable names
>> seemingly at random! Almost always the braces are just noise, so
>> remove all the unnecessary ones.
>> * cfg.mk (sc_useless_braces_in_variable_derefs): New syntax
>> check rule to ensure we only reintroduce braced variable
>> dereferences if they are followed by a valid variable name
>> character.
>> build-aux/general.m4sh, build-aux/git-hooks/commit-msg,
>> build-aux/ltmain.m4sh, build-aux/options-parser, configure.ac,
>> libltdl/configure.ac, m4/libtool.m4, m4/ltdl.m4,
>> m4/ltoptions.m4, tests/defs.m4sh, tests/demo-nopic.test,
>> tests/depdemo/configure.ac, tests/flags.at, tests/link.test,
>> tests/objectlist.test, tests/quote.test, tests/static.at: Remove
>> spurious braces.
>>
>> +++ b/build-aux/ltmain.m4sh
>
>> @@ -152,7 +152,7 @@ exec_cmd=
>> # Append VALUE to the end of shell variable VAR.
>> func_append ()
>> {
>> -eval "${1}=\$${1}\${2}"
>> +eval "$1=\$$1\$2"
>
> Not necessarily correct. One of the reasons for using ${1} in any m4
> code that comprises the m4_define() definition of a macro is that $1 is
> replaced by an argument by m4 in expanding the macro, while ${1} is
> preserved unchanged through m4 expansion to be saved for the resulting
> shell code. I fear that your global search-and-replace may have been
> too eager in m4-related files, but haven't read it closely to check for
> sure; even if it didn't, the stylistic convention of ${1} for shell
> expansion vs. $1 for m4 expansion made the file slightly easier to maintain.
I went back and forth on this myself.
In the end, I didn't want to exclude .m4sh tests from the syntax check
because there's a ton of our messiest shell code in those files, which
is most of the reason for adding the new syntax-checks in the first place.
While it's true that the shell braces do prevent M4 from substituting its
own positional parameters, the vast majority of uses in our .m4sh code are
wrapped in M4SH_VERBATIM([[...]]), so they are left unexpanded for the
shell even with the braces removed. In the handful of cases where we had
previously saved positional parameters from being prematurely expanded by
M4 using braces, again I'm reluctant to exclude the whole file from the
syntax-check, so I used quadrigraphs instead to keep all the tools happy :)
Cheers,
--
Gary V. Vaughan (gary AT gnu DOT org)
Re: [PATCH 6/7] syntax-check: fix violations and implement sc_prohibit_test_const_follows_var.
Hi Eric, On 22 Nov 2011, at 03:07, Eric Blake wrote: > On 11/21/2011 07:47 AM, Gary V. Vaughan wrote: >> To safely use a non-literal first argument to `test', you must >> always prepend a literal non-`-' character, but often the second >> operand is a constant that doesn't begin with a `-' already, so >> always use `test a = "$b"' instead of noisy `test "X$b" = Xa'. > > Not true. > > test a = "$b" > > is just as likely to trigger improper evaluation in buggy test(1) > implementations as: > > test "$b" = a :-o For real? On non-museum pieces? I tested a bunch of /bin/sh, bash, ksh and zsh versions that I have easy access to, and for all of them, the only way to upset test with leading hyphens is in the first argument. > If you cannot guarantee the contents of "$b", then you MUST prefix both > sides of the comparison with x or X. Conversely, if you CAN guarantee > the contents of "$b" (for example, if you did b=$?, then you KNOW that b > is a numeric tring with no problematic characters), then you might as > well use the more idiomatic comparison of variable to constant. I don't suppose you can point me at a shell that chokes or fails on: test a != -b || echo bug ? Or at least some reliable documentation that shows we're not dealing with outdated dogma here? I'll postpone pushing this patch until we get to the bottom of the issue though. Cheers, -- Gary V. Vaughan (gary AT gnu DOT org)
Re: [PATCH 6/7] syntax-check: fix violations and implement sc_prohibit_test_const_follows_var.
Hi Stefano, On 22 Nov 2011, at 03:13, Stefano Lattarini wrote: > Hi Gary. Few more random nits... Thanks ;) > On Monday 21 November 2011, Gary V wrote: >> To safely use a non-literal first argument to `test', you must >> always prepend a literal non-`-' character, but often the second >> operand is a constant that doesn't begin with a `-' already, so >> always use `test a = "$b"' instead of noisy `test "X$b" = Xa'. >> > This seems "back-bending" to me, and slightly unclear to read. Also, > it goes against the (unofficial) conventions of autoconf, which is > to use either `test "x$b" = xa' or `test "x$b" = Xa'. I was unable to find any shells that choke on: test a != -b || echo bug Where it's easy to upset test with: test -b != a > Also ... > >> # Bootstrap this package from checked-out sources. >> # Written by Gary V. Vaughan, 2010 >> @@ -1760,7 +1760,7 @@ func_ifcontains () >> ;; >> esac >> >> -test "$_G_status" -eq 0 || exit $_G_status >> +test 0 -eq $_G_status || exit $_G_status >> } > ... changes like this seems overly paranoid, in case $_G_status is > expected (as I surmise it is) to be a non-negative integer. And > if this assumption stps to hold dur to a bug in your code, you are > going to be bitten by much worse problem anyway: Well, in addition to saving a few characters of typing, and being consistent with other uses of test after this patch, it also prevents the syntax-check from triggering. I certainly wouldn't expect any difference in behaviour either way, even on buggy shells/test implementations. > # $shell is either Solaris 1 0or AT&T ksh, Solaris 10 XPG4 sh, or > # zsh 4.3.12. > $ $shell -c 'exit t; echo foo'; echo status = $? > status = 0 Cheers, -- Gary V. Vaughan (gary AT gnu DOT org)
