Re: [PATCH 8/8] refs: check refnames as fully qualified when resolving

From: Patrick Steinhardt <ps@pks.im>
Date: Apr 30, 2024, 11:25 UTC
Message-ID: <ZjDVLAKA0_4pTAS7@tanuki>
In-Reply-To: <20240430104152.GF1279403@coredump.intra.peff.net>

On Tue, Apr 30, 2024 at 06:41:52AM -0400, Jeff King wrote:

Show 31 quoted lines

> On Tue, Apr 30, 2024 at 06:54:12AM +0200, Patrick Steinhardt wrote:
> 
> > > diff --git a/t/t1430-bad-ref-name.sh b/t/t1430-bad-ref-name.sh
> > > index 120e1557d7..5fb780cb08 100755
> > > --- a/t/t1430-bad-ref-name.sh
> > > +++ b/t/t1430-bad-ref-name.sh
> > > @@ -400,4 +400,14 @@ test_expect_success 'update-ref refuses non-underscore outside of refs/' '
> > >  	test_grep "refusing to update ref with bad name" err
> > >  '
> > >  
> > > +test_expect_success REFFILES 'rev-parse refuses non-pseudoref outside of refs/' '
> > > +	git rev-parse HEAD >.git/bad &&
> > > +	test_must_fail git rev-parse --verify bad
> > > +'
> > > +
> > > +test_expect_success REFFILES 'rev-parse recognizes non-pseudoref via worktree' '
> > > +	git rev-parse HEAD >.git/bad &&
> > > +	test_must_fail git rev-parse --verify main-worktree/bad
> > > +'
> > 
> > Are these really specific to the REFFILES backend? I would expect that
> > the reftable backend sohuld fail to parse those, too. The fact that we
> > write into the repository directly during the test setup doesn't change
> > this, because all this patch is about is that we don't want to parse
> > random files in the Git repo. And that is something we should want to
> > enforce for all backends.
> 
> So this is where I will show my ignorance of reftables. I assume it
> still has to implement FETCH_HEAD as a file (since it holds extra data).
> But does it do the same for other names outside of "refs/"? I am
> assuming not in the paragraph below.

No, that's why we originally introduced the "special refs" syntax, as defined in gitglossary(7). There are only two files that behave like refs, but circumvent the ref backend: FETCH_HEAD and MERGE_HEAD. Both of these have special syntax and carry additional metadata, and as such they cannot be stored generically in a ref backend.

All other root refs are stored via the ref backend.

Show 11 quoted lines

> I would expect the test to succeed after my patches on any ref backend,
> since we'd enforce the syntax outside of the backend-specific code. But
> for a backend which does not look for the root name "foo" directly in
> .git/, it is not an interesting test. The looked-for name does not
> exist for it, so even if we did try the lookup, it would fail. We cannot
> distinguish the two cases from the outcome we see.
> 
> So I think dropping REFFILES it would still pass, but we are not really
> testing anything that interesting for reftables. That said, I would be
> OK dropping the REFFILES in the name of simplicity and just documenting
> it in the commit message.

Yeah, I'd prefer to drop it. We should only specify the REFFILES prereq as sparingly as possible to ensure that behaviour is as consistent as possible across the implementations.

Patrick