Patrick Steinhardt wrote:

quoted

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).

quoted

+	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