Re: [PATCH 04/11] t: convert tests to not write references via the filesystem

From: Patrick Steinhardt <ps@pks.im>
Date: Oct 23, 2023, 13:58 UTC
Message-ID: <ZTZ8ATohRe7GVu5D@tanuki>
In-Reply-To: <xmqq1qdru6ds.fsf@gitster.g>

On Wed, Oct 18, 2023 at 11:34:23AM -0700, Junio C Hamano wrote:

Show 26 quoted lines

> Patrick Steinhardt <ps@pks.im> writes:
> 
> > -test_expect_success "fail to create $n" '
> > -	test_when_finished "rm -f .git/$n_dir" &&
> > -	touch .git/$n_dir &&
> > -	test_must_fail git update-ref $n $A
> > +test_expect_success "fail to create $n due to file/directory conflict" '
> > +	test_when_finished "git update-ref -d refs/heads/gu" &&
> > +	git update-ref refs/heads/gu $A &&
> > +	test_must_fail git update-ref refs/heads/gu/fixes $A
> >  '
> 
> OK, the original checks "if a random garbage file, which may not
> necessarily be a ref, exists at $n_dir, we cannot create a ref at
> $n_dir/fixes, due to D/F conflict" more directly, but as long as our
> intention is to enforce the D/F restriction across different ref
> backends [*], creating a ref at $n_dir and making sure $n_dir/fixes
> cannot be created is an equivalent check that is better (because it
> can be applied for other backends).
> 
>     Side note: there is no fundamental need to, though, and there
>          are cases where being able to have the 'seen' branch and
>          'seen/ps/ref-test-tools' branches at the same time is
>          beneficial---packed-refs and ref-table backends would not
>          have such an inherent limitation, but they can of course be
>          castrated to match what files-backend can(not) do.

I think initially it is beneficial to keep any such restriction and cut back new backends to match them, even though it's more work. The main reason why I think this makes sense is that hosting providers are the most likely to update to the newer backend fast. It would be detrimental to the users though if hosting providers that converted to the newer backend were able to store such references that would be conflicting with the files backend because they wouldn't be able to clone such repositories anymore. And this isn't only true for hosting providers, but essentially whenever two repositories that use different backends try to interact with each other.

So even though I wish for a future where we don't need to care for D/F conflicts anymore, I think the time isn't ripe for it and we should for now aim for matching behaviour.

Show 11 quoted lines

> > @@ -222,7 +220,7 @@ test_expect_success 'delete symref without dereference when the referred ref is
> >  
> >  test_expect_success 'update-ref -d is not confused by self-reference' '
> >  	git symbolic-ref refs/heads/self refs/heads/self &&
> > -	test_when_finished "rm -f .git/refs/heads/self" &&
> > +	test_when_finished "test-tool ref-store main delete-refs REF_NO_DEREF refs/heads/self" &&
> >  	test_path_is_file .git/refs/heads/self &&
> 
> I trust that this will be corrected to use some wrapper around "git
> symbolic-ref" (or an equivalent for it as a test-tool subcommand) in
> some future patch, if not in this series?

Yup, this is getting fixed in a subsequent patch. I had two different options to structure this series:

    - Either by test file so that questions like this don't come up when
      a reviewer sees in the context that there are still tests that
      exercise the filesystem directly. This was my first approach.

    - The alternative is to write it such that we address common
      patterns globally, which I then rewrote my first version to.

There were two reasons why I didn't like the first iteration:

    - Commit messages would basically have to reexplain the exact same
      thing for every single testcase as the motivation is the same
      everywhere.

    - I think it's easier to review when you only see the same kind of
      transformation per patch.

But yes, it does have the downside that the reader is now left wondering why the other call to `test_path_is_file` still exists.

Patrick

Show 19 quoted lines

> >  	test_must_fail git update-ref -d refs/heads/self &&
> >  	test_path_is_file .git/refs/heads/self
> 
> Likewise.
> 
> > @@ -230,7 +228,7 @@ test_expect_success 'update-ref -d is not confused by self-reference' '
> >  
> >  test_expect_success 'update-ref --no-deref -d can delete self-reference' '
> >  	git symbolic-ref refs/heads/self refs/heads/self &&
> > -	test_when_finished "rm -f .git/refs/heads/self" &&
> > +	test_when_finished "test-tool ref-store main delete-refs REF_NO_DEREF refs/heads/self" &&
> >  	test_path_is_file .git/refs/heads/self &&
> >  	git update-ref --no-deref -d refs/heads/self &&
> >  	test_must_fail git show-ref --verify -q refs/heads/self
> 
> We already have the "ref is missing" test here.
> 
> I'll stop at this point for now; will hopefully continue in a
> separate message later.  Thanks.