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

From: Junio C Hamano <[email-masked]>
Date: Mon, 18 May 2015 12:08:59 -0700

Stefan Beller <[email-masked]> writes:

> On Mon, May 18, 2015 at 11:30 AM, Junio C Hamano <[email-masked]> 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.