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

Stefan Beller [off-list ref] 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 <redacted>
---

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.

On Mon, May 18, 2015 at 2:21 PM, Junio C Hamano [off-list ref] wrote:

Stefan Beller [off-list ref] writes:

quoted

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

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?

quoted

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

[...]

quoted

quoted

              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

Stefan Beller [off-list ref] 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.