patchshell 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 <naota@elisp.net>
---

Testing on FreeBSD failed because of this "bash-ism". After applying this patch, I've verified the test to pass on FreeBSD. (and it worked well also with GNU sed)

 t/t5560-http-backend-noserver.sh |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

Am 9/5/2011 7:11, schrieb Naohiro Aota:

These are not bashism, but features require by POSIX.

I'd rather suspect that the failures are not because FreeBSD sh does not have ${%} or ${#}, but rather that it interprets the meaning of the backslash in this case in a way different from other shells.

What happens if you write these as

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

i.e., drop the double-quotes?

-- Hannes

Johannes Sixt <j.sixt@viscovery.net> writes:

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.

Thanks.

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

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.

Other shells seem to parse the pattern following % and # in a different mode, which keeps the quoting ability of the backslash even inside double-quotes... (And to me it looks like those shells are wrong.)

Without double-quotes, backslashes (that are not themselves quoted) are always removed and give the subsequent character its literal meaning. Hence, in my version, the question mark would unambiguously (I think) act as a literal rather than a wildcard.

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

-- Hannes

Johannes Sixt <j.sixt@viscovery.net> writes:

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.

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.

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

They are not the same.

First of all, the value of $IFS is irrelevant whether or not you need double-quotes on the RHS of an assignment, because it is purely a syntactic matter; $IFS plays no role during syntax analysis. It is only the presence of white-space that sometimes[*] requires quoting of some form.

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

$ foo="a\xb" env | grep foo; foo=a\xb env | grep foo foo=a\xb foo=axb

But it is the same elsewhere in a command:

$ echo "a\xb"; echo a\xb a\xb axb

The reason is that a backslash inside double-quotes remains as a literal character when it is not followed by a special character, whereas outside double-quotes an unquoted backslash is always removed.

[*] No quoting is required in cases like this: VAR=$(echo foo)

Good point, and I shall prefer this solution as well.

-- Hannes

Am 9/5/2011 10:16, schrieb Johannes Sixt:

I should have said: "The difference that I can see immediately is...". I suspect there are other subtle differences that are not so obvious to me.

-- Hannes

Ah, seems I was misundertanding much of things :(

Johannes Sixt <j.sixt@viscovery.net> writes:

not worked, even increased the number of failure...

Junio C Hamano <gitster@pobox.com> writes:

Thanks, I'll try this.

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.

This worked on my box. hm, then the problem should be in /bin/sh

Naohiro Aota <naota@elisp.net> writes:

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.

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

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

--- 8< ---
From: Johannes Sixt <j6t@kdbg.org>
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 <j6t@kdbg.org>
---
 Documentation/CodingGuidelines |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

Johannes Sixt <j.sixt@viscovery.net> writes:

I know it is in POSIX, but not in the subset we allowed so far. I do not recall the details offhand, but we must have seen some shell that lacked it or something.

Johannes Sixt <j.sixt@viscovery.net> writes:

I would prefer to play it safe at least for now, especially before 1.7.7 ships.

 Documentation/CodingGuidelines |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

From: Brandon Casey <drafnel@gmail.com>

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

On 09/05/2011 02:09 AM, Junio C Hamano wrote:

Perhaps we should add a test for this shell feature.

-Brandon

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

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.

make 3.80 produces the desired results, as does 3.77 which I have installed on an old machine. GNU make 3.82 seems to be the latest but I don't have access to it. If anyone does, I'd appreciate if you could test.

-Brandon

On 09/06/2011 02:09 PM, Brandon Casey wrote:

On 09/06/2011 02:32 PM, Brandon Casey wrote:

Just a clarification, I didn't mean you'd actually be able to test the patch for correctness, but the above would at least allow you to stress the code path.

But, with the Makefile in its current form (patch or no patch) the above still works. Setting SHELL_PATH=/bin/false produces the desired error message.

There still appears to be a bug in make 3.81 which is triggered when using an ancient shell, it just manifests itself in a different way using our current Makefile. Right now, make 3.81 will enter an infinite loop when it tries to include the GIT-VERSION-FILE. When something like /bin/sh on Solaris processes the GIT-VERSION-GEN script, it produces the following incorrect string in the GIT-VERSION-FILE:

   GIT_VERSION = $(expr $(echo $(git describe --match v[0-9]* --abbrev=4 HEAD 2>/dev/null) | sed -e s/-/./g) : v*\(.*\))

which then becomes part of the Makefile when GIT-VERSION-FILE is included on line 264. GNU make then begins to print the following to the terminal repeatedly:

   GIT_VERSION = $(expr $(echo $(git describe --match v[0-9]* --abbrev=4 HEAD 2>/dev/null) | sed -e s/-/./g) : v*\(.*\))

GIT-VERSION-FILE should really have a dependency on shell_compatibility_test since it calls GIT-VERSION-GEN which may use shell features that are not provided by the configured shell. If that dependency is added so that the GIT-VERSION-FILE rule looks like this:

   GIT-VERSION-FILE: shell_compatibility_test FORCE
   	@$(SHELL_PATH) ./GIT-VERSION-GEN
   -include GIT-VERSION-FILE

_then_, we get the behavior I described originally, where

   make SHELL_PATH=/bin/false

sends the make process into an infinite loop, with no output to the terminal.

Either way, with GNU make 3.81, you get an infinite loop when you use a shell that should trigger our error message.

-Brandon

Brandon Casey <casey@nrlssc.navy.mil> writes:

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.

On 09/06/2011 03:01 PM, Junio C Hamano wrote:

Ah, I didn't read through closely enough to notice that the above syntax was not also an issue, as was mentioned in the original email.

Sorry for the noise.

-Brandon

Brandon Casey <casey@nrlssc.navy.mil> writes:

Perhaps

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

instead?

On 09/06/2011 03:03 PM, Junio C Hamano wrote:

Looks right.

Naohiro, can you test?  Or someone else with FreeBSD?

make should produce an error message like this:

   gmake: *** [please_set_SHELL_PATH_to_a_more_modern_shell] Error 1

-Brandon