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

On Wed, Jul 14 2021, Taylor Blau wrote:

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

quoted

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.

quoted

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.

I don't really care, it just looked a bit weird at first, and I wondered
why it couldn't return -1.

Aside from this case do you mean that such a cast would be too expensive
in general, or fears abou going past the 32 bits? I assumed that there
would be checks here for that already (and if not, we'd have wrap-around
now...).