threads / patch / 39360

patcht1020: cleanup subdirectory tests a little

Subject: [PATCH] t1020: cleanup subdirectory tests a little

## tl;dr

10 messages between May 18, 2015 and May 19, 2015. Diffs are folded; open one to read it.

replies: 9people: 3as markdown or json

Stefan Beller· May 18, 2015, 18:13 UTC · lore

When looking through existing tests to point out good style I came across t1020, which has a test commented out and the comment wasn't helping me either of what the test should accomplish in the future. The code of the test is the same as the test before except setting GIT_DIR=. explicitly, so it did not ring a bell for me as well.

This removes the test, which may have been confusing readers since 2010.

Additionally moves the "rm -fr foo.git" of the next test (where it is unrelated) to the previous test, where it makes sense as a cleanup.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 t/t1020-subdirectory.sh | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)
Show changes to t/t1020-subdirectory.sh +1 −10
diff --git a/t/t1020-subdirectory.sh b/t/t1020-subdirectory.sh
index 2edb4f2..4470ede 100755
--- a/t/t1020-subdirectory.sh
+++ b/t/t1020-subdirectory.sh
@@ -163,6 +163,7 @@ test_expect_success 'no file/rev ambiguity check inside .git' '
 '
 
 test_expect_success 'no file/rev ambiguity check inside a bare repo' '
+	test_when_finished "rm -fr foo.git" &&
 	git clone -s --bare .git foo.git &&
 	(
 		cd foo.git &&
@@ -170,17 +171,7 @@ test_expect_success 'no file/rev ambiguity check inside a bare repo' '
 	)
 '
 
-# This still does not work as it should...
-: test_expect_success 'no file/rev ambiguity check inside a bare repo' '
-	git clone -s --bare .git foo.git &&
-	(
-		cd foo.git &&
-		git show -s HEAD
-	)
-'
-
 test_expect_success SYMLINKS 'detection should not be fooled by a symlink' '
-	rm -fr foo.git &&
 	git clone -s .git another &&
 	ln -s another yetanother &&
 	(

-- 
2.4.0.194.gc518059
Junio C Hamano· May 18, 2015, 18:29 UTC · re: Stefan Beller · lore

Re: [PATCH] t1020: cleanup subdirectory tests a little

Stefan Beller <sbeller@google.com> writes:
Show 5 quoted lines
> When looking through existing tests to point out good style I came across
> t1020, which has a test commented out and the comment wasn't helping me
> either of what the test should accomplish in the future. The code of the
> test is the same as the test before except setting GIT_DIR=. explicitly,
> so it did not ring a bell for me as well.

I think this one should be clear, especially if you did notice the one that sets GIT_DIR=. explicitly. It is saying that "git show -s HEAD" inside the bare repository should be intelligent enough to realize that it is inside bare repository (hence HEAD cannot be a file in the working tree); the user's asking for "HEAD" therefore must mean "the tip commit", and never "(by default the tip commit) filtered to the pathspec HEAD".

If it does not still work, shouldn't it be marked as test_expect_failure instead of being commented out?

The first and last hunk to use when-finished looks like a good change, but is unrelated.

Show 41 quoted lines
>
> This removes the test, which may have been confusing readers since 2010.
>
> Additionally moves the "rm -fr foo.git" of the next test (where it is
> unrelated) to the previous test, where it makes sense as a cleanup.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  t/t1020-subdirectory.sh | 11 +----------
>  1 file changed, 1 insertion(+), 10 deletions(-)
>
> diff --git a/t/t1020-subdirectory.sh b/t/t1020-subdirectory.sh
> index 2edb4f2..4470ede 100755
> --- a/t/t1020-subdirectory.sh
> +++ b/t/t1020-subdirectory.sh
> @@ -163,6 +163,7 @@ test_expect_success 'no file/rev ambiguity check inside .git' '
>  '
>  
>  test_expect_success 'no file/rev ambiguity check inside a bare repo' '
> +	test_when_finished "rm -fr foo.git" &&
>  	git clone -s --bare .git foo.git &&
>  	(
>  		cd foo.git &&
> @@ -170,17 +171,7 @@ test_expect_success 'no file/rev ambiguity check inside a bare repo' '
>  	)
>  '
>  
> -# This still does not work as it should...
> -: test_expect_success 'no file/rev ambiguity check inside a bare repo' '
> -	git clone -s --bare .git foo.git &&
> -	(
> -		cd foo.git &&
> -		git show -s HEAD
> -	)
> -'
> -
>  test_expect_success SYMLINKS 'detection should not be fooled by a symlink' '
> -	rm -fr foo.git &&
>  	git clone -s .git another &&
>  	ln -s another yetanother &&
>  	(
Junio C Hamano· May 18, 2015, 18:30 UTC · re: Junio C Hamano · lore

Re: [PATCH] t1020: cleanup subdirectory tests a little

Junio C Hamano <gitster@pobox.com> writes:
Show 15 quoted lines
> Stefan Beller <sbeller@google.com> writes:
>
>> When looking through existing tests to point out good style I came across
>> t1020, which has a test commented out and the comment wasn't helping me
>> either of what the test should accomplish in the future. The code of the
>> test is the same as the test before except setting GIT_DIR=. explicitly,
>> so it did not ring a bell for me as well.
>
> I think this one should be clear, especially if you did notice the
> one that sets GIT_DIR=. explicitly.  It is saying that "git show -s
> HEAD" inside the bare repository should be intelligent enough to
> realize that it is inside bare repository (hence HEAD cannot be a
> file in the working tree); the user's asking for "HEAD" therefore
> must mean "the tip commit", and never "(by default the tip commit)
> filtered to the pathspec HEAD".

I forgot to conclude that sentence: "... and it should be able to do so without the help of an explicit GIT_DIR=."

> If it does not still work, shouldn't it be marked as
> test_expect_failure instead of being commented out?
Stefan Beller· May 18, 2015, 18:36 UTC · re: Junio C Hamano · lore

Re: [PATCH] t1020: cleanup subdirectory tests a little

On Mon, May 18, 2015 at 11:30 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 20 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Stefan Beller <sbeller@google.com> writes:
>>
>>> When looking through existing tests to point out good style I came across
>>> t1020, which has a test commented out and the comment wasn't helping me
>>> either of what the test should accomplish in the future. The code of the
>>> test is the same as the test before except setting GIT_DIR=. explicitly,
>>> so it did not ring a bell for me as well.
>>
>> I think this one should be clear, especially if you did notice the
>> one that sets GIT_DIR=. explicitly.  It is saying that "git show -s
>> HEAD" inside the bare repository should be intelligent enough to
>> realize that it is inside bare repository (hence HEAD cannot be a
>> file in the working tree); the user's asking for "HEAD" therefore
>> must mean "the tip commit", and never "(by default the tip commit)
>> filtered to the pathspec HEAD".
>
> I forgot to conclude that sentence: "... and it should be able to do
> so without the help of an explicit GIT_DIR=."

Not sure I follow you there. So currently there are 2 tests having the same name, and doing exactly the same thing, apart from setting the GIT_DIR and one of them being commented out.

So I don't understand, what should be tested *additionally* in the second test, where GIT_DIR is not set. (The naming doesn't hint at testing explicit GIT_DIR, so we test the 'no file/rev ambiguity check inside a bare repo' again.)

Maybe this diff is better for review:
Show changes to t/t1020-subdirectory.sh +1 −10
diff --git a/t/t1020-subdirectory.sh b/t/t1020-subdirectory.sh
index 2edb4f2..4470ede 100755
--- a/t/t1020-subdirectory.sh
+++ b/t/t1020-subdirectory.sh
@@ -163,24 +163,15 @@ test_expect_success 'no file/rev ambiguity check
inside .git' '
 '

 test_expect_success 'no file/rev ambiguity check inside a bare repo' '
+ test_when_finished "rm -fr foo.git" &&
  git clone -s --bare .git foo.git &&
  (
  cd foo.git &&
  GIT_DIR=. git show -s HEAD
  )
 '

-# This still does not work as it should...
-: test_expect_success 'no file/rev ambiguity check inside a bare repo' '
- git clone -s --bare .git foo.git &&
- (
- cd foo.git &&
- git show -s HEAD
- )
-'
-
 test_expect_success SYMLINKS 'detection should not be fooled by a symlink' '
- rm -fr foo.git &&
  git clone -s .git another &&
  ln -s another yetanother &&
  (

-- 
2.4.0.194.gc518059



>
>> If it does not still work, shouldn't it be marked as
>> test_expect_failure instead of being commented out?

When commenting in, it doesn't work because
git clone -s --bare .git foo.git fails, as foo.git is already there.

That problem removed it succeeds.
Junio C Hamano· May 18, 2015, 19:08 UTC · re: Stefan Beller · lore

Re: [PATCH] t1020: cleanup subdirectory tests a little

Stefan Beller <sbeller@google.com> writes:
> On Mon, May 18, 2015 at 11:30 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
>> If it does not still work, shouldn't it be marked as
>> test_expect_failure instead of being commented out?
> When commenting in, it doesn't work because
> git clone -s --bare .git foo.git fails, as foo.git is already there.
> That problem removed it succeeds.

That is to be expected no? If we want to have both working, we would need to rename to allow them to co-exist.

I think what is going on is this (quoting the original without any of your patches):

| test_expect_success 'no file/rev ambiguity check inside .git' '
| 	git commit -a -m 1 &&
| 	(
| 		cd .git &&
| 		git show -s HEAD
| 	)
| '

Inside a $GIT_DIR that has a working tree associated to it, does Git notice that the user could never have meant a pathspec HEAD and proceed without complaining?

| test_expect_success 'no file/rev ambiguity check inside a bare repo' '
| 	git clone -s --bare .git foo.git &&
| 	(
| 		cd foo.git &&
| 		GIT_DIR=. git show -s HEAD
| 	)
| '

Inside a bare repository, does Git notice that the user could never have meant a pathspec HEAD and proceed without complaining, *IF* we helped Git by an explicit GIT_DIR=. exported?

I suspect that back when this test was written, Git didn't correctly work *WITHOUT* the explicit help, which is what the comment says for the other commented-out one.

| # This still does not work as it should...
| : test_expect_success 'no file/rev ambiguity check inside a bare repo' '
| 	git clone -s --bare .git foo.git &&
| 	(
| 		cd foo.git &&
| 		git show -s HEAD
| 	)
| '

And this was the real test the previous one wanted to be back then but somehow couldn't.

I agree that the temporarly test put in place with helping GIT_DIR=. should have been commented why it has a seemingly unnecessary GIT_DIR=. in it. Without it it may be confusing, unless you can correctly guess why the commented-out one is there and the helping one is not exactly what we wanted to test. Perhaps

	git clone -s --bare .git foo.git &&
	(
		cd foo.git &&
                # older Git needed help by exporting GIT_DIR=.
                # to realize that it is inside a bare repository
                GIT_DIR=. git show -s HEAD
	)
or something.

I do not know if we have fixed that bug over time offhand, so we'd need to do some digging to verify the "older" above.

But assuming we have, I would say that the right thing to do is to keep the original one with help (because we do not want it to break in the future), and reinstate the commented-out one that does the right thing without us helping. But we would obviously need to rename foo.git to bar.git or something else while doing so.

If we still have that bug, then I think the right thing is to do the same as above, but mark the one that exhibits the bug to expect failure. That way, people can hunt and fix the bug (probably in setup.c somewhere).

Thanks.
Stefan Beller· May 18, 2015, 21:10 UTC · re: Junio C Hamano · lore

[PATCH] subdirectory tests: code cleanup, uncomment test

Originally the test in t1020 was meant to not include setting the GIT_DIR when testing inside a bare repository as it did not work without setting GIT_DIR explicitly.

Nowadays the test as originally intended works, so add it to the test suite. We'll keep the test, which has been run through all years as another test for finding regressions.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
Junio, thanks for providing the context!

I tried tracking down when this changes via bisect, though I messed up. By looking through the code I find these commits most promising to have fixed the underlying issue (I am no expert on subdirectory treatment) 337e51c (Use git_config_early() instead of git_config() during repo setup, 2010-11-26) 72183cb (Fix gitdir detection when in subdir of gitdir, 2009-01-16) 9951d3b (setup: clean up setup_discovered_git_dir(), 2010-11-26)

 t/t1020-subdirectory.sh | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)
Show changes to t/t1020-subdirectory.sh +7 −4
diff --git a/t/t1020-subdirectory.sh b/t/t1020-subdirectory.sh
index 2edb4f2..022641d 100755
--- a/t/t1020-subdirectory.sh
+++ b/t/t1020-subdirectory.sh
@@ -162,16 +162,20 @@ test_expect_success 'no file/rev ambiguity check inside .git' '
 	)
 '
 
-test_expect_success 'no file/rev ambiguity check inside a bare repo' '
+test_expect_success '(historic) no file/rev ambiguity check inside a bare repo' '
+	test_when_finished "rm -fr foo.git" &&
 	git clone -s --bare .git foo.git &&
 	(
 		cd foo.git &&
+		# older Git needed help by exporting GIT_DIR=.
+		# to realize that it is inside a bare repository.
+		# We keep this test around for regression testing.
 		GIT_DIR=. git show -s HEAD
 	)
 '
 
-# This still does not work as it should...
-: test_expect_success 'no file/rev ambiguity check inside a bare repo' '
+test_expect_success 'no file/rev ambiguity check inside a bare repo' '
+	test_when_finished "rm -fr foo.git" &&
 	git clone -s --bare .git foo.git &&
 	(
 		cd foo.git &&
@@ -180,7 +184,6 @@ test_expect_success 'no file/rev ambiguity check inside a bare repo' '
 '
 
 test_expect_success SYMLINKS 'detection should not be fooled by a symlink' '
-	rm -fr foo.git &&
 	git clone -s .git another &&
 	ln -s another yetanother &&
 	(

-- 
2.4.0.194.gc518059
Junio C Hamano· May 18, 2015, 21:21 UTC · re: Stefan Beller · lore

Re: [PATCH] subdirectory tests: code cleanup, uncomment test

Stefan Beller <sbeller@google.com> writes:
Show 19 quoted lines
> Originally the test in t1020 was meant to not include setting the GIT_DIR
> when testing inside a bare repository as it did not work without setting
> GIT_DIR explicitly.
>
> Nowadays the test as originally intended works, so add it to the test
> suite. We'll keep the test, which has been run through all years as another
> test for finding regressions.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>
> Junio, thanks for providing the context!
>
> I tried tracking down when this changes via bisect, though I messed up.
> By looking through the code I find these commits most promising to have
> fixed the underlying issue (I am no expert on subdirectory treatment)
> 337e51c (Use git_config_early() instead of git_config() during repo setup, 2010-11-26)
> 72183cb (Fix gitdir detection when in subdir of gitdir, 2009-01-16)
> 9951d3b (setup: clean up setup_discovered_git_dir(), 2010-11-26)
Thanks for digging.

I personally do not think we would need to say "historic" (as it makes it sound as if we do not care if the use case is deprecated and dropped in the future) but I do not offhand think of a better label for that test (other than doing the cop-out "test (1)" vs "test (2)"), so let's queue this as-is.

Show 40 quoted lines
>
>  t/t1020-subdirectory.sh | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/t/t1020-subdirectory.sh b/t/t1020-subdirectory.sh
> index 2edb4f2..022641d 100755
> --- a/t/t1020-subdirectory.sh
> +++ b/t/t1020-subdirectory.sh
> @@ -162,16 +162,20 @@ test_expect_success 'no file/rev ambiguity check inside .git' '
>  	)
>  '
>  
> -test_expect_success 'no file/rev ambiguity check inside a bare repo' '
> +test_expect_success '(historic) no file/rev ambiguity check inside a bare repo' '
> +	test_when_finished "rm -fr foo.git" &&
>  	git clone -s --bare .git foo.git &&
>  	(
>  		cd foo.git &&
> +		# older Git needed help by exporting GIT_DIR=.
> +		# to realize that it is inside a bare repository.
> +		# We keep this test around for regression testing.
>  		GIT_DIR=. git show -s HEAD
>  	)
>  '
>  
> -# This still does not work as it should...
> -: test_expect_success 'no file/rev ambiguity check inside a bare repo' '
> +test_expect_success 'no file/rev ambiguity check inside a bare repo' '
> +	test_when_finished "rm -fr foo.git" &&
>  	git clone -s --bare .git foo.git &&
>  	(
>  		cd foo.git &&
> @@ -180,7 +184,6 @@ test_expect_success 'no file/rev ambiguity check inside a bare repo' '
>  '
>  
>  test_expect_success SYMLINKS 'detection should not be fooled by a symlink' '
> -	rm -fr foo.git &&
>  	git clone -s .git another &&
>  	ln -s another yetanother &&
>  	(
Stefan Beller· May 18, 2015, 21:29 UTC · re: Junio C Hamano · lore

Re: [PATCH] subdirectory tests: code cleanup, uncomment test

On Mon, May 18, 2015 at 2:21 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 29 quoted lines
> Stefan Beller <sbeller@google.com> writes:
>
>> Originally the test in t1020 was meant to not include setting the GIT_DIR
>> when testing inside a bare repository as it did not work without setting
>> GIT_DIR explicitly.
>>
>> Nowadays the test as originally intended works, so add it to the test
>> suite. We'll keep the test, which has been run through all years as another
>> test for finding regressions.
>>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>>
>> Junio, thanks for providing the context!
>>
>> I tried tracking down when this changes via bisect, though I messed up.
>> By looking through the code I find these commits most promising to have
>> fixed the underlying issue (I am no expert on subdirectory treatment)
>> 337e51c (Use git_config_early() instead of git_config() during repo setup, 2010-11-26)
>> 72183cb (Fix gitdir detection when in subdir of gitdir, 2009-01-16)
>> 9951d3b (setup: clean up setup_discovered_git_dir(), 2010-11-26)
>
> Thanks for digging.
>
> I personally do not think we would need to say "historic" (as it
> makes it sound as if we do not care if the use case is deprecated
> and dropped in the future) but I do not offhand think of a better
> label for that test (other than doing the cop-out "test (1)" vs
> "test (2)"), so let's queue this as-is.
At first I was in the mood of labeling that test
    no file/rev ambiguity check inside a bare repo with GIT_DIR help
for older Gits

as in nursing homes for elder people, but I refrained from doing so as it sounded derogatory in my mind and it broke the line limit.

I am not happy with (historic) either, maybe "(explicit GIT_DIR)" is describing the test better without giving the reader the thoughts as you raised here?

Show 42 quoted lines
>
>
>>
>>  t/t1020-subdirectory.sh | 11 +++++++----
>>  1 file changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/t/t1020-subdirectory.sh b/t/t1020-subdirectory.sh
>> index 2edb4f2..022641d 100755
>> --- a/t/t1020-subdirectory.sh
>> +++ b/t/t1020-subdirectory.sh
>> @@ -162,16 +162,20 @@ test_expect_success 'no file/rev ambiguity check inside .git' '
>>       )
>>  '
>>
>> -test_expect_success 'no file/rev ambiguity check inside a bare repo' '
>> +test_expect_success '(historic) no file/rev ambiguity check inside a bare repo' '
>> +     test_when_finished "rm -fr foo.git" &&
>>       git clone -s --bare .git foo.git &&
>>       (
>>               cd foo.git &&
>> +             # older Git needed help by exporting GIT_DIR=.
>> +             # to realize that it is inside a bare repository.
>> +             # We keep this test around for regression testing.
>>               GIT_DIR=. git show -s HEAD
>>       )
>>  '
>>
>> -# This still does not work as it should...
>> -: test_expect_success 'no file/rev ambiguity check inside a bare repo' '
>> +test_expect_success 'no file/rev ambiguity check inside a bare repo' '
>> +     test_when_finished "rm -fr foo.git" &&
>>       git clone -s --bare .git foo.git &&
>>       (
>>               cd foo.git &&
>> @@ -180,7 +184,6 @@ test_expect_success 'no file/rev ambiguity check inside a bare repo' '
>>  '
>>
>>  test_expect_success SYMLINKS 'detection should not be fooled by a symlink' '
>> -     rm -fr foo.git &&
>>       git clone -s .git another &&
>>       ln -s another yetanother &&
>>       (
Jonathan Nieder· May 18, 2015, 22:03 UTC · re: Stefan Beller · lore

Re: [PATCH] subdirectory tests: code cleanup, uncomment test

Stefan Beller wrote:
> I am not happy with (historic) either, maybe "(explicit GIT_DIR)"
> is describing the test better without giving the reader the thoughts
> as you raised here?

The general principle I use is to try to briefly describe what hypothesis the code is trying to test, so that if it fails someone knows what that means.

In this case, I could do
	test_expect_success 'no file/rev ambiguity with explicit GIT_DIR=.' '
[...]
Show 5 quoted lines
>>>               cd foo.git &&
>>> +             # older Git needed help by exporting GIT_DIR=.
>>> +             # to realize that it is inside a bare repository.
>>> +             # We keep this test around for regression testing.
>>>               GIT_DIR=. git show -s HEAD

I don't think this comment is needed, since it doesn't make it clearer what the test is about.

Thanks, Jonathan

Junio C Hamano· May 19, 2015, 21:45 UTC · re: Stefan Beller · lore

Re: [PATCH] subdirectory tests: code cleanup, uncomment test

Stefan Beller <sbeller@google.com> writes:
> I am not happy with (historic) either, maybe "(explicit GIT_DIR)"
> is describing the test better without giving the reader the thoughts
> as you raised here?

Yeah, there are different ways for us to notice that we are in a bare repository and (explicit GIT_DIR) may be a good label for this. 

Thanks.

← back to recent threads