shell portability: Use sed instead of non-portable variable expansion

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

Signed-off-by Naohiro Aota <[email protected]>

---

Testing on FreeBSD failed because of this "bash-ism".

t/t5560-http-backend-noserver.sh test_have_prereq MINGW && export GREP_OPTIONS=-U · line 13

Quoted 4 lines

run_backend() {

t/t5560-http-backend-noserver.sh +0 -2

 	echo "$2" |
-	QUERY_STRING="${1#*\?}" \
-	PATH_TRANSLATED="$HTTPD_DOCUMENT_ROOT_PATH/${1%%\?*}" \

What happens if you write these as

	QUERY_STRING=${1#*\?} \
	PATH_TRANSLATED=$HTTPD_DOCUMENT_ROOT_PATH/${1%%\?*} \

i.e., drop the double-quotes?

Quoted 7 lines · Johannes Sixt

t/t5560-http-backend-noserver.sh test_have_prereq MINGW && export GREP_OPTIONS=-U · line 13

Quoted 4 lines

run_backend() {

t/t5560-http-backend-noserver.sh +0 -2

 	echo "$2" |
-	QUERY_STRING="${1#*\?}" \
-	PATH_TRANSLATED="$HTTPD_DOCUMENT_ROOT_PATH/${1%%\?*}" \

What happens if you write these as

Quoted 4 lines

	QUERY_STRING=${1#*\?} \
	PATH_TRANSLATED=$HTTPD_DOCUMENT_ROOT_PATH/${1%%\?*} \

i.e., drop the double-quotes?

Interesting. Your conjecture is that the shell may be dropping the backslash inside dq context when it does not understand what follows the backslash, i.e. "\?" -> "?", losing the quote. I find it very plausible.

If that is the case, either the above or my [?] would work it around, I would think.

Actually, it's the opposite: Within double-quotes, a backslash is only removed when the next character has a special meaning (essentially $, `, ", \), otherwise, it remains and loses its quoting ability. This means, that the backslash would remain as a literal character in our patterns on the right of % or #, and they would not work anymore as intended.

Quoted 2 lines

If that is the case, either the above or my [?] would work it around, I would think.

[?] instead of \? is certainly also worth a try.

Quoted 5 lines · Johannes Sixt

Actually, it's the opposite: Within double-quotes, a backslash is only removed when the next character has a special meaning (essentially $, `, ", \), otherwise, it remains and loses its quoting ability. This means, that the backslash would remain as a literal character in our patterns on the right of % or #, and they would not work anymore as intended.

That's strange...

I thought that VAR=<any string without $IFS character in it> would behave identically to VAR="<the same string as above>". You seem to be saying that they should act differently.

Quoted 1 line

[?] instead of \? is certainly also worth a try.

I obviously agree. Besides, [?] would sidestep the tricky backslash vs double quote issue entirely, so it would be a more robust solution to leave it around than "sometimes you need to avoid double-quote and some other times you would need double-quote" for other people to mimic writing tests later.

The most visible difference is a backslash that is followed by a character that is not special:

What happens if you write these as

	QUERY_STRING=${1#*\?} \
	PATH_TRANSLATED=$HTTPD_DOCUMENT_ROOT_PATH/${1%%\?*} \

i.e., drop the double-quotes?

Quoted 2 lines · Naohiro Aota

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.

t/t5560-http-backend-noserver.sh +0 -0

index 0ad7ce0..c8bbacc 100755

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

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

Am 9/5/2011 9:09, schrieb Junio C Hamano:

Quoted 2 lines

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

Actually, no. It is perfectly valid POSIX. So we would need this patch.

--- 8< ---
From: Johannes Sixt <[email protected]>
Subject: [PATCH] CodingGuidelines: ${#parameter} is POSIX and should be allowed

See http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_06_02.

Signed-off-by Johannes Sixt <[email protected]>

---

Am 9/5/2011 9:09, schrieb Junio C Hamano:

Quoted 2 lines

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

Actually, no. It is perfectly valid POSIX. So we would need this patch.

Quoted 2 lines · Naohiro Aota

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.

From: Brandon Casey <[email protected]>

Add an entry to the please_set_SHELL_PATH_to_a_more_modern_shell target which tests whether the shell supports ${parameter%word} expansion. I assume this one test is enough to indicate whether the shell supports the entire family of prefix and suffix removal syntax:

   ${parameter%word}
   ${parameter%%word}
   ${parameter#word}
   ${parameter##word}

FreeBSD, for one, has a /bin/sh that, apparently, supports $() notation but not the above prefix/suffix removal notation.
---

Quoted 5 lines · Junio C Hamano

Quoted 2 lines · Naohiro Aota

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

Quoted 8 lines

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.

Perhaps we should add a test for this shell feature.

(quoted diff) +0 -1

-Brandon
 
 Makefile |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

Makefile +1 -0

index 8d6d451..46d9c5d 100644
@@ -1738,6 +1738,7 @@ endif
 
 please_set_SHELL_PATH_to_a_more_modern_shell:
 	@$$(:)
+	@foo=bar_suffix && test bar = "$${foo%_*}"
 
 shell_compatibility_test: please_set_SHELL_PATH_to_a_more_modern_shell
 

FYI:
It should be possible to test this patch on a modern system by doing something like:

make SHELL_PATH=/bin/false

and you should see something like this:

make: *** [please_set_SHELL_PATH_to_a_more_modern_shell] Error 1

But beware, GNU make 3.81 seems to have a bug which sends it into an infinite loop.

Quoted 10 lines · Brandon Casey

Makefile +1 -0

index 8d6d451..46d9c5d 100644
@@ -1738,6 +1738,7 @@ endif
 
 please_set_SHELL_PATH_to_a_more_modern_shell:
 	@$$(:)
+	@foo=bar_suffix && test bar = "$${foo%_*}"
 

Makefile endif · line 1743

Quoted 1 line

Makefile +0 -0

 shell_compatibility_test: please_set_SHELL_PATH_to_a_more_modern_shell

Perhaps

	@foo='bar?suffix' && test bar = "$${foo%\?*}"

instead?

From: Brandon Casey <[email protected]>

Add an entry to the please_set_SHELL_PATH_to_a_more_modern_shell target which tests whether the shell supports ${parameter%word} expansion. I assume this one test is enough to indicate whether the shell supports the entire family of prefix and suffix removal syntax:

   ${parameter%word}
   ${parameter%%word}
   ${parameter#word}
   ${parameter##word}

FreeBSD, for one, has a /bin/sh that, apparently, supports $() notation but not the above prefix/suffix removal notation.

Quoted 12 lines · Brandon Casey

From: Brandon Casey <[email protected]>

Add an entry to the please_set_SHELL_PATH_to_a_more_modern_shell target which tests whether the shell supports ${parameter%word} expansion. I assume this one test is enough to indicate whether the shell supports the entire family of prefix and suffix removal syntax:

   ${parameter%word}
   ${parameter%%word}
   ${parameter#word}
   ${parameter##word}

Quoted 2 lines

FreeBSD, for one, has a /bin/sh that, apparently, supports $() notation but not the above prefix/suffix removal notation.

My reading of the later part of the thread you are basing the above is somewhat different from your diagnosis. The funny seems to happen only when there is a backslash-quoted glob special inside double-quotes (e.g. "${parameter%\?*}") and the same shell does not seem to be choking on many prefix/suffix expansion used in other test scripts.