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

From: Victoria Dye <vdye@github.com>
Date: Nov 7, 2023, 18:41 UTC
Message-ID: <ecd7b723-0ff7-412b-a332-8c011ff12f86@github.com>
In-Reply-To: <ZUoWT8GyrZlvH_Go@tanuki>

Patrick Steinhardt wrote:

Show 36 quoted lines

> 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/