Message ID | 20250207-477-refs-migrate-add-a-flag-to-ignore-reflogs-during-migration-v1-1-7d40f3b4e30b@gmail.com (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | builtin/refs: add '--skip-reflog' flag to bypass reflog migration | expand |
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 :) > 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
On 25/02/07 12:57PM, 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 246cebe320/246cebe320/ > 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. > > 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. Just to clarify, does a repository already without reflogs see improved migration performance with this `--skip-reflog` flag? Or is the improved performance soley due to repositories with reflogs skipping that part of the migration? > > Signed-off-by: Karthik Nayak <karthik.188@gmail.com> > --- > --- > builtin/refs.c | 3 +++ > refs.c | 8 +++++--- > refs.h | 5 ++++- > t/t1460-refs-migrate.sh | 17 +++++++++++++++-- > 4 files changed, 27 insertions(+), 6 deletions(-) > > diff --git a/builtin/refs.c b/builtin/refs.c > index a29f19583474518ee0942ea53c39cbdf9661c5e2..30be0254c14dd3d07693d70c25dddc9990756e9c 100644 > --- a/builtin/refs.c > +++ b/builtin/refs.c > @@ -30,6 +30,9 @@ static int cmd_refs_migrate(int argc, const char **argv, const char *prefix, > OPT_BIT(0, "dry-run", &flags, > N_("perform a non-destructive dry-run"), > REPO_MIGRATE_REF_STORAGE_FORMAT_DRYRUN), > + OPT_BIT(0, "skip-reflog", &flags, > + N_("skip migrating reflogs"), > + REPO_MIGRATE_REF_STORAGE_FORMAT_SKIP_REFLOG), > OPT_END(), > }; > struct strbuf errbuf = STRBUF_INIT; > 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; > + } When the `REPO_MIGRATE_REF_STORAGE_FORMAT_SKIP_REFLOG` flag is set, we now skip over all the logs to perform the reflog migration. Makes sense. -Justin > > ret = ref_transaction_commit(transaction, errbuf); > if (ret < 0) > diff --git a/refs.h b/refs.h > index a0cdd99250e8286b55808b697b0a94afac5d8319..ccee8fc6705e6e93a1c6234e7395180f8dfd815b 100644 > --- a/refs.h > +++ b/refs.h > @@ -1157,8 +1157,11 @@ int is_pseudo_ref(const char *refname); > * - REPO_MIGRATE_REF_STORAGE_FORMAT_DRYRUN: perform a dry-run migration > * without touching the main repository. The result will be written into a > * temporary ref storage directory. > + * > + * - REPO_MIGRATE_REF_STORAGE_FORMAT_SKIP_REFLOG: skip migration of reflogs. > */ > -#define REPO_MIGRATE_REF_STORAGE_FORMAT_DRYRUN (1 << 0) > +#define REPO_MIGRATE_REF_STORAGE_FORMAT_DRYRUN (1 << 0) > +#define REPO_MIGRATE_REF_STORAGE_FORMAT_SKIP_REFLOG (1 << 1) > > /* > * Migrate the ref storage format used by the repository to the > 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 )) && > git -C "$repo" for-each-ref --include-root-refs \ > --format='%(refname) %(objectname) %(symref)' >expect && > if ! $skip_reflog_verify > @@ -25,7 +27,7 @@ test_migration () { > git -C "$repo" reflog list >expect_log_list > fi && > > - git -C "$repo" refs migrate --ref-format="$2" && > + git -C "$repo" refs migrate --ref-format="$format" $@ && > > git -C "$repo" for-each-ref --include-root-refs \ > --format='%(refname) %(objectname) %(symref)' >actual && > @@ -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) && > + test_migration repo "$to_format" true --skip-reflog && > + # there should be no reflogs post migration. > + test 0 = $(git -C repo reflog --all | wc -l) > + ' > done > done > > > --- > > base-commit: bc204b742735ae06f65bb20291c95985c9633b7f > change-id: 20250207-477-refs-migrate-add-a-flag-to-ignore-reflogs-during-migration-8066f6df50ac > > Thanks > - Karthik > >
Justin Tobler <jltobler@gmail.com> writes: > On 25/02/07 12:57PM, 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 246cebe320/246cebe320/ > Thanks. >> 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. >> >> 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. > > Just to clarify, does a repository already without reflogs see improved > migration performance with this `--skip-reflog` flag? Or is the improved > performance soley due to repositories with reflogs skipping that part of > the migration? > Since we iterate over all reflogs and add them, the perf gain would only be for repositories which already have reflogs. Will modify accordingly. >> >> Signed-off-by: Karthik Nayak <karthik.188@gmail.com> >> --- >> --- >> builtin/refs.c | 3 +++ >> refs.c | 8 +++++--- >> refs.h | 5 ++++- >> t/t1460-refs-migrate.sh | 17 +++++++++++++++-- >> 4 files changed, 27 insertions(+), 6 deletions(-) >> >> diff --git a/builtin/refs.c b/builtin/refs.c >> index a29f19583474518ee0942ea53c39cbdf9661c5e2..30be0254c14dd3d07693d70c25dddc9990756e9c 100644 >> --- a/builtin/refs.c >> +++ b/builtin/refs.c >> @@ -30,6 +30,9 @@ static int cmd_refs_migrate(int argc, const char **argv, const char *prefix, >> OPT_BIT(0, "dry-run", &flags, >> N_("perform a non-destructive dry-run"), >> REPO_MIGRATE_REF_STORAGE_FORMAT_DRYRUN), >> + OPT_BIT(0, "skip-reflog", &flags, >> + N_("skip migrating reflogs"), >> + REPO_MIGRATE_REF_STORAGE_FORMAT_SKIP_REFLOG), >> OPT_END(), >> }; >> struct strbuf errbuf = STRBUF_INIT; >> 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; >> + } > > When the `REPO_MIGRATE_REF_STORAGE_FORMAT_SKIP_REFLOG` flag is set, we > now skip over all the logs to perform the reflog migration. Makes sense. > Yup, thanks for the review. > -Justin >
Patrick Steinhardt <ps@pks.im> writes: > 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. > Fair enough, I agree that, finally, we mostly care about not having reflogs in the end. I'll modify accordingly. >> 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. >> 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 :) > True. I think this is because of lack of conditionals in the templating. >> 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. > I was skeptical too, while not a complete test, the CI seemed to not complain. >> @@ -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. >> + 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
diff --git a/builtin/refs.c b/builtin/refs.c index a29f19583474518ee0942ea53c39cbdf9661c5e2..30be0254c14dd3d07693d70c25dddc9990756e9c 100644 --- a/builtin/refs.c +++ b/builtin/refs.c @@ -30,6 +30,9 @@ static int cmd_refs_migrate(int argc, const char **argv, const char *prefix, OPT_BIT(0, "dry-run", &flags, N_("perform a non-destructive dry-run"), REPO_MIGRATE_REF_STORAGE_FORMAT_DRYRUN), + OPT_BIT(0, "skip-reflog", &flags, + N_("skip migrating reflogs"), + REPO_MIGRATE_REF_STORAGE_FORMAT_SKIP_REFLOG), OPT_END(), }; struct strbuf errbuf = STRBUF_INIT; 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) diff --git a/refs.h b/refs.h index a0cdd99250e8286b55808b697b0a94afac5d8319..ccee8fc6705e6e93a1c6234e7395180f8dfd815b 100644 --- a/refs.h +++ b/refs.h @@ -1157,8 +1157,11 @@ int is_pseudo_ref(const char *refname); * - REPO_MIGRATE_REF_STORAGE_FORMAT_DRYRUN: perform a dry-run migration * without touching the main repository. The result will be written into a * temporary ref storage directory. + * + * - REPO_MIGRATE_REF_STORAGE_FORMAT_SKIP_REFLOG: skip migration of reflogs. */ -#define REPO_MIGRATE_REF_STORAGE_FORMAT_DRYRUN (1 << 0) +#define REPO_MIGRATE_REF_STORAGE_FORMAT_DRYRUN (1 << 0) +#define REPO_MIGRATE_REF_STORAGE_FORMAT_SKIP_REFLOG (1 << 1) /* * Migrate the ref storage format used by the repository to the 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 )) && git -C "$repo" for-each-ref --include-root-refs \ --format='%(refname) %(objectname) %(symref)' >expect && if ! $skip_reflog_verify @@ -25,7 +27,7 @@ test_migration () { git -C "$repo" reflog list >expect_log_list fi && - git -C "$repo" refs migrate --ref-format="$2" && + git -C "$repo" refs migrate --ref-format="$format" $@ && git -C "$repo" for-each-ref --include-root-refs \ --format='%(refname) %(objectname) %(symref)' >actual && @@ -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) && + test_migration repo "$to_format" true --skip-reflog && + # there should be no reflogs post migration. + test 0 = $(git -C repo reflog --all | wc -l) + ' done done
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). 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. 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. Signed-off-by: Karthik Nayak <karthik.188@gmail.com> --- --- builtin/refs.c | 3 +++ refs.c | 8 +++++--- refs.h | 5 ++++- t/t1460-refs-migrate.sh | 17 +++++++++++++++-- 4 files changed, 27 insertions(+), 6 deletions(-) --- base-commit: bc204b742735ae06f65bb20291c95985c9633b7f change-id: 20250207-477-refs-migrate-add-a-flag-to-ignore-reflogs-during-migration-8066f6df50ac Thanks - Karthik