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

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 5886 bytes --]

On Wed, Oct 18, 2023 at 02:18:27PM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > @@ -434,7 +432,7 @@ test_expect_success 'Query "main@{2005-05-28}" (past end of history)' '
> >  	test_i18ngrep -F "warning: log for ref $m unexpectedly ended on $ld" e
> >  '
> >  
> > -rm -f .git/$m .git/logs/$m expect
> > +git update-ref -d $m
> 
> We are not clearing "expect" file.  I do not know if it matters
> here, but I am only recording what I noticed.

Oops, will fix.

> > diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh
> > index 10a539158c4..5cce24f1006 100755
> > --- a/t/t1450-fsck.sh
> > +++ b/t/t1450-fsck.sh
> > @@ -115,15 +115,16 @@ test_expect_success 'zlib corrupt loose object output ' '
> >  '
> >  
> >  test_expect_success 'branch pointing to non-commit' '
> > -	git rev-parse HEAD^{tree} >.git/refs/heads/invalid &&
> > +	tree_oid=$(git rev-parse --verify HEAD^{tree}) &&
> > +	test-tool ref-store main update-ref msg refs/heads/invalid $tree_oid $ZERO_OID REF_SKIP_OID_VERIFICATION &&
> 
> I have mixed feelings on this.
> 
> In olden days, plumbing commands tended to allow to pass anything
> the user told them to use, but in more recent versions of Git, we,
> probably by mistake, managed to butcher some of the plumbing
> commands to make them unable to deliberately "break" repositories,
> one victim being "update-ref", i.e.
> 
>     $ git update-ref refs/heads/invalid HEAD^{tree}
> 
> is rejected these days (I just checked with v1.3.0 and it allows me
> to do this), and that is one of the reasons why we manually broke
> the repository in these tests.

My first try was indeed to use git-update-ref(1) to update the ref to
the tree object, and I was surprised to learn that it would not let me
do so.

> We need to have a warning message in
> comments near the implementation of "ref-store update-ref" that says
> never ever attempt to share code with the production version of
> update-ref---otherwise this (or the "safety" given to the plumbing
> command, possibly by mistake) will be broken, depending on which
> direction such a sharing goes.  On the other hand, forcing us to
> keep two separate implementations, one deliberately loose to allow
> us corrupt repositories, the other for production and actively used,
> would mean the former one that is only used for validation would risk
> bitrotting.

Wouldn't any eventual bitrot be contained by the tests though? As our
use of the test helper grows via this patch series its behaviour will
also be verified more thoroughly.

> >  	test_when_finished "git update-ref -d refs/heads/invalid" &&
> 
> Not a problem this patch introduces, but I think it is a better
> discipline to have when_finished clean-up routine prepared before we
> do actual damage, i.e. if I were writing this test today from scratch,
> I would expect it to be before "git rev-parse >.git/refs/heads/invalid"
> is done.

I'll fix this while at it.

> >  	test_must_fail git fsck 2>out &&
> >  	test_i18ngrep "not a commit" out
> >  '
> 
> A #leftoverbit that is not relevant to the topic; we should clean
> these test_i18ngrep and replace them with a plain "grep".
> 
> >  test_expect_success 'HEAD link pointing at a funny object' '
> > -	test_when_finished "mv .git/SAVED_HEAD .git/HEAD" &&
> > -	mv .git/HEAD .git/SAVED_HEAD &&
> > +	saved_head=$(git rev-parse --verify HEAD) &&
> > +	test_when_finished "git update-ref HEAD ${saved_head}" &&
> >  	echo $ZERO_OID >.git/HEAD &&
> 
> Are you sure .git/HEAD when this test is entered is a detached HEAD?
> The title of the test says "HEAD link", and I take it to mean HEAD
> is a symlink, and we save it away, while we create a loose ref that
> points at 0{40} in a detached HEAD state.  Actually, the original
> would also work if HEAD is detached on entry.  In either case,
> moving SAVED_HEAD back to HEAD would restore the original state.
> 
> But the updated one only works if HEAD upon entry is already
> detached.  Is this intended?

Yes and no -- it's a reflection of the state when this test runs. One
problem in this test suite here is that many of the tests' states are
heavily interwoven with each other, which only makes this harder to
refactor without making any assumptions.

Well. Instead of restoring to whatever the state was previous to the
test we could also restore it to something sane-ish like "master". That
of course breaks other tests though... I'll investigate.

> > @@ -131,8 +132,8 @@ test_expect_success 'HEAD link pointing at a funny object' '
> >  '
> >  
> >  test_expect_success 'HEAD link pointing at a funny place' '
> > -	test_when_finished "mv .git/SAVED_HEAD .git/HEAD" &&
> > -	mv .git/HEAD .git/SAVED_HEAD &&
> > +	saved_head=$(git rev-parse --verify HEAD) &&
> > +	test_when_finished "git update-ref --no-deref HEAD ${saved_head}" &&
> 
> Likewise.  Use of "update-ref" in the previous one vs "update-ref
> --no-deref" in this one to recover from the damage the tests make
> makes me feel that we may be assuming too much.
> 
> >  	echo "ref: refs/funny/place" >.git/HEAD &&
> 
> Even though "git symbolic-ref" refuses to point HEAD outside refs/,
> as plumbing command should, it allows it to point it outside refs/heads/.
> so this line should probably become
> 
> 	git symbolic-ref HEAD refs/funny/place
> 
> in the same spirit as the rest of the series.

Yup, this will be adressed in a later patch.

Patrick

> > @@ -391,7 +393,7 @@ test_expect_success 'tag pointing to nonexistent' '
> >  
> >  	tag=$(git hash-object -t tag -w --stdin <invalid-tag) &&
> >  	test_when_finished "remove_object $tag" &&
> > -	echo $tag >.git/refs/tags/invalid &&
> > +	git update-ref refs/tags/invalid $tag &&
> 
> Good (not just this one, but similar ones throughout this patch).
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]