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

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

[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. 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(-)

diff --git a/t/t5560-http-backend-noserver.sh b/t/t5560-http-backend-noserver.sh
index 0ad7ce0..c8237ef 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=$(echo "$1"|sed -e 's/^[^?]*?\(.*\)$/\1/') \
+	PATH_TRANSLATED="$HTTPD_DOCUMENT_ROOT_PATH/$(echo "$1"|sed -e 's/^\([^?]*\)?.*$/\1/')" \
 	git http-backend >act.out 2>act.err
 }
 
-- 
1.7.6

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

[Johannes Sixt]

Am 9/5/2011 7:11, schrieb 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. 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".

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.

>  run_backend() {
>  	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?

-- Hannes

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

[Junio C Hamano]

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

>>  run_backend() {
>>  	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?

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.

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

[Johannes Sixt]

Am 9/5/2011 9:15, schrieb Junio C Hamano:
> Johannes Sixt <j.sixt@viscovery.net> writes:
> 
>>>  run_backend() {
>>>  	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?
> 
> 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.

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.

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

-- Hannes

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

[Junio C Hamano]

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

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

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

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.

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

[Johannes Sixt]

Am 9/5/2011 9:45, schrieb Junio C Hamano:
> Johannes Sixt <j.sixt@viscovery.net> writes:
> 
>> 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.

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)

>> [?] 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.

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

-- Hannes

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

[Johannes Sixt]

Am 9/5/2011 10:16, schrieb Johannes Sixt:
> The most visible difference is a backslash that is followed by a character
> that is not special:

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

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

[Naohiro Aota]

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

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

[Junio C Hamano]

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
 }
 

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

[Johannes Sixt]

Am 9/5/2011 9:09, schrieb Junio C Hamano:
> 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 <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(-)

diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
index fe1c1e5..df0b620 100644
--- a/Documentation/CodingGuidelines
+++ b/Documentation/CodingGuidelines
@@ -52,8 +52,6 @@ For shell scripts specifically (not exhaustive):
 
    - No shell arrays.
 
-   - No strlen ${#parameter}.
-
    - No pattern replacement ${parameter/pattern/string}.
 
  - We use Arithmetic Expansion $(( ... )).
-- 
1.7.7.rc0.211.g5ac7e

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

[Junio C Hamano]

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

> Am 9/5/2011 9:09, schrieb Junio C Hamano:
>> 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.

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.

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

[Junio C Hamano]

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

> Am 9/5/2011 9:09, schrieb Junio C Hamano:
>> 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 <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>
> ---

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(-)

diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
index fe1c1e5..594fb76 100644
--- a/Documentation/CodingGuidelines
+++ b/Documentation/CodingGuidelines
@@ -52,7 +52,7 @@ For shell scripts specifically (not exhaustive):
 
    - No shell arrays.
 
-   - No strlen ${#parameter}.
+   - No strlen ${#parameter} (even though it is in POSIX).
 
    - No pattern replacement ${parameter/pattern/string}.
 

[PATCH] Makefile: abort on shells that do not support ${parameter%word} expansion

[Brandon Casey]

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

Perhaps we should add a test for this shell feature.

-Brandon

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

diff --git a/Makefile b/Makefile
index 8d6d451..46d9c5d 100644
--- a/Makefile
+++ b/Makefile
@@ -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
 
-- 
1.7.6.1

Re: [PATCH] Makefile: abort on shells that do not support ${parameter%word} expansion

[Brandon Casey]

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:
> 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:
>> 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.
> 
> Perhaps we should add a test for this shell feature.
> 
> -Brandon
> 
>  Makefile |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index 8d6d451..46d9c5d 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -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
>  

Re: [PATCH] Makefile: abort on shells that do not support ${parameter%word} expansion

[Brandon Casey]

On 09/06/2011 02:32 PM, Brandon Casey wrote:
> 
> 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.

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

Re: [PATCH] Makefile: abort on shells that do not support ${parameter%word} expansion

[Junio C Hamano]

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

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

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.

Re: [PATCH] Makefile: abort on shells that do not support ${parameter%word} expansion

[Brandon Casey]

On 09/06/2011 03:01 PM, Junio C Hamano wrote:
> Brandon Casey <casey@nrlssc.navy.mil> writes:
> 
>> 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.
> 
> 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.

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

Re: [PATCH] Makefile: abort on shells that do not support ${parameter%word} expansion

[Junio C Hamano]

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

> diff --git a/Makefile b/Makefile
> index 8d6d451..46d9c5d 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -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

Perhaps

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

instead?

Re: [PATCH] Makefile: abort on shells that do not support ${parameter%word} expansion

[Brandon Casey]

On 09/06/2011 03:03 PM, Junio C Hamano wrote:
> Brandon Casey <casey@nrlssc.navy.mil> writes:
> 
>> diff --git a/Makefile b/Makefile
>> index 8d6d451..46d9c5d 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -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
> 
> Perhaps
> 
> 	@foo='bar?suffix' && test bar = "$${foo%\?*}"
> 
> instead?

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