Re: [PATCH] revision: mark blobs needed for resolve-undo as reachable
From: Ævar Arnfjörð Bjarmason <hidden>
Date: 2022-06-16 14:31:25
Subsystem:
the rest · Maintainer:
Linus Torvalds
On Tue, Jun 14 2022, Derrick Stolee wrote:
On 6/13/2022 8:24 PM, Ævar Arnfjörð Bjarmason wrote:quoted
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.
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); } }