Christian Couder [off-list ref] writes:

On Mon, Dec 9, 2024 at 12:11 PM Karthik Nayak [off-list ref] wrote:

quoted

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

s/a update/an update/

or: s/a update/a single update/

'a single update' sounds the best here, will add.

quoted

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.

quoted

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

I think with Patricks comment, this whole code will be removed for
something simpler.

quoted

        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.

The reason I added this was because in the reftable backend, the writer
sorts logs before writing. So if the multiple reflogs contained the same
update_index, their order might be changed. But for migrating reflogs,
we need to ensure we maintain the order. Using a map here, allowed us to
increment the update_index for reflogs for a given refname.

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.

This is fair point, I decided to scrap this ultimately and simply append
`u->index` to the update_index. Which would provide the same desired
effect.

quoted

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

?

The temp variable can be removed here indeed. But I'll remove all of
this in the next version. Thanks

quoted

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