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

[Patrick Steinhardt <ps@pks.im>]

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