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

[Christian Couder <christian.couder@xxxxxxxxx>]

On Mon, Dec 9, 2024 at 12:11 PM Karthik Nayak <karthik.188@xxxxxxxxx> 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.


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

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

>         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);