Re: [PATCH] shell portability: Use sed instead of non-portable variable expansion
From: Naohiro Aota <hidden>
Date: 2016-06-15 22:51:58
Ah, seems I was misundertanding much of things :( Johannes Sixt [off-list ref] writes:
What happens if you write these as
QUERY_STRING=${1#*\?} \
PATH_TRANSLATED=$HTTPD_DOCUMENT_ROOT_PATH/${1%%\?*} \
i.e., drop the double-quotes?not worked, even increased the number of failure... Junio C Hamano [off-list ref] writes:
Naohiro Aota [off-list ref] writes:quoted
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.
Thanks, I'll try this.
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} &&
I've tried t[0-9]{4}-*.sh and all of them passed. (t9010 had some known
breakages) yeah, my patch was taking wrong way.
quoted hunk
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.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 }
This worked on my box. hm, then the problem should be in /bin/sh