Re: [PATCH] builtin/refs: add '--skip-reflog' flag to bypass reflog migration
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Feb 7, 2025, 15:23 UTC
- Message-ID
- <Z6YlcpKp2U2V8Yd3@pks.im>
- In-Reply-To
- <20250207-477-refs-migrate-add-a-flag-to-ignore-reflogs-during-migration-v1-1-7d40f3b4e30b@gmail.com>
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@gmail.com> > --- > ---
Another thing to teach b4: skip the comment in a single-patch patch series in case you don't supply a cover letter :)
Show 19 quoted lines
> 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.
Show 19 quoted lines
> 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.
Show 11 quoted lines
> @@ -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