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

From: Taylor Blau <me@ttaylorr.com>
Date: Jan 14, 2021, 17:06 UTC
Message-ID: <YAB6DNk4wPBVbGtU@nand.local>
In-Reply-To: <xmqq1reoypzy.fsf@gitster.c.googlers.com>

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

Show 16 quoted lines

> 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 :-).

Show 9 quoted lines

> 	... 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.

Show 9 quoted lines

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