Re: [PATCH v2 01/20] pack-revindex: introduce a new API

On Wed, Jan 13, 2021 at 10:46:57PM -0800, Junio C Hamano wrote:

Taylor Blau [off-list ref] writes:

quoted

+/*
+ * offset_to_pack_pos converts an object offset to a pack position. This
+ * function returns zero on success, and a negative number otherwise. The
+ * parameter 'pos' is usable only on success.
+ *
+ * If the reverse index has not yet been loaded, this function loads it lazily,
+ * and returns an negative number if an error was encountered.

It is somewhat strange to see a function that yields a non-negative
"position" on success and a negative value to signal a failure to
have a separate pointer to the location to receive the true return
value.  Do we truly care the upper half of "uint32_t" (in other
words, do we seriously want to support more than 2G positions in a
pack)?

I don't think that we care about that as much as we do about potential
misuse of a signed return value. There are indeed a couple of spots
where a potential negative return value is ignored, and then used to
lookup an object in a pack, or some such.

And that's part of the goal of this API: we have strict guidelines about
when the output parameter is and isn't usable. That makes it more
difficult to accidentally use an uninitialized value / negative number.

What I'm trying to get at is that [...] is more natural than [...] but
now I wrote it down and laid it out in front of my eyes, the latter
does not look too bad.

OK, good :-).

	... later comes back after reading through the series ...

	The new callers all looked quite nice to eyes.  Because we
	discourage assignment inside if() condition, the converted
	result does not make the code more verbose than the
	original.  In fact, it makes it even clearer that we are
	checking for an error return from a function call.

	Quite nice.

Thank you :-D.

quoted

+ * This function runs in time O(log N) with the number of objects in the pack.

Is it a good idea to commit to such performance characteristics as a
promise to callers like this (the comment applies to all three
functions)?

It depends on how a developer is helped by this comment when
deciding whether to use this function, or find other ways, to
implement what s/he wants to do.

I don't mind it. If they all had the same performance characteristics, I
wouldn't be for it, but since they don't, I think that it's good to
know. Peff suggested this back in [1].

Thanks,
Taylor

[1]: https://lore.kernel.org/git/X%2F1guCOGWybOzIS7@coredump.intra.peff.net/ (local)