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

From: Patrick Steinhardt <ps@pks.im>
Date: Dec 13, 2024, 12:24 UTC
Message-ID: <Z1wngv1Kox_QQcwK@pks.im>
In-Reply-To: <20241213-320-git-refs-migrate-reflogs-v2-8-f28312cdb6c0@gmail.com>

On Fri, Dec 13, 2024 at 11:36:53AM +0100, Karthik Nayak wrote:

Show 35 quoted lines

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

Show 5 quoted lines

> @@ -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

Also, is there any specific reason why we start at 1 and not 0? Just curious.

Patrick