Patrick Steinhardt <ps@xxxxxx> writes:
> 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.
>
Fair enough, I agree that, finally, we mostly care about not having
reflogs in the end. I'll modify accordingly.
>> 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.
>
Will cleanup.
>> Signed-off-by: Karthik Nayak <karthik.188@xxxxxxxxx>
>> ---
>> ---
>
> Another thing to teach b4: skip the comment in a single-patch patch
> series in case you don't supply a cover letter :)
>
True. I think this is because of lack of conditionals in the templating.
>> 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.
>
I was skeptical too, while not a complete test, the CI seemed to not
complain.
>> @@ -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
>
This looks cleaner.
>> + 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`.
>
Will amend, Thanks for the review.
> Thanks!
>
> Patrick
Attachment:
signature.asc
Description: PGP signature
