| Message ID | e66b1bcc62139f62866dc9f25856eaebfe107056.1605077740.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | update-ref: allow creation of multiple transactions | expand |
On Wed, Nov 11, 2020 at 07:58:38AM +0100, Patrick Steinhardt wrote: > 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 | 72 +++++++++++++++++++++++-------------------- > 1 file changed, 39 insertions(+), 33 deletions(-) > > diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh > index 4c01e08551..d7a57488ed 100755 > --- a/t/t1400-update-ref.sh > +++ b/t/t1400-update-ref.sh > @@ -188,27 +194,26 @@ 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" && There is a leftover 'rm -f' here. > 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 > '
On Wed, Nov 11, 2020 at 10:04:54AM +0100, SZEDER Gábor wrote: > On Wed, Nov 11, 2020 at 07:58:38AM +0100, Patrick Steinhardt wrote: > > 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 | 72 +++++++++++++++++++++++-------------------- > > 1 file changed, 39 insertions(+), 33 deletions(-) > > > > diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh > > index 4c01e08551..d7a57488ed 100755 > > --- a/t/t1400-update-ref.sh > > +++ b/t/t1400-update-ref.sh > > > @@ -188,27 +194,26 @@ 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" && > > There is a leftover 'rm -f' here. Oops, thanks a lot for catching. Funny that it still passed. Patrick
On Wed, Nov 11, 2020 at 11:00:15AM +0100, Patrick Steinhardt wrote: > On Wed, Nov 11, 2020 at 10:04:54AM +0100, SZEDER Gábor wrote: > > On Wed, Nov 11, 2020 at 07:58:38AM +0100, Patrick Steinhardt wrote: > > > 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 | 72 +++++++++++++++++++++++-------------------- > > > 1 file changed, 39 insertions(+), 33 deletions(-) > > > > > > diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh > > > index 4c01e08551..d7a57488ed 100755 > > > --- a/t/t1400-update-ref.sh > > > +++ b/t/t1400-update-ref.sh > > > > > @@ -188,27 +194,26 @@ 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" && > > > > There is a leftover 'rm -f' here. > > Oops, thanks a lot for catching. Funny that it still passed. The '-f' option makes it ignore missing files like 'git', 'update-ref' and whatever $m happens to expand to in this test, and '/bin/rm' from coreutils (and apparently on macOS) understands the '-d' option ("remove empty directories"), so there was no error to fail this test on common setups. (I didn't investigate whether and how the not deleted $m ref affects subsequent tests...) But nowadays we have a CI job running the tests with 'busybox sh' whose builtin 'rm' doesn't have a '-d' option, and it errors out with: + rm -f git update-ref -d refs/heads/master rm: unrecognized option: d BusyBox v1.31.1 () multi-call binary. Usage: ...... https://travis-ci.org/github/git/git/jobs/742890453#L2853
On Wed, Nov 11, 2020 at 07:58:38AM +0100, Patrick Steinhardt wrote: > 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. Do you want to mention the switch away from using HEAD in some tests here? > +# 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 Do we still need this, now that we're not deleting HEAD? I think we do still delete a branch via HEAD, but that should leave an unborn branch, which is still a valid repo. > 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 > ' E.g., these ones should leave a valid repo (and must remain HEAD, because it's special for reflogging). > -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 > + git symbolic-ref SYMREF $m && > + git update-ref --no-deref -d SYMREF && > + test_must_fail git show-ref --verify -q SYMREF > ' And now this one is safe. Good. I wonder, though...is it still testing the same thing as the original? This is not related to the use of SYMREF vs HEAD, but wouldn't show-ref similarly fail if we had deleted $m, but left SYMREF in place (i.e., if --no-deref didn't actually do anything)? Perhaps this would be better: # confirm that the pointed-to ref is still there git show-ref --verify $m && # but our symref is not test_must_fail git show-ref --verify SYMREF && test_must_fail git symbolic-ref SYMREF > test_expect_success 'delete symref without dereference when the referred ref is packed' ' > - test_when_finished "cp -f .git/HEAD.orig .git/HEAD" && > echo foo >foo.c && > git add foo.c && > git commit -m foo && > + git symbolic-ref SYMREF $m && > git pack-refs --all && > - git update-ref --no-deref -d HEAD && > - test_path_is_missing .git/HEAD > + git update-ref --no-deref -d SYMREF && > + test_must_fail git show-ref --verify -q SYMREF > ' Likewise here. -Peff
On Wed, Nov 11, 2020 at 06:06:59PM -0500, Jeff King wrote: > On Wed, Nov 11, 2020 at 07:58:38AM +0100, Patrick Steinhardt wrote: > > +# 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 > > Do we still need this, now that we're not deleting HEAD? I think we do > still delete a branch via HEAD, but that should leave an unborn branch, > which is still a valid repo. Good point, we don't. > > 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 > > ' > > E.g., these ones should leave a valid repo (and must remain HEAD, > because it's special for reflogging). > > > -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 > > + git symbolic-ref SYMREF $m && > > + git update-ref --no-deref -d SYMREF && > > + test_must_fail git show-ref --verify -q SYMREF > > ' > > And now this one is safe. Good. > > I wonder, though...is it still testing the same thing as the original? > This is not related to the use of SYMREF vs HEAD, but wouldn't show-ref > similarly fail if we had deleted $m, but left SYMREF in place (i.e., if > --no-deref didn't actually do anything)? > > Perhaps this would be better: > > # confirm that the pointed-to ref is still there > git show-ref --verify $m && > # but our symref is not > test_must_fail git show-ref --verify SYMREF && > test_must_fail git symbolic-ref SYMREF It would be, but I bailed at this point because we don't actually have "$m" at this point. But agreed, i'll also include this into both tests. Patrick > > test_expect_success 'delete symref without dereference when the referred ref is packed' ' > > - test_when_finished "cp -f .git/HEAD.orig .git/HEAD" && > > echo foo >foo.c && > > git add foo.c && > > git commit -m foo && > > + git symbolic-ref SYMREF $m && > > git pack-refs --all && > > - git update-ref --no-deref -d HEAD && > > - test_path_is_missing .git/HEAD > > + git update-ref --no-deref -d SYMREF && > > + test_must_fail git show-ref --verify -q SYMREF > > ' > > Likewise here. > > -Peff
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh index 4c01e08551..d7a57488ed 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,27 +194,26 @@ 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 + git symbolic-ref SYMREF $m && + git update-ref --no-deref -d SYMREF && + test_must_fail git show-ref --verify -q SYMREF ' test_expect_success 'delete symref without dereference when the referred ref is packed' ' - test_when_finished "cp -f .git/HEAD.orig .git/HEAD" && echo foo >foo.c && git add foo.c && git commit -m foo && + git symbolic-ref SYMREF $m && git pack-refs --all && - git update-ref --no-deref -d HEAD && - test_path_is_missing .git/HEAD + git update-ref --no-deref -d SYMREF && + test_must_fail git show-ref --verify -q SYMREF ' git update-ref -d $m @@ -226,25 +231,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 +259,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 +289,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 +303,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 +1393,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 +1506,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 +1517,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 +1529,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 | 72 +++++++++++++++++++++++-------------------- 1 file changed, 39 insertions(+), 33 deletions(-)