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

[Taylor Blau <me@ttaylorr.com>]

On Wed, Jan 13, 2021 at 10:46:57PM -0800, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
>
> > +/*
> > + * 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.

> > + * 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/