diff mbox series

[v2] builtin/refs: add '--skip-reflog' flag to bypass reflog migration

Message ID 20250211-477-refs-migrate-add-a-flag-to-ignore-reflogs-during-migration-v2-1-991a2ec9a796@gmail.com (mailing list archive)
State Superseded
Headers show
Series [v2] builtin/refs: add '--skip-reflog' flag to bypass reflog migration | expand

Commit Message

Karthik Nayak Feb. 11, 2025, 11:42 a.m. UTC 
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
    -    In 246cebe320 (refs: add support for migrating reflogs, 2024-12-16).
    +    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.
    +    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.
     
    -    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.
    +    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>
     
      ## builtin/refs.c ##
    @@ t/t1460-refs-migrate.sh: do
     +			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) &&
    ++			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)
    ++			git -C repo reflog --all >reflogs &&
    ++			test_must_be_empty reflogs
     +		'
      	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(-)


---

base-commit: bc204b742735ae06f65bb20291c95985c9633b7f
change-id: 20250207-477-refs-migrate-add-a-flag-to-ignore-reflogs-during-migration-8066f6df50ac

Thanks
- Karthik

Comments

Patrick Steinhardt Feb. 11, 2025, 12:21 p.m. UTC | #1 
On Tue, Feb 11, 2025 at 12:42:18PM +0100, Karthik Nayak wrote:
> 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.

Thanks, this version looks good to me.

Patrick
Junio C Hamano Feb. 11, 2025, 5:48 p.m. UTC | #2 
Karthik Nayak <karthik.188@gmail.com> 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@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.

> 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 ...


>  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.

>  	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.

>  	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
Karthik Nayak Feb. 12, 2025, 5:43 p.m. UTC | #3 
Junio C Hamano <gitster@pobox.com> writes:

> Karthik Nayak <karthik.188@gmail.com> 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@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.
>
>> 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
Karthik Nayak Feb. 13, 2025, 9:25 a.m. UTC | #4 
Junio C Hamano <gitster@pobox.com> writes:

> Karthik Nayak <karthik.188@gmail.com> 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@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.
>

I missed responding to this in my previous email, but I've been using b4
for sending patches and this was primarily due to how it sends single
patch series.

I've fixed the template to show the diff-stat first, and this should be
the case for upcoming patches.

Thanks for pointing it out!

[snip]
diff mbox series

Patch

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..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'.
 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,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