threads / patch / 27959

patcht7400: fix bogus test failure with symlinked trash

Subject: [PATCH] t7400: fix bogus test failure with symlinked trash

## tl;dr

4 messages between Jul 30, 2011 and Jul 30, 2011. Diffs are folded; open one to read it.

replies: 3people: 2as markdown or json

Jeff King· Jul 30, 2011, 00:36 UTC · lore

One of the tests in t7400 fails if the trash directory has a symlink anywhere in its path. E.g.:

  $ mkdir /tmp/git-test
  $ mkdir /tmp/git-test/real
  $ ln -s real /tmp/git-test/link

  $ ./t7400-submodule-basic --root=/tmp/git-test/real
  ...
  # passed all 44 test(s)

  $ ./t7400-submodule-basic --root=/tmp/git-test/link
  ...
  not ok - 41 use superproject as upstream when path is relative and no url is set there

The failing test does:

  git submodule add ../repo relative &&
  ...
  git submodule sync relative &&
  test "$(git config submodule.relative.url)" = "$submodurl/repo"

where $submodurl comes from the $TRASH_DIRECTORY the user gave us. However, git will resolve symlinks when converting the relative path into an absolute one, leading them to be textually different (even though they point to the same directory).

Fix this by asking git to canonicalize the name of the trash directory for us.

Signed-off-by: Jeff King <peff@peff.net>
---
This feels a little funny, because we are probably using the same
"convert relative to absolute" code to generate our expected value, as
well as in the test itself. So any bug in that code is likely to be
masked. But this test isn't really about checking the absolute path
code, but rather making sure that it is invoked properly.

 t/t7400-submodule-basic.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

Show changes to t/t7400-submodule-basic.sh +1 −1

diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index 5afe6cc..12200ca 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -48,7 +48,7 @@ test_expect_success 'setup - repository to add submodules to' '
 
 # The 'submodule add' tests need some repository to add as a submodule.
 # The trash directory is a good one as any.
-submodurl=$TRASH_DIRECTORY
+submodurl=`git rev-parse --show-toplevel`
 
 listbranches() {
 	git for-each-ref --format='%(refname)' 'refs/heads/*'

-- 
1.7.5.4.31.ge4d5e

Jens Lehmann· Jul 30, 2011, 10:41 UTC · re: Jeff King · lore

Re: [PATCH] t7400: fix bogus test failure with symlinked trash

Am 30.07.2011 02:36, schrieb Jeff King:

Show 5 quoted lines

> This feels a little funny, because we are probably using the same
> "convert relative to absolute" code to generate our expected value, as
> well as in the test itself. So any bug in that code is likely to be
> masked. But this test isn't really about checking the absolute path
> code, but rather making sure that it is invoked properly.

While the patch itself is good (apart from using backticks, I think a "$()" should be used there), I share your concerns about loosing an opportunity to test git functionality against the real world.

What about doing the following instead?

submodurl=$(cd "$TRASH_DIRECTORY"; pwd -P)

(That pattern is already used in t/t0000-basic.sh)

But that is just nitpicking ...

Show 16 quoted lines

>  t/t7400-submodule-basic.sh |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
> index 5afe6cc..12200ca 100755
> --- a/t/t7400-submodule-basic.sh
> +++ b/t/t7400-submodule-basic.sh
> @@ -48,7 +48,7 @@ test_expect_success 'setup - repository to add submodules to' '
>  
>  # The 'submodule add' tests need some repository to add as a submodule.
>  # The trash directory is a good one as any.
> -submodurl=$TRASH_DIRECTORY
> +submodurl=`git rev-parse --show-toplevel`
>  
>  listbranches() {
>  	git for-each-ref --format='%(refname)' 'refs/heads/*'

Jeff King· Jul 30, 2011, 15:05 UTC · re: Jens Lehmann · lore

Re: [PATCH] t7400: fix bogus test failure with symlinked trash

On Sat, Jul 30, 2011 at 12:41:26PM +0200, Jens Lehmann wrote:

Show 16 quoted lines

> Am 30.07.2011 02:36, schrieb Jeff King:
> > This feels a little funny, because we are probably using the same
> > "convert relative to absolute" code to generate our expected value, as
> > well as in the test itself. So any bug in that code is likely to be
> > masked. But this test isn't really about checking the absolute path
> > code, but rather making sure that it is invoked properly.
> 
> While the patch itself is good (apart from using backticks, I think
> a "$()" should be used there), I share your concerns about loosing
> an opportunity to test git functionality against the real world.
> 
> What about doing the following instead?
> 
> submodurl=$(cd "$TRASH_DIRECTORY"; pwd -P)
> 
> (That pattern is already used in t/t0000-basic.sh)

That's fine by me. I assumed we didn't have any portable external tool we could rely on, but if we have been using "pwd -P" in t0000, it's probably OK.

So how about:

-- >8 --
Subject: [PATCH] t7400: fix bogus test failure with symlinked trash

One of the tests in t7400 fails if the trash directory has a symlink anywhere in its path. E.g.:

  $ mkdir /tmp/git-test
  $ mkdir /tmp/git-test/real
  $ ln -s real /tmp/git-test/link

  $ ./t7400-submodule-basic --root=/tmp/git-test/real
  ...
  # passed all 44 test(s)

  $ ./t7400-submodule-basic --root=/tmp/git-test/link
  ...
  not ok - 41 use superproject as upstream when path is relative and no url is set there

The failing test does:

  git submodule add ../repo relative &&
  ...
  git submodule sync relative &&
  test "$(git config submodule.relative.url)" = "$submodurl/repo"

Fix this by asking pwd to canonicalize the name of the trash directory for us.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t7400-submodule-basic.sh |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

Show changes to t/t7400-submodule-basic.sh +4 −2

diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index 5afe6cc..14dc927 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -47,8 +47,10 @@ test_expect_success 'setup - repository to add submodules to' '
 '
 
 # The 'submodule add' tests need some repository to add as a submodule.
-# The trash directory is a good one as any.
-submodurl=$TRASH_DIRECTORY
+# The trash directory is a good one as any. We need to canonicalize
+# the name, though, as some tests compare it to the absolute path git
+# generates, which will expand symbolic links.
+submodurl=$(pwd -P)
 
 listbranches() {
 	git for-each-ref --format='%(refname)' 'refs/heads/*'

-- 
1.7.5.4.31.ge4d5e

Jens Lehmann· Jul 30, 2011, 15:44 UTC · re: Jeff King · lore

Re: [PATCH] t7400: fix bogus test failure with symlinked trash

Am 30.07.2011 17:05, schrieb Jeff King:

Show 24 quoted lines

> On Sat, Jul 30, 2011 at 12:41:26PM +0200, Jens Lehmann wrote:
> 
>> Am 30.07.2011 02:36, schrieb Jeff King:
>>> This feels a little funny, because we are probably using the same
>>> "convert relative to absolute" code to generate our expected value, as
>>> well as in the test itself. So any bug in that code is likely to be
>>> masked. But this test isn't really about checking the absolute path
>>> code, but rather making sure that it is invoked properly.
>>
>> While the patch itself is good (apart from using backticks, I think
>> a "$()" should be used there), I share your concerns about loosing
>> an opportunity to test git functionality against the real world.
>>
>> What about doing the following instead?
>>
>> submodurl=$(cd "$TRASH_DIRECTORY"; pwd -P)
>>
>> (That pattern is already used in t/t0000-basic.sh)
> 
> That's fine by me. I assumed we didn't have any portable external tool
> we could rely on, but if we have been using "pwd -P" in t0000, it's
> probably OK.
> 
> So how about:

Thanks! Unless there is a reason not to use "pwd -P" this is
Acked-by: Jens Lehmann <Jens.Lehmann@web.de>

Show 56 quoted lines

> -- >8 --
> Subject: [PATCH] t7400: fix bogus test failure with symlinked trash
> 
> One of the tests in t7400 fails if the trash directory has a
> symlink anywhere in its path. E.g.:
> 
>   $ mkdir /tmp/git-test
>   $ mkdir /tmp/git-test/real
>   $ ln -s real /tmp/git-test/link
> 
>   $ ./t7400-submodule-basic --root=/tmp/git-test/real
>   ...
>   # passed all 44 test(s)
> 
>   $ ./t7400-submodule-basic --root=/tmp/git-test/link
>   ...
>   not ok - 41 use superproject as upstream when path is relative and no url is set there
> 
> The failing test does:
> 
>   git submodule add ../repo relative &&
>   ...
>   git submodule sync relative &&
>   test "$(git config submodule.relative.url)" = "$submodurl/repo"
> 
> where $submodurl comes from the $TRASH_DIRECTORY the user
> gave us. However, git will resolve symlinks when converting
> the relative path into an absolute one, leading them to be
> textually different (even though they point to the same
> directory).
> 
> Fix this by asking pwd to canonicalize the name of the trash
> directory for us.
> 
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  t/t7400-submodule-basic.sh |    6 ++++--
>  1 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
> index 5afe6cc..14dc927 100755
> --- a/t/t7400-submodule-basic.sh
> +++ b/t/t7400-submodule-basic.sh
> @@ -47,8 +47,10 @@ test_expect_success 'setup - repository to add submodules to' '
>  '
>  
>  # The 'submodule add' tests need some repository to add as a submodule.
> -# The trash directory is a good one as any.
> -submodurl=$TRASH_DIRECTORY
> +# The trash directory is a good one as any. We need to canonicalize
> +# the name, though, as some tests compare it to the absolute path git
> +# generates, which will expand symbolic links.
> +submodurl=$(pwd -P)
>  
>  listbranches() {
>  	git for-each-ref --format='%(refname)' 'refs/heads/*'