pack-bitmap-write.c: gracefully fail to write non-closed bitmaps
The set of objects covered by a bitmap must be closed under reachability, since it must be the case that there is a valid bit position assigned for every possible reachable object (otherwise the bitmaps would be incomplete). Pack bitmaps are never written from 'git repack' unless repacking all-into-one, and so we never write non-closed bitmaps (except in the case of partial clones where we aren't guaranteed to have all objects). But multi-pack bitmaps change this, since it isn't known whether the set of objects in the MIDX is closed under reachability until walking them. Plumb through a bit that is set when a reachable object isn't found. As soon as a reachable object isn't found in the set of objects to include in the bitmap, bitmap_writer_build() knows that the set is not closed, and so it now fails gracefully. A test is added in t0410 to trigger a bitmap write without full reachability closure by removing local copies of some reachable objects from a promisor remote. Signed-off-by: Taylor Blau <me@ttaylorr.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Taylor Blau committed
Aug 24, 2021 at 12:15 UTC
3ba3d0621b6822e974f06123db9394b33e454ed7
4 files changed
+64
-26
builtin/pack-objects.c
+2
-1
@@ -1256,7 +1256,8 @@ static void write_pack_file(void)
1256
1257
bitmap_writer_show_progress(progress);
1258
bitmap_writer_select_commits(indexed_commits, indexed_commits_nr, -1);
1259
- bitmap_writer_build(&to_pack);
1259
+ if (bitmap_writer_build(&to_pack) < 0)
1260
+ die(_("failed to write bitmap index"));
1261
bitmap_writer_finish(written_list, nr_written,
1262
tmpname.buf, write_bitmap_options);
1263
write_bitmap_index = 0;
pack-bitmap-write.c
+53
-23
@@ -125,15 +125,20 @@ static inline void push_bitmapped_commit(struct commit *commit)
125
writer.selected_nr++;
126
}
127
128
-static uint32_t find_object_pos(const struct object_id *oid)
128
+static uint32_t find_object_pos(const struct object_id *oid, int *found)
129
{
130
struct object_entry *entry = packlist_find(writer.to_pack, oid);
131
132
if (!entry) {
133
- die("Failed to write bitmap index. Packfile doesn't have full closure "
133
+ if (found)
134
+ *found = 0;
135
+ warning("Failed to write bitmap index. Packfile doesn't have full closure "
136
"(object %s is missing)", oid_to_hex(oid));
137
+ return 0;
138
}
139
140
+ if (found)
141
+ *found = 1;
142
return oe_in_pack_pos(writer.to_pack, entry);
143
}
144
@@ -331,9 +336,10 @@ static void bitmap_builder_clear(struct bitmap_builder *bb)
336
bb->commits_nr = bb->commits_alloc = 0;
337
}
338
334
-static void fill_bitmap_tree(struct bitmap *bitmap,
335
- struct tree *tree)
339
+static int fill_bitmap_tree(struct bitmap *bitmap,
340
+ struct tree *tree)
341
{
342
+ int found;
343
uint32_t pos;
344
struct tree_desc desc;
345
struct name_entry entry;
@@ -342,9 +348,11 @@ static void fill_bitmap_tree(struct bitmap *bitmap,
348
* If our bit is already set, then there is nothing to do. Both this
349
* tree and all of its children will be set.
350
*/
345
- pos = find_object_pos(&tree->object.oid);
351
+ pos = find_object_pos(&tree->object.oid, &found);
352
+ if (!found)
353
+ return -1;
354
if (bitmap_get(bitmap, pos))
347
- return;
355
+ return 0;
356
bitmap_set(bitmap, pos);
357
358
if (parse_tree(tree) < 0)
@@ -355,11 +363,15 @@ static void fill_bitmap_tree(struct bitmap *bitmap,
363
while (tree_entry(&desc, &entry)) {
364
switch (object_type(entry.mode)) {
365
case OBJ_TREE:
358
- fill_bitmap_tree(bitmap,
359
- lookup_tree(the_repository, &entry.oid));
366
+ if (fill_bitmap_tree(bitmap,
367
+ lookup_tree(the_repository, &entry.oid)) < 0)
368
+ return -1;
369
break;
370
case OBJ_BLOB:
362
- bitmap_set(bitmap, find_object_pos(&entry.oid));
371
+ pos = find_object_pos(&entry.oid, &found);
372
+ if (!found)
373
+ return -1;
374
+ bitmap_set(bitmap, pos);
375
break;
376
default:
377
/* Gitlink, etc; not reachable */
@@ -368,15 +380,18 @@ static void fill_bitmap_tree(struct bitmap *bitmap,
380
}
381
382
free_tree_buffer(tree);
383
+ return 0;
384
}
385
373
-static void fill_bitmap_commit(struct bb_commit *ent,
374
- struct commit *commit,
375
- struct prio_queue *queue,
376
- struct prio_queue *tree_queue,
377
- struct bitmap_index *old_bitmap,
378
- const uint32_t *mapping)
386
+static int fill_bitmap_commit(struct bb_commit *ent,
387
+ struct commit *commit,
388
+ struct prio_queue *queue,
389
+ struct prio_queue *tree_queue,
390
+ struct bitmap_index *old_bitmap,
391
+ const uint32_t *mapping)
392
{
393
+ int found;
394
+ uint32_t pos;
395
if (!ent->bitmap)
396
ent->bitmap = bitmap_new();
397
@@ -401,11 +416,16 @@ static void fill_bitmap_commit(struct bb_commit *ent,
416
* Mark ourselves and queue our tree. The commit
417
* walk ensures we cover all parents.
418
*/
404
- bitmap_set(ent->bitmap, find_object_pos(&c->object.oid));
419
+ pos = find_object_pos(&c->object.oid, &found);
420
+ if (!found)
421
+ return -1;
422
+ bitmap_set(ent->bitmap, pos);
423
prio_queue_put(tree_queue, get_commit_tree(c));
424
425
for (p = c->parents; p; p = p->next) {
408
- int pos = find_object_pos(&p->item->object.oid);
426
+ pos = find_object_pos(&p->item->object.oid, &found);
427
+ if (!found)
428
+ return -1;
429
if (!bitmap_get(ent->bitmap, pos)) {
430
bitmap_set(ent->bitmap, pos);
431
prio_queue_put(queue, p->item);
@@ -413,8 +433,12 @@ static void fill_bitmap_commit(struct bb_commit *ent,
433
}
434
}
435
416
- while (tree_queue->nr)
417
- fill_bitmap_tree(ent->bitmap, prio_queue_get(tree_queue));
436
+ while (tree_queue->nr) {
437
+ if (fill_bitmap_tree(ent->bitmap,
438
+ prio_queue_get(tree_queue)) < 0)
439
+ return -1;
440
+ }
441
+ return 0;
442
}
443
444
static void store_selected(struct bb_commit *ent, struct commit *commit)
@@ -432,7 +456,7 @@ static void store_selected(struct bb_commit *ent, struct commit *commit)
456
kh_value(writer.bitmaps, hash_pos) = stored;
457
}
458
435
-void bitmap_writer_build(struct packing_data *to_pack)
459
+int bitmap_writer_build(struct packing_data *to_pack)
460
{
461
struct bitmap_builder bb;
462
size_t i;
@@ -441,6 +465,7 @@ void bitmap_writer_build(struct packing_data *to_pack)
465
struct prio_queue tree_queue = { NULL };
466
struct bitmap_index *old_bitmap;
467
uint32_t *mapping;
468
+ int closed = 1; /* until proven otherwise */
469
470
writer.bitmaps = kh_init_oid_map();
471
writer.to_pack = to_pack;
@@ -463,8 +488,11 @@ void bitmap_writer_build(struct packing_data *to_pack)
488
struct commit *child;
489
int reused = 0;
490
466
- fill_bitmap_commit(ent, commit, &queue, &tree_queue,
467
- old_bitmap, mapping);
491
+ if (fill_bitmap_commit(ent, commit, &queue, &tree_queue,
492
+ old_bitmap, mapping) < 0) {
493
+ closed = 0;
494
+ break;
495
+ }
496
497
if (ent->selected) {
498
store_selected(ent, commit);
@@ -499,7 +527,9 @@ void bitmap_writer_build(struct packing_data *to_pack)
527
528
stop_progress(&writer.progress);
529
502
- compute_xor_offsets();
530
+ if (closed)
531
+ compute_xor_offsets();
532
+ return closed ? 0 : -1;
533
}
534
535
/**
pack-bitmap.h
+1
-1
@@ -87,7 +87,7 @@ struct ewah_bitmap *bitmap_for_commit(struct bitmap_index *bitmap_git,
87
struct commit *commit);
88
void bitmap_writer_select_commits(struct commit **indexed_commits,
89
unsigned int indexed_commits_nr, int max_bitmaps);
90
-void bitmap_writer_build(struct packing_data *to_pack);
90
+int bitmap_writer_build(struct packing_data *to_pack);
91
void bitmap_writer_finish(struct pack_idx_entry **index,
92
uint32_t index_nr,
93
const char *filename,
t/t0410-partial-clone.sh
+8
-1
@@ -536,7 +536,13 @@ test_expect_success 'gc does not repack promisor objects if there are none' '
536
repack_and_check () {
537
rm -rf repo2 &&
538
cp -r repo repo2 &&
539
- git -C repo2 repack $1 -d &&
539
+ if test x"$1" = "x--must-fail"
540
+ then
541
+ shift
542
+ test_must_fail git -C repo2 repack $1 -d
543
+ else
544
+ git -C repo2 repack $1 -d
545
+ fi &&
546
git -C repo2 fsck &&
547
548
git -C repo2 cat-file -e $2 &&
@@ -561,6 +567,7 @@ test_expect_success 'repack -d does not irreversibly delete promisor objects' '
567
printf "$THREE\n" | pack_as_from_promisor &&
568
delete_object repo "$ONE" &&
569
570
+ repack_and_check --must-fail -ab "$TWO" "$THREE" &&
571
repack_and_check -a "$TWO" "$THREE" &&
572
repack_and_check -A "$TWO" "$THREE" &&
573
repack_and_check -l "$TWO" "$THREE"