Re: [PATCH] builtin/refs: add '--skip-reflog' flag to bypass reflog migration
From: Patrick Steinhardt <hidden>
Date: 2025-02-07 15:23:40
On Fri, Feb 07, 2025 at 12:57:31PM +0100, Karthik Nayak wrote:
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//
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.
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.
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 :)
quoted hunk ↗ jump to hunk
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 hunk ↗ jump to hunk
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.
quoted hunk ↗ jump to hunk
@@ -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
+ 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`. Thanks! Patrick