Re: [PATCH v2 1/4] t1400: Avoid touching refs on filesystem

From: Jeff King <peff@peff.net>
Date: Nov 9, 2020, 22:28 UTC
Message-ID: <20201109222807.GB670413@coredump.intra.peff.net>
In-Reply-To: <xmqqpn4mb9bv.fsf@gitster.c.googlers.com>

On Mon, Nov 09, 2020 at 11:48:20AM -0800, Junio C Hamano wrote:

Show 6 quoted lines

> This is a tangent, but what makes this test doubly interesting is
> that "git update-ref -d HEAD" would have allowed us to make it a
> non-repository if HEAD were detached, and it seems that we do not
> require "--force" to do so.  We probably should forbid removing HEAD
> that id detached without "--force", which is such a destructive
> operation.

Yeah, I'd agree that is a good direction (but it definitely is a tangent that should come in a separate series).

Show 12 quoted lines

> >  cp -f .git/HEAD .git/HEAD.orig
> >  test_expect_success 'delete symref without dereference' '
> >  	test_when_finished "cp -f .git/HEAD.orig .git/HEAD" &&
> >  	git update-ref --no-deref -d HEAD &&
> > -	test_path_is_missing .git/HEAD
> > +	test_must_fail git show-ref --verify -q HEAD
> >  '
> 
> This is an example of breaking the repository.  I am not sure if the
> test_must_fail is a good replacement--it would fail even if you say
> "git show-ref --verify -q refs/heads/$branch" where $branch is a
> name of a branch that exists, no?

Perhaps we could more directly check that the repository is broken. Coupled with the ceiling-limit from earlier in the script, then:

  git rev-parse --git-dir

should fail. Maybe that reveals the intent of what we're expecting a little more clearly (and more so than your "show-ref before and after restoring HEAD" example does, in my opinion).

I do have to wonder if this test even cares about HEAD. Could it equally well work on another symref, like refs/remotes/origin/HEAD?

-Peff