Re: [PATCH] revision: mark blobs needed for resolve-undo as reachable

From: Derrick Stolee <derrickstolee@github.com>
Date: Jun 14, 2022, 14:35 UTC
Message-ID: <31f406b1-b4e8-5da2-40af-5747938de634@github.com>
In-Reply-To: <220614.86czfcytlz.gmgdl@evledraar.gmail.com>

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

Show 24 quoted lines

> 
> On Mon, Jun 13 2022, Derrick Stolee wrote:
> 
>> On 6/9/2022 7:44 PM, Junio C Hamano wrote:
>>
>>> +	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