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

From: Patrick Steinhardt <ps@pks.im>
Date: Dec 16, 2024, 07:25 UTC
Message-ID: <Z1_KzlKc7RBfas4L@pks.im>
In-Reply-To: <20241215-320-git-refs-migrate-reflogs-v3-8-4127fe707b98@gmail.com>

On Sun, Dec 15, 2024 at 05:25:45PM +0100, Karthik Nayak wrote:

Show 45 quoted lines

> @@ -2705,6 +2707,52 @@ 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;
> +	struct strbuf *sb;
> +};
> +
> +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;
> +	const char *date;
> +	int ret;
> +
> +	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));
> +
> +	ret = ref_transaction_update_reflog(data->transaction, data->refname,
> +					    new_oid, old_oid, data->sb->buf,
> +					    REF_HAVE_NEW | REF_HAVE_OLD, msg,
> +					    data->index++, data->errbuf);
> +	return ret;
> +}
> +
> +static int migrate_one_reflog(const char *refname, void *cb_data)
> +{
> +	struct migration_data *migration_data = cb_data;
> +	struct reflog_migration_data data;
> +
> +	data.refname = refname;
> +	data.old_refs = migration_data->old_refs;
> +	data.transaction = migration_data->transaction;
> +	data.errbuf = migration_data->errbuf;
> +	data.sb = &migration_data->sb;

The `index` variable isn't getting initialized here anymore, so its value is essenitally random. I'd propose to use designated initializers for `data` to fix this:

    struct reflog_migration_data data = {
        .refname = refname,
        .old_refs = migration_data->old_refs,
        .transaction = migration_data->transaction,
        .errbuf = migration_data->errbuf,
        .sb = &migration_data->sb,
    };

Maybe that fixes the issue that Junio has seen?

Patrick