threads / patch / 13500
Subject: [PATCH 0/4] freebsd portability fixes
Jeff KingI've been setting up auto-builders for a few platforms to try to catch portability problems early. Unfortunately, it was made harder by the fact that the _current_ test scripts don't pass on all platforms.
With these patches, I can successfully run all test scripts from the current 'master' on FreeBSD 6.1 (some of them probably affect OS X, too -- I recall somebody complaining about the '! foo | bar' construct recently).
-Peff
Jeff King[PATCH 1/4] fix bsd shell negation
On some shells (notably /bin/sh on FreeBSD 6.1), the construct
! foo | bar
does not negate the exit value of the pipeline, but rather of 'foo', producing the opposite of the expected value. We can work around this by specifying it as
! (foo | bar)
Signed-off-by: Jeff King <peff@peff.net> --- git-rebase.sh | 2 +- t/t3400-rebase.sh | 4 ++-- t/t3700-add.sh | 6 +++--- t/t7501-commit.sh | 2 +- 4 files changed, 7 insertions(+), 7 deletions(-)
git-rebase.sh, t/t3400-rebase.sh, t/t3700-add.sh, t/t7501-commit.sh
diff --git a/git-rebase.sh b/git-rebase.sh index 9b13b83..fbb0f28 100755 --- a/git-rebase.sh +++ b/git-rebase.sh @@ -353,7 +353,7 @@ orig_head=$branch mb=$(git merge-base "$onto" "$branch") if test "$upstream" = "$onto" && test "$mb" = "$onto" && # linear history? - ! git rev-list --parents "$onto".."$branch" | grep " .* " > /dev/null + ! (git rev-list --parents "$onto".."$branch" | grep " .* ") > /dev/null then # Lazily switch to the target branch if needed... test -z "$switch_to" || git checkout "$switch_to" diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh index 496f4ec..fdad7da 100755 --- a/t/t3400-rebase.sh +++ b/t/t3400-rebase.sh @@ -44,13 +44,13 @@ test_expect_success 'rebase against master' ' test_expect_success \ 'the rebase operation should not have destroyed author information' \ - '! git log | grep "Author:" | grep "<>"' + '! (git log | grep "Author:" | grep "<>")' test_expect_success 'rebase after merge master' ' git reset --hard topic && git merge master && git rebase master && - ! git show | grep "^Merge:" + ! (git show | grep "^Merge:") ' test_expect_success 'rebase of history with merges is linearized' ' diff --git a/t/t3700-add.sh b/t/t3700-add.sh index 287e058..68c5dde 100755 --- a/t/t3700-add.sh +++ b/t/t3700-add.sh @@ -81,17 +81,17 @@ test_expect_success '.gitignore test setup' ' test_expect_success '.gitignore is honored' ' git add . && - ! git ls-files | grep "\\.ig" + ! (git ls-files | grep "\\.ig") ' test_expect_success 'error out when attempting to add ignored ones without -f' ' ! git add a.?? && - ! git ls-files | grep "\\.ig" + ! (git ls-files | grep "\\.ig") ' test_expect_success 'error out when attempting to add ignored ones without -f' ' ! git add d.?? && - ! git ls-files | grep "\\.ig" + ! (git ls-files | grep "\\.ig") ' test_expect_success 'add ignored ones with -f' ' diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh index c0288f3..89710af 100755 --- a/t/t7501-commit.sh +++ b/t/t7501-commit.sh @@ -41,7 +41,7 @@ test_expect_success \ test_expect_success \ "using paths with --interactive" \ "echo bong-o-bong >file && - ! echo 7 | git-commit -m foo --interactive file" + ! (echo 7 | git-commit -m foo --interactive file)" test_expect_success \ "using invalid commit with -C" \
-- 1.5.5.1.296.gf618c
Junio C HamanoRe: [PATCH 1/4] fix bsd shell negation
Jeff King <peff@peff.net> writes:
> On some shells (notably /bin/sh on FreeBSD 6.1), the > construct > > ! foo | bar > > does not negate the exit value of the pipeline, but rather > of 'foo', producing the opposite of the expected value. We > can work around this by specifying it as > > ! (foo | bar)
After applying this patch, we still seem to have fair number of hits:
$ git grep -n -e '![^(]* |' -- '*.sh'
t/t5302-pack-index.sh:68: ! echo "$msg" | grep "pack too large .* off_t" t/t7002-grep.sh:111: ! git grep -c test $H | grep -q /dev/null t/t7600-merge.sh:378: if ! grep "^ file | *2 +-$" diffstat.txt t/t7600-merge.sh:392: if ! grep "^ file | *2 +-$" diffstat.txt t/t9400-git-cvsserver-server.sh:160: ! cat request-anonymous | t/t9400-git-cvsserver-server.sh:169: ! cat request-anonymous | t/t9400-git-cvsserver-server.sh:187: ! cat request-anonymous |
Didn't you have trouble with them, or the tests themselves are broken in some other way that these did not triggger?
Jeff KingRe: [PATCH 1/4] fix bsd shell negation
On Tue, May 13, 2008 at 07:27:54PM -0700, Junio C Hamano wrote:
> After applying this patch, we still seem to have fair number of hits: > > $ git grep -n -e '![^(]* |' -- '*.sh' > > t/t5302-pack-index.sh:68: ! echo "$msg" | grep "pack too large .* off_t"
This one didn't trigger for me because it is the second half of a short-circuit conditional. But it should be fixed.
> t/t7002-grep.sh:111: ! git grep -c test $H | grep -q /dev/null
Hmm. I think my previous analysis is not quite right. The problem is not exactly "! foo | bar". It must happen as part of a logic chain.
So:
# all sane
$ ! true | true; echo $?; \
! true | false; echo $?; \
! false | true; echo $?; \
! false | false; echo $?
1
0
1
0 # but not this
$ true && ! true | true; echo $?; \
true && ! true | false; echo $?; \
true && ! false | true; echo $?; \
true && ! false | false; echo $?
0
1
0
1(whereas with bash, one gets the same "1010" response for the second set).
So it seems like it is parsing as:
(true && ! true) | true
instead of
true && ! (true | true)
But that is not quite right, either, since the output of the first command doesn't go to the third. E.g.,:
$ echo foo && ! echo bar | nl
foo
1 barSo just the exit code gets munged.
However, explicitly grouping with a subshell does seem to fix it. In the t7002-grep.sh case, though, there are no other logic operators, so it works either way.
> t/t7600-merge.sh:378: if ! grep "^ file | *2 +-$" diffstat.txt > t/t7600-merge.sh:392: if ! grep "^ file | *2 +-$" diffstat.txt
These are false positives to your grep; the | is part of a string. ;)
> t/t9400-git-cvsserver-server.sh:160: ! cat request-anonymous | > t/t9400-git-cvsserver-server.sh:169: ! cat request-anonymous | > t/t9400-git-cvsserver-server.sh:187: ! cat request-anonymous |
Same precedence issue as above.
Below is an updated patch 1/4; it includes the one extra fix mentioned above and fixes the inaccuracies in the commit message.
-- >8 -- fix bsd shell negation
On some shells (notably /bin/sh on FreeBSD 6.1), the construct
foo && ! bar | baz
is true if
foo && baz
whereas for most other shells (such as bash) is true if
foo && ! baz
We can work around this by specifying
foo && ! (bar | baz)
which works everywhere.
Signed-off-by: Jeff King <peff@peff.net> --- git-rebase.sh | 2 +- t/t3400-rebase.sh | 4 ++-- t/t3700-add.sh | 6 +++--- t/t5302-pack-index.sh | 2 +- t/t7501-commit.sh | 2 +- 5 files changed, 8 insertions(+), 8 deletions(-)
git-rebase.sh, t/t3400-rebase.sh, t/t3700-add.sh, t/t5302-pack-index.sh, t/t7501-commit.sh
diff --git a/git-rebase.sh b/git-rebase.sh index 9b13b83..fbb0f28 100755 --- a/git-rebase.sh +++ b/git-rebase.sh @@ -353,7 +353,7 @@ orig_head=$branch mb=$(git merge-base "$onto" "$branch") if test "$upstream" = "$onto" && test "$mb" = "$onto" && # linear history? - ! git rev-list --parents "$onto".."$branch" | grep " .* " > /dev/null + ! (git rev-list --parents "$onto".."$branch" | grep " .* ") > /dev/null then # Lazily switch to the target branch if needed... test -z "$switch_to" || git checkout "$switch_to" diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh index 496f4ec..fdad7da 100755 --- a/t/t3400-rebase.sh +++ b/t/t3400-rebase.sh @@ -44,13 +44,13 @@ test_expect_success 'rebase against master' ' test_expect_success \ 'the rebase operation should not have destroyed author information' \ - '! git log | grep "Author:" | grep "<>"' + '! (git log | grep "Author:" | grep "<>")' test_expect_success 'rebase after merge master' ' git reset --hard topic && git merge master && git rebase master && - ! git show | grep "^Merge:" + ! (git show | grep "^Merge:") ' test_expect_success 'rebase of history with merges is linearized' ' diff --git a/t/t3700-add.sh b/t/t3700-add.sh index 287e058..68c5dde 100755 --- a/t/t3700-add.sh +++ b/t/t3700-add.sh @@ -81,17 +81,17 @@ test_expect_success '.gitignore test setup' ' test_expect_success '.gitignore is honored' ' git add . && - ! git ls-files | grep "\\.ig" + ! (git ls-files | grep "\\.ig") ' test_expect_success 'error out when attempting to add ignored ones without -f' ' ! git add a.?? && - ! git ls-files | grep "\\.ig" + ! (git ls-files | grep "\\.ig") ' test_expect_success 'error out when attempting to add ignored ones without -f' ' ! git add d.?? && - ! git ls-files | grep "\\.ig" + ! (git ls-files | grep "\\.ig") ' test_expect_success 'add ignored ones with -f' ' diff --git a/t/t5302-pack-index.sh b/t/t5302-pack-index.sh index b88b5bb..09fd917 100755 --- a/t/t5302-pack-index.sh +++ b/t/t5302-pack-index.sh @@ -65,7 +65,7 @@ test_expect_success \ have_64bits= if msg=$(git verify-pack -v "test-3-${pack3}.pack" 2>&1) || - ! echo "$msg" | grep "pack too large .* off_t" + ! (echo "$msg" | grep "pack too large .* off_t") then have_64bits=t else diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh index c0288f3..89710af 100755 --- a/t/t7501-commit.sh +++ b/t/t7501-commit.sh @@ -41,7 +41,7 @@ test_expect_success \ test_expect_success \ "using paths with --interactive" \ "echo bong-o-bong >file && - ! echo 7 | git-commit -m foo --interactive file" + ! (echo 7 | git-commit -m foo --interactive file)" test_expect_success \ "using invalid commit with -C" \
-- 1.5.5.1.219.g82ba.dirty
Jeff King[PATCH 2/4] t5000: tar portability fix
The output of 'tar tv' varies from system to system. In particular, the t5000 was expecting to parse the date from something like:
-rw-rw-r-- root/root 0 2008-05-13 04:27 file
but FreeBSD's tar produces this:
-rw-rw-r-- 0 root root 0 May 13 04:27 file
Instead of relying on tar's output, let's just extract the file using tar and stat the result using perl.
Signed-off-by: Jeff King <peff@peff.net> --- t/t5000-tar-tree.sh | 8 ++++---- 1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh index fa62b6a..9b0baac 100755 --- a/t/t5000-tar-tree.sh +++ b/t/t5000-tar-tree.sh @@ -67,10 +67,10 @@ test_expect_success \ test_expect_success \ 'validate file modification time' \ - 'TZ=GMT $TAR tvf b.tar a/a | - awk \{print\ \$4,\ \(length\(\$5\)\<7\)\ ?\ \$5\":00\"\ :\ \$5\} \ - >b.mtime && - echo "2005-05-27 22:00:00" >expected.mtime && + 'mkdir extract && + $TAR xf b.tar -C extract a/a && + perl -e '\''print((stat("extract/a/a"))[9], "\n")'\'' >b.mtime && + echo "1117231200" >expected.mtime && diff expected.mtime b.mtime' test_expect_success \
-- 1.5.5.1.296.gf618c
Jeff King[PATCH 3/4] clone: bsd shell portability fix
When using /bin/sh from FreeBSD 6.1, the value of $? is lost when calling a function inside the 'trap' action. This resulted in clone erroneously indicating success when it should have reported failure.
As a workaround, we save the value of $? before calling any functions.
Signed-off-by: Jeff King <peff@peff.net> --- git-clone.sh | 3 +-- 1 files changed, 1 insertions(+), 2 deletions(-)
diff --git a/git-clone.sh b/git-clone.sh index 9d88d1c..547228e 100755 --- a/git-clone.sh +++ b/git-clone.sh @@ -240,7 +240,6 @@ die "working tree '$GIT_WORK_TREE' already exists." D= W= cleanup() { - err=$? test -z "$D" && rm -rf "$dir" test -z "$W" && test -n "$GIT_WORK_TREE" && rm -rf "$GIT_WORK_TREE" cd .. @@ -248,7 +247,7 @@ cleanup() { test -n "$W" && rm -rf "$W" exit $err } -trap cleanup 0 +trap 'err=$?; cleanup' 0 mkdir -p "$dir" && D=$(cd "$dir" && pwd) || usage test -n "$GIT_WORK_TREE" && mkdir -p "$GIT_WORK_TREE" && W=$(cd "$GIT_WORK_TREE" && pwd) && GIT_WORK_TREE="$W" && export GIT_WORK_TREE
-- 1.5.5.1.296.gf618c
Jeff King[PATCH 4/4] filter-branch: fix variable export logic
filter-branch tries to restore "old" copies of some environment variables by using the construct:
unset var test -z "$old_var" || var="$old_var" && export var
However, by the short-circuit logic, we will always run 'export var'. On bash and dash, exporting an unset variable has no effect. However, on some shells (such as FreeBSD's /bin/sh), the shell exports the empty value.
This manifested itself in this case as git-filter-branch setting GIT_INDEX_FILE to the empty string, which in turn caused its call to git-read-tree to fail, leaving the working tree pointing at the original HEAD instead of the rewritten one.
To fix this, we change the short-circuit logic to better match the intent:
test -z "$old_var" || { var="$old_var" && export var; }Signed-off-by: Jeff King <peff@peff.net> --- git-filter-branch.sh | 10 +++++----- 1 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/git-filter-branch.sh b/git-filter-branch.sh index 333f6a8..0304dc5 100755 --- a/git-filter-branch.sh +++ b/git-filter-branch.sh @@ -435,11 +435,11 @@ rm -rf "$tempdir" trap - 0 unset GIT_DIR GIT_WORK_TREE GIT_INDEX_FILE -test -z "$ORIG_GIT_DIR" || GIT_DIR="$ORIG_GIT_DIR" && export GIT_DIR -test -z "$ORIG_GIT_WORK_TREE" || GIT_WORK_TREE="$ORIG_GIT_WORK_TREE" && - export GIT_WORK_TREE -test -z "$ORIG_GIT_INDEX_FILE" || GIT_INDEX_FILE="$ORIG_GIT_INDEX_FILE" && - export GIT_INDEX_FILE +test -z "$ORIG_GIT_DIR" || { GIT_DIR="$ORIG_GIT_DIR" && export GIT_DIR; } +test -z "$ORIG_GIT_WORK_TREE" || { GIT_WORK_TREE="$ORIG_GIT_WORK_TREE" && + export GIT_WORK_TREE; } +test -z "$ORIG_GIT_INDEX_FILE" || { GIT_INDEX_FILE="$ORIG_GIT_INDEX_FILE" && + export GIT_INDEX_FILE; } git read-tree -u -m HEAD exit $ret
-- 1.5.5.1.296.gf618c
Junio C HamanoRe: [PATCH 4/4] filter-branch: fix variable export logic
Jeff King <peff@peff.net> writes:
> filter-branch tries to restore "old" copies of some > environment variables by using the construct: > > unset var > test -z "$old_var" || var="$old_var" && export var > > However, by the short-circuit logic, we will always run > 'export var'.
Thanks.
I was confused by this description ("short-circuit logic"), but I do not think there is no short-circuit going on. This is a simple ignorance of shell syntax.
In a shell scriptlet:
a || b && c
AND list operator (&&) and OR list operator (||) have the same precedence and bind to the left. Because the second part of OR list is always true, we always export.
I have a mild suspecion that this was simply an artifcat of a careless conversion from export var="$val" form we did in the past. I should have caught them back then.
The patch is fine, but I find this easier to read:
+test -z "$ORIG_GIT_DIR" || { + GIT_DIR="$ORIG_GIT_DIR" && export GIT_DIR +} +test -z "$ORIG_GIT_WORK_TREE" || { + GIT_WORK_TREE="$ORIG_GIT_WORK_TREE" && + export GIT_WORK_TREE +} +test -z "$ORIG_GIT_INDEX_FILE" || { + GIT_INDEX_FILE="$ORIG_GIT_INDEX_FILE" && + export GIT_INDEX_FILE +}
Jeff KingRe: [PATCH 4/4] filter-branch: fix variable export logic
On Tue, May 13, 2008 at 09:18:53PM -0700, Junio C Hamano wrote:
> I was confused by this description ("short-circuit logic"), but I do not
> think there is no short-circuit going on. This is a simple ignorance of
> shell syntax.Sorry, I meant "you might think we are not going to run this because of short circuit, but we always will." So I think the original author was confused.
But please feel free to reword in a way that makes more sense.
> I have a mild suspecion that this was simply an artifcat of a careless > conversion from export var="$val" form we did in the past. I should have > caught them back then.
Nope, it was originally that way (46eb449). Thank goodness for git-blame! :)
> The patch is fine, but I find this easier to read:
>
> +test -z "$ORIG_GIT_DIR" || {
> + GIT_DIR="$ORIG_GIT_DIR" && export GIT_DIR
> +}
> +test -z "$ORIG_GIT_WORK_TREE" || {
> + GIT_WORK_TREE="$ORIG_GIT_WORK_TREE" &&
> + export GIT_WORK_TREE
> +}
> +test -z "$ORIG_GIT_INDEX_FILE" || {
> + GIT_INDEX_FILE="$ORIG_GIT_INDEX_FILE" &&
> + export GIT_INDEX_FILE
> +}Yes, that is easier to read. Although what also confused me at first was the double negation. It is trying to say "if the original existed, restore it", but it is written as "the original has no content, OR restore it". So
if test -n "$ORIG_GIT_DIR"
then
...
fiwould be even clearer, though I'm not sure if "-n" has any portability concerns.
-Peff
Paolo BonziniRe: [PATCH 4/4] filter-branch: fix variable export logic
> if test -n "$ORIG_GIT_DIR" > then > ... > fi > > would be even clearer, though I'm not sure if "-n" has any portability > concerns.
It has not. Some autoconf scripts do
if test "x$ORIG_GIT_DIR" != x; then
...
fi(and similarly for -z) but that's being phased out.
Paolo
Jeff KingRe: [PATCH 0/4] freebsd portability fixes
On Tue, May 13, 2008 at 04:43:39AM -0400, Jeff King wrote:
> With these patches, I can successfully run all test scripts from the > current 'master' on FreeBSD 6.1 (some of them probably affect OS X, too > -- I recall somebody complaining about the '! foo | bar' construct > recently).
Ah, nevermind about OS X. It was 97ad535b from Alex, and he specifically mentioned FreeBSD 4.
Alex, you might want to try re-running the tests with these patches.
-Peff
Alex RiesenRe: [PATCH 0/4] freebsd portability fixes
Jeff King, Tue, May 13, 2008 11:04:24 +0200:
> On Tue, May 13, 2008 at 04:43:39AM -0400, Jeff King wrote: > > > With these patches, I can successfully run all test scripts from the > > current 'master' on FreeBSD 6.1 (some of them probably affect OS X, too > > -- I recall somebody complaining about the '! foo | bar' construct > > recently). > > Ah, nevermind about OS X. It was 97ad535b from Alex, and he specifically > mentioned FreeBSD 4. > > Alex, you might want to try re-running the tests with these patches. >
Very good! GnuPG is missing on the system, so t7004 was skipped, but everything besides that ran fine. My config.mak:
NO_STRTOUMAX=Yes NO_C99_FORMAT=Yes SHELL_PATH=/usr/local/bin/bash
perl 5.8.6
I also have the test-tr patches I sent some time ago in that tree.
Alex RiesenRe: [PATCH 0/4] freebsd portability fixes
Alex Riesen, Tue, May 13, 2008 22:39:31 +0200:
> Jeff King, Tue, May 13, 2008 11:04:24 +0200: > > On Tue, May 13, 2008 at 04:43:39AM -0400, Jeff King wrote: > > > > > With these patches, I can successfully run all test scripts from the > > > current 'master' on FreeBSD 6.1 (some of them probably affect OS X, too > > > -- I recall somebody complaining about the '! foo | bar' construct > > > recently). > > > > Ah, nevermind about OS X. It was 97ad535b from Alex, and he specifically > > mentioned FreeBSD 4. > > > > Alex, you might want to try re-running the tests with these patches. > > > > Very good! GnuPG is missing on the system, so t7004 was skipped, > but everything besides that ran fine. My config.mak: > > NO_STRTOUMAX=Yes > NO_C99_FORMAT=Yes > SHELL_PATH=/usr/local/bin/bash > > perl 5.8.6 > > I also have the test-tr patches I sent some time ago in that tree. >
I also have the following in git-compat-util.h:
diff --git a/git-compat-util.h b/git-compat-util.h index 01c4045..161bd50 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -191,6 +191,7 @@ extern size_t gitstrlcpy(char *, const char *, size_t); #ifdef NO_STRTOUMAX #define strtoumax gitstrtoumax +typedef unsigned long long uintmax_t; extern uintmax_t gitstrtoumax(const char *, char **, int); #endif Which is obviously a hack, but I considered too minor an issue. It (the missing uintmax_t) is probably something very specific to this particular system. I am probably wrong about the issue being minor...