On 3/4/2022 5:15 PM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@xxxxxxxxx> 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
