Patrick Steinhardt [off-list ref] writes:

On Fri, Feb 07, 2025 at 12:57:31PM +0100, Karthik Nayak wrote:

quoted

The 'git-refs(1)' migrate subcommand, which transfers repositories
between reference backends, currently migrates reflogs by default as of
In 246cebe320 (refs: add support for migrating reflogs, 2024-12-16).

s/In//

quoted

While this behavior is desirable for most client-side repositories,
server-side repositories typically don't use reflogs and the migration
of these entries is unnecessary overhead.

Nit: if the server-side repository doesn't _have_ reflogs, then there
cannot be any overhead caused by their migration either, right? I still
think that the flag makes sense (well, I proposed it). But to me the
argument is rather that we don't _expect_ there to be any reflogs, but
due to historic reasons there actually _might_ be some. This could for
example be caused by a bugs, misconfiguration, or an admin who has
enabled reflogs on the server-side to debug something.

So even if there are some reflogs, we don't want to migrate them. Which
coincidentally helps us to improve performance, but the real value-add
here is that it makes the result match our expectations.

Fair enough, I agree that, finally, we mostly care about not having
reflogs in the end. I'll modify accordingly.

quoted

Add a '--skip-reflog' flag to the migrate subcommand to make reflog
migration optional. This is particularly useful for server-side
migrations where reflogs are not needed, improving migration performance
in these scenarios.

The second sentence of this paragraph feels duplicated with what you
have already been saying in the preceding paragraph.

Will cleanup.

quoted

Signed-off-by: Karthik Nayak <redacted>
---
---

Another thing to teach b4: skip the comment in a single-patch patch
series in case you don't supply a cover letter :)

True. I think this is because of lack of conditionals in the templating.

quoted

diff --git a/refs.c b/refs.c
index f4094a326a9f88f979654b668cc9c3d27d83cb5d..5e8f5c06fa68d16c93ee11edd9742995eea994b6 100644
--- a/refs.c
+++ b/refs.c

@@ -3035,9 +3035,11 @@ int repo_migrate_ref_storage_format(struct repository *repo,
 	if (ret < 0)
 		goto done;

-	ret = refs_for_each_reflog(old_refs, migrate_one_reflog, &data);
-	if (ret < 0)
-		goto done;
+	if (!(flags & REPO_MIGRATE_REF_STORAGE_FORMAT_SKIP_REFLOG)) {
+		ret = refs_for_each_reflog(old_refs, migrate_one_reflog, &data);
+		if (ret < 0)
+			goto done;
+	}

 	ret = ref_transaction_commit(transaction, errbuf);
 	if (ret < 0)

Nice and simple, as expected.

quoted

diff --git a/t/t1460-refs-migrate.sh b/t/t1460-refs-migrate.sh
index a6d9b35a46eb59350aa0d59d982a2fbfaecf1448..9059d4c4121842a9d2e77dc4e54c537eeff8afab 100755
--- a/t/t1460-refs-migrate.sh
+++ b/t/t1460-refs-migrate.sh

@@ -9,14 +9,16 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME

 # Migrate the provided repository from one format to the other and
 # verify that the references and logs are migrated over correctly.
-# Usage: test_migration <repo> <format> <skip_reflog_verify>
+# Usage: test_migration <repo> <format> <skip_reflog_verify> <...options>
 #   <repo> is the relative path to the repo to be migrated.
 #   <format> is the ref format to be migrated to.
 #   <skip_reflog_verify> (true or false) whether to skip reflog verification.
+#   <...options> are other options be passed directly to 'git refs migrate'.
 test_migration () {
 	repo=$1 &&
 	format=$2 &&
 	skip_reflog_verify=${3:-false} &&
+	shift $(( $# >= 3 ? 3 : 2 )) &&

I honestly have no idea whether this works with all supported shells. If
it does it's a bit funky, but should work alright for our purpose. I was
thinking a bit about how to improve this, but ultimately came to the
conclusion that there isn't really a need to overengineer this simple
test function.

I was skeptical too, while not a complete test, the CI seemed to not
complain.

quoted

@@ -241,6 +243,17 @@ do
 				test_cmp expect.reflog actual.reflog
 			)
 		'
+
+		test_expect_success "$from_format -> $to_format: skip reflog with --skip-reflog" '
+			test_when_finished "rm -rf repo" &&
+			git init --ref-format=$from_format repo &&
+			test_commit -C repo initial &&
+			# we see that the repository contains reflogs.
+			test 2 = $(git -C repo reflog --all | wc -l) &&

Nit: we don't want to have Git on the left-hand side of a pipe, as it
might make use lose its exit code. This could instead be:

    git -C repo reflog --all >reflogs &&
    test_line_count = 2 reflogs

This looks cleaner.

quoted

+			test_migration repo "$to_format" true --skip-reflog &&
+			# there should be no reflogs post migration.
+			test 0 = $(git -C repo reflog --all | wc -l)

And here we could use `test_must_be_empty`.

Will amend, Thanks for the review.

Thanks!

Patrick