[PATCH 00/10] t: more compatibility fixes with reftables

[PATCH 00/10] t: more compatibility fixes with reftables

[Patrick Steinhardt]

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

Hi,

this is the second patch series that refactors tests to become
compatible with the upcoming reftables backend. It's in the same spirit
as the first round of patches [1], where most of the refactorings are to
use plumbing tools to access refs or the reflog instead of modifying
on-disk data structures directly.

Patrick

[1]: <cover.1697607222.git.ps@pks.im>

Patrick Steinhardt (10):
  t0410: mark tests to require the reffiles backend
  t1400: split up generic reflog tests from the reffile-specific ones
  t1401: stop treating FETCH_HEAD as real reference
  t1410: use test-tool to create empty reflog
  t1417: make `reflog --updateref` tests backend agnostic
  t3310: stop checking for reference existence via `test -f`
  t4013: simplify magic parsing and drop "failure"
  t5401: speed up creation of many branches
  t5551: stop writing packed-refs directly
  t6301: write invalid object ID via `test-tool ref-store`

 t/t0410-partial-clone.sh              |  4 +--
 t/t1400-update-ref.sh                 | 41 +++++++++++++++++++++++----
 t/t1401-symbolic-ref.sh               |  4 +--
 t/t1410-reflog.sh                     |  4 +--
 t/t1417-reflog-updateref.sh           | 10 +++++--
 t/t3310-notes-merge-manual-resolve.sh |  6 ++--
 t/t4013-diff-various.sh               | 27 ++++++++----------
 t/t5401-update-hooks.sh               |  6 ++--
 t/t5551-http-fetch-smart.sh           |  4 ++-
 t/t6301-for-each-ref-errors.sh        | 13 ++++-----
 10 files changed, 74 insertions(+), 45 deletions(-)

-- 
2.43.0


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

[PATCH 01/10] t0410: mark tests to require the reffiles backend

[Patrick Steinhardt]

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

Two of our tests in t0410 verify whether partial clones end up with the
correct repository format version and extensions. These checks require
the reffiles backend because every other backend would by necessity bump
the repository format version to be at least 1.

Mark the tests accordingly.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t0410-partial-clone.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh
index 5b7bee888d..6b6424b3df 100755
--- a/t/t0410-partial-clone.sh
+++ b/t/t0410-partial-clone.sh
@@ -49,7 +49,7 @@ test_expect_success 'convert shallow clone to partial clone' '
 	test_cmp_config -C client 1 core.repositoryformatversion
 '
 
-test_expect_success SHA1 'convert to partial clone with noop extension' '
+test_expect_success SHA1,REFFILES 'convert to partial clone with noop extension' '
 	rm -fr server client &&
 	test_create_repo server &&
 	test_commit -C server my_commit 1 &&
@@ -60,7 +60,7 @@ test_expect_success SHA1 'convert to partial clone with noop extension' '
 	git -C client fetch --unshallow --filter="blob:none"
 '
 
-test_expect_success SHA1 'converting to partial clone fails with unrecognized extension' '
+test_expect_success SHA1,REFFILES 'converting to partial clone fails with unrecognized extension' '
 	rm -fr server client &&
 	test_create_repo server &&
 	test_commit -C server my_commit 1 &&
-- 
2.43.0


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

[PATCH 02/10] t1400: split up generic reflog tests from the reffile-specific ones

[Patrick Steinhardt]

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

We have a bunch of tests in t1400 that check whether we correctly read
reflog entries. These tests create the reflog by manually writing to the
respective loose file, which makes them specific to the files backend.
But while some of them do indeed exercise very specific edge cases in
the reffiles backend, most of the tests exercise generic functionality
that should be common to all backends.

Unfortunately, we can't easily adapt all of the tests to work with all
backends. Instead, split out the reffile-specific tests from the ones
that should work with all backends and refactor the generic ones to not
write to the on-disk files directly anymore.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t1400-update-ref.sh | 41 +++++++++++++++++++++++++++++++++++------
 1 file changed, 35 insertions(+), 6 deletions(-)

diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 9ac4b7036b..c41cd9b6bf 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -354,14 +354,28 @@ test_expect_success "verifying $m's log (logged by config)" '
 '
 
 test_expect_success 'set up for querying the reflog' '
+	git update-ref -d $m &&
+	test-tool ref-store main delete-reflog $m &&
+
+	GIT_COMMITTER_DATE="1117150320 -0500" git update-ref $m $C &&
+	GIT_COMMITTER_DATE="1117150350 -0500" git update-ref $m $A &&
+	GIT_COMMITTER_DATE="1117150380 -0500" git update-ref $m $B &&
+	GIT_COMMITTER_DATE="1117150680 -0500" git update-ref $m $F &&
+	GIT_COMMITTER_DATE="1117150980 -0500" git update-ref $m $E &&
 	git update-ref $m $D &&
-	cat >.git/logs/$m <<-EOF
+	# Delete the last reflog entry so that the tip of m and the reflog for
+	# it disagree.
+	git reflog delete $m@{0} &&
+
+	cat >expect <<-EOF &&
 	$Z $C $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150320 -0500
 	$C $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150350 -0500
 	$A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150380 -0500
-	$F $Z $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150680 -0500
-	$Z $E $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150980 -0500
+	$B $F $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150680 -0500
+	$F $E $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150980 -0500
 	EOF
+	test-tool ref-store main for-each-reflog-ent $m >actual &&
+	test_cmp expect actual
 '
 
 ed="Thu, 26 May 2005 18:32:00 -0500"
@@ -409,13 +423,12 @@ test_expect_success 'Query "main@{2005-05-26 23:33:01}" (middle of history with
 	test_when_finished "rm -f o e" &&
 	git rev-parse --verify "main@{2005-05-26 23:33:01}" >o 2>e &&
 	echo "$B" >expect &&
-	test_cmp expect o &&
-	test_grep -F "warning: log for ref $m has gap after $gd" e
+	test_cmp expect o
 '
 test_expect_success 'Query "main@{2005-05-26 23:38:00}" (middle of history)' '
 	test_when_finished "rm -f o e" &&
 	git rev-parse --verify "main@{2005-05-26 23:38:00}" >o 2>e &&
-	echo "$Z" >expect &&
+	echo "$F" >expect &&
 	test_cmp expect o &&
 	test_must_be_empty e
 '
@@ -436,6 +449,22 @@ test_expect_success 'Query "main@{2005-05-28}" (past end of history)' '
 
 rm -f .git/$m .git/logs/$m expect
 
+test_expect_success REFFILES 'query reflog with gap' '
+	test_when_finished "git update-ref -d $m" &&
+
+	git update-ref $m $F &&
+	cat >.git/logs/$m <<-EOF &&
+	$Z $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150320 -0500
+	$A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150380 -0500
+	$D $F $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150680 -0500
+	EOF
+
+	git rev-parse --verify "main@{2005-05-26 23:33:01}" >actual 2>stderr &&
+	echo "$B" >expect &&
+	test_cmp expect actual &&
+	test_grep -F "warning: log for ref $m has gap after $gd" stderr
+'
+
 test_expect_success 'creating initial files' '
 	test_when_finished rm -f M &&
 	echo TEST >F &&
-- 
2.43.0


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

[PATCH 03/10] t1401: stop treating FETCH_HEAD as real reference

[Patrick Steinhardt]

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

One of the tests in t1401 asserts that we can create a symref from a
symbolic reference to a top-level reference, which is done by linking
from `refs/heads/top-level` to `FETCH_HEAD`. But `FETCH_HEAD` is not a
proper reference and doesn't even follow the loose reference format, so
it is not a good candidate for the logic under test.

Refactor the test to use `ORIG_HEAD` instead of `FETCH_HEAD`. This also
works with other backends than the reffiles one.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t1401-symbolic-ref.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t1401-symbolic-ref.sh b/t/t1401-symbolic-ref.sh
index c7745e1bf6..3241d35917 100755
--- a/t/t1401-symbolic-ref.sh
+++ b/t/t1401-symbolic-ref.sh
@@ -171,8 +171,8 @@ test_expect_success 'symbolic-ref refuses invalid target for non-HEAD' '
 '
 
 test_expect_success 'symbolic-ref allows top-level target for non-HEAD' '
-	git symbolic-ref refs/heads/top-level FETCH_HEAD &&
-	git update-ref FETCH_HEAD HEAD &&
+	git symbolic-ref refs/heads/top-level ORIG_HEAD &&
+	git update-ref ORIG_HEAD HEAD &&
 	test_cmp_rev top-level HEAD
 '
 
-- 
2.43.0


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

[PATCH 04/10] t1410: use test-tool to create empty reflog

[Patrick Steinhardt]

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

One of the tests in t1410 is marked to be specific to the files
reference backend, which is because we create a reflog manually by
creating the respective file. Refactor the test to instead use our
`test-tool ref-store` helper to create the reflog so that it works with
other reference backends, as well.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t1410-reflog.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh
index aeddc2fb3f..a0ff8d51f0 100755
--- a/t/t1410-reflog.sh
+++ b/t/t1410-reflog.sh
@@ -469,11 +469,11 @@ test_expect_success 'expire one of multiple worktrees' '
 	)
 '
 
-test_expect_success REFFILES 'empty reflog' '
+test_expect_success 'empty reflog' '
 	test_when_finished "rm -rf empty" &&
 	git init empty &&
 	test_commit -C empty A &&
-	>empty/.git/logs/refs/heads/foo &&
+	test-tool ref-store main create-reflog refs/heads/foo &&
 	git -C empty reflog expire --all 2>err &&
 	test_must_be_empty err
 '
-- 
2.43.0


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

[PATCH 05/10] t1417: make `reflog --updateref` tests backend agnostic

[Patrick Steinhardt]

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

The tests for `git reflog delete --updateref` are currently marked to
only run with the reffiles backend. There is no inherent reason that
this should be the case other than the fact that the setup messes with
the on-disk reflogs directly.

Refactor the test to stop doing so and drop the REFFILES prerequisite.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t1417-reflog-updateref.sh | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/t/t1417-reflog-updateref.sh b/t/t1417-reflog-updateref.sh
index 14f13b57c6..0eb5e674bc 100755
--- a/t/t1417-reflog-updateref.sh
+++ b/t/t1417-reflog-updateref.sh
@@ -14,9 +14,13 @@ test_expect_success 'setup' '
 		test_commit B &&
 		test_commit C &&
 
-		cp .git/logs/HEAD HEAD.old &&
+		git reflog HEAD >expect &&
 		git reset --hard HEAD~ &&
-		cp HEAD.old .git/logs/HEAD
+		# Make sure that the reflog does not point to the same commit
+		# as HEAD.
+		git reflog delete HEAD@{0} &&
+		git reflog HEAD >actual &&
+		test_cmp expect actual
 	)
 '
 
@@ -25,7 +29,7 @@ test_reflog_updateref () {
 	shift
 	args="$@"
 
-	test_expect_success REFFILES "get '$exp' with '$args'"  '
+	test_expect_success "get '$exp' with '$args'"  '
 		test_when_finished "rm -rf copy" &&
 		cp -R repo copy &&
 
-- 
2.43.0


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

[PATCH 06/10] t3310: stop checking for reference existence via `test -f`

[Patrick Steinhardt]

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

One of the tests in t3310 exercises whether the special references
`NOTES_MERGE_PARTIAL` and `NOTES_MERGE_REF` exist as expected when the
notes subsystem runs into a merge conflict. This is done by checking
on-disk data structures directly though instead of asking the reference
backend.

Refactor the test to use git-rev-parse(1) instead.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t3310-notes-merge-manual-resolve.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/t/t3310-notes-merge-manual-resolve.sh b/t/t3310-notes-merge-manual-resolve.sh
index 60d6ed2dc8..597df5ebc0 100755
--- a/t/t3310-notes-merge-manual-resolve.sh
+++ b/t/t3310-notes-merge-manual-resolve.sh
@@ -561,9 +561,9 @@ y and z notes on 4th commit
 EOF
 	# Fail to finalize merge
 	test_must_fail git notes merge --commit >output 2>&1 &&
-	# .git/NOTES_MERGE_* must remain
-	test -f .git/NOTES_MERGE_PARTIAL &&
-	test -f .git/NOTES_MERGE_REF &&
+	# NOTES_MERGE_* refs and .git/NOTES_MERGE_* state files must remain
+	git rev-parse --verify NOTES_MERGE_PARTIAL &&
+	git rev-parse --verify NOTES_MERGE_REF &&
 	test -f .git/NOTES_MERGE_WORKTREE/$commit_sha1 &&
 	test -f .git/NOTES_MERGE_WORKTREE/$commit_sha2 &&
 	test -f .git/NOTES_MERGE_WORKTREE/$commit_sha3 &&
-- 
2.43.0


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

[PATCH 07/10] t4013: simplify magic parsing and drop "failure"

[Patrick Steinhardt]

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

In t14013, we have various different tests that verify whether certain
diffs are generated as expected. As much of the logic is the same across
many of the tests we some common code in there that generates the actual
test cases for us.

As some diffs are more special than others depending on the command line
parameters passed to git-diff(1), these tests need to adapt behaviour to
the specific test case sometimes. This is done via colon-prefixed magic
commands, of which we currently know "failure" and "noellipses". The
logic to parse this magic is a bit convoluted though and hard to grasp,
also due to the rather unnecessary nesting.

Un-nest the cases so that it becomes a bit more straightfoward. The
logic is further simplified by removing support for the "failure" magic,
which is not actually used anymore.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t4013-diff-various.sh | 27 ++++++++++++---------------
 1 file changed, 12 insertions(+), 15 deletions(-)

diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index 5cc17c2e0d..76b8619e2e 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -178,32 +178,29 @@ process_diffs () {
 V=$(git version | sed -e 's/^git version //' -e 's/\./\\./g')
 while read magic cmd
 do
-	status=success
 	case "$magic" in
 	'' | '#'*)
 		continue ;;
-	:*)
-		magic=${magic#:}
+	:noellipses)
+		magic=noellipses
 		label="$magic-$cmd"
-		case "$magic" in
-		noellipses) ;;
-		failure)
-			status=failure
-			magic=
-			label="$cmd" ;;
-		*)
-			BUG "unknown magic $magic" ;;
-		esac ;;
+		;;
+	:*)
+		BUG "unknown magic $magic"
+		;;
 	*)
-		cmd="$magic $cmd" magic=
-		label="$cmd" ;;
+		cmd="$magic $cmd"
+		magic=
+		label="$cmd"
+		;;
 	esac
+
 	test=$(echo "$label" | sed -e 's|[/ ][/ ]*|_|g')
 	pfx=$(printf "%04d" $test_count)
 	expect="$TEST_DIRECTORY/t4013/diff.$test"
 	actual="$pfx-diff.$test"
 
-	test_expect_$status "git $cmd # magic is ${magic:-(not used)}" '
+	test_expect_success "git $cmd # magic is ${magic:-(not used)}" '
 		{
 			echo "$ git $cmd"
 			case "$magic" in
-- 
2.43.0


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

[PATCH 08/10] t5401: speed up creation of many branches

[Patrick Steinhardt]

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

One of the tests in t5401 creates a bunch of branches by calling
git-branch(1) for every one of them. This is quite inefficient and takes
a comparatively long time even on Unix systems where spawning processes
is comparatively fast. Refactor it to instead use git-update-ref(1),
which leads to an almost 10-fold speedup:

```
Benchmark 1: ./t5401-update-hooks.sh (rev = HEAD)
  Time (mean ± σ):     983.2 ms ±  97.6 ms    [User: 328.8 ms, System: 679.2 ms]
  Range (min … max):   882.9 ms … 1078.0 ms    3 runs

Benchmark 2: ./t5401-update-hooks.sh (rev = HEAD~)
  Time (mean ± σ):      9.312 s ±  0.398 s    [User: 2.766 s, System: 6.617 s]
  Range (min … max):    8.885 s …  9.674 s    3 runs

Summary
  ./t5401-update-hooks.sh (rev = HEAD) ran
    9.47 ± 1.02 times faster than ./t5401-update-hooks.sh (rev = HEAD~)
```

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t5401-update-hooks.sh | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/t/t5401-update-hooks.sh b/t/t5401-update-hooks.sh
index 001b7a17ad..8b8bc47dc0 100755
--- a/t/t5401-update-hooks.sh
+++ b/t/t5401-update-hooks.sh
@@ -133,10 +133,8 @@ test_expect_success 'pre-receive hook that forgets to read its input' '
 	EOF
 	rm -f victim.git/hooks/update victim.git/hooks/post-update &&
 
-	for v in $(test_seq 100 999)
-	do
-		git branch branch_$v main || return
-	done &&
+	printf "create refs/heads/branch_%d main\n" $(test_seq 100 999) >input &&
+	git update-ref --stdin <input &&
 	git push ./victim.git "+refs/heads/*:refs/heads/*"
 '
 
-- 
2.43.0


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

[PATCH 09/10] t5551: stop writing packed-refs directly

[Patrick Steinhardt]

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

We have multiple tests in t5551 that write thousands of tags. To do so
efficiently we generate the tags by writing the `packed-refs` file
directly, which of course assumes that the reference database is backed
by the files backend.

Refactor the code to instead use a single `git update-ref --stdin`
command to write the tags. While the on-disk end result is not the same
as we now have a bunch of loose refs instead of a single packed-refs
file, the distinction shouldn't really matter for any of the tests that
use this helper.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t5551-http-fetch-smart.sh | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index 8a41adf1e1..e069737b80 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -359,7 +359,9 @@ create_tags () {
 
 	# now assign tags to all the dangling commits we created above
 	tag=$(perl -e "print \"bla\" x 30") &&
-	sed -e "s|^:\([^ ]*\) \(.*\)$|\2 refs/tags/$tag-\1|" <marks >>packed-refs
+	sed -e "s|^:\([^ ]*\) \(.*\)$|create refs/tags/$tag-\1 \2|" <marks >input &&
+	git update-ref --stdin <input &&
+	rm input
 }
 
 test_expect_success 'create 2,000 tags in the repo' '
-- 
2.43.0


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

[PATCH 10/10] t6301: write invalid object ID via `test-tool ref-store`

[Patrick Steinhardt]

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

One of the tests in t6301 verifies that the reference backend correctly
warns about the case where a reference points to a non-existent object.
This is done by writing the object ID into the loose reference directly,
which is quite intimate with how the files backend works.

Refactor the code to instead use `test-tool ref-store` to write the
reference, which is backend-agnostic.

There are two more tests in this file which write loose files directly,
as well. But both of them are indeed quite specific to the loose files
backend and cannot be easily ported to other backends. We thus mark them
as requiring the REFFILES prerequisite.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t6301-for-each-ref-errors.sh | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/t/t6301-for-each-ref-errors.sh b/t/t6301-for-each-ref-errors.sh
index 2667dd13fe..83b8a19d94 100755
--- a/t/t6301-for-each-ref-errors.sh
+++ b/t/t6301-for-each-ref-errors.sh
@@ -15,7 +15,7 @@ test_expect_success setup '
 	git for-each-ref --format="%(objectname) %(refname)" >brief-list
 '
 
-test_expect_success 'Broken refs are reported correctly' '
+test_expect_success REFFILES 'Broken refs are reported correctly' '
 	r=refs/heads/bogus &&
 	: >.git/$r &&
 	test_when_finished "rm -f .git/$r" &&
@@ -25,7 +25,7 @@ test_expect_success 'Broken refs are reported correctly' '
 	test_cmp broken-err err
 '
 
-test_expect_success 'NULL_SHA1 refs are reported correctly' '
+test_expect_success REFFILES 'NULL_SHA1 refs are reported correctly' '
 	r=refs/heads/zeros &&
 	echo $ZEROS >.git/$r &&
 	test_when_finished "rm -f .git/$r" &&
@@ -39,15 +39,14 @@ test_expect_success 'NULL_SHA1 refs are reported correctly' '
 '
 
 test_expect_success 'Missing objects are reported correctly' '
-	r=refs/heads/missing &&
-	echo $MISSING >.git/$r &&
-	test_when_finished "rm -f .git/$r" &&
-	echo "fatal: missing object $MISSING for $r" >missing-err &&
+	test_when_finished "git update-ref -d refs/heads/missing" &&
+	test-tool ref-store main update-ref msg refs/heads/missing "$MISSING" "$ZERO_OID" REF_SKIP_OID_VERIFICATION &&
+	echo "fatal: missing object $MISSING for refs/heads/missing" >missing-err &&
 	test_must_fail git for-each-ref 2>err &&
 	test_cmp missing-err err &&
 	(
 		cat brief-list &&
-		echo "$MISSING $r"
+		echo "$MISSING refs/heads/missing"
 	) | sort -k 2 >missing-brief-expected &&
 	git for-each-ref --format="%(objectname) %(refname)" >brief-out 2>brief-err &&
 	test_cmp missing-brief-expected brief-out &&
-- 
2.43.0


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

Re: [PATCH 01/10] t0410: mark tests to require the reffiles backend

[Taylor Blau]

On Wed, Nov 29, 2023 at 08:24:40AM +0100, Patrick Steinhardt wrote:
> Two of our tests in t0410 verify whether partial clones end up with the
> correct repository format version and extensions. These checks require
> the reffiles backend because every other backend would by necessity bump
> the repository format version to be at least 1.
>
> Mark the tests accordingly.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  t/t0410-partial-clone.sh | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh
> index 5b7bee888d..6b6424b3df 100755
> --- a/t/t0410-partial-clone.sh
> +++ b/t/t0410-partial-clone.sh
> @@ -49,7 +49,7 @@ test_expect_success 'convert shallow clone to partial clone' '
>  	test_cmp_config -C client 1 core.repositoryformatversion
>  '
>
> -test_expect_success SHA1 'convert to partial clone with noop extension' '
> +test_expect_success SHA1,REFFILES 'convert to partial clone with noop extension' '

I thought for a second that the SHA1 prerequisite would cover this
already, but that's not right since you can be in SHA1 mode even if your
repositoryformatversion is 1. So this change makes sense to me and is
well-reasoned.

Thanks,
Taylor

Re: [PATCH 08/10] t5401: speed up creation of many branches

[Taylor Blau]

On Wed, Nov 29, 2023 at 08:25:09AM +0100, Patrick Steinhardt wrote:
> One of the tests in t5401 creates a bunch of branches by calling
> git-branch(1) for every one of them. This is quite inefficient and takes
> a comparatively long time even on Unix systems where spawning processes
> is comparatively fast. Refactor it to instead use git-update-ref(1),
> which leads to an almost 10-fold speedup:
>
> ```
> Benchmark 1: ./t5401-update-hooks.sh (rev = HEAD)
>   Time (mean ± σ):     983.2 ms ±  97.6 ms    [User: 328.8 ms, System: 679.2 ms]
>   Range (min … max):   882.9 ms … 1078.0 ms    3 runs
>
> Benchmark 2: ./t5401-update-hooks.sh (rev = HEAD~)
>   Time (mean ± σ):      9.312 s ±  0.398 s    [User: 2.766 s, System: 6.617 s]
>   Range (min … max):    8.885 s …  9.674 s    3 runs
>
> Summary
>   ./t5401-update-hooks.sh (rev = HEAD) ran
>     9.47 ± 1.02 times faster than ./t5401-update-hooks.sh (rev = HEAD~)

Very nice ;-).

> diff --git a/t/t5401-update-hooks.sh b/t/t5401-update-hooks.sh
> index 001b7a17ad..8b8bc47dc0 100755
> --- a/t/t5401-update-hooks.sh
> +++ b/t/t5401-update-hooks.sh
> @@ -133,10 +133,8 @@ test_expect_success 'pre-receive hook that forgets to read its input' '
>  	EOF
>  	rm -f victim.git/hooks/update victim.git/hooks/post-update &&
>
> -	for v in $(test_seq 100 999)
> -	do
> -		git branch branch_$v main || return
> -	done &&
> +	printf "create refs/heads/branch_%d main\n" $(test_seq 100 999) >input &&
> +	git update-ref --stdin <input &&

Not that it really matters here, but you could pipe the output of your
printf directly into git update-ref. I don't think we rely on the value
of "input" after this point, and git is on the right-hand side of the
pipe, so this is safe to do.

But it doesn't matter much either way, just something I noticed while
reading.

Thanks,
Taylor

Re: [PATCH 09/10] t5551: stop writing packed-refs directly

[Taylor Blau]

On Wed, Nov 29, 2023 at 08:25:14AM +0100, Patrick Steinhardt wrote:
> We have multiple tests in t5551 that write thousands of tags. To do so
> efficiently we generate the tags by writing the `packed-refs` file
> directly, which of course assumes that the reference database is backed
> by the files backend.
>
> Refactor the code to instead use a single `git update-ref --stdin`
> command to write the tags. While the on-disk end result is not the same
> as we now have a bunch of loose refs instead of a single packed-refs
> file, the distinction shouldn't really matter for any of the tests that
> use this helper.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  t/t5551-http-fetch-smart.sh | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
> index 8a41adf1e1..e069737b80 100755
> --- a/t/t5551-http-fetch-smart.sh
> +++ b/t/t5551-http-fetch-smart.sh
> @@ -359,7 +359,9 @@ create_tags () {
>
>  	# now assign tags to all the dangling commits we created above
>  	tag=$(perl -e "print \"bla\" x 30") &&
> -	sed -e "s|^:\([^ ]*\) \(.*\)$|\2 refs/tags/$tag-\1|" <marks >>packed-refs
> +	sed -e "s|^:\([^ ]*\) \(.*\)$|create refs/tags/$tag-\1 \2|" <marks >input &&
> +	git update-ref --stdin <input &&
> +	rm input

Same note here as in the previous patch, although here I think we'd
prefer to pipe from sed to git update-ref directly, rather than writing
an intermediate file which we have to clean up afterwords.

Thanks,
Taylor

Re: [PATCH 00/10] t: more compatibility fixes with reftables

[Taylor Blau]

On Wed, Nov 29, 2023 at 08:24:36AM +0100, Patrick Steinhardt wrote:
> Hi,
>
> this is the second patch series that refactors tests to become
> compatible with the upcoming reftables backend. It's in the same spirit
> as the first round of patches [1], where most of the refactorings are to
> use plumbing tools to access refs or the reflog instead of modifying
> on-disk data structures directly.

All looks good to me. Thanks for a pleasant read :-).

Thanks,
Taylor

Re: [PATCH 00/10] t: more compatibility fixes with reftables

[Patrick Steinhardt]

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

On Wed, Nov 29, 2023 at 06:30:47PM -0500, Taylor Blau wrote:
> On Wed, Nov 29, 2023 at 08:24:36AM +0100, Patrick Steinhardt wrote:
> > Hi,
> >
> > this is the second patch series that refactors tests to become
> > compatible with the upcoming reftables backend. It's in the same spirit
> > as the first round of patches [1], where most of the refactorings are to
> > use plumbing tools to access refs or the reflog instead of modifying
> > on-disk data structures directly.
> 
> All looks good to me. Thanks for a pleasant read :-).
> 
> Thanks,
> Taylor

If you don't mind, I'll refrain from sending a v2 only to start piping
output into git-update-ref(1) directly. I'll incorporate that feedback
though if there is a need for v2 due to other reasons.

Thanks for your review!

Patrick

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