Re: [PATCH v2 02/24] pack-bitmap-write.c: gracefully fail to write non-closed bitmaps

On Fri, Jun 25, 2021 at 01:23:40AM +0200, Ævar Arnfjörð Bjarmason wrote:

On Mon, Jun 21 2021, Taylor Blau wrote:

quoted

-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);
 }

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.

Right. You could imagine using the most-significant bit to indicate
"bad" (which in this case is "I couldn't find this object that I'm
supposed to be able to reach"), but of course it cuts our maximum number
of objects in a bitmap in half.

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:

I'm not sure. It does save the extra argument, which is arguably more
convenient for callers, but the cost for doing so is a cast from a
signed integer type to an unsigned one (and a narrower destination type,
at that).

That seems easier to get wrong to me than passing a pointer to a pure
"int" and keeping the return type a uint32_t. So, I'm probably more
content to leave it as-is rather than change it.

I don't feel too strongly about it, though, so if you do I'd be happy to
hear more.

Thanks,
Taylor