From: Junio C Hamano <hidden> Date: 2016-06-15 22:51:58
Naohiro Aota [off-list ref] 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.
From: Brandon Casey <redacted>
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 [off-list ref] writes:
quoted
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(-)
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:
quoted hunk
From: Brandon Casey <redacted>
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:
quoted
Naohiro Aota [off-list ref] writes:
quoted
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(-)
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