| Message ID | 9b49e849eaf6786c63016d767d2ad56112d08d51.1604908834.git.ps@pks.im (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | update-ref: Allow creation of multiple transactions | expand |
Patrick Steinhardt <ps@pks.im> writes: > The testcase t1400 exercises the git-update-ref(1) utility. To do so, > many tests directly read and write references via the filesystem, > assuming that we always use loose and/or packed references. While this > is true now, it'll change with the introduction of the reftable backend. > > Convert those tests to use git-update-ref(1) and git-show-ref(1) where > possible. As some tests exercise behaviour with broken references and > neither of those tools actually allows writing or reading broken > references, this commit doesn't adjust all tests. > > Signed-off-by: Patrick Steinhardt <ps@pks.im> > --- > t/t1400-update-ref.sh | 63 ++++++++++++++++++++++++------------------- > 1 file changed, 35 insertions(+), 28 deletions(-) > > diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh > index 4c01e08551..957bef272d 100755 > --- a/t/t1400-update-ref.sh > +++ b/t/t1400-update-ref.sh > @@ -14,6 +14,12 @@ n=$n_dir/fixes > outside=refs/foo > bare=bare-repo > > +# Some of the tests delete HEAD, which causes us to not treat the current > +# working directory as a Git repository anymore. To avoid using any potential > +# parent repository to be discovered, we need to set up the ceiling directories. > +GIT_CEILING_DIRECTORIES="$PWD/.." > +export GIT_CEILING_DIRECTORIES > + Interesting. The current tests do not need to do this because the HEAD-less broken state is transitory and is corrected without using "git" at all (e.g. echoing a valid value into .git/HEAD), I presume? > create_test_commits () > { > prfx="$1" > @@ -48,17 +54,17 @@ test_expect_success "fail to delete $m with stale ref" ' > test $B = "$(git show-ref -s --verify $m)" > ' > test_expect_success "delete $m" ' > - test_when_finished "rm -f .git/$m" && > + test_when_finished "git update-ref -d $m" && > git update-ref -d $m $B && > - test_path_is_missing .git/$m > + test_must_fail git show-ref --verify -q $m > ' > > test_expect_success "delete $m without oldvalue verification" ' > - test_when_finished "rm -f .git/$m" && > + test_when_finished "git update-ref -d $m" && > git update-ref $m $A && > test $A = $(git show-ref -s --verify $m) && > git update-ref -d $m && > - test_path_is_missing .git/$m > + test_must_fail git show-ref --verify -q $m > ' > > test_expect_success "fail to create $n" ' > @@ -80,26 +86,26 @@ test_expect_success "fail to delete $m (by HEAD) with stale ref" ' > test $B = $(git show-ref -s --verify $m) > ' > test_expect_success "delete $m (by HEAD)" ' > - test_when_finished "rm -f .git/$m" && > + test_when_finished "git update-ref -d $m" && > git update-ref -d HEAD $B && > - test_path_is_missing .git/$m > + test_must_fail git show-ref --verify -q $m > ' During the above test, we are on the branch ${m#refs/heads/}, so "update-ref -d HEAD" is removing the underlying branch ref, making it an unborn branch, without destroying the repository, so this is perfectly sensible. This is a tangent, but what makes this test doubly interesting is that "git update-ref -d HEAD" would have allowed us to make it a non-repository if HEAD were detached, and it seems that we do not require "--force" to do so. We probably should forbid removing HEAD that id detached without "--force", which is such a destructive operation. > cp -f .git/HEAD .git/HEAD.orig > test_expect_success 'delete symref without dereference' ' > test_when_finished "cp -f .git/HEAD.orig .git/HEAD" && > git update-ref --no-deref -d HEAD && > - test_path_is_missing .git/HEAD > + test_must_fail git show-ref --verify -q HEAD > ' This is an example of breaking the repository. I am not sure if the test_must_fail is a good replacement--it would fail even if you say "git show-ref --verify -q refs/heads/$branch" where $branch is a name of a branch that exists, no? For now, I think this is fine, but we'd probably clean it up so that without --force update-ref would not corrupt the repository like this. When used with --force, or before adding such a safety measure, how we test if we successfully corrupted the repository is an interesting matter. I'd say git update-ref --force --no-deref -d HEAD && test_must_fail git show-ref --verify -q HEAD && cp -f .git/HEAD.orig .git/HEAD && git show-ref --verify -q "$m" to ensure that other than removing HEAD and corrupting the repository, it did not cause permanent damage by removing the underlying ref, perhaps. We may want to add some NEEDSWORK comment around such tests. > test_expect_success 'delete symref without dereference when the referred ref is packed' ' > @@ -208,7 +214,7 @@ test_expect_success 'delete symref without dereference when the referred ref is > git commit -m foo && > git pack-refs --all && > git update-ref --no-deref -d HEAD && > - test_path_is_missing .git/HEAD > + test_must_fail git show-ref --verify -q HEAD > ' Does this share the same issue as the above? Thanks.
On Mon, Nov 09, 2020 at 11:48:20AM -0800, Junio C Hamano wrote: > This is a tangent, but what makes this test doubly interesting is > that "git update-ref -d HEAD" would have allowed us to make it a > non-repository if HEAD were detached, and it seems that we do not > require "--force" to do so. We probably should forbid removing HEAD > that id detached without "--force", which is such a destructive > operation. Yeah, I'd agree that is a good direction (but it definitely is a tangent that should come in a separate series). > > cp -f .git/HEAD .git/HEAD.orig > > test_expect_success 'delete symref without dereference' ' > > test_when_finished "cp -f .git/HEAD.orig .git/HEAD" && > > git update-ref --no-deref -d HEAD && > > - test_path_is_missing .git/HEAD > > + test_must_fail git show-ref --verify -q HEAD > > ' > > This is an example of breaking the repository. I am not sure if the > test_must_fail is a good replacement--it would fail even if you say > "git show-ref --verify -q refs/heads/$branch" where $branch is a > name of a branch that exists, no? Perhaps we could more directly check that the repository is broken. Coupled with the ceiling-limit from earlier in the script, then: git rev-parse --git-dir should fail. Maybe that reveals the intent of what we're expecting a little more clearly (and more so than your "show-ref before and after restoring HEAD" example does, in my opinion). I do have to wonder if this test even cares about HEAD. Could it equally well work on another symref, like refs/remotes/origin/HEAD? -Peff
On Mon, Nov 09, 2020 at 11:48:20AM -0800, Junio C Hamano wrote: > Patrick Steinhardt <ps@pks.im> writes: > > diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh > > index 4c01e08551..957bef272d 100755 > > --- a/t/t1400-update-ref.sh > > +++ b/t/t1400-update-ref.sh > > @@ -14,6 +14,12 @@ n=$n_dir/fixes > > outside=refs/foo > > bare=bare-repo > > > > +# Some of the tests delete HEAD, which causes us to not treat the current > > +# working directory as a Git repository anymore. To avoid using any potential > > +# parent repository to be discovered, we need to set up the ceiling directories. > > +GIT_CEILING_DIRECTORIES="$PWD/.." > > +export GIT_CEILING_DIRECTORIES > > + > > Interesting. The current tests do not need to do this because the > HEAD-less broken state is transitory and is corrected without using > "git" at all (e.g. echoing a valid value into .git/HEAD), I presume? Correct. > > @@ -80,26 +86,26 @@ test_expect_success "fail to delete $m (by HEAD) with stale ref" ' > > test $B = $(git show-ref -s --verify $m) > > ' > > test_expect_success "delete $m (by HEAD)" ' > > - test_when_finished "rm -f .git/$m" && > > + test_when_finished "git update-ref -d $m" && > > git update-ref -d HEAD $B && > > - test_path_is_missing .git/$m > > + test_must_fail git show-ref --verify -q $m > > ' > > During the above test, we are on the branch ${m#refs/heads/}, so > "update-ref -d HEAD" is removing the underlying branch ref, making > it an unborn branch, without destroying the repository, so this is > perfectly sensible. > > This is a tangent, but what makes this test doubly interesting is > that "git update-ref -d HEAD" would have allowed us to make it a > non-repository if HEAD were detached, and it seems that we do not > require "--force" to do so. We probably should forbid removing HEAD > that id detached without "--force", which is such a destructive > operation. That'd make sense to me, yes. It also took me quite some time to actually figure out why tests were misbehaving when I converted it. Until I finally realized: this is not a Git repo anymore, and refs have now been modified directly in the real git.git repository. Just this morning I still found some invalid refs created by the test in git.git. > > cp -f .git/HEAD .git/HEAD.orig > > test_expect_success 'delete symref without dereference' ' > > test_when_finished "cp -f .git/HEAD.orig .git/HEAD" && > > git update-ref --no-deref -d HEAD && > > - test_path_is_missing .git/HEAD > > + test_must_fail git show-ref --verify -q HEAD > > ' > > This is an example of breaking the repository. I am not sure if the > test_must_fail is a good replacement--it would fail even if you say > "git show-ref --verify -q refs/heads/$branch" where $branch is a > name of a branch that exists, no? Right, it's not. We're not detecting HEAD deletion by means of checking that it doesn't exist anymore, but only detect it because the repo is not a repo anymore. Which would in fact cause most git commands to fail now. > For now, I think this is fine, but we'd probably clean it up so that > without --force update-ref would not corrupt the repository like > this. When used with --force, or before adding such a safety > measure, how we test if we successfully corrupted the repository is > an interesting matter. I'd say > > git update-ref --force --no-deref -d HEAD && > test_must_fail git show-ref --verify -q HEAD && > cp -f .git/HEAD.orig .git/HEAD && > git show-ref --verify -q "$m" > > to ensure that other than removing HEAD and corrupting the > repository, it did not cause permanent damage by removing the > underlying ref, perhaps. > > We may want to add some NEEDSWORK comment around such tests. I think the proper way to do the test would be to create a non-mandatory symref and delete it. That'd also be a nice preparation for the `--force` parameter already. > > test_expect_success 'delete symref without dereference when the referred ref is packed' ' > > @@ -208,7 +214,7 @@ test_expect_success 'delete symref without dereference when the referred ref is > > git commit -m foo && > > git pack-refs --all && > > git update-ref --no-deref -d HEAD && > > - test_path_is_missing .git/HEAD > > + test_must_fail git show-ref --verify -q HEAD > > ' > > Does this share the same issue as the above? Yup, it does. Patrick
Patrick Steinhardt <ps@pks.im> writes: >> For now, I think this is fine, but we'd probably clean it up so that >> without --force update-ref would not corrupt the repository like >> this. When used with --force, or before adding such a safety >> measure, how we test if we successfully corrupted the repository is >> an interesting matter. I'd say >> >> git update-ref --force --no-deref -d HEAD && >> test_must_fail git show-ref --verify -q HEAD && >> cp -f .git/HEAD.orig .git/HEAD && >> git show-ref --verify -q "$m" >> >> to ensure that other than removing HEAD and corrupting the >> repository, it did not cause permanent damage by removing the >> underlying ref, perhaps. >> >> We may want to add some NEEDSWORK comment around such tests. > > I think the proper way to do the test would be to create a non-mandatory > symref and delete it. That'd also be a nice preparation for the > `--force` parameter already. Excellent. Yes, the test is about creation and removal of a symref; the symref used for testing does not have to be the HEAD. Thanks.
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh index 4c01e08551..957bef272d 100755 --- a/t/t1400-update-ref.sh +++ b/t/t1400-update-ref.sh @@ -14,6 +14,12 @@ n=$n_dir/fixes outside=refs/foo bare=bare-repo +# Some of the tests delete HEAD, which causes us to not treat the current +# working directory as a Git repository anymore. To avoid using any potential +# parent repository to be discovered, we need to set up the ceiling directories. +GIT_CEILING_DIRECTORIES="$PWD/.." +export GIT_CEILING_DIRECTORIES + create_test_commits () { prfx="$1" @@ -48,17 +54,17 @@ test_expect_success "fail to delete $m with stale ref" ' test $B = "$(git show-ref -s --verify $m)" ' test_expect_success "delete $m" ' - test_when_finished "rm -f .git/$m" && + test_when_finished "git update-ref -d $m" && git update-ref -d $m $B && - test_path_is_missing .git/$m + test_must_fail git show-ref --verify -q $m ' test_expect_success "delete $m without oldvalue verification" ' - test_when_finished "rm -f .git/$m" && + test_when_finished "git update-ref -d $m" && git update-ref $m $A && test $A = $(git show-ref -s --verify $m) && git update-ref -d $m && - test_path_is_missing .git/$m + test_must_fail git show-ref --verify -q $m ' test_expect_success "fail to create $n" ' @@ -80,26 +86,26 @@ test_expect_success "fail to delete $m (by HEAD) with stale ref" ' test $B = $(git show-ref -s --verify $m) ' test_expect_success "delete $m (by HEAD)" ' - test_when_finished "rm -f .git/$m" && + test_when_finished "git update-ref -d $m" && git update-ref -d HEAD $B && - test_path_is_missing .git/$m + test_must_fail git show-ref --verify -q $m ' test_expect_success "deleting current branch adds message to HEAD's log" ' - test_when_finished "rm -f .git/$m" && + test_when_finished "git update-ref -d $m" && git update-ref $m $A && git symbolic-ref HEAD $m && git update-ref -m delete-$m -d $m && - test_path_is_missing .git/$m && + test_must_fail git show-ref --verify -q $m && grep "delete-$m$" .git/logs/HEAD ' test_expect_success "deleting by HEAD adds message to HEAD's log" ' - test_when_finished "rm -f .git/$m" && + test_when_finished "git update-ref -d $m" && git update-ref $m $A && git symbolic-ref HEAD $m && git update-ref -m delete-by-head -d HEAD && - test_path_is_missing .git/$m && + test_must_fail git show-ref --verify -q $m && grep "delete-by-head$" .git/logs/HEAD ' @@ -188,17 +194,17 @@ test_expect_success "move $m (by HEAD)" ' test $B = $(git show-ref -s --verify $m) ' test_expect_success "delete $m (by HEAD) should remove both packed and loose $m" ' - test_when_finished "rm -f .git/$m" && + test_when_finished "rm -f git update-ref -d $m" && git update-ref -d HEAD $B && ! grep "$m" .git/packed-refs && - test_path_is_missing .git/$m + test_must_fail git show-ref --verify -q $m ' cp -f .git/HEAD .git/HEAD.orig test_expect_success 'delete symref without dereference' ' test_when_finished "cp -f .git/HEAD.orig .git/HEAD" && git update-ref --no-deref -d HEAD && - test_path_is_missing .git/HEAD + test_must_fail git show-ref --verify -q HEAD ' test_expect_success 'delete symref without dereference when the referred ref is packed' ' @@ -208,7 +214,7 @@ test_expect_success 'delete symref without dereference when the referred ref is git commit -m foo && git pack-refs --all && git update-ref --no-deref -d HEAD && - test_path_is_missing .git/HEAD + test_must_fail git show-ref --verify -q HEAD ' git update-ref -d $m @@ -226,25 +232,25 @@ test_expect_success 'update-ref --no-deref -d can delete self-reference' ' test_when_finished "rm -f .git/refs/heads/self" && test_path_is_file .git/refs/heads/self && git update-ref --no-deref -d refs/heads/self && - test_path_is_missing .git/refs/heads/self + test_must_fail git show-ref --verify -q refs/heads/self ' test_expect_success 'update-ref --no-deref -d can delete reference to bad ref' ' >.git/refs/heads/bad && test_when_finished "rm -f .git/refs/heads/bad" && git symbolic-ref refs/heads/ref-to-bad refs/heads/bad && - test_when_finished "rm -f .git/refs/heads/ref-to-bad" && + test_when_finished "git update-ref -d refs/heads/ref-to-bad" && test_path_is_file .git/refs/heads/ref-to-bad && git update-ref --no-deref -d refs/heads/ref-to-bad && - test_path_is_missing .git/refs/heads/ref-to-bad + test_must_fail git show-ref --verify -q refs/heads/ref-to-bad ' test_expect_success '(not) create HEAD with old sha1' ' test_must_fail git update-ref HEAD $A $B ' test_expect_success "(not) prior created .git/$m" ' - test_when_finished "rm -f .git/$m" && - test_path_is_missing .git/$m + test_when_finished "git update-ref -d $m" && + test_must_fail git show-ref --verify -q $m ' test_expect_success 'create HEAD' ' @@ -254,7 +260,7 @@ test_expect_success '(not) change HEAD with wrong SHA1' ' test_must_fail git update-ref HEAD $B $Z ' test_expect_success "(not) changed .git/$m" ' - test_when_finished "rm -f .git/$m" && + test_when_finished "git update-ref -d $m" && ! test $B = $(git show-ref -s --verify $m) ' @@ -284,8 +290,8 @@ test_expect_success 'empty directory removal' ' test_path_is_file .git/refs/heads/d1/d2/r1 && test_path_is_file .git/logs/refs/heads/d1/d2/r1 && git branch -d d1/d2/r1 && - test_path_is_missing .git/refs/heads/d1/d2 && - test_path_is_missing .git/logs/refs/heads/d1/d2 && + test_must_fail git show-ref --verify -q refs/heads/d1/d2 && + test_must_fail git show-ref --verify -q logs/refs/heads/d1/d2 && test_path_is_file .git/refs/heads/d1/r2 && test_path_is_file .git/logs/refs/heads/d1/r2 ' @@ -298,8 +304,8 @@ test_expect_success 'symref empty directory removal' ' test_path_is_file .git/refs/heads/e1/e2/r1 && test_path_is_file .git/logs/refs/heads/e1/e2/r1 && git update-ref -d HEAD && - test_path_is_missing .git/refs/heads/e1/e2 && - test_path_is_missing .git/logs/refs/heads/e1/e2 && + test_must_fail git show-ref --verify -q refs/heads/e1/e2 && + test_must_fail git show-ref --verify -q logs/refs/heads/e1/e2 && test_path_is_file .git/refs/heads/e1/r2 && test_path_is_file .git/logs/refs/heads/e1/r2 && test_path_is_file .git/logs/HEAD @@ -1388,7 +1394,8 @@ test_expect_success 'handle per-worktree refs in refs/bisect' ' git rev-parse refs/bisect/something >../worktree-head && git for-each-ref | grep refs/bisect/something ) && - test_path_is_missing .git/refs/bisect && + git show-ref >actual && + ! grep 'refs/bisect' actual && test_must_fail git rev-parse refs/bisect/something && git update-ref refs/bisect/something HEAD && git rev-parse refs/bisect/something >main-head && @@ -1500,7 +1507,7 @@ test_expect_success 'transaction can handle abort' ' git update-ref --stdin <stdin >actual && printf "%s: ok\n" start abort >expect && test_cmp expect actual && - test_path_is_missing .git/$b + test_must_fail git show-ref --verify -q $b ' test_expect_success 'transaction aborts by default' ' @@ -1511,7 +1518,7 @@ test_expect_success 'transaction aborts by default' ' git update-ref --stdin <stdin >actual && printf "%s: ok\n" start >expect && test_cmp expect actual && - test_path_is_missing .git/$b + test_must_fail git show-ref --verify -q $b ' test_expect_success 'transaction with prepare aborts by default' ' @@ -1523,7 +1530,7 @@ test_expect_success 'transaction with prepare aborts by default' ' git update-ref --stdin <stdin >actual && printf "%s: ok\n" start prepare >expect && test_cmp expect actual && - test_path_is_missing .git/$b + test_must_fail git show-ref --verify -q $b ' test_done
The testcase t1400 exercises the git-update-ref(1) utility. To do so, many tests directly read and write references via the filesystem, assuming that we always use loose and/or packed references. While this is true now, it'll change with the introduction of the reftable backend. Convert those tests to use git-update-ref(1) and git-show-ref(1) where possible. As some tests exercise behaviour with broken references and neither of those tools actually allows writing or reading broken references, this commit doesn't adjust all tests. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- t/t1400-update-ref.sh | 63 ++++++++++++++++++++++++------------------- 1 file changed, 35 insertions(+), 28 deletions(-)