Re: [PATCH 02/11] revision: put object filter into struct rev_info

[Derrick Stolee <derrickstolee@github.com>]

On 3/4/2022 5:15 PM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>>  static int try_bitmap_count(struct rev_info *revs,
>> -			    struct list_objects_filter_options *filter,
>>  			    int filter_provided_objects)
> 
> This makes quite a lot of sense as filter is now available as
> revs->filter.
> 
>>  {
>>  	uint32_t commit_count = 0,
>> @@ -436,7 +434,8 @@ static int try_bitmap_count(struct rev_info *revs,
>>  	 */
>>  	max_count = revs->max_count;
>>  
>> -	bitmap_git = prepare_bitmap_walk(revs, filter, filter_provided_objects);
>> +	bitmap_git = prepare_bitmap_walk(revs, revs->filter,
>> +					 filter_provided_objects);
> 
> And we should be able to do the same to prepare_bitmap_walk().  It
> is OK if such a change comes later and not as part of this commit.
> 
> Perhaps it is deliberate.  Unlike the helpers this step touches,
> namely, try_bitmap_count(), try_bitmap_traversal(), and
> try_bitmap_disk_usage(), prepare_bitmap_walk() is not a file-scope
> static helper and updating it will need touching many more places.

I'm making a note that this cleanup can happen in a follow-up series.
 
>> @@ -597,13 +595,17 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
>>  		}
>>  
>>  		if (skip_prefix(arg, ("--" CL_ARG__FILTER "="), &arg)) {
> 
> #leftoverbit.  We need to remember to clean this up, now "--filter"
> is well established (I am assuming this literal-string pasting is
> because we didn't know what the right and final word to be used as
> the option name back when this code was originally written), when
> the code around here is quiescent.

Good point.

>> -			parse_list_objects_filter(&filter_options, arg);
>> -			if (filter_options.choice && !revs.blob_objects)
>> +			if (!revs.filter)
>> +				CALLOC_ARRAY(revs.filter, 1);
>> +			parse_list_objects_filter(revs.filter, arg);
>> +			if (revs.filter->choice && !revs.blob_objects)
>>  				die(_("object filtering requires --objects"));
>>  			continue;
> 
> OK.  The original "filter_options" was a structure and not a pointer
> to a structure; now we have a pointer to a structure in revs as a
> member so we need an on-demand allocation.  CALLOC_ARRAY() instead
> of xcalloc(), when we know we are creating one element and not an
> array of elements whose size happens to be one, is not wrong but it
> does look strange.  Was there a reason why we avoid xcalloc()?

I think I've been using CALLOC_ARRAY(..., 1) over "... = xcalloc()"
for simplicity's sake for a while. I see quite a few across the
codebase, too, but I can swap the usage here if you feel that is
important.

> Makes me also wonder how big the filter_options structure is;
> because we will not use unbounded many revs structure, it may have
> been a simpler conversion to turn a static struct into an embedded
> struct member in a struct (instead of a member of a struct that is a
> pointer to a struct).  That way, we did not have to ...
>>>  		}
>>  		if (!strcmp(arg, ("--no-" CL_ARG__FILTER))) {
>> -			list_objects_filter_set_no_filter(&filter_options);
>> +			if (!revs.filter)
>> +				CALLOC_ARRAY(revs.filter, 1);
> 
> ... repeat the on-demand allocation.  If some code used to pass
> &filter_options in a parameter to helper functions, and such calling
> sites get rewritten to pass the value in the revs.filter pointer,
> and if revs hasn't gone through this codepath, these helper functions
> will start receiving NULL in their filter_options parameter, which
> they may or may not be prepared to take.  This "we get rid of a
> global struct and replace it with an on-demand allocated structure,
> pointer to which is stored in the revs structure" rewrite somehow
> makes me nervous.

I think the main idea is that the filter being NULL indicates "no
filter is used. Do a full object walk." If we use a static struct,
then we need to instead use revs->filter.filter_spec.nr, but that
is already being used as a BUG() statement:

const char *list_objects_filter_spec(struct list_objects_filter_options *filter)
{
	if (!filter->filter_spec.nr)
		BUG("no filter_spec available for this filter");

so a non-NULL filter with no specs is considered invalid, while
a NULL filter _is_ currently considered valid.

While we _could_ make that switch to using a static struct and
change our checks to allow empty specs, that would be a more
involved change. Maybe we can leave it for a follow up?

Thanks,
-Stolee