On 25/07/22 01:20PM, Patrick Steinhardt wrote:
> 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@xxxxxx>
>
> 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.
Ok so IIUC, the bug is the result of passing the full committer info to
the mail field in `fmt_ident()` and leaving the name field unset. To
properly address we need to first deconstruct the committer info into
separate name and mail components and pass them separately to
`fmt_ident()`.
> 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@xxxxxx>
> ---
> 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;
Ok now we first deconstruct the committer info.
> +
> + 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);
The name and mail components get stored separately.
> +
> date = show_date(timestamp, tz, DATE_MODE(NORMAL));
> strbuf_reset(data->sb);
> /* committer contains name and email */
> - 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));
`fmt_ident()` now receives the expected information. Looks good
>
> 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,
I was a bit confused at first why we cared to assign the name and mail
fields here as it didn't look like we actually use them, but it looks
like we do this to release the the underlying strbuf as we don't free it
from `reflog_migration_data`.
> };
>
> 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
>
>
