Re: [PATCH 6/7] refs: allow multiple reflog entries for the same refname

From: Christian Couder <christian.couder@gmail.com>
Date: Dec 11, 2024, 10:44 UTC
Message-ID: <CAP8UFD2zogHD_g5FOVNFvBswPJF749wd4f2s9Pzgef-pv=U0Gg@mail.gmail.com>
In-Reply-To: <20241209-320-git-refs-migrate-reflogs-v1-6-d4bc37ee860f@gmail.com>

On Mon, Dec 9, 2024 at 12:11 PM Karthik Nayak <karthik.188@gmail.com> wrote:

>
> The reference transaction only allows a update for a given reference to

s/a update/an update/

or: s/a update/a single update/

> avoid conflicts. This, however, isn't an issue for reflogs. There are no
> conflicts to be resolved in reflogs and when migrating reflogs between
> backends we'd have multiple reflog entries for the same refname.

Show 5 quoted lines

> @@ -1302,6 +1303,7 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data
>         struct ident_split committer_ident = {0};
>         size_t logs_nr = 0, logs_alloc = 0, i;
>         const char *committer_info;
> +       struct strintmap logs_ts;

Here a comment might help explain what logs_ts is used for.

Show 8 quoted lines

>         int ret = 0;
>
>         committer_info = git_committer_info(0);
> @@ -1310,6 +1312,8 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data
>
>         QSORT(arg->updates, arg->updates_nr, transaction_update_cmp);
>
> +       strintmap_init(&logs_ts, ts);

I am not sure I understand what logs_ts is used for and why its default value is set to ts.

Also ts is an uint64_t while the second argument to strintmap_init() is an int. I wonder if it could be an issue especially on 32 bits platforms.

Show 20 quoted lines

>         reftable_writer_set_limits(writer, ts, ts);
>
>         for (i = 0; i < arg->updates_nr; i++) {
> @@ -1391,6 +1395,7 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data
>
>                         if (create_reflog) {
>                                 struct ident_split c;
> +                               uint64_t update_index;
>
>                                 ALLOC_GROW(logs, logs_nr + 1, logs_alloc);
>                                 log = &logs[logs_nr++];
> @@ -1405,7 +1410,11 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data
>                                 }
>
>                                 fill_reftable_log_record(log, &c);
> -                               log->update_index = ts;
> +
> +                               update_index = strintmap_get(&logs_ts, u->refname);
> +                               log->update_index = update_index;
> +                               strintmap_set(&logs_ts, u->refname, update_index+1);

s/update_index+1/update_index + 1/

Also is the 'update_index' var really needed or could we just do:

                               log->update_index =
strintmap_get(&logs_ts, u->refname);
                               strintmap_set(&logs_ts, u->refname,
log->update_index + 1);

>                                 log->refname = xstrdup(u->refname);
>                                 memcpy(log->value.update.new_hash,
>                                        u->new_oid.hash, GIT_MAX_RAWSZ);