[PATCH] t: use portable wrapper for readlink(1)

[PATCH] t: use portable wrapper for readlink(1)

[Jeff King]

Not all systems have a readlink program available for use by the shell.
This causes t3210 to fail on at least AIX. Let's provide a perl
one-liner to do the same thing, and use it there.

I also updated calls in t9802. Nobody reported failure there, but it's
the same issue. Presumably nobody actually tests with p4 on AIX in the
first place (if it is even available there).

I left the use of readlink in the "--valgrind" setup in test-lib.sh, as
valgrind isn't available on exotic platforms anyway (and I didn't want
to increase dependencies between test-lib.sh and test-lib-functions.sh).

There's one other curious case. Commit d2addc3b96 (t7800: readlink may
not be available, 2016-05-31) fixed a similar case. We can't use our
wrapper function there, though, as it's inside a sub-script triggered by
Git. It uses a slightly different technique ("ls" piped to "sed"). I
chose not to use that here as it gives confusing "ls -l" output if the
file is unexpectedly not a symlink (which is OK for its limited use, but
potentially confusing for general use within the test suite). The perl
version emits the empty string.

Reported-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Jeff King <peff@peff.net>
---
This is a re-post that doesn't seem to have made it into "seen"; the
original[1] was buried in a thread, but Ævar reported there that it
fixes t3210 on his AIX build.

[1] https://lore.kernel.org/git/YLk0Zm2J6VOA%2Flks@coredump.intra.peff.net/

 t/t3210-pack-refs.sh       | 2 +-
 t/t9802-git-p4-filetype.sh | 4 ++--
 t/test-lib-functions.sh    | 6 ++++++
 3 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/t/t3210-pack-refs.sh b/t/t3210-pack-refs.sh
index 3b7cdc56ec..577f32dc71 100755
--- a/t/t3210-pack-refs.sh
+++ b/t/t3210-pack-refs.sh
@@ -253,7 +253,7 @@ test_expect_success SYMLINKS 'pack symlinked packed-refs' '
 	git for-each-ref >all-refs-packed &&
 	test_cmp all-refs-before all-refs-packed &&
 	test -h .git/packed-refs &&
-	test "$(readlink .git/packed-refs)" = "my-deviant-packed-refs"
+	test "$(test_readlink .git/packed-refs)" = "my-deviant-packed-refs"
 '
 
 test_done
diff --git a/t/t9802-git-p4-filetype.sh b/t/t9802-git-p4-filetype.sh
index 94edebe272..19073c6e9f 100755
--- a/t/t9802-git-p4-filetype.sh
+++ b/t/t9802-git-p4-filetype.sh
@@ -263,7 +263,7 @@ test_expect_success SYMLINKS 'ensure p4 symlink parsed correctly' '
 	(
 		cd "$git" &&
 		test -L symlink &&
-		test $(readlink symlink) = symlink-target
+		test $(test_readlink symlink) = symlink-target
 	)
 '
 
@@ -329,7 +329,7 @@ test_expect_success SYMLINKS 'empty symlink target' '
 	git p4 clone --dest="$git" //depot@all &&
 	(
 		cd "$git" &&
-		test $(readlink empty-symlink) = target2
+		test $(test_readlink empty-symlink) = target2
 	)
 '
 
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index f0448daa74..b2810478a2 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -1708,3 +1708,9 @@ test_region () {
 
 	return 0
 }
+
+# Print the destination of symlink(s) provided as arguments. Basically
+# the same as the readlink command, but it's not available everywhere.
+test_readlink () {
+	perl -le 'print readlink($_) for @ARGV' "$@"
+}
-- 
2.32.0.352.gff02c21e72

Re: [PATCH] t: use portable wrapper for readlink(1)

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1932 bytes --]

On 2021-06-18 at 16:32:22, Jeff King wrote:
> Not all systems have a readlink program available for use by the shell.
> This causes t3210 to fail on at least AIX. Let's provide a perl
> one-liner to do the same thing, and use it there.
> 
> I also updated calls in t9802. Nobody reported failure there, but it's
> the same issue. Presumably nobody actually tests with p4 on AIX in the
> first place (if it is even available there).
> 
> I left the use of readlink in the "--valgrind" setup in test-lib.sh, as
> valgrind isn't available on exotic platforms anyway (and I didn't want
> to increase dependencies between test-lib.sh and test-lib-functions.sh).
> 
> There's one other curious case. Commit d2addc3b96 (t7800: readlink may
> not be available, 2016-05-31) fixed a similar case. We can't use our
> wrapper function there, though, as it's inside a sub-script triggered by
> Git. It uses a slightly different technique ("ls" piped to "sed"). I
> chose not to use that here as it gives confusing "ls -l" output if the
> file is unexpectedly not a symlink (which is OK for its limited use, but
> potentially confusing for general use within the test suite). The perl
> version emits the empty string.
> 
> Reported-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> This is a re-post that doesn't seem to have made it into "seen"; the
> original[1] was buried in a thread, but Ævar reported there that it
> fixes t3210 on his AIX build.
>
> [1] https://lore.kernel.org/git/YLk0Zm2J6VOA%2Flks@coredump.intra.peff.net/

In case I didn't say it up in the previous post, this looks fine to me.
Using Perl here seems like a fine solution.  If we needed to in the
future, we could add this to test-tool and use the real readlink(2), but
we can hold off until we decide we need to.
-- 
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [PATCH] t: use portable wrapper for readlink(1)

[Jeff King]

On Fri, Jun 18, 2021 at 07:13:31PM +0000, brian m. carlson wrote:

> > This is a re-post that doesn't seem to have made it into "seen"; the
> > original[1] was buried in a thread, but Ævar reported there that it
> > fixes t3210 on his AIX build.
> >
> > [1] https://lore.kernel.org/git/YLk0Zm2J6VOA%2Flks@coredump.intra.peff.net/
> 
> In case I didn't say it up in the previous post, this looks fine to me.
> Using Perl here seems like a fine solution.  If we needed to in the
> future, we could add this to test-tool and use the real readlink(2), but
> we can hold off until we decide we need to.

Agreed on all counts (if we do want to drop perl from the test suite,
there are a lot of these little one-liners that would need converting).

Thanks.

-Peff

Re: [PATCH] t: use portable wrapper for readlink(1)

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Not all systems have a readlink program available for use by the shell.
> This causes t3210 to fail on at least AIX. Let's provide a perl
> one-liner to do the same thing, and use it there.
>
> I also updated calls in t9802. Nobody reported failure there, but it's
> the same issue. Presumably nobody actually tests with p4 on AIX in the
> first place (if it is even available there).
>
> I left the use of readlink in the "--valgrind" setup in test-lib.sh, as
> valgrind isn't available on exotic platforms anyway (and I didn't want
> to increase dependencies between test-lib.sh and test-lib-functions.sh).
>
> There's one other curious case. Commit d2addc3b96 (t7800: readlink may
> not be available, 2016-05-31) fixed a similar case. We can't use our
> wrapper function there, though, as it's inside a sub-script triggered by
> Git. It uses a slightly different technique ("ls" piped to "sed"). I
> chose not to use that here as it gives confusing "ls -l" output if the
> file is unexpectedly not a symlink (which is OK for its limited use, but
> potentially confusing for general use within the test suite). The perl
> version emits the empty string.
>
> Reported-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> This is a re-post that doesn't seem to have made it into "seen"; the
> original[1] was buried in a thread, but Ævar reported there that it
> fixes t3210 on his AIX build.

Yeah, I think I've seen this one and thought I have queued it.
Thanks for resurrecting.

By the way, I'll be mostly offline next week and won't be back to
full speed til the end of the month (it's time to migrate in the
other direction).

> [1] https://lore.kernel.org/git/YLk0Zm2J6VOA%2Flks@coredump.intra.peff.net/
>
>  t/t3210-pack-refs.sh       | 2 +-
>  t/t9802-git-p4-filetype.sh | 4 ++--
>  t/test-lib-functions.sh    | 6 ++++++
>  3 files changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/t/t3210-pack-refs.sh b/t/t3210-pack-refs.sh
> index 3b7cdc56ec..577f32dc71 100755
> --- a/t/t3210-pack-refs.sh
> +++ b/t/t3210-pack-refs.sh
> @@ -253,7 +253,7 @@ test_expect_success SYMLINKS 'pack symlinked packed-refs' '
>  	git for-each-ref >all-refs-packed &&
>  	test_cmp all-refs-before all-refs-packed &&
>  	test -h .git/packed-refs &&
> -	test "$(readlink .git/packed-refs)" = "my-deviant-packed-refs"
> +	test "$(test_readlink .git/packed-refs)" = "my-deviant-packed-refs"
>  '
>  
>  test_done
> diff --git a/t/t9802-git-p4-filetype.sh b/t/t9802-git-p4-filetype.sh
> index 94edebe272..19073c6e9f 100755
> --- a/t/t9802-git-p4-filetype.sh
> +++ b/t/t9802-git-p4-filetype.sh
> @@ -263,7 +263,7 @@ test_expect_success SYMLINKS 'ensure p4 symlink parsed correctly' '
>  	(
>  		cd "$git" &&
>  		test -L symlink &&
> -		test $(readlink symlink) = symlink-target
> +		test $(test_readlink symlink) = symlink-target
>  	)
>  '
>  
> @@ -329,7 +329,7 @@ test_expect_success SYMLINKS 'empty symlink target' '
>  	git p4 clone --dest="$git" //depot@all &&
>  	(
>  		cd "$git" &&
> -		test $(readlink empty-symlink) = target2
> +		test $(test_readlink empty-symlink) = target2
>  	)
>  '
>  
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index f0448daa74..b2810478a2 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -1708,3 +1708,9 @@ test_region () {
>  
>  	return 0
>  }
> +
> +# Print the destination of symlink(s) provided as arguments. Basically
> +# the same as the readlink command, but it's not available everywhere.
> +test_readlink () {
> +	perl -le 'print readlink($_) for @ARGV' "$@"
> +}

Re: [PATCH v3 00/30] Create a reference backend for packed refs

[Jonathan Nieder]

+cc: dawalker, who reported the bug
Stefan Beller wrote:

> We have a user that reports:
>
>   The issue is for users who have a mirrored repository, "git pack-refs"
>   now overwrites the .git/packed-refs symlink instead of following it and
>   replacing the file it points to.
>
> I suspect this series to be at fault, as the bug report came in a day after
> we deployed next containing these changes.
>
> Do symlinks and packed-refs ring a bell for this series?

contrib/workdir/git-new-workdir installs packed-refs as a symlink.
The reported scenario was with another tool that does something
similar for similar reasons.

Dave Walker wrote:

> In the meantime, since this is linked to "git gc", it can crop up
> nearly at any time you modify things from a mirror. I'd recommend
> extreme care until this is sorted out, and it's probably safest to
> avoid using the mirror for branch-modifying operations.
[...]
> The change at fault is this one:
> https://github.com/gitster/git/commit/42dfa7ecef22191b004862fb56074b408c94fc97

That's "commit_packed_refs(): use a staging file separate from the
lockfile", 2017-06-23, which would indeed appear to explain the
symptoms.

I'll try to make a reproduction recipe.

Thanks,
Jonathan

[PATCH] packed_ref_store: handle a packed-refs file that is a symlink

[Michael Haggerty]

One of the tricks that `contrib/workdir/git-new-workdir` plays is to
making `packed-refs` in the new workdir a symlink to the `packed-refs`
file in the original repository. Before
42dfa7ecef ("commit_packed_refs(): use a staging file separate from
the lockfile", 2017-06-23), a lockfile was used as the staging file,
and because the `LOCK_NO_DEREF` was not used, the pointed-to file was
locked and modified.

But after that commit, the staging file was created using a tempfile,
with the end result that rewriting the `packed-refs` file in the
workdir overwrote the symlink rather than the original `packed-refs`
file.

Change `commit_packed_refs()` to use `get_locked_file_path()` to find
the path of the file that it should overwrite. Since that path was
properly resolved when the lockfile was created, this restores the
pre-42dfa7ecef behavior.

Also add a test case to document this use case and prevent a
regression like this from recurring.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
Sorry for the slow response; I've been traveling and very busy.

I didn't even know that a symlinked `packed-refs` file is a thing. The
attached patch should fix the problem. It applies on top of v3 of
mh/packed-ref-store [1] (which is already in next).

Michael

[1] http://public-inbox.org/git/cover.1498933362.git.mhagger@alum.mit.edu/

 refs/packed-backend.c | 24 ++++++++++++++++++------
 t/t3210-pack-refs.sh  | 15 +++++++++++++++
 2 files changed, 33 insertions(+), 6 deletions(-)

diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index a28befbfa3..59e7d1a509 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -610,19 +610,27 @@ int commit_packed_refs(struct ref_store *ref_store, struct strbuf *err)
 	struct packed_ref_cache *packed_ref_cache =
 		get_packed_ref_cache(refs);
 	int ok;
+	int ret = -1;
 	struct strbuf sb = STRBUF_INIT;
 	FILE *out;
 	struct ref_iterator *iter;
+	char *packed_refs_path;
 
 	if (!is_lock_file_locked(&refs->lock))
 		die("BUG: commit_packed_refs() called when unlocked");
 
-	strbuf_addf(&sb, "%s.new", refs->path);
+	/*
+	 * If packed-refs is a symlink, we want to overwrite the
+	 * symlinked-to file, not the symlink itself. Also, put the
+	 * staging file next to it:
+	 */
+	packed_refs_path = get_locked_file_path(&refs->lock);
+	strbuf_addf(&sb, "%s.new", packed_refs_path);
 	if (create_tempfile(&refs->tempfile, sb.buf) < 0) {
 		strbuf_addf(err, "unable to create file %s: %s",
 			    sb.buf, strerror(errno));
 		strbuf_release(&sb);
-		return -1;
+		goto out;
 	}
 	strbuf_release(&sb);
 
@@ -660,17 +668,21 @@ int commit_packed_refs(struct ref_store *ref_store, struct strbuf *err)
 		goto error;
 	}
 
-	if (rename_tempfile(&refs->tempfile, refs->path)) {
+	if (rename_tempfile(&refs->tempfile, packed_refs_path)) {
 		strbuf_addf(err, "error replacing %s: %s",
 			    refs->path, strerror(errno));
-		return -1;
+		goto out;
 	}
 
-	return 0;
+	ret = 0;
+	goto out;
 
 error:
 	delete_tempfile(&refs->tempfile);
-	return -1;
+
+out:
+	free(packed_refs_path);
+	return ret;
 }
 
 /*
diff --git a/t/t3210-pack-refs.sh b/t/t3210-pack-refs.sh
index 2bb4b25ed9..0d8a03e2a9 100755
--- a/t/t3210-pack-refs.sh
+++ b/t/t3210-pack-refs.sh
@@ -238,4 +238,19 @@ test_expect_success 'retry acquiring packed-refs.lock' '
 	git -c core.packedrefstimeout=3000 pack-refs --all --prune
 '
 
+test_expect_success 'pack symlinked packed-refs' '
+	# First make sure that symlinking works when reading:
+	git update-ref refs/heads/loosy refs/heads/master &&
+	git for-each-ref >all-refs-before &&
+	mv .git/packed-refs .git/my-deviant-packed-refs &&
+	ln -s my-deviant-packed-refs .git/packed-refs &&
+	git for-each-ref >all-refs-linked &&
+	test_cmp all-refs-before all-refs-linked &&
+	git pack-refs --all --prune &&
+	git for-each-ref >all-refs-packed &&
+	test_cmp all-refs-before all-refs-packed &&
+	test -h .git/packed-refs &&
+	test "$(readlink .git/packed-refs)" = "my-deviant-packed-refs"
+'
+
 test_done
-- 
2.11.0

Re: [PATCH] packed_ref_store: handle a packed-refs file that is a symlink

[Ævar Arnfjörð Bjarmason]

On Wed, Jul 26 2017, Michael Haggerty wrote:

> [...]
> +test_expect_success 'pack symlinked packed-refs' '
> +	# First make sure that symlinking works when reading:
> +	git update-ref refs/heads/loosy refs/heads/master &&
> +	git for-each-ref >all-refs-before &&
> +	mv .git/packed-refs .git/my-deviant-packed-refs &&
> +	ln -s my-deviant-packed-refs .git/packed-refs &&
> +	git for-each-ref >all-refs-linked &&
> +	test_cmp all-refs-before all-refs-linked &&
> +	git pack-refs --all --prune &&
> +	git for-each-ref >all-refs-packed &&
> +	test_cmp all-refs-before all-refs-packed &&
> +	test -h .git/packed-refs &&
> +	test "$(readlink .git/packed-refs)" = "my-deviant-packed-refs"
> +'

FWIW this broke tests on AIX because we can't assume readlink(1) exists
at all. See d2addc3b96 (t7800: readlink may not be available,
2016-05-31) for a workaround.

Re: [PATCH] packed_ref_store: handle a packed-refs file that is a symlink

[Jeff King]

On Mon, May 31, 2021 at 04:18:46PM +0200, Ævar Arnfjörð Bjarmason wrote:

> 
> On Wed, Jul 26 2017, Michael Haggerty wrote:
> 
> > [...]
> > +test_expect_success 'pack symlinked packed-refs' '
> > +	# First make sure that symlinking works when reading:
> > +	git update-ref refs/heads/loosy refs/heads/master &&
> > +	git for-each-ref >all-refs-before &&
> > +	mv .git/packed-refs .git/my-deviant-packed-refs &&
> > +	ln -s my-deviant-packed-refs .git/packed-refs &&
> > +	git for-each-ref >all-refs-linked &&
> > +	test_cmp all-refs-before all-refs-linked &&
> > +	git pack-refs --all --prune &&
> > +	git for-each-ref >all-refs-packed &&
> > +	test_cmp all-refs-before all-refs-packed &&
> > +	test -h .git/packed-refs &&
> > +	test "$(readlink .git/packed-refs)" = "my-deviant-packed-refs"
> > +'
> 
> FWIW this broke tests on AIX because we can't assume readlink(1) exists
> at all. See d2addc3b96 (t7800: readlink may not be available,
> 2016-05-31) for a workaround.

Hmm. So obviously we can use a fix similar to the one in t7800 (though
it's sufficiently complicated that I'd be tempted to wrap it in a helper
function). There are a few other calls that could be changed, too.

But it's interesting to me that it sounds like the tests have been
broken on AIX for 4 years, and nobody noticed. I assume you ran into
this on the gcc build-farm machines. Our traditional approach for
portability has been: if somebody is using the platform and cares enough
to submit patches, then we'll support it. But testing on the build-farm
means preemptively finding these problems, whether anyone actually cares
about AIX or not. :)

I'm not really arguing either way here, just thinking out loud.

Preemptively finding portability problems may save work in the long
term. And people may even be using Git on AIX and just ignoring test
failures, or they have GNU coreutils installed anyway, etc. But it would
also save work if we can ignore platforms that nobody uses.

-Peff

[PATCH] t: use portable wrapper for readlink(1)

[Jeff King]

On Thu, Jun 03, 2021 at 03:39:53PM -0400, Jeff King wrote:

> > FWIW this broke tests on AIX because we can't assume readlink(1) exists
> > at all. See d2addc3b96 (t7800: readlink may not be available,
> > 2016-05-31) for a workaround.
> 
> Hmm. So obviously we can use a fix similar to the one in t7800 (though
> it's sufficiently complicated that I'd be tempted to wrap it in a helper
> function). There are a few other calls that could be changed, too.

Here's a patch to do that. Can you confirm that it fixes your test
failure?

-- >8 --
Subject: [PATCH] t: use portable wrapper for readlink(1)

Not all systems have a readlink program available for use by the shell.
This causes t3210 to fail on at least AIX. Let's provide a perl
one-liner to do the same thing, and use it there.

I also updated calls in t9802. Nobody reported failure there, but it's
the same issue. Presumably nobody actually tests with p4 on AIX in the
first place (if it is even available there).

I left the use of readlink in the "--valgrind" setup in test-lib.sh, as
valgrind isn't available on exotic platforms anyway (and I didn't want
to increase dependencies between test-lib.sh and test-lib-functions.sh).

There's one other curious case. Commit d2addc3b96 (t7800: readlink may
not be available, 2016-05-31) fixed a similar case. We can't use our
wrapper function there, though, as it's inside a sub-script triggered by
Git. It uses a slightly different technique ("ls" piped to "sed"). I
chose not to use that here as it gives confusing "ls -l" output if the
file is unexpectedly not a symlink (which is OK for its limited use, but
potentially confusing for general use within the test suite). The perl
version emits the empty string.

Reported-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Jeff King <peff@peff.net>
---
 t/t3210-pack-refs.sh       | 2 +-
 t/t9802-git-p4-filetype.sh | 4 ++--
 t/test-lib-functions.sh    | 6 ++++++
 3 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/t/t3210-pack-refs.sh b/t/t3210-pack-refs.sh
index 3b7cdc56ec..577f32dc71 100755
--- a/t/t3210-pack-refs.sh
+++ b/t/t3210-pack-refs.sh
@@ -253,7 +253,7 @@ test_expect_success SYMLINKS 'pack symlinked packed-refs' '
 	git for-each-ref >all-refs-packed &&
 	test_cmp all-refs-before all-refs-packed &&
 	test -h .git/packed-refs &&
-	test "$(readlink .git/packed-refs)" = "my-deviant-packed-refs"
+	test "$(test_readlink .git/packed-refs)" = "my-deviant-packed-refs"
 '
 
 test_done
diff --git a/t/t9802-git-p4-filetype.sh b/t/t9802-git-p4-filetype.sh
index 94edebe272..19073c6e9f 100755
--- a/t/t9802-git-p4-filetype.sh
+++ b/t/t9802-git-p4-filetype.sh
@@ -263,7 +263,7 @@ test_expect_success SYMLINKS 'ensure p4 symlink parsed correctly' '
 	(
 		cd "$git" &&
 		test -L symlink &&
-		test $(readlink symlink) = symlink-target
+		test $(test_readlink symlink) = symlink-target
 	)
 '
 
@@ -329,7 +329,7 @@ test_expect_success SYMLINKS 'empty symlink target' '
 	git p4 clone --dest="$git" //depot@all &&
 	(
 		cd "$git" &&
-		test $(readlink empty-symlink) = target2
+		test $(test_readlink empty-symlink) = target2
 	)
 '
 
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index b823c14027..661f376077 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -1692,3 +1692,9 @@ test_region () {
 
 	return 0
 }
+
+# Print the destination of symlink(s) provided as arguments. Basically
+# the same as the readlink command, but it's not available everywhere.
+test_readlink () {
+	perl -le 'print readlink($_) for @ARGV' "$@"
+}
-- 
2.32.0.rc3.525.gfa939c0632

Re: [PATCH] t: use portable wrapper for readlink(1)

[Ævar Arnfjörð Bjarmason]

On Thu, Jun 03 2021, Jeff King wrote:

> On Thu, Jun 03, 2021 at 03:39:53PM -0400, Jeff King wrote:
>
>> > FWIW this broke tests on AIX because we can't assume readlink(1) exists
>> > at all. See d2addc3b96 (t7800: readlink may not be available,
>> > 2016-05-31) for a workaround.
>> 
>> Hmm. So obviously we can use a fix similar to the one in t7800 (though
>> it's sufficiently complicated that I'd be tempted to wrap it in a helper
>> function). There are a few other calls that could be changed, too.
>
> Here's a patch to do that. Can you confirm that it fixes your test
> failure?

It does, thanks, t3210*.sh is broken on current master on AIX, fixed
with this patch. I don't have p4 on that machine (or any box I hack on),
so I can't test t9802*.sh.

> -- >8 --
> Subject: [PATCH] t: use portable wrapper for readlink(1)
>
> Not all systems have a readlink program available for use by the shell.
> This causes t3210 to fail on at least AIX. Let's provide a perl
> one-liner to do the same thing, and use it there.
>
> I also updated calls in t9802. Nobody reported failure there, but it's
> the same issue. Presumably nobody actually tests with p4 on AIX in the
> first place (if it is even available there).
>
> I left the use of readlink in the "--valgrind" setup in test-lib.sh, as
> valgrind isn't available on exotic platforms anyway (and I didn't want
> to increase dependencies between test-lib.sh and test-lib-functions.sh).
>
> There's one other curious case. Commit d2addc3b96 (t7800: readlink may
> not be available, 2016-05-31) fixed a similar case. We can't use our
> wrapper function there, though, as it's inside a sub-script triggered by
> Git. It uses a slightly different technique ("ls" piped to "sed"). I
> chose not to use that here as it gives confusing "ls -l" output if the
> file is unexpectedly not a symlink (which is OK for its limited use, but
> potentially confusing for general use within the test suite). The perl
> version emits the empty string.
>
> Reported-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  t/t3210-pack-refs.sh       | 2 +-
>  t/t9802-git-p4-filetype.sh | 4 ++--
>  t/test-lib-functions.sh    | 6 ++++++
>  3 files changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/t/t3210-pack-refs.sh b/t/t3210-pack-refs.sh
> index 3b7cdc56ec..577f32dc71 100755
> --- a/t/t3210-pack-refs.sh
> +++ b/t/t3210-pack-refs.sh
> @@ -253,7 +253,7 @@ test_expect_success SYMLINKS 'pack symlinked packed-refs' '
>  	git for-each-ref >all-refs-packed &&
>  	test_cmp all-refs-before all-refs-packed &&
>  	test -h .git/packed-refs &&
> -	test "$(readlink .git/packed-refs)" = "my-deviant-packed-refs"
> +	test "$(test_readlink .git/packed-refs)" = "my-deviant-packed-refs"
>  '
>  
>  test_done
> diff --git a/t/t9802-git-p4-filetype.sh b/t/t9802-git-p4-filetype.sh
> index 94edebe272..19073c6e9f 100755
> --- a/t/t9802-git-p4-filetype.sh
> +++ b/t/t9802-git-p4-filetype.sh
> @@ -263,7 +263,7 @@ test_expect_success SYMLINKS 'ensure p4 symlink parsed correctly' '
>  	(
>  		cd "$git" &&
>  		test -L symlink &&
> -		test $(readlink symlink) = symlink-target
> +		test $(test_readlink symlink) = symlink-target
>  	)
>  '
>  
> @@ -329,7 +329,7 @@ test_expect_success SYMLINKS 'empty symlink target' '
>  	git p4 clone --dest="$git" //depot@all &&
>  	(
>  		cd "$git" &&
> -		test $(readlink empty-symlink) = target2
> +		test $(test_readlink empty-symlink) = target2
>  	)
>  '
>  
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index b823c14027..661f376077 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -1692,3 +1692,9 @@ test_region () {
>  
>  	return 0
>  }
> +
> +# Print the destination of symlink(s) provided as arguments. Basically
> +# the same as the readlink command, but it's not available everywhere.
> +test_readlink () {
> +	perl -le 'print readlink($_) for @ARGV' "$@"
> +}