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