Skip to content

Commit 8ba42f1

Browse files
KarthikNayakdscho
authored andcommitted
reftable: write correct max_update_index to header
In 297c09e (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 `update_index` for reflogs with multiple indices. During writing the logs, the `max_update_index` of the writer was modified to ensure the limits were raised to the modified `update_index`s. However, since ref entries are written before the modification to the `max_update_index`, if there are multiple blocks to be written, the reftable backend writes the header with the old `max_update_index`. When all logs are finally written, the footer will be written with the new `min_update_index`. This causes a mismatch between the header and the footer and causes the reftable file to be corrupted. The existing tests only spawn a single block and since headers are lazily written with the first block, the tests didn't capture this bug. 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. Add a test which creates a few thousand reference updates with multiple reflog entries, which should trigger the bug. Reported-by: brian m. carlson <[email protected]> Signed-off-by: Karthik Nayak <[email protected]> Signed-off-by: Junio C Hamano <[email protected]>
1 parent 5cc2d6c commit 8ba42f1

File tree

4 files changed

+33
-10
lines changed

4 files changed

+33
-10
lines changed

refs.c

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1345,6 +1345,13 @@ int ref_transaction_update_reflog(struct ref_transaction *transaction,
13451345
update->flags &= ~REF_HAVE_OLD;
13461346
update->index = index;
13471347

1348+
/*
1349+
* Reference backends may need to know the max index to optimize
1350+
* their writes. So we store the max_index on the transaction level.
1351+
*/
1352+
if (index > transaction->max_index)
1353+
transaction->max_index = index;
1354+
13481355
return 0;
13491356
}
13501357

refs/refs-internal.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -203,6 +203,7 @@ struct ref_transaction {
203203
enum ref_transaction_state state;
204204
void *backend_data;
205205
unsigned int flags;
206+
unsigned int max_index;
206207
};
207208

208209
/*

refs/reftable-backend.c

Lines changed: 13 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -942,6 +942,7 @@ struct write_transaction_table_arg {
942942
size_t updates_nr;
943943
size_t updates_alloc;
944944
size_t updates_expected;
945+
unsigned int max_index;
945946
};
946947

947948
struct reftable_transaction_data {
@@ -1019,6 +1020,7 @@ static int prepare_transaction_update(struct write_transaction_table_arg **out,
10191020
arg->updates_nr = 0;
10201021
arg->updates_alloc = 0;
10211022
arg->updates_expected = 0;
1023+
arg->max_index = 0;
10221024
}
10231025

10241026
arg->updates_expected++;
@@ -1428,7 +1430,6 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data
14281430
struct reftable_log_record *logs = NULL;
14291431
struct ident_split committer_ident = {0};
14301432
size_t logs_nr = 0, logs_alloc = 0, i;
1431-
uint64_t max_update_index = ts;
14321433
const char *committer_info;
14331434
int ret = 0;
14341435

@@ -1438,7 +1439,12 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data
14381439

14391440
QSORT(arg->updates, arg->updates_nr, transaction_update_cmp);
14401441

1441-
reftable_writer_set_limits(writer, ts, ts);
1442+
/*
1443+
* During reflog migration, we add indexes for a single reflog with
1444+
* multiple entries. Each entry will contain a different update_index,
1445+
* so set the limits accordingly.
1446+
*/
1447+
reftable_writer_set_limits(writer, ts, ts + arg->max_index);
14421448

14431449
for (i = 0; i < arg->updates_nr; i++) {
14441450
struct reftable_transaction_update *tx_update = &arg->updates[i];
@@ -1540,12 +1546,6 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data
15401546
*/
15411547
log->update_index = ts + u->index;
15421548

1543-
/*
1544-
* Note the max update_index so the limit can be set later on.
1545-
*/
1546-
if (log->update_index > max_update_index)
1547-
max_update_index = log->update_index;
1548-
15491549
log->refname = xstrdup(u->refname);
15501550
memcpy(log->value.update.new_hash,
15511551
u->new_oid.hash, GIT_MAX_RAWSZ);
@@ -1609,8 +1609,6 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data
16091609
* and log blocks.
16101610
*/
16111611
if (logs) {
1612-
reftable_writer_set_limits(writer, ts, max_update_index);
1613-
16141612
ret = reftable_writer_add_logs(writer, logs, logs_nr);
16151613
if (ret < 0)
16161614
goto done;
@@ -1631,7 +1629,12 @@ static int reftable_be_transaction_finish(struct ref_store *ref_store UNUSED,
16311629
struct reftable_transaction_data *tx_data = transaction->backend_data;
16321630
int ret = 0;
16331631

1632+
if (tx_data->args)
1633+
tx_data->args->max_index = transaction->max_index;
1634+
16341635
for (size_t i = 0; i < tx_data->args_nr; i++) {
1636+
tx_data->args[i].max_index = transaction->max_index;
1637+
16351638
ret = reftable_addition_add(tx_data->args[i].addition,
16361639
write_transaction_table, &tx_data->args[i]);
16371640
if (ret < 0)

t/t1460-refs-migrate.sh

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -227,6 +227,18 @@ do
227227
done
228228
done
229229

230+
test_expect_success 'multiple reftable blocks with multiple entries' '
231+
test_when_finished "rm -rf repo" &&
232+
git init --ref-format=files repo &&
233+
test_commit -C repo first &&
234+
printf "create refs/heads/ref-%d HEAD\n" $(test_seq 5000) >stdin &&
235+
git -C repo update-ref --stdin <stdin &&
236+
test_commit -C repo second &&
237+
printf "update refs/heads/ref-%d HEAD\n" $(test_seq 3000) >stdin &&
238+
git -C repo update-ref --stdin <stdin &&
239+
test_migration repo reftable
240+
'
241+
230242
test_expect_success 'migrating from files format deletes backend files' '
231243
test_when_finished "rm -rf repo" &&
232244
git init --ref-format=files repo &&

0 commit comments

Comments
 (0)