git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date
Jun 16, 2022, 14:10 UTC
Message-ID
<220616.86bkuswuh5.gmgdl@evledraar.gmail.com>
In-Reply-To
<31f406b1-b4e8-5da2-40af-5747938de634@github.com>
On Tue, Jun 14 2022, Derrick Stolee wrote:
Show 28 quoted lines
> 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...

Show 10 quoted lines
> 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".

Show 23 quoted lines
> 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);
 			}
 		}
Previous: Junio C HamanoNext: Taylor Blau
Message 10 of 14 in “revision: mark blobs needed for resolve-undo as reachable”
  1. revision: mark blobs needed for resolve-undo as reachableJunio C Hamano, Jun 9, 2022
  2. Derrick StoleeJun 13, 2022
  3. Junio C HamanoJun 13, 2022
  4. Ævar Arnfjörð BjarmasonJun 14, 2022
  5. Derrick StoleeJun 14, 2022
  6. Taylor BlauJun 15, 2022
  7. Jeff KingJun 15, 2022
  8. Taylor BlauJun 15, 2022
  9. Junio C HamanoJun 15, 2022
  10. Ævar Arnfjörð BjarmasonJun 16, 2022
  11. Taylor BlauJun 14, 2022
  12. fsck segfault (was: Re: [PATCH] revision: mark blobs needed for resolve-undo as reachable)SZEDER Gábor, Jul 11, 2022
  13. Junio C HamanoJul 11, 2022
  14. 2/1 fsck: do not dereference NULL while checking resolve-undo dataJunio C Hamano, Jul 11, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.