Junio C Hamano <gitster@xxxxxxxxx> writes: > Karthik Nayak <karthik.188@xxxxxxxxx> writes: > >> The 'git-refs(1)' migrate subcommand, which transfers repositories >> between reference backends, currently migrates reflogs by default as of >> 246cebe320 (refs: add support for migrating reflogs, 2024-12-16). >> >> While this behavior is desirable for most client-side repositories, >> server-side repositories are not expected to contain reflogs. However, >> due to historical reasons, some may still have them. This could be >> caused, for example, by bugs, misconfiguration, or an administrator >> enabling reflogs on the server for debugging purposes. >> >> To address this, introduce the --skip-reflog flag, allowing users to >> bypass reflog migration. This ensures that the repository ends up in the >> expected state after migration. >> >> Helped-by: Patrick Steinhardt <ps@xxxxxx> >> Signed-off-by: Karthik Nayak <karthik.188@xxxxxxxxx> >> --- >> Changes in v2: >> - Fix typo in commit mesasge and clarify the intent. >> - Modify the test to use `test_line_count` and `test_must_be_empty`. >> - Link to v1: https://lore.kernel.org/r/20250207-477-refs-migrate-add-a-flag-to-ignore-reflogs-during-migration-v1-1-7d40f3b4e30b@xxxxxxxxx >> --- >> Range-diff versus v1: >> >> 1: ce14d3d07e ! 1: 6b83089348 builtin/refs: add '--skip-reflog' flag to bypass reflog migration >> @@ Commit message >> >> The 'git-refs(1)' migrate subcommand, which transfers repositories >> between reference backends, currently migrates reflogs by default as of >> ... >> + ' >> done >> done >> --- >> builtin/refs.c | 3 +++ >> refs.c | 8 +++++--- >> refs.h | 5 ++++- >> t/t1460-refs-migrate.sh | 19 +++++++++++++++++-- >> 4 files changed, 29 insertions(+), 6 deletions(-) > > This is tangent that is totally unrelated to the theme of this > patch, but I find that the placement of range-diff makes it very > hard to follow. > > After skimming the proposed log message, the next thing I would want > to see is the list of paths that are modified, before deciding I > want to review the patch now. Once I decide to read it _now_, the > changes from the previous iteration and range-diff becomes relevant. > > Is it just me who decides in what order to review the patches and > then reviews them in that order? > > Anyway. > >> 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; > > OK. > >> 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) > > These are quite a mouthful, but they are only used between the refs > subsystem and "git refs" command, so being overly verbose and > specific is a good thing. > >> /* >> * 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..28c0024a4c8cb8282bf586840265edba442f5056 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'. > > Yuck. I've never seen preceding three-dots. Makes readers wonder > how this thing behaves differently from the usual <options...>. > > Doubly yuck is ... > I will change it to <options...>. Thanks for pointing out. >> test_migration () { >> repo=$1 && >> format=$2 && >> skip_reflog_verify=${3:-false} && >> + shift $(( $# >= 3 ? 3 : 2 )) && > > ... this thing. The above usage declares that <skip_reflog_verify>, > just like <repo> and <format>, are required so I do not see why we > need to be conditional here. If you want to make it optional, > writing it this way instead ... > Right, so <skip_reflog_verify> and <options...> should be optional. So let me change the description to be: Usage: test_migration <repo> <format> [<skip_reflog_verify> [<options...>]] Which is more accurate. > repo=$1 && > format=$2 && > shift 2 && > > skip_reflog_verify=false && > if test $# -gt 1 > then > skip_reflog_verify=$1 > shift > fi > > ... would make it easier to extend when you ever want to add the > next optional positional parameter. > Thanks this looks good too, let me use it :) >> 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" $@ && > > It is a good change to use the named variable we captured upfront, > but did you mean to leave $@ unquoted? > Good catch. I didn't mean to. > Unless you want to use "$@" to invoke the magic "pass each parameter > separately, even if it has $IFS whitespace in it" semantics, > choosing between $* (when you want to split a parameter into many > when it has $IFS whitespaces) and "$*" (when you want everything in > one string) is preferred. > The intention is to keep the arguments as provided, so "$@" would be the best choice here. Will change. >> git -C "$repo" for-each-ref --include-root-refs \ >> --format='%(refname) %(objectname) %(symref)' >actual && >> @@ -241,6 +243,19 @@ 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. >> + 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. >> + git -C repo reflog --all >reflogs && >> + test_must_be_empty reflogs >> + ' >> done >> done >> >> >> --- >> >> base-commit: bc204b742735ae06f65bb20291c95985c9633b7f >> change-id: 20250207-477-refs-migrate-add-a-flag-to-ignore-reflogs-during-migration-8066f6df50ac >> >> Thanks >> - Karthik
Attachment:
signature.asc
Description: PGP signature
