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

[Junio C Hamano <gitster@pobox.com>]

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.

> 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.  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.

>  	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.

>  	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?

> @@ -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.

> @@ -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).