Re: [PATCH 6/8] refs: fix identity for migrated reflogs

From: Karthik Nayak <karthik.188@gmail.com>
Date: Jul 24, 2025, 09:41 UTC
Message-ID: <CAOLa=ZRQj+-WShOn8vQL_fnrPy12n3KB1jrarmJ8Mvz8DnQ-Fg@mail.gmail.com>
In-Reply-To: <20250722-pks-reflog-append-v1-6-183e5949de16@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

Show 15 quoted lines

> When migrating reflog entries between different storage formats we must
> reconstruct the identity of reflog entries. This is done by passing the
> committer passed to the `migrate_one_reflog_entry()` callback function
> to `fmt_ident()`.
>
> This results in an invalid identity though: `fmt_ident()` expects the
> caller to provide both name and mail of the author, but we pass the full
> identity as mail. This leads to an identity like:
>
>     pks <Patrick Steinhardt ps@pks.im>
>
> Fix the bug by splitting the identity line first. This allows us to
> extract both the name and mail so that we can pass them to `fmt_ident()`
> separately.
>

Well explained.

Show 51 quoted lines

> This commit does not yet add any tests as there is another bug in the
> reflog migration that will be fixed in a subsequent commit. Once that
> bug is fixed we'll make the reflog verification in t1450 stricter, and
> that will catch both this bug here and the other bug.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  refs.c | 21 ++++++++++++++++++---
>  1 file changed, 18 insertions(+), 3 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index 188989e4113..64544300dc3 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2945,7 +2945,7 @@ struct migration_data {
>  	struct ref_store *old_refs;
>  	struct ref_transaction *transaction;
>  	struct strbuf *errbuf;
> -	struct strbuf sb;
> +	struct strbuf sb, name, mail;
>  };
>
>  static int migrate_one_ref(const char *refname, const char *referent UNUSED, const struct object_id *oid,
> @@ -2984,7 +2984,7 @@ struct reflog_migration_data {
>  	struct ref_store *old_refs;
>  	struct ref_transaction *transaction;
>  	struct strbuf *errbuf;
> -	struct strbuf *sb;
> +	struct strbuf *sb, *name, *mail;
>  };
>
>  static int migrate_one_reflog_entry(struct object_id *old_oid,
> @@ -2994,13 +2994,22 @@ static int migrate_one_reflog_entry(struct object_id *old_oid,
>  				    const char *msg, void *cb_data)
>  {
>  	struct reflog_migration_data *data = cb_data;
> +	struct ident_split ident;
>  	const char *date;
>  	int ret;
>
> +	if (split_ident_line(&ident, committer, strlen(committer)) < 0)
> +		return -1;
> +
> +	strbuf_reset(data->name);
> +	strbuf_add(data->name, ident.name_begin, ident.name_end - ident.name_begin);
> +	strbuf_reset(data->mail);
> +	strbuf_add(data->mail, ident.mail_begin, ident.mail_end - ident.mail_begin);
> +
>  	date = show_date(timestamp, tz, DATE_MODE(NORMAL));
>  	strbuf_reset(data->sb);
>  	/* committer contains name and email */

Nit: This comment is now stale

> -	strbuf_addstr(data->sb, fmt_ident("", committer, WANT_BLANK_IDENT, date, 0));
> +	strbuf_addstr(data->sb, fmt_ident(data->name->buf, data->mail->buf, WANT_BLANK_IDENT, date, 0));
>

I was a bit stuck on why we use `WANT_BLANK_IDENT`, since we explicitly (since we do a split_ident() and that would error out if there is no name/email) pass the 'name' and the 'email' here as non-null values. So this seems to be the only option for the enum:

enum want_ident {
	WANT_BLANK_IDENT,
	WANT_AUTHOR_IDENT,
	WANT_COMMITTER_IDENT
};

Since we don't want to extract author or committer information. However, in fmt_ident() we only use the 'want_ident' value, when either 'name' or 'email' is not set. I found this a bit confusing, perhaps a simple change of name from 'whose_ident' to 'fallback_ident' would be much more easier to read and understand. Anyways, this is not for your patch.

Show 32 quoted lines

>  	ret = ref_transaction_update_reflog(data->transaction, data->refname,
>  					    new_oid, old_oid, data->sb->buf,
> @@ -3017,6 +3026,8 @@ static int migrate_one_reflog(const char *refname, void *cb_data)
>  		.transaction = migration_data->transaction,
>  		.errbuf = migration_data->errbuf,
>  		.sb = &migration_data->sb,
> +		.name = &migration_data->name,
> +		.mail = &migration_data->mail,
>  	};
>
>  	return refs_for_each_reflog_ent(migration_data->old_refs, refname,
> @@ -3115,6 +3126,8 @@ int repo_migrate_ref_storage_format(struct repository *repo,
>  	struct strbuf new_gitdir = STRBUF_INIT;
>  	struct migration_data data = {
>  		.sb = STRBUF_INIT,
> +		.name = STRBUF_INIT,
> +		.mail = STRBUF_INIT,
>  	};
>  	int did_migrate_refs = 0;
>  	int ret;
> @@ -3290,6 +3303,8 @@ int repo_migrate_ref_storage_format(struct repository *repo,
>  	ref_transaction_free(transaction);
>  	strbuf_release(&new_gitdir);
>  	strbuf_release(&data.sb);
> +	strbuf_release(&data.name);
> +	strbuf_release(&data.mail);
>  	return ret;
>  }
>
>
> --
> 2.50.1.465.gcb3da1c9e6.dirty

The patch look good. Thanks