Junio C Hamano <gitster@xxxxxxxxx> writes: > Karthik Nayak <karthik.188@xxxxxxxxx> writes: > >> Subject: [PATCH] reftable: write correct max_update_index to header >> >> In 297c09eabb (refs: allow multiple reflog entries for the same refname, >> 2024-12-16), the reftable backend learned to handle multiple reflog >> entries within the same transaction. This was done modifying the > > "done" -> "done by", I think. > Yup, thanks! >> To fix the issue, the appropriate `max_update_index` limit must be set >> even before the first block is written. Add a `max_index` field to the >> transaction which holds the `max_index` within all its updates, then >> propagate this value to the reftable backend, wherein this is used to >> the set the `max_update_index` correctly. > > >> diff --git a/refs.c b/refs.c >> index 0f41b2fd4a..f7b6f0f897 100644 >> --- a/refs.c >> +++ b/refs.c >> @@ -1345,6 +1345,13 @@ int ref_transaction_update_reflog(struct > > Not the focus of this topic/fix, but I notice that the only caller > of this ref_transaction_update_reflog() function is a static > function migrate_one_reflog_entry() in the same file. Do we expect > that we would add more callers? Otherwise we should make it a file > scope static and remove it from <refs.h> file. > I don't think we expect any other callers now. Maybe someday we'd expose creating reflogs to the users. But for now, making it static is more worthwhile. Let's follow the boy scout rule and clean this up, I'll add a commit to do the same in v2 of my patch. >> ref_transaction *transaction, >> update->flags &= ~REF_HAVE_OLD; >> update->index = index; >> >> + /* >> + * Reference backends may need to know the max index to optimize >> + * their writes. So we store the max_index on the transaction level. >> + */ >> + if (index > transaction->max_index) >> + transaction->max_index = index; >> + >> return 0; >> } > > So from the problem description, whenever we consume an index by > assigning it to an update that belongs to a transaction, we must > make sure that transaction's max_index covers that value of the > index? I was wondering if we should have a less error prone way to > do that by having a helper function that takes ref_update and > ref_transaction objects to do the above, but this is exclusively > used by reflog migration code path and nowhere else, so it may > probably be fine as-is. I dunno. Yup, that's correct. It should be okay, but let me do it anyways, makes it safer for the future.
Attachment:
signature.asc
Description: PGP signature
