On 6/13/2022 8:24 PM, Ævar Arnfjörð Bjarmason wrote:

On Mon, Jun 13 2022, Derrick Stolee wrote:

quoted

On 6/9/2022 7:44 PM, Junio C Hamano wrote:

quoted

+	struct string_list *resolve_undo = istate->resolve_undo;
+
+	if (!resolve_undo)
+		return 0;
+
+	for_each_string_list_item(item, resolve_undo) {

I see this is necessary since for_each_string_list_item() does
not handle NULL lists. After attempting to allow it to handle
NULL lists, I see that the compiler complains about the cases
where it would _never_ be NULL, so that change appears to be
impossible.
 
The patch looks good. I liked the comments for the three phases
of the test.

I think it's probably good to keep for_each_string_list_item()
implemented the way it is, given that all existing callers of it feed
non-NULL lists to it.

We are talking right now about an example where it would be cleaner to
allow a NULL value.

This guarded example also exists in http.c (we would still need to guard
on NULL options):

	/* Add additional headers here */
	if (options && options->extra_headers) {
		const struct string_list_item *item;
		for_each_string_list_item(item, options->extra_headers) {
			headers = curl_slist_append(headers, item->string);
		}
	}

These guarded examples in ref_filter_match() would be greatly simplified:

	if (exclude_patterns && exclude_patterns->nr) {
		for_each_string_list_item(item, exclude_patterns) {
			if (match_ref_pattern(refname, item))
				return 0;
		}
	}

	if (include_patterns && include_patterns->nr) {
		for_each_string_list_item(item, include_patterns) {
			if (match_ref_pattern(refname, item))
				return 1;
		}
		return 0;
	}

	if (exclude_patterns_config && exclude_patterns_config->nr) {
		for_each_string_list_item(item, exclude_patterns_config) {
			if (match_ref_pattern(refname, item))
				return 0;
		}
	}

(The include_patterns check would still be needed for that extra
return 0; in the middle.)

There are more examples, but I'll stop listing them here.

But why is it impossible to make it handle NULL lists? This works for
me, and passes the tests:

	 /** Iterate over each item, as a macro. */
	 #define for_each_string_list_item(item,list)            \
	-	for (item = (list)->items;                      \
	+	for (item = (((list) && (list)->items) ? ((list)->items) : NULL); \

I thinks I had something like

	for ((list) && item = (list)->items; (list) && item && ...

but even with your suggestion, I get this compiler error:

In file included from convert.h:8,
                 from cache.h:10,
                 from apply.c:10:
apply.c: In function ‘write_out_results’:
string-list.h:146:22: error: the address of ‘cpath’ will always evaluate as ‘true’ [-Werror=address]
  146 |         for (item = ((list) && (list)->items) ? (list)->items : NULL;     \
      |                      ^
apply.c:4652:25: note: in expansion of macro ‘for_each_string_list_item’
 4652 |                         for_each_string_list_item(item, &cpath)
      |   

(along with many other examples).

Junio is right that we would need to convert this into a method with a
function pointer instead of a for_each_* macro. That's quite a big lift
for some small convenience for the callers.

Thanks,
-Stolee