{"thread":{"id":"39360","subject":"[PATCH] t1020: cleanup subdirectory tests a little","startedAt":"2015-05-18T18:13:37Z","lastAt":"2015-05-19T21:45:04Z","messageCount":10,"participants":["Stefan Beller","Junio C Hamano","Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"261485","messageId":"1431972817-18178-1-git-send-email-sbeller@google.com","threadId":"39360","inReplyTo":null,"subject":"[PATCH] t1020: cleanup subdirectory tests a little","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-05-18T18:13:37Z","receivedAt":"2015-05-18T18:13:37Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"When looking through existing tests to point out good style I came across\nt1020, which has a test commented out and the comment wasn't helping me\neither of what the test should accomplish in the future. The code of the\ntest is the same as the test before except setting GIT_DIR=. explicitly,\nso it did not ring a bell for me as well.\n\nThis removes the test, which may have been confusing readers since 2010.\n\nAdditionally moves the \"rm -fr foo.git\" of the next test (where it is\nunrelated) to the previous test, where it makes sense as a cleanup.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n t/t1020-subdirectory.sh | 11 +----------\n 1 file changed, 1 insertion(+), 10 deletions(-)\n\ndiff --git a/t/t1020-subdirectory.sh b/t/t1020-subdirectory.sh\nindex 2edb4f2..4470ede 100755\n--- a/t/t1020-subdirectory.sh\n+++ b/t/t1020-subdirectory.sh\n@@ -163,6 +163,7 @@ test_expect_success 'no file/rev ambiguity check inside .git' '\n '\n \n test_expect_success 'no file/rev ambiguity check inside a bare repo' '\n+\ttest_when_finished \"rm -fr foo.git\" &&\n \tgit clone -s --bare .git foo.git &&\n \t(\n \t\tcd foo.git &&\n@@ -170,17 +171,7 @@ test_expect_success 'no file/rev ambiguity check inside a bare repo' '\n \t)\n '\n \n-# This still does not work as it should...\n-: test_expect_success 'no file/rev ambiguity check inside a bare repo' '\n-\tgit clone -s --bare .git foo.git &&\n-\t(\n-\t\tcd foo.git &&\n-\t\tgit show -s HEAD\n-\t)\n-'\n-\n test_expect_success SYMLINKS 'detection should not be fooled by a symlink' '\n-\trm -fr foo.git &&\n \tgit clone -s .git another &&\n \tln -s another yetanother &&\n \t(\n-- \n2.4.0.194.gc518059\n"},{"id":"261488","messageId":"xmqqwq05buz1.fsf@gitster.dls.corp.google.com","threadId":"39360","inReplyTo":"1431972817-18178-1-git-send-email-sbeller@google.com","subject":"Re: [PATCH] t1020: cleanup subdirectory tests a little","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-18T18:29:06Z","receivedAt":"2015-05-18T18:29:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> When looking through existing tests to point out good style I came across\n> t1020, which has a test commented out and the comment wasn't helping me\n> either of what the test should accomplish in the future. The code of the\n> test is the same as the test before except setting GIT_DIR=. explicitly,\n> so it did not ring a bell for me as well.\n\nI think this one should be clear, especially if you did notice the\none that sets GIT_DIR=. explicitly.  It is saying that \"git show -s\nHEAD\" inside the bare repository should be intelligent enough to\nrealize that it is inside bare repository (hence HEAD cannot be a\nfile in the working tree); the user's asking for \"HEAD\" therefore\nmust mean \"the tip commit\", and never \"(by default the tip commit)\nfiltered to the pathspec HEAD\".\n\nIf it does not still work, shouldn't it be marked as\ntest_expect_failure instead of being commented out?\n\nThe first and last hunk to use when-finished looks like a good\nchange, but is unrelated.\n\n>\n> This removes the test, which may have been confusing readers since 2010.\n>\n> Additionally moves the \"rm -fr foo.git\" of the next test (where it is\n> unrelated) to the previous test, where it makes sense as a cleanup.\n>\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n>  t/t1020-subdirectory.sh | 11 +----------\n>  1 file changed, 1 insertion(+), 10 deletions(-)\n>\n> diff --git a/t/t1020-subdirectory.sh b/t/t1020-subdirectory.sh\n> index 2edb4f2..4470ede 100755\n> --- a/t/t1020-subdirectory.sh\n> +++ b/t/t1020-subdirectory.sh\n> @@ -163,6 +163,7 @@ test_expect_success 'no file/rev ambiguity check inside .git' '\n>  '\n>  \n>  test_expect_success 'no file/rev ambiguity check inside a bare repo' '\n> +\ttest_when_finished \"rm -fr foo.git\" &&\n>  \tgit clone -s --bare .git foo.git &&\n>  \t(\n>  \t\tcd foo.git &&\n> @@ -170,17 +171,7 @@ test_expect_success 'no file/rev ambiguity check inside a bare repo' '\n>  \t)\n>  '\n>  \n> -# This still does not work as it should...\n> -: test_expect_success 'no file/rev ambiguity check inside a bare repo' '\n> -\tgit clone -s --bare .git foo.git &&\n> -\t(\n> -\t\tcd foo.git &&\n> -\t\tgit show -s HEAD\n> -\t)\n> -'\n> -\n>  test_expect_success SYMLINKS 'detection should not be fooled by a symlink' '\n> -\trm -fr foo.git &&\n>  \tgit clone -s .git another &&\n>  \tln -s another yetanother &&\n>  \t(\n"},{"id":"261489","messageId":"xmqqsiatbuwb.fsf@gitster.dls.corp.google.com","threadId":"39360","inReplyTo":"xmqqwq05buz1.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] t1020: cleanup subdirectory tests a little","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-18T18:30:44Z","receivedAt":"2015-05-18T18:30:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> When looking through existing tests to point out good style I came across\n>> t1020, which has a test commented out and the comment wasn't helping me\n>> either of what the test should accomplish in the future. The code of the\n>> test is the same as the test before except setting GIT_DIR=. explicitly,\n>> so it did not ring a bell for me as well.\n>\n> I think this one should be clear, especially if you did notice the\n> one that sets GIT_DIR=. explicitly.  It is saying that \"git show -s\n> HEAD\" inside the bare repository should be intelligent enough to\n> realize that it is inside bare repository (hence HEAD cannot be a\n> file in the working tree); the user's asking for \"HEAD\" therefore\n> must mean \"the tip commit\", and never \"(by default the tip commit)\n> filtered to the pathspec HEAD\".\n\nI forgot to conclude that sentence: \"... and it should be able to do\nso without the help of an explicit GIT_DIR=.\"\n\n> If it does not still work, shouldn't it be marked as\n> test_expect_failure instead of being commented out?\n"},{"id":"261490","messageId":"CAGZ79kZjjKD6J3bQ0SWBUO-6bTY1Ld=Y7mRHVbw+RO5AFN+r5A@mail.gmail.com","threadId":"39360","inReplyTo":"xmqqsiatbuwb.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] t1020: cleanup subdirectory tests a little","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-05-18T18:36:49Z","receivedAt":"2015-05-18T18:36:49Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, May 18, 2015 at 11:30 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n>> Stefan Beller <sbeller@google.com> writes:\n>>\n>>> When looking through existing tests to point out good style I came across\n>>> t1020, which has a test commented out and the comment wasn't helping me\n>>> either of what the test should accomplish in the future. The code of the\n>>> test is the same as the test before except setting GIT_DIR=. explicitly,\n>>> so it did not ring a bell for me as well.\n>>\n>> I think this one should be clear, especially if you did notice the\n>> one that sets GIT_DIR=. explicitly.  It is saying that \"git show -s\n>> HEAD\" inside the bare repository should be intelligent enough to\n>> realize that it is inside bare repository (hence HEAD cannot be a\n>> file in the working tree); the user's asking for \"HEAD\" therefore\n>> must mean \"the tip commit\", and never \"(by default the tip commit)\n>> filtered to the pathspec HEAD\".\n>\n> I forgot to conclude that sentence: \"... and it should be able to do\n> so without the help of an explicit GIT_DIR=.\"\n\nNot sure I follow you there. So currently there are 2 tests having the same\nname, and doing exactly the same thing, apart from setting the GIT_DIR\nand one of them being commented out.\n\nSo I don't understand, what should be tested *additionally* in the second test,\nwhere GIT_DIR is not set. (The naming doesn't hint at testing explicit GIT_DIR,\nso we test the  'no file/rev ambiguity check inside a bare repo' again.)\n\nMaybe this diff is better for review:\n\ndiff --git a/t/t1020-subdirectory.sh b/t/t1020-subdirectory.sh\nindex 2edb4f2..4470ede 100755\n--- a/t/t1020-subdirectory.sh\n+++ b/t/t1020-subdirectory.sh\n@@ -163,24 +163,15 @@ test_expect_success 'no file/rev ambiguity check\ninside .git' '\n '\n\n test_expect_success 'no file/rev ambiguity check inside a bare repo' '\n+ test_when_finished \"rm -fr foo.git\" &&\n  git clone -s --bare .git foo.git &&\n  (\n  cd foo.git &&\n  GIT_DIR=. git show -s HEAD\n  )\n '\n\n-# This still does not work as it should...\n-: test_expect_success 'no file/rev ambiguity check inside a bare repo' '\n- git clone -s --bare .git foo.git &&\n- (\n- cd foo.git &&\n- git show -s HEAD\n- )\n-'\n-\n test_expect_success SYMLINKS 'detection should not be fooled by a symlink' '\n- rm -fr foo.git &&\n  git clone -s .git another &&\n  ln -s another yetanother &&\n  (\n-- \n2.4.0.194.gc518059\n\n\n\n>\n>> If it does not still work, shouldn't it be marked as\n>> test_expect_failure instead of being commented out?\n\nWhen commenting in, it doesn't work because\ngit clone -s --bare .git foo.git fails, as foo.git is already there.\n\nThat problem removed it succeeds.\n"},{"id":"261496","messageId":"xmqqlhglbt4k.fsf@gitster.dls.corp.google.com","threadId":"39360","inReplyTo":"CAGZ79kZjjKD6J3bQ0SWBUO-6bTY1Ld=Y7mRHVbw+RO5AFN+r5A@mail.gmail.com","subject":"Re: [PATCH] t1020: cleanup subdirectory tests a little","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-18T19:08:59Z","receivedAt":"2015-05-18T19:08:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> On Mon, May 18, 2015 at 11:30 AM, Junio C Hamano <gitster@pobox.com> wrote:\n>\n>> If it does not still work, shouldn't it be marked as\n>> test_expect_failure instead of being commented out?\n\n> When commenting in, it doesn't work because\n> git clone -s --bare .git foo.git fails, as foo.git is already there.\n> That problem removed it succeeds.\n\nThat is to be expected no?  If we want to have both working, we\nwould need to rename to allow them to co-exist.\n\nI think what is going on is this (quoting the original without any\nof your patches):\n\n| test_expect_success 'no file/rev ambiguity check inside .git' '\n| \tgit commit -a -m 1 &&\n| \t(\n| \t\tcd .git &&\n| \t\tgit show -s HEAD\n| \t)\n| '\n\nInside a $GIT_DIR that has a working tree associated to it, does Git\nnotice that the user could never have meant a pathspec HEAD and\nproceed without complaining?\n\n| test_expect_success 'no file/rev ambiguity check inside a bare repo' '\n| \tgit clone -s --bare .git foo.git &&\n| \t(\n| \t\tcd foo.git &&\n| \t\tGIT_DIR=. git show -s HEAD\n| \t)\n| '\n\nInside a bare repository, does Git notice that the user could never\nhave meant a pathspec HEAD and proceed without complaining, *IF* we\nhelped Git by an explicit GIT_DIR=. exported?\n\nI suspect that back when this test was written, Git didn't correctly\nwork *WITHOUT* the explicit help, which is what the comment says for\nthe other commented-out one.\n\n| # This still does not work as it should...\n| : test_expect_success 'no file/rev ambiguity check inside a bare repo' '\n| \tgit clone -s --bare .git foo.git &&\n| \t(\n| \t\tcd foo.git &&\n| \t\tgit show -s HEAD\n| \t)\n| '\n\nAnd this was the real test the previous one wanted to be back then\nbut somehow couldn't.\n\nI agree that the temporarly test put in place with helping GIT_DIR=.\nshould have been commented why it has a seemingly unnecessary\nGIT_DIR=. in it.  Without it it may be confusing, unless you can\ncorrectly guess why the commented-out one is there and the helping\none is not exactly what we wanted to test.  Perhaps\n\n\tgit clone -s --bare .git foo.git &&\n\t(\n\t\tcd foo.git &&\n                # older Git needed help by exporting GIT_DIR=.\n                # to realize that it is inside a bare repository\n                GIT_DIR=. git show -s HEAD\n\t)\n\nor something.\n\nI do not know if we have fixed that bug over time offhand, so we'd\nneed to do some digging to verify the \"older\" above.\n\nBut assuming we have, I would say that the right thing to do is to\nkeep the original one with help (because we do not want it to break\nin the future), and reinstate the commented-out one that does the\nright thing without us helping.  But we would obviously need to\nrename foo.git to bar.git or something else while doing so.\n\nIf we still have that bug, then I think the right thing is to do the\nsame as above, but mark the one that exhibits the bug to expect\nfailure.  That way, people can hunt and fix the bug (probably in\nsetup.c somewhere).\n\nThanks.\n"},{"id":"261514","messageId":"1431983426-4346-1-git-send-email-sbeller@google.com","threadId":"39360","inReplyTo":"xmqqlhglbt4k.fsf@gitster.dls.corp.google.com","subject":"[PATCH] subdirectory tests: code cleanup, uncomment test","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-05-18T21:10:26Z","receivedAt":"2015-05-18T21:10:26Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Originally the test in t1020 was meant to not include setting the GIT_DIR\nwhen testing inside a bare repository as it did not work without setting\nGIT_DIR explicitly.\n\nNowadays the test as originally intended works, so add it to the test\nsuite. We'll keep the test, which has been run through all years as another\ntest for finding regressions.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n\nJunio, thanks for providing the context!\n\nI tried tracking down when this changes via bisect, though I messed up.\nBy looking through the code I find these commits most promising to have\nfixed the underlying issue (I am no expert on subdirectory treatment)\n337e51c (Use git_config_early() instead of git_config() during repo setup, 2010-11-26)\n72183cb (Fix gitdir detection when in subdir of gitdir, 2009-01-16)\n9951d3b (setup: clean up setup_discovered_git_dir(), 2010-11-26)\n\n\n t/t1020-subdirectory.sh | 11 +++++++----\n 1 file changed, 7 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t1020-subdirectory.sh b/t/t1020-subdirectory.sh\nindex 2edb4f2..022641d 100755\n--- a/t/t1020-subdirectory.sh\n+++ b/t/t1020-subdirectory.sh\n@@ -162,16 +162,20 @@ test_expect_success 'no file/rev ambiguity check inside .git' '\n \t)\n '\n \n-test_expect_success 'no file/rev ambiguity check inside a bare repo' '\n+test_expect_success '(historic) no file/rev ambiguity check inside a bare repo' '\n+\ttest_when_finished \"rm -fr foo.git\" &&\n \tgit clone -s --bare .git foo.git &&\n \t(\n \t\tcd foo.git &&\n+\t\t# older Git needed help by exporting GIT_DIR=.\n+\t\t# to realize that it is inside a bare repository.\n+\t\t# We keep this test around for regression testing.\n \t\tGIT_DIR=. git show -s HEAD\n \t)\n '\n \n-# This still does not work as it should...\n-: test_expect_success 'no file/rev ambiguity check inside a bare repo' '\n+test_expect_success 'no file/rev ambiguity check inside a bare repo' '\n+\ttest_when_finished \"rm -fr foo.git\" &&\n \tgit clone -s --bare .git foo.git &&\n \t(\n \t\tcd foo.git &&\n@@ -180,7 +184,6 @@ test_expect_success 'no file/rev ambiguity check inside a bare repo' '\n '\n \n test_expect_success SYMLINKS 'detection should not be fooled by a symlink' '\n-\trm -fr foo.git &&\n \tgit clone -s .git another &&\n \tln -s another yetanother &&\n \t(\n-- \n2.4.0.194.gc518059\n"},{"id":"261515","messageId":"xmqqvbfpa8ff.fsf@gitster.dls.corp.google.com","threadId":"39360","inReplyTo":"1431983426-4346-1-git-send-email-sbeller@google.com","subject":"Re: [PATCH] subdirectory tests: code cleanup, uncomment test","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-18T21:21:24Z","receivedAt":"2015-05-18T21:21:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> Originally the test in t1020 was meant to not include setting the GIT_DIR\n> when testing inside a bare repository as it did not work without setting\n> GIT_DIR explicitly.\n>\n> Nowadays the test as originally intended works, so add it to the test\n> suite. We'll keep the test, which has been run through all years as another\n> test for finding regressions.\n>\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n>\n> Junio, thanks for providing the context!\n>\n> I tried tracking down when this changes via bisect, though I messed up.\n> By looking through the code I find these commits most promising to have\n> fixed the underlying issue (I am no expert on subdirectory treatment)\n> 337e51c (Use git_config_early() instead of git_config() during repo setup, 2010-11-26)\n> 72183cb (Fix gitdir detection when in subdir of gitdir, 2009-01-16)\n> 9951d3b (setup: clean up setup_discovered_git_dir(), 2010-11-26)\n\nThanks for digging.\n\nI personally do not think we would need to say \"historic\" (as it\nmakes it sound as if we do not care if the use case is deprecated\nand dropped in the future) but I do not offhand think of a better\nlabel for that test (other than doing the cop-out \"test (1)\" vs\n\"test (2)\"), so let's queue this as-is.\n\n\n>\n>  t/t1020-subdirectory.sh | 11 +++++++----\n>  1 file changed, 7 insertions(+), 4 deletions(-)\n>\n> diff --git a/t/t1020-subdirectory.sh b/t/t1020-subdirectory.sh\n> index 2edb4f2..022641d 100755\n> --- a/t/t1020-subdirectory.sh\n> +++ b/t/t1020-subdirectory.sh\n> @@ -162,16 +162,20 @@ test_expect_success 'no file/rev ambiguity check inside .git' '\n>  \t)\n>  '\n>  \n> -test_expect_success 'no file/rev ambiguity check inside a bare repo' '\n> +test_expect_success '(historic) no file/rev ambiguity check inside a bare repo' '\n> +\ttest_when_finished \"rm -fr foo.git\" &&\n>  \tgit clone -s --bare .git foo.git &&\n>  \t(\n>  \t\tcd foo.git &&\n> +\t\t# older Git needed help by exporting GIT_DIR=.\n> +\t\t# to realize that it is inside a bare repository.\n> +\t\t# We keep this test around for regression testing.\n>  \t\tGIT_DIR=. git show -s HEAD\n>  \t)\n>  '\n>  \n> -# This still does not work as it should...\n> -: test_expect_success 'no file/rev ambiguity check inside a bare repo' '\n> +test_expect_success 'no file/rev ambiguity check inside a bare repo' '\n> +\ttest_when_finished \"rm -fr foo.git\" &&\n>  \tgit clone -s --bare .git foo.git &&\n>  \t(\n>  \t\tcd foo.git &&\n> @@ -180,7 +184,6 @@ test_expect_success 'no file/rev ambiguity check inside a bare repo' '\n>  '\n>  \n>  test_expect_success SYMLINKS 'detection should not be fooled by a symlink' '\n> -\trm -fr foo.git &&\n>  \tgit clone -s .git another &&\n>  \tln -s another yetanother &&\n>  \t(\n"},{"id":"261517","messageId":"CAGZ79kZ9h=Oev_mymFBRNyAGokAe8-vWh_D12VZYHTUv20Djvg@mail.gmail.com","threadId":"39360","inReplyTo":"xmqqvbfpa8ff.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] subdirectory tests: code cleanup, uncomment test","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-05-18T21:29:47Z","receivedAt":"2015-05-18T21:29:47Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, May 18, 2015 at 2:21 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> Originally the test in t1020 was meant to not include setting the GIT_DIR\n>> when testing inside a bare repository as it did not work without setting\n>> GIT_DIR explicitly.\n>>\n>> Nowadays the test as originally intended works, so add it to the test\n>> suite. We'll keep the test, which has been run through all years as another\n>> test for finding regressions.\n>>\n>> Signed-off-by: Stefan Beller <sbeller@google.com>\n>> ---\n>>\n>> Junio, thanks for providing the context!\n>>\n>> I tried tracking down when this changes via bisect, though I messed up.\n>> By looking through the code I find these commits most promising to have\n>> fixed the underlying issue (I am no expert on subdirectory treatment)\n>> 337e51c (Use git_config_early() instead of git_config() during repo setup, 2010-11-26)\n>> 72183cb (Fix gitdir detection when in subdir of gitdir, 2009-01-16)\n>> 9951d3b (setup: clean up setup_discovered_git_dir(), 2010-11-26)\n>\n> Thanks for digging.\n>\n> I personally do not think we would need to say \"historic\" (as it\n> makes it sound as if we do not care if the use case is deprecated\n> and dropped in the future) but I do not offhand think of a better\n> label for that test (other than doing the cop-out \"test (1)\" vs\n> \"test (2)\"), so let's queue this as-is.\n\nAt first I was in the mood of labeling that test\n\n    no file/rev ambiguity check inside a bare repo with GIT_DIR help\nfor older Gits\n\nas in nursing homes for elder people, but I refrained from doing so\nas it sounded derogatory in my mind and it broke the line limit.\n\nI am not happy with (historic) either, maybe \"(explicit GIT_DIR)\"\nis describing the test better without giving the reader the thoughts\nas you raised here?\n\n>\n>\n>>\n>>  t/t1020-subdirectory.sh | 11 +++++++----\n>>  1 file changed, 7 insertions(+), 4 deletions(-)\n>>\n>> diff --git a/t/t1020-subdirectory.sh b/t/t1020-subdirectory.sh\n>> index 2edb4f2..022641d 100755\n>> --- a/t/t1020-subdirectory.sh\n>> +++ b/t/t1020-subdirectory.sh\n>> @@ -162,16 +162,20 @@ test_expect_success 'no file/rev ambiguity check inside .git' '\n>>       )\n>>  '\n>>\n>> -test_expect_success 'no file/rev ambiguity check inside a bare repo' '\n>> +test_expect_success '(historic) no file/rev ambiguity check inside a bare repo' '\n>> +     test_when_finished \"rm -fr foo.git\" &&\n>>       git clone -s --bare .git foo.git &&\n>>       (\n>>               cd foo.git &&\n>> +             # older Git needed help by exporting GIT_DIR=.\n>> +             # to realize that it is inside a bare repository.\n>> +             # We keep this test around for regression testing.\n>>               GIT_DIR=. git show -s HEAD\n>>       )\n>>  '\n>>\n>> -# This still does not work as it should...\n>> -: test_expect_success 'no file/rev ambiguity check inside a bare repo' '\n>> +test_expect_success 'no file/rev ambiguity check inside a bare repo' '\n>> +     test_when_finished \"rm -fr foo.git\" &&\n>>       git clone -s --bare .git foo.git &&\n>>       (\n>>               cd foo.git &&\n>> @@ -180,7 +184,6 @@ test_expect_success 'no file/rev ambiguity check inside a bare repo' '\n>>  '\n>>\n>>  test_expect_success SYMLINKS 'detection should not be fooled by a symlink' '\n>> -     rm -fr foo.git &&\n>>       git clone -s .git another &&\n>>       ln -s another yetanother &&\n>>       (\n"},{"id":"261520","messageId":"20150518220318.GE5586@google.com","threadId":"39360","inReplyTo":"CAGZ79kZ9h=Oev_mymFBRNyAGokAe8-vWh_D12VZYHTUv20Djvg@mail.gmail.com","subject":"Re: [PATCH] subdirectory tests: code cleanup, uncomment test","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2015-05-18T22:03:19Z","receivedAt":"2015-05-18T22:03:19Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Stefan Beller wrote:\n\n> I am not happy with (historic) either, maybe \"(explicit GIT_DIR)\"\n> is describing the test better without giving the reader the thoughts\n> as you raised here?\n\nThe general principle I use is to try to briefly describe what\nhypothesis the code is trying to test, so that if it fails someone knows\nwhat that means.\n\nIn this case, I could do\n\n\ttest_expect_success 'no file/rev ambiguity with explicit GIT_DIR=.' '\n\n[...]\n>>>               cd foo.git &&\n>>> +             # older Git needed help by exporting GIT_DIR=.\n>>> +             # to realize that it is inside a bare repository.\n>>> +             # We keep this test around for regression testing.\n>>>               GIT_DIR=. git show -s HEAD\n\nI don't think this comment is needed, since it doesn't make it clearer\nwhat the test is about.\n\nThanks,\nJonathan\n"},{"id":"261603","messageId":"xmqq4mn86y3j.fsf@gitster.dls.corp.google.com","threadId":"39360","inReplyTo":"CAGZ79kZ9h=Oev_mymFBRNyAGokAe8-vWh_D12VZYHTUv20Djvg@mail.gmail.com","subject":"Re: [PATCH] subdirectory tests: code cleanup, uncomment test","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-19T21:45:04Z","receivedAt":"2015-05-19T21:45:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> I am not happy with (historic) either, maybe \"(explicit GIT_DIR)\"\n> is describing the test better without giving the reader the thoughts\n> as you raised here?\n\nYeah, there are different ways for us to notice that we are in a\nbare repository and (explicit GIT_DIR) may be a good label for\nthis.  \n\nThanks.\n"}]}