Re: [PATCH 7/9] ref-filter.c: filter & format refs in the same callback

[Victoria Dye <vdye@github.com>]

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