Re: [PATCH v2 8/8] refs: add support for migrating reflogs

> On Fri, Dec 13, 2024 at 11:36:53AM +0100, Karthik Nayak wrote:
>> diff --git a/refs.c b/refs.c
>> index 9f539369bc94a25594adc3e95847f2fe72f58a08..f19292d50f0003881220e8f7cfcf6c7eb4b2e749 100644
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -2708,6 +2710,53 @@ static int migrate_one_ref(const char *refname, const char *referent UNUSED, con
>>  	return ret;
>>  }
>>
>> +struct reflog_migration_data {
>> +	unsigned int *index;
>> +	const char *refname;
>> +	struct ref_store *old_refs;
>> +	struct ref_transaction *transaction;
>> +	struct strbuf *errbuf;
>> +};
>> +
>> +static int migrate_one_reflog_entry(struct object_id *old_oid,
>> +				    struct object_id *new_oid,
>> +				    const char *committer,
>> +				    timestamp_t timestamp, int tz,
>> +				    const char *msg, void *cb_data)
>> +{
>> +	struct reflog_migration_data *data = cb_data;
>> +	struct strbuf sb = STRBUF_INIT;
>> +	const char *date;
>> +	int ret;
>> +
>> +	date = show_date(timestamp, tz, DATE_MODE(NORMAL));
>> +	/* committer contains name and email */
>> +	strbuf_addstr(&sb, fmt_ident("", committer, WANT_BLANK_IDENT, date, 0));
>> +
>> +	ret = ref_transaction_update_reflog(data->transaction, data->refname,
>> +					    new_oid, old_oid, sb.buf,
>> +					    REF_HAVE_NEW | REF_HAVE_OLD, msg,
>> +					    (*data->index)++, data->errbuf);
>
> This is where we now increment the reflog index to ensure a proper
> ordering.
>
>> +	strbuf_release(&sb);
>> +
>> +	return ret;
>> +}
>
> We're now allocating one buffer per reflog entry. We may want to
> optimize this by having a scratch buffer in `migration_data`, which we
> could then pass on via `reflog_migration_data`.
>

>> @@ -2910,6 +2940,11 @@ int repo_migrate_ref_storage_format(struct repository *repo,
>>  	if (ret < 0)
>>  		goto done;
>>
>> +	data.reflog_index = 1;
>
> I'm a bit surprised that we initialize the relfog entry here, because
> that means we now have a globally increasing counter across all reflogs.
> Couldn't we initialize the index per reflog instead? It ultimately does
> not really matter, but feels like the more obvious design to me