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

[Naohiro Aota <naota@elisp.net>]

Ah, seems I was misundertanding much of things :(

Johannes Sixt <j.sixt@viscovery.net> 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 <gitster@pobox.com> writes:

> 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.

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.

> 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