Re: [PATCH 04/11] t: convert tests to not write references via the filesystem
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Oct 18, 2023, 21:18 UTC
- Message-ID
- <xmqqfs27r5ng.fsf@gitster.g>
- In-Reply-To
- <c79431c0bf117d756e1d584f4c9415888d9ff9eb.1697607222.git.ps@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
Show 6 quoted lines
> @@ -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 $mWe are not clearing "expect" file. I do not know if it matters here, but I am only recording what I noticed.
Show 11 quoted lines
> 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".
Show 6 quoted lines
> 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?
Show 8 quoted lines
> @@ -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.
Show 6 quoted lines
> @@ -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).