Re: [PATCH 6/9] ref-filter.c: refactor to create common helper functions

[Victoria Dye <vdye@github.com>]

Patrick Steinhardt wrote:
> On Tue, Nov 07, 2023 at 01:25:58AM +0000, Victoria Dye via GitGitGadget wrote:
>> From: Victoria Dye <vdye@github.com>
>>
>> Factor out parts of 'ref_array_push()', 'ref_filter_handler()', and
>> 'filter_refs()' into new helper functions ('ref_array_append()',
>> 'apply_ref_filter()', and 'do_filter_refs()' respectively), as well as
>> rename 'ref_filter_handler()' to 'filter_one()'. In this and later
>> patches, these helpers will be used by new ref-filter API functions. This
>> patch does not result in any user-facing behavior changes or changes to
>> callers outside of 'ref-filter.c'.
>>
>> The changes are as follows:
>>
>> * The logic to grow a 'struct ref_array' and append a given 'struct
>>   ref_array_item *' to it is extracted from 'ref_array_push()' into
>>   'ref_array_append()'.
>> * 'ref_filter_handler()' is renamed to 'filter_one()' to more clearly
>>   distinguish it from other ref filtering callbacks that will be added in
>>   later patches. The "*_one()" naming convention is common throughout the
>>   codebase for iteration callbacks.
>> * The code to filter a given ref by refname & object ID then create a new
>>   'struct ref_array_item' is moved out of 'filter_one()' and into
>>   'apply_ref_filter()'. 'apply_ref_filter()' returns either NULL (if the ref
>>   does not match the given filter) or a 'struct ref_array_item *' created
>>   with 'new_ref_array_item()'; 'filter_one()' appends that item to
>>   its ref array with 'ref_array_append()'.
>> * The filter pre-processing, contains cache creation, and ref iteration of
>>   'filter_refs()' is extracted into 'do_filter_refs()'. 'do_filter_refs()'
>>   takes its ref iterator function & callback data as an input from the
>>   caller, setting it up to be used with additional filtering callbacks in
>>   later patches.
> 
> To me, a bulleted list spelling out the different changes I'm doing
> often indicates that I might want to split up the commit into one for
> each of the items. I don't feel strongly about this, but think that it
> might help the reviewer in this case.

While that's a good guideline to keep in mind, it's not universally
applicable. In this case, (almost) all of the changes are done the same way,
focused on the same goal: extract bits of 'filter_refs()' into generic,
internal helpers so we can use those bits elsewhere in later patches.
Splitting those extractions into multiple patches would essentially lead to
a handful of very small patches that more-or-less have the same commit
message. As I mentioned in [1], I think there's value to having the
immediate context of related changes in a single patch (as long as that
single patch doesn't become unwieldy), so I'm not inclined to split this up.

That said, I did say "(almost) all" of the changes are conceptually similar.
Looking at this now, the rename of 'ref_filter_handler()' => 'filter_one()'
doesn't really fit the "extract into helper functions" theme of the rest of
the patch, I'll pull that out into its own.

[1] https://lore.kernel.org/git/a833b5a7-0201-4c2e-8821-f2a1930cb403@github.com/