Christian Couder <christian.couder@xxxxxxxxx> writes:
> 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/
'a single update' sounds the best here, will add.
>> 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.
>
I think with Patricks comment, this whole code will be removed for
something simpler.
>> 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.
>> 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
>> log->refname = xstrdup(u->refname);
>> memcpy(log->value.update.new_hash,
>> u->new_oid.hash, GIT_MAX_RAWSZ);
Attachment:
signature.asc
Description: PGP signature
