Patrick Steinhardt <ps@xxxxxx> writes:
> 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`.
>
That makes sense, let me do that.
>> @@ -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
Yes, this was needed cause I initially didn't understand how the
udpate_index worked and assumed two logs couldn't have the same
update_index. I missed changing it, like you said, it works, but I'll
fix it.
> Also, is there any specific reason why we start at 1 and not 0? Just curious.
Not really, I wanted to distinguish between index entries vs non-indexed
entries. But logically, no, I'll remove it, to remove any confusion.
> Patrick
Thanks
Attachment:
signature.asc
Description: PGP signature
