Patrick Steinhardt wrote:

On Tue, Nov 07, 2023 at 01:25:58AM +0000, Victoria Dye via GitGitGadget wrote:

quoted

From: Victoria Dye <redacted>

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/ (local)