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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Tue, Jun 14 2022, Derrick Stolee wrote:

> On 6/13/2022 8:24 PM, Ævar Arnfjörð Bjarmason wrote:
>> 
>> 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.

First, I've read the thread and I see the compile error you ran into
below, and Jeff King's downthread workaround, so we could do this, and
I'm not at all opposed...

> 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);
> 		}
> 	}

I think this and probably many other examples you can find are ones
where we should just stop using a maybe-NULL "struct string_list", as in
the WIP (but segfaulting) patch below, but I think you get the
idea. I.e. in that case we're passing an "options" struct that can be
NULL, but could just have an initializer.

I.e. we should probably just have a non-NULL options with sensible
defaults, which would also allow for changing the adjacent "options &&
options->no_cache" etc. code to just "options->no_cache".

> 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;
> 		}
> 	}

First, regardless of any bigger change, I think all of those except the
middle one can drop the "nr" check.

But more generally, isn't something like [2] below a better change than
chasing the case of "what if it's NULL?".

Very WIP of course, and I just checked if it compiled, there's probably
more lurking bugs, but I think you get the idea.

One commit (of mine) on "master" that goes in that direction is
0000e81811b (builtin/remote.c: add and use SHOW_INFO_INIT, 2021-10-01).

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

Maybe there's better ones, but from my own past spelunking into "struct
string_list" users I've mostly found cases like 0000e81811b and the
below.

I.e. sure, handling NULL was a hassle, but the underlying problem was
usually *why is it NULL*. I.e. can't we just have the list be 0..N, why
do we need NULL, 0..N?

1.

diff --git a/http.c b/http.c
index 11c6f69facd..c2fa2b78db8 100644
--- a/http.c
+++ b/http.c
@@ -1808,6 +1808,7 @@ static int http_request(const char *url,
 	struct strbuf buf = STRBUF_INIT;
 	const char *accept_language;
 	int ret;
+	const struct string_list_item *item;
 
 	slot = get_active_slot();
 	curl_easy_setopt(slot->curl, CURLOPT_HTTPGET, 1);
@@ -1844,12 +1845,8 @@ static int http_request(const char *url,
 	headers = curl_slist_append(headers, buf.buf);
 
 	/* 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);
-		}
-	}
+	for_each_string_list_item(item, &options->extra_headers)
+		headers = curl_slist_append(headers, item->string);
 
 	curl_easy_setopt(slot->curl, CURLOPT_URL, url);
 	curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, headers);
@@ -2055,7 +2052,7 @@ static char *fetch_pack_index(unsigned char *hash, const char *base_url)
 	strbuf_addf(&buf, "%s.temp", sha1_pack_index_name(hash));
 	tmp = strbuf_detach(&buf, NULL);
 
-	if (http_get_file(url, tmp, NULL) != HTTP_OK) {
+	if (http_get_file(url, tmp, NULL /* segfaults due to this */) != HTTP_OK) {
 		error("Unable to get pack index %s", url);
 		FREE_AND_NULL(tmp);
 	}
diff --git a/http.h b/http.h
index ba303cfb372..41596fbf443 100644
--- a/http.h
+++ b/http.h
@@ -144,7 +144,7 @@ struct http_get_options {
 	 * request. The strings in the list must not be freed until after the
 	 * request has completed.
 	 */
-	struct string_list *extra_headers;
+	struct string_list extra_headers;
 };
 
 /* Return values for http_get_*() */
diff --git a/remote-curl.c b/remote-curl.c
index 67f178b1120..30235577487 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -449,7 +449,6 @@ static struct discovery *discover_refs(const char *service, int for_push)
 	struct strbuf refs_url = STRBUF_INIT;
 	struct strbuf effective_url = STRBUF_INIT;
 	struct strbuf protocol_header = STRBUF_INIT;
-	struct string_list extra_headers = STRING_LIST_INIT_DUP;
 	struct discovery *last = last_discovery;
 	int http_ret, maybe_smart = 0;
 	struct http_get_options http_options;
@@ -478,16 +477,18 @@ static struct discovery *discover_refs(const char *service, int for_push)
 	if (version == protocol_v2 && !strcmp("git-receive-pack", service))
 		version = protocol_v0;
 
+	/* antipattern, we should have an initializer for "struct http_get_options"... */
+	memset(&http_options, 0, sizeof(http_options));
+	string_list_init_nodup(&http_options.extra_headers);
+
 	/* Add the extra Git-Protocol header */
 	if (get_protocol_http_header(version, &protocol_header))
-		string_list_append(&extra_headers, protocol_header.buf);
+		string_list_append(&http_options.extra_headers, protocol_header.buf);
 
-	memset(&http_options, 0, sizeof(http_options));
 	http_options.content_type = &type;
 	http_options.charset = &charset;
 	http_options.effective_url = &effective_url;
 	http_options.base_url = &url;
-	http_options.extra_headers = &extra_headers;
 	http_options.initial_request = 1;
 	http_options.no_cache = 1;
 
@@ -538,7 +539,8 @@ static struct discovery *discover_refs(const char *service, int for_push)
 	strbuf_release(&effective_url);
 	strbuf_release(&buffer);
 	strbuf_release(&protocol_header);
-	string_list_clear(&extra_headers, 0);
+	/*... and a proper destructor... */
+	string_list_clear(&http_options.extra_headers, 0);
 	last_discovery = last;
 	return last;
 }

2.

diff --git a/builtin/log.c b/builtin/log.c
index 88a5e98875a..4af1503887e 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -168,12 +168,11 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,
 	struct userformat_want w;
 	int quiet = 0, source = 0, mailmap;
 	static struct line_opt_callback_data line_cb = {NULL, NULL, STRING_LIST_INIT_DUP};
-	static struct string_list decorate_refs_exclude = STRING_LIST_INIT_NODUP;
-	static struct string_list decorate_refs_exclude_config = STRING_LIST_INIT_NODUP;
-	static struct string_list decorate_refs_include = STRING_LIST_INIT_NODUP;
-	struct decoration_filter decoration_filter = {&decorate_refs_include,
-						      &decorate_refs_exclude,
-						      &decorate_refs_exclude_config};
+	struct decoration_filter decoration_filter = {
+		.include_ref_pattern = STRING_LIST_INIT_NODUP,
+		.exclude_ref_pattern = STRING_LIST_INIT_NODUP,
+		.exclude_ref_config_pattern = STRING_LIST_INIT_NODUP,
+	};
 	static struct revision_sources revision_sources;
 
 	const struct option builtin_log_options[] = {
@@ -181,9 +180,9 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,
 		OPT_BOOL(0, "source", &source, N_("show source")),
 		OPT_BOOL(0, "use-mailmap", &mailmap, N_("use mail map file")),
 		OPT_ALIAS(0, "mailmap", "use-mailmap"),
-		OPT_STRING_LIST(0, "decorate-refs", &decorate_refs_include,
+		OPT_STRING_LIST(0, "decorate-refs", &decoration_filter.include_ref_pattern,
 				N_("pattern"), N_("only decorate refs that match <pattern>")),
-		OPT_STRING_LIST(0, "decorate-refs-exclude", &decorate_refs_exclude,
+		OPT_STRING_LIST(0, "decorate-refs-exclude", &decoration_filter.exclude_ref_pattern,
 				N_("pattern"), N_("do not decorate refs that match <pattern>")),
 		OPT_CALLBACK_F(0, "decorate", NULL, NULL, N_("decorate options"),
 			       PARSE_OPT_OPTARG, decorate_callback),
@@ -272,7 +271,7 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,
 		if (config_exclude) {
 			struct string_list_item *item;
 			for_each_string_list_item(item, config_exclude)
-				string_list_append(&decorate_refs_exclude_config,
+				string_list_append(&decoration_filter.exclude_ref_config_pattern,
 						   item->string);
 		}
 
diff --git a/log-tree.c b/log-tree.c
index d0ac0a6327a..f7d475f652f 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -104,32 +104,23 @@ static int ref_filter_match(const char *refname,
 			    const struct decoration_filter *filter)
 {
 	struct string_list_item *item;
-	const struct string_list *exclude_patterns = filter->exclude_ref_pattern;
-	const struct string_list *include_patterns = filter->include_ref_pattern;
-	const struct string_list *exclude_patterns_config =
-				filter->exclude_ref_config_pattern;
 
-	if (exclude_patterns && exclude_patterns->nr) {
-		for_each_string_list_item(item, exclude_patterns) {
-			if (match_ref_pattern(refname, item))
-				return 0;
-		}
-	}
+	for_each_string_list_item(item, &filter->exclude_ref_pattern)
+		if (match_ref_pattern(refname, item))
+			return 0;
 
-	if (include_patterns && include_patterns->nr) {
-		for_each_string_list_item(item, include_patterns) {
+	
+	if (filter->include_ref_pattern.nr) {
+		for_each_string_list_item(item, &filter->include_ref_pattern) {
 			if (match_ref_pattern(refname, item))
 				return 1;
+			return 0;
 		}
-		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;
-		}
-	}
+	for_each_string_list_item(item, &filter->exclude_ref_config_pattern)
+		if (match_ref_pattern(refname, item))
+			return 0;
 
 	return 1;
 }
@@ -202,13 +193,13 @@ void load_ref_decorations(struct decoration_filter *filter, int flags)
 	if (!decoration_loaded) {
 		if (filter) {
 			struct string_list_item *item;
-			for_each_string_list_item(item, filter->exclude_ref_pattern) {
+			for_each_string_list_item(item, &filter->exclude_ref_pattern) {
 				normalize_glob_ref(item, NULL, item->string);
 			}
-			for_each_string_list_item(item, filter->include_ref_pattern) {
+			for_each_string_list_item(item, &filter->include_ref_pattern) {
 				normalize_glob_ref(item, NULL, item->string);
 			}
-			for_each_string_list_item(item, filter->exclude_ref_config_pattern) {
+			for_each_string_list_item(item, &filter->exclude_ref_config_pattern) {
 				normalize_glob_ref(item, NULL, item->string);
 			}
 		}