Re: [PATCH 1/1] sha1-file: split OBJECT_INFO_FOR_PREFETCH

[Derrick Stolee <stolee@gmail.com>]

On 5/28/2019 4:54 PM, Jeff King wrote:
> On Tue, May 28, 2019 at 08:19:07AM -0700, Derrick Stolee via GitGitGadget wrote:
> 
>> From: Derrick Stolee <dstolee@microsoft.com>
>>
>> The OBJECT_INFO_FOR_PREFETCH bitflag was added to sha1-file.c in 0f4a4fb1
>> (sha1-file: support OBJECT_INFO_FOR_PREFETCH, 2019-03-29) and is used to
>> prevent the fetch_objects() method when enabled.
>>
>> However, there is a problem with the current use. The definition of
>> OBJECT_INFO_FOR_PREFETCH is given by adding 32 to OBJECT_INFO_QUICK. This is
>> clearly stated above the definition (in a comment) that this is so
>> OBJECT_INFO_FOR_PREFETCH implies OBJECT_INFO_QUICK. The problem is that using
>> "flag & OBJECT_INFO_FOR_PREFETCH" means that OBJECT_INFO_QUICK also implies
>> OBJECT_INFO_FOR_PREFETCH.
>>
>> Split out the single bit from OBJECT_INFO_FOR_PREFETCH into a new
>> OBJECT_INFO_SKIP_FETCH_OBJECT as the single bit and keep
>> OBJECT_INFO_FOR_PREFETCH as the union of two flags. This allows a clearer use
>> of flag checking while also keeping the implication of OBJECT_INFO_QUICK.
> 
> Oof. I actually suggested splitting these up for review, but thought it
> was only a clarity/flexibility issue, and completely missed the
> correctness aspect of checking when the bit is set.
> 
> I agree with Junio's other response that using "==" would be the right
> way for a multi-bit check, in general. But I like the split here,
> because I think the result is more clear to read and harder to get
> wrong for future checks.

Thanks, for the feedback, both of you.

> I'd even go so far as to say...
> 
>> + * This is meant for bulk prefetching of missing blobs in a partial
>> + * clone. Implies OBJECT_INFO_SKIP_FETCH_OBJECT and OBJECT_INFO_QUICK
>> + */
>> +#define OBJECT_INFO_FOR_PREFETCH (OBJECT_INFO_SKIP_FETCH_OBJECT | OBJECT_INFO_QUICK)
> 
> we could dump this, and callers should just say what they mean (i.e.,
> specify both flags).

Dropping the _PREFETCH flag also makes oid_object_info_extended() slightly
less "coupled" to the prefetch feature, and instead describes more explicitly
the way the flag is changing the behavior of the method.
 
> There are only two of them, and I think both would be more readable with
> a helper more like:
> 
>   int should_prefetch_object(struct repository *r,
>                              const struct object_id *oid) {
> 	return !oid_object_info_extended(r, oid, NULL,
> 	                                 OBJECT_INFO_SKIP_FETCH_OBJECT |
> 					 OBJECT_INFO_QUICK);
>   }
> 
> but unless everybody is immediately on-board with "yes, that is much
> nicer", I don't want bikeshedding to hold up your important and
> obviously-correct fix.

I'll come back with another series to drop the _PREFETCH flag after the
release calms down. It can give more time for others to chime in here.

Thanks, Junio for the quick turnaround in taking the patch.

Thanks,
-Stolee