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

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