[PATCH] t1020: cleanup subdirectory tests a little

[PATCH] t1020: cleanup subdirectory tests a little

[Stefan Beller]

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

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

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

[Junio C Hamano]

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

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.

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

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

[Junio C Hamano]

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

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

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

[Stefan Beller]

On Mon, May 18, 2015 at 11:30 AM, Junio C Hamano <gitster@pobox.com> wrote:
> 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:

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.

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

[Junio C Hamano]

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.

[PATCH] subdirectory tests: code cleanup, uncomment test

[Stefan Beller]

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

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

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

[Junio C Hamano]

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.


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

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

[Stefan Beller]

On Mon, May 18, 2015 at 2:21 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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?

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

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

[Jonathan Nieder]

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

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

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

[Junio C Hamano]

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.