Patrick Steinhardt wrote:
>> diff --git a/ref-filter.c b/ref-filter.c
>> index ff00ab4b8d8..384cf1595ff 100644
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -2863,6 +2863,44 @@ static void free_array_item(struct ref_array_item *item)
>> free(item);
>> }
>>
>> +struct ref_filter_and_format_cbdata {
>> + struct ref_filter *filter;
>> + struct ref_format *format;
>> +
>> + struct ref_filter_and_format_internal {
>> + int count;
>> + } internal;
>> +};
>> +
>> +static int filter_and_format_one(const char *refname, const struct object_id *oid, int flag, void *cb_data)
>> +{
>> + struct ref_filter_and_format_cbdata *ref_cbdata = cb_data;
>> + struct ref_array_item *ref;
>> + struct strbuf output = STRBUF_INIT, err = STRBUF_INIT;
>> +
>> + ref = apply_ref_filter(refname, oid, flag, ref_cbdata->filter);
>> + if (!ref)
>> + return 0;
>> +
>> + if (format_ref_array_item(ref, ref_cbdata->format, &output, &err))
>> + die("%s", err.buf);
>> +
>> + if (output.len || !ref_cbdata->format->array_opts.omit_empty) {
>> + fwrite(output.buf, 1, output.len, stdout);
>> + putchar('\n');
>> + }
>> +
>> + strbuf_release(&output);
>> + strbuf_release(&err);
>> + free_array_item(ref);
>> +
>> + if (ref_cbdata->format->array_opts.max_count &&
>> + ++ref_cbdata->internal.count >= ref_cbdata->format->array_opts.max_count)
>> + return -1;
>
> It feels a bit weird to return a negative value here, which usually
> indicates that an error has happened whereas we only use it here to
> abort the iteration. But we ignore the return value of
> `do_iterate_refs()` anyway, so it doesn't make much of a difference.
I'll update it to 1, and also add a comment that the non-zero return value
stops iteration since it's not immediately clear from other 'each_ref_fn's
what that means. For reference, there appears to only be one other
'each_ref_fn' that even has the potential to return a nonzero return value
('ref_present()' in 'refs/files-backend.c).
>
>> + return 0;
>> +}
>> +
>> /* Free all memory allocated for ref_array */
>> void ref_array_clear(struct ref_array *array)
>> {
>> @@ -3046,16 +3084,46 @@ int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int
>> return ret;
>> }
>>
>> +static inline int can_do_iterative_format(struct ref_filter *filter,
>> + struct ref_sorting *sorting,
>> + struct ref_format *format)
>> +{
>> + /*
>> + * Refs can be filtered and formatted in the same iteration as long
>> + * as we aren't filtering on reachability, sorting the results, or
>> + * including ahead-behind information in the formatted output.
>> + */
>
> Do we want to format this as a bulleted list so that it's more readily
> extensible if we ever need to pay attention to new options here? Also, I
> noted that this commit doesn't add any new tests -- do we already
> exercise all of these conditions?
Sure, I'll convert it to a bulleted list. I don't really expect it to change
much, though; to have any effect on this condition, the new filter/format
would need to act on the pre-filtered ref_array, which isn't particularly
common.
And yes, the existing tests cover scenarios where this function returns true
(e.g. 'git for-each-ref --no-sort') & where it returns false (essentially
anything else).
>
> More generally, I worry a bit about maintainability of this code snippet
> as we need to remember to always update this condition whenever we add a
> new option, and this can be quite easy to miss. The performance benefit
> might be worth the effort though.
I'll add more detailed comments to clarify what's going on here.
In practice, though, I don't think this would be all that easy to miss. As I
noted above, the only filters/formats that affect this are ones that need to
loop over an entire filtered ref_array after the initial
'for_each_fullref_in()'. To have it actually apply to commands that use
'filter_and_format_refs()', they'll need to add that behavior here (like
'filter_ahead_behind()'), where it should be apparent that
'can_do_iterative_format()' is relevant to their change.
>
> Patrick
