On Mon, Jun 21 2021, Taylor Blau wrote:
> -static uint32_t find_object_pos(const struct object_id *oid)
> +static uint32_t find_object_pos(const struct object_id *oid, int *found)
> {
> struct object_entry *entry = packlist_find(writer.to_pack, oid);
>
> if (!entry) {
> - die("Failed to write bitmap index. Packfile doesn't have full closure "
> + if (found)
> + *found = 0;
> + warning("Failed to write bitmap index. Packfile doesn't have full closure "
> "(object %s is missing)", oid_to_hex(oid));
> + return 0;
> }
>
> + if (found)
> + *found = 1;
> return oe_in_pack_pos(writer.to_pack, entry);
> }
>
> @@ -331,9 +336,10 @@ static void bitmap_builder_clear(struct bitmap_builder *bb)
> bb->commits_nr = bb->commits_alloc = 0;
> }
>
> -static void fill_bitmap_tree(struct bitmap *bitmap,
> - struct tree *tree)
> +static int fill_bitmap_tree(struct bitmap *bitmap,
> + struct tree *tree)
> {
> + int found;
> uint32_t pos;
> struct tree_desc desc;
> struct name_entry entry;
> @@ -342,9 +348,11 @@ static void fill_bitmap_tree(struct bitmap *bitmap,
> * If our bit is already set, then there is nothing to do. Both this
> * tree and all of its children will be set.
> */
> - pos = find_object_pos(&tree->object.oid);
> + pos = find_object_pos(&tree->object.oid, &found);
> + if (!found)
> + return -1;
So, a function that returns an unsigned 32 bit int won't (presumably)
have enough space for an "is bad", but before it died so it didn't
matter.
Now it warns, so it needs a "is bad", so we add another "int" to pass
that information around.
So if we're already paying for that extra space (which, on some
platforms would already be a 64 bit int, and on some so would the
uint32_t, it's just "at least 32 bits").
Wouldn't it be more idiomatic to just have find_object_pos() return
int64_t now, if it's -1 it's an error, otherwise the "pos" is cast to
uint32_t:
diff --git a/pack-bitmap-write.c b/pack-bitmap-write.c
index 88d9e696a54..d71fa6f607a 100644
--- a/pack-bitmap-write.c
+++ b/pack-bitmap-write.c
@@ -125,14 +125,12 @@ static inline void push_bitmapped_commit(struct commit *commit)
writer.selected_nr++;
}
-static uint32_t find_object_pos(const struct object_id *oid)
+static int64_t find_object_pos(const struct object_id *oid)
{
struct object_entry *entry = packlist_find(writer.to_pack, oid);
- if (!entry) {
- die("Failed to write bitmap index. Packfile doesn't have full closure "
- "(object %s is missing)", oid_to_hex(oid));
- }
+ if (!entry)
+ return -1;
return oe_in_pack_pos(writer.to_pack, entry);
}
@@ -334,7 +332,7 @@ static void bitmap_builder_clear(struct bitmap_builder *bb)
static void fill_bitmap_tree(struct bitmap *bitmap,
struct tree *tree)
{
- uint32_t pos;
+ int64_t pos;
struct tree_desc desc;
struct name_entry entry;
@@ -343,6 +341,9 @@ static void fill_bitmap_tree(struct bitmap *bitmap,
* tree and all of its children will be set.
*/
pos = find_object_pos(&tree->object.oid);
+ if (pos < 0)
+ die("unhappy: %s", oid_to_hex(&tree->object.oid));
+
if (bitmap_get(bitmap, pos))
return;
bitmap_set(bitmap, pos);
I mean, you don't want the die() part of that, but to me the rest looks
better.
> [...]
> diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh
> index 584a039b85..1667450917 100755
> --- a/t/t0410-partial-clone.sh
> +++ b/t/t0410-partial-clone.sh
> @@ -536,7 +536,13 @@ test_expect_success 'gc does not repack promisor objects if there are none' '
> repack_and_check () {
> rm -rf repo2 &&
> cp -r repo repo2 &&
> - git -C repo2 repack $1 -d &&
> + if test x"$1" = "x--must-fail"
> + then
> + shift
> + test_must_fail git -C repo2 repack $1 -d
> + else
> + git -C repo2 repack $1 -d
> + fi &&
> git -C repo2 fsck &&
This sent me down the rabbit hole of
https://lore.kernel.org/git/60c67bdf5b77e_f569220858@natae.notmuch/
again.
