[PATCH 0/4] freebsd portability fixes

[PATCH 0/4] freebsd portability fixes

[Jeff King]

I'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

[PATCH 1/4] fix bsd shell negation

[Jeff King]

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

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

Re: [PATCH 1/4] fix bsd shell negation

[Junio C Hamano]

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?



    
    

Re: [PATCH 1/4] fix bsd shell negation

[Jeff King]

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 bar

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

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

[PATCH 2/4] t5000: tar portability fix

[Jeff King]

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

[PATCH 3/4] clone: bsd shell portability fix

[Jeff King]

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

[PATCH 4/4] filter-branch: fix variable export logic

[Jeff King]

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

Re: [PATCH 4/4] filter-branch: fix variable export logic

[Junio C Hamano]

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
+}

Re: [PATCH 4/4] filter-branch: fix variable export logic

[Jeff King]

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

would be even clearer, though I'm not sure if "-n" has any portability
concerns.

-Peff

Re: [PATCH 4/4] filter-branch: fix variable export logic

[Paolo Bonzini]

>   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

Re: [PATCH 0/4] freebsd portability fixes

[Jeff King]

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

Re: [PATCH 0/4] freebsd portability fixes

[Alex Riesen]

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.

Re: [PATCH 0/4] freebsd portability fixes

[Alex Riesen]

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