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 <karthik.188@xxxxxxxxx>
> ---
> ---
Another thing to teach b4: skip the comment in a single-patch patch
series in case you don't supply a cover letter :)
> 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.
> 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.
> @@ -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
