public inbox for git@vger.kernel.org 
 help / color / mirror / Atom feed

From: Junio C Hamano <gitster@pobox•com>
To: Stefan Beller <sbeller@google•com>
Cc: Johannes Schindelin <Johannes.Schindelin@gmx•de>,
	Jens.Lehmann@web•de, "git\@vger.kernel.org" <git@vger•kernel.org>
Subject: Re: [PATCH] t1020: cleanup subdirectory tests a little
Date: Mon, 18 May 2015 12:08:59 -0700	[thread overview]
Message-ID: <xmqqlhglbt4k.fsf@gitster.dls.corp.google.com> (raw)
In-Reply-To: <CAGZ79kZjjKD6J3bQ0SWBUO-6bTY1Ld=Y7mRHVbw+RO5AFN+r5A@mail.gmail.com> (Stefan Beller's message of "Mon, 18 May 2015 11:36:49 -0700")

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.

next prev parent reply	other threads:[~2015-05-18 19:09 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-05-18 18:13 [PATCH] t1020: cleanup subdirectory tests a little Stefan Beller
2015-05-18 18:29 ` Junio C Hamano
2015-05-18 18:30   ` Junio C Hamano
2015-05-18 18:36     ` Stefan Beller
2015-05-18 19:08       ` Junio C Hamano [this message]
2015-05-18 21:10         ` [PATCH] subdirectory tests: code cleanup, uncomment test Stefan Beller
2015-05-18 21:21           ` Junio C Hamano
2015-05-18 21:29             ` Stefan Beller
2015-05-18 22:03               ` Jonathan Nieder
2015-05-19 21:45               ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqlhglbt4k.fsf@gitster.dls.corp.google.com \
    --to=gitster@pobox$(echo .)com \
    --cc=Jens.Lehmann@web$(echo .)de \
    --cc=Johannes.Schindelin@gmx$(echo .)de \
    --cc=git@vger$(echo .)kernel.org \
    --cc=sbeller@google$(echo .)com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox