Re: [PATCH 6/7] refs: allow multiple reflog entries for the same refname
- From
karthik nayak <karthik.188@gmail.com>
- Date
- Dec 12, 2024, 14:52 UTC
- Message-ID
- <CAOLa=ZTFJ+U3RmDQs8hwTCnfNfTarpxE1otcYAAV-FSOogAq=Q@mail.gmail.com>
- In-Reply-To
- <CAP8UFD2zogHD_g5FOVNFvBswPJF749wd4f2s9Pzgef-pv=U0Gg@mail.gmail.com>
Christian Couder <christian.couder@gmail.com> writes:
Show 7 quoted lines
> 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/
'a single update' sounds the best here, will add.
Show 13 quoted lines
>> 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.
Show 12 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. >
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.
Show 32 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);
>
> ?
>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);