Re: [PATCH] shell portability: Use sed instead of non-portable variable expansion

Naohiro Aota [off-list ref] writes:

Variable expansions like "${foo#bar}" or "${foo%bar}" doesn't work on
shells like FreeBSD sh and they made the test to fail.

Sorry, I do appreciate the effort, but a patch like this takes us in the
wrong direction.

While we do not allow blatant bashisms like ${parameter:offset:length}
(substring expansion), ${parameter/pattern/string} (pattern substitution),
"local" variables, "function" noiseword, and shell arrays in our shell
scripts, the two kinds of substitution you quoted above are purely POSIX,
and our coding guideline does allow them to be used in the scripts.

Even though you may be able to rewrite trivial cases easily in some
scripts (either tests or Porcelain), some Porcelain scripts we ship
(e.g. "git bisect", "git stash", "git pull", etc.) do use these POSIX
constructs, and we do not want to butcher them with extra forks and
reduced readability.

Please use $SHELL_PATH and point to a POSIX compliant shell on your
platform instead. "make test" should pick it up and pass it down to
t/Makefile to be used when it runs these test scripts.

Besides, even inside t/ directory, there are many other instances of these
prefix/postfix substitution, not just 5560. Do the following tests pass on
your box without a similar patch?

$ git grep -n -e '\${[^}]*[#%]' -- t/\*.sh
t/t1410-reflog.sh:33:	aa=${1%??????????????????????????????????????} zz=${1#??}
t/t1410-reflog.sh:38:	aa=${1%??????????????????????????????????????} zz=${1#??}
t/t2030-unresolve-info.sh:125:	rerere_id=${rerere_id%/postimage} &&
t/t2030-unresolve-info.sh:151:	rerere_id=${rerere_id%/postimage} &&
t/t5560-http-backend-noserver.sh:12:	QUERY_STRING="${1#*\?}" \
t/t5560-http-backend-noserver.sh:13:	PATH_TRANSLATED="$HTTPD_DOCUMENT_ROOT_PATH/${1%%\?*}" \
t/t6050-replace.sh:124:     aa=${HASH2%??????????????????????????????????????} &&
t/t9010-svn-fe.sh:17:		printf "%s\n" "K ${#property}" &&
t/t9010-svn-fe.sh:19:		printf "%s\n" "V ${#value}" &&
t/t9010-svn-fe.sh:30:	printf "%s\n" "Text-content-length: ${#text}" &&
t/t9010-svn-fe.sh:31:	printf "%s\n" "Content-length: $((${#text} + 10))" &&
t/test-lib.sh:838:		test_results_path="$test_results_dir/${0%.sh}-$$.counts"
t/test-lib.sh:1047:this_test=${0##*/}
t/test-lib.sh:1048:this_test=${this_test%%-*}
t/valgrind/analyze.sh:98:				test $output = ${output%.message} &&

Looking at the above output, I suspect that it _might_ be that your shell
is almost POSIX but does not handle the backslash-quoted question mark
correctly or something silly like that, in which case a stupid patch like
the attached might be an acceptable compromise, until the shell is fixed.

By the way, t9010 uses ${#parameter} (strlen) which is bashism we forbid,
and it needs to be rewritten (David CC'ed).

Thanks.

diff --git a/t/t5560-http-backend-noserver.sh b/t/t5560-http-backend-noserver.sh
index 0ad7ce0..c8bbacc 100755
--- a/t/t5560-http-backend-noserver.sh
+++ b/t/t5560-http-backend-noserver.sh

@@ -9,8 +9,8 @@ test_have_prereq MINGW && export GREP_OPTIONS=-U
 
 run_backend() {
 	echo "$2" |
-	QUERY_STRING="${1#*\?}" \
-	PATH_TRANSLATED="$HTTPD_DOCUMENT_ROOT_PATH/${1%%\?*}" \
+	QUERY_STRING="${1#*[?]}" \
+	PATH_TRANSLATED="$HTTPD_DOCUMENT_ROOT_PATH/${1%%[?]*}" \
 	git http-backend >act.out 2>act.err
 }