Re: [PATCH] shell portability: Use sed instead of non-portable variable expansion
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Sep 5, 2011, 07:09 UTC
- Message-ID
- <7vbouzxy7g.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <8762l73758.fsf@elisp.net>
Naohiro Aota <naota@elisp.net> 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 }