Re: [PATCH v2] builtin/refs: add '--skip-reflog' flag to bypass reflog migration
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Feb 11, 2025, 17:48 UTC
- Message-ID
- <xmqqcyfoz82h.fsf@gitster.g>
- In-Reply-To
- <20250211-477-refs-migrate-add-a-flag-to-ignore-reflogs-during-migration-v2-1-991a2ec9a796@gmail.com>
Karthik Nayak <karthik.188@gmail.com> writes:
Show 39 quoted lines
> 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@pks.im> > Signed-off-by: Karthik Nayak <karthik.188@gmail.com> > --- > 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@gmail.com > --- > 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.
Show 14 quoted lines
> 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.
Show 33 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)
> 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.
Show 16 quoted lines
> /* > * 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 ...
Show 5 quoted lines
> 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 ...
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.
Show 9 quoted lines
> 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?
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.
Show 30 quoted lines
> 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