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

On 1/14/2021 1:46 AM, 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)?

What I'm trying to get at is that

		int pos = offset_to_pack_pos(...);
		if (pos < 0)
			error();
		else
			use(pos);

is more natural than

I agree that this is used commonly, but usually in the case that
we are finding a position in the list _or where such an item would
be inserted_. For example:

	pos = index_name_pos(istate, dirname, len);
	if (pos < 0)
		pos = -pos-1;
	while (pos < istate->cache_nr) {
		...

But that does not apply in this case. Knowing that the requested
offset lies between object 'i' and object 'i + 1' isn't helpful,
since the offset still does not correspond to the start of an
object.

		uint32_t pos;
                if (offset_to_pack_pos(..., &pos) < 0)
			error();
		else
			use(pos);

but now I wrote it down and laid it out in front of my eyes, the
latter does not look too bad.

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

As someone who spends a decent amount of time working in C#, I
also like this pattern. The APIs in C# work this way, too, such
as:

	if (!set.TryGetValue(key, out value))
		return false;

	// Use 'value', which is initialized now.

Thanks,
-Stolee