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

From: Derrick Stolee <stolee@gmail.com>
Date: Jan 14, 2021, 12:00 UTC
Message-ID: <685081d3-0cca-4c61-52b2-e9a8006803b1@gmail.com>
In-Reply-To: <xmqq1reoypzy.fsf@gitster.c.googlers.com>

On 1/14/2021 1:46 AM, Junio C Hamano wrote:

Show 26 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)?
> 
> 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.

Show 18 quoted lines

> 		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