{"thread":{"id":"58414","subject":"[PATCH 0/4] list-objects-filter cleanups","startedAt":"2022-09-11T04:57:43Z","lastAt":"2022-09-19T19:06:04Z","messageCount":11,"participants":["Jeff King","Junio C Hamano","Derrick Stolee"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"462903","messageId":"Yx1qwlpNbxGEiUgc@coredump.intra.peff.net","threadId":"58414","inReplyTo":null,"subject":"[PATCH 0/4] list-objects-filter cleanups","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-09-11T04:57:38Z","receivedAt":"2022-09-11T04:57:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In the recent jk/plug-list-object-filter-leaks topic[1], I stopped short\nof fixing all of the callers to actually initialize the filter struct\nbeyond zero-ing it.\n\nThis series does the cleanup that I was afraid to do there. :)\n\nI think the end result is less confusing and error-prone. And as you can\nsee in patch 4, it matches how the code originally hoped to be written,\nbut the author was also afraid of the zero-initialization thing.\n\nIt is kind of churny, and carries some risk of regression (if I missed a\nspot). IMHO it's worth it, but even if we don't take it, we should pick\nup the first two patches, which are small bug-lets that the conversion\nturned up.\n\nThese patches should be applied on top of jk/plug-list-object-filter-leaks,\nwhich is currently in next.\n\n  [1/4]: list-objects-filter: don't memset after releasing filter struct\n  [2/4]: list-objects-filter: handle null default filter spec\n  [3/4]: list-objects-filter: add and use initializers\n  [4/4]: list-objects-filter: convert filter_spec to a strbuf\n\n builtin/clone.c               |  2 +-\n builtin/fetch-pack.c          |  1 +\n builtin/fetch.c               |  2 +-\n builtin/submodule--helper.c   |  8 ++--\n bundle.h                      |  1 +\n list-objects-filter-options.c | 75 ++++++++++++++---------------------\n list-objects-filter-options.h |  5 ++-\n revision.c                    |  1 +\n transport-helper.c            |  2 +\n transport.c                   |  1 +\n upload-pack.c                 |  1 +\n 11 files changed, 47 insertions(+), 52 deletions(-)\n\n-Peff\n\n[1] https://lore.kernel.org/git/Yxl1BNQoy6Drf0Oe@coredump.intra.peff.net/\n"},{"id":"462904","messageId":"Yx1q4crkHGNzwLhh@coredump.intra.peff.net","threadId":"58414","inReplyTo":"Yx1qwlpNbxGEiUgc@coredump.intra.peff.net","subject":"[PATCH 1/4] list-objects-filter: don't memset after releasing filter struct","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-09-11T04:58:09Z","receivedAt":"2022-09-11T04:58:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If we see an error while parsing a \"combine\" filter, we call\nlist_objects_filter_release() to free any allocated memory,\nand then use memset() to return the struct to a known state. But the\nrelease function already does that reinitializing. Doing it again is\npointless.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n list-objects-filter-options.c | 4 +---\n 1 file changed, 1 insertion(+), 3 deletions(-)\n\ndiff --git a/list-objects-filter-options.c b/list-objects-filter-options.c\nindex 6cc4eb8e1c..ea989db260 100644\n--- a/list-objects-filter-options.c\n+++ b/list-objects-filter-options.c\n@@ -187,10 +187,8 @@ static int parse_combine_filter(\n \n cleanup:\n \tstrbuf_list_free(subspecs);\n-\tif (result) {\n+\tif (result)\n \t\tlist_objects_filter_release(filter_options);\n-\t\tmemset(filter_options, 0, sizeof(*filter_options));\n-\t}\n \treturn result;\n }\n \n-- \n2.37.3.1242.g835d375f85\n\n"},{"id":"462905","messageId":"Yx1rfYtQAlZGfAak@coredump.intra.peff.net","threadId":"58414","inReplyTo":"Yx1qwlpNbxGEiUgc@coredump.intra.peff.net","subject":"[PATCH 2/4] list-objects-filter: handle null default filter spec","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-09-11T05:00:45Z","receivedAt":"2022-09-11T05:00:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we have a remote.*.promisor config variable, we know that we're in\na partial clone. Usually there's a matching remote.*.partialclonefilter\noption, which tells us which filter to use with the remote. If that\noption is missing, we skip setting up the filter at all. But something\nfunny happens: we stick a NULL entry into the string_list storing the\ntext filter spec.\n\nThis is a weird state, and could possibly segfault if anybody called\ncalled list_objects_filter_spec(), etc. In practice, nobody does,\nbecause filter->choice will still be LOFC_DISABLED, so code generally\nrealizes there's no filter to use. And the string_list itself is OK,\nbecause it starts in non-dup mode until we actually parse a filter spec.\nSo it blindly stores the NULL without even looking at it.\n\nBut it's probably worth avoiding this confused state. It's an accident\nwaiting to happen, and it will be a problem if we replace the lazy\ninitialization from 7e2619d8ff (list_objects_filter_options: plug leak\nof filter_spec strings, 2022-09-08) with a real initialization function.\n\nThe history is a little interesting here, as the bug was introduced\nduring the merge resolution in 627b826834 (Merge branch\n'md/list-objects-filter-combo', 2019-09-18).\n\nThe original logic comes from cac1137dc4 (list-objects: check if filter\nis NULL before using, 2018-06-11), where we had a single string via\ncore.partialCloneFilter, and a simple NULL check was sufficient. And it\neven added a test in t0410 that covers this situation.\n\nLater, that was expanded to allow per-remote filters in fa3d1b63e8\n(promisor-remote: parse remote.*.partialclonefilter, 2019-06-25). After\nthat commit, we get a promisor struct with a partial_clone_filter\nstring, which could be NULL. The commit checks only that the struct\npointer is non-NULL, which is enough. It may pass NULL to\ngently_parse_list_objects_filter(), but that function is smart enough to\nconsider it a noop.\n\nBut in parallel, cf9ceb5a12 (list-objects-filter-options: make\nfilter_spec a string_list, 2019-06-27) added a new line of code: before\nwe call gently_parse_list_objets_filter(), we append the filter spec to\nthe string_list. By itself that was OK, since we'd have returned early\nif the string was NULL.\n\nWhen the two were merged in 627b826834, the result is that we return\nearly only if the struct is NULL, but not the string. And we append to\nthe string_list, meaning we may append NULL.\n\nThe solution is to return early if either is NULL, as it would mean we\ndon't have a configured filter.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nSorry, I know it's a long message for a one-line fix! I found the\nhistory of it so interesting, though, because it's rare that I see\nbugs introduced during merges.\n\n list-objects-filter-options.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/list-objects-filter-options.c b/list-objects-filter-options.c\nindex ea989db260..18c51001dc 100644\n--- a/list-objects-filter-options.c\n+++ b/list-objects-filter-options.c\n@@ -399,7 +399,7 @@ void partial_clone_get_default_filter_spec(\n \t/*\n \t * Parse default value, but silently ignore it if it is invalid.\n \t */\n-\tif (!promisor)\n+\tif (!promisor || !promisor->partial_clone_filter)\n \t\treturn;\n \n \tstring_list_append(&filter_options->filter_spec,\n-- \n2.37.3.1242.g835d375f85\n\n"},{"id":"462906","messageId":"Yx1sC6w2yDZRXFdE@coredump.intra.peff.net","threadId":"58414","inReplyTo":"Yx1qwlpNbxGEiUgc@coredump.intra.peff.net","subject":"[PATCH 3/4] list-objects-filter: add and use initializers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-09-11T05:03:07Z","receivedAt":"2022-09-11T05:03:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In 7e2619d8ff (list_objects_filter_options: plug leak of filter_spec\nstrings, 2022-09-08), we noted that the filter_spec string_list was\ninconsistent in how it handled memory ownership of strings stored in the\nlist. The fix there was a bit of a band-aid to set the \"strdup_strings\"\nvariable right before adding anything.\n\nThat works OK, and it lets the users of the API continue to\nzero-initialize the struct. But it makes the code a bit hard to follow\nand accident-prone, as any other spots appending the filter_spec need to\nthink about whether to set the strdup_strings value, too (there's one\nsuch spot in partial_clone_get_default_filter_spec(), which is probably\na possible memory leak).\n\nSo let's do that full cleanup now. We'll introduce a\nLIST_OBJECTS_FILTER_INIT macro and matching function, and use them as\nappropriate (though it is for the \"_options\" struct, this matches the\ncorresponding list_objects_filter_release() function).\n\nThis is harder than it seems! Many other structs, like\ngit_transport_data, embed the filter struct. So they need to initialize\nit themselves even if the rest of the enclosing struct is OK with\nzero-initialization. I found all of the relevant spots by grepping\nmanually for declarations of list_objects_filter_options. And then doing\nso recursively for structs which embed it, and ones which embed those,\nand so on.\n\nI'm pretty sure I got everything, but there's no change that would alert\nthe compiler if any topics in flight added new declarations. To catch\nthis case, we now double-check in the parsing function that things were\ninitialized as expected and BUG() if appropriate.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/clone.c               |  2 +-\n builtin/fetch-pack.c          |  1 +\n builtin/fetch.c               |  2 +-\n builtin/submodule--helper.c   |  8 ++++----\n bundle.h                      |  1 +\n list-objects-filter-options.c | 20 +++++++++++---------\n list-objects-filter-options.h |  3 +++\n revision.c                    |  1 +\n transport-helper.c            |  2 ++\n transport.c                   |  1 +\n upload-pack.c                 |  1 +\n 11 files changed, 27 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex e21d42dfee..d269d6fec6 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -73,7 +73,7 @@ static struct string_list option_optional_reference = STRING_LIST_INIT_NODUP;\n static int option_dissociate;\n static int max_jobs = -1;\n static struct string_list option_recurse_submodules = STRING_LIST_INIT_NODUP;\n-static struct list_objects_filter_options filter_options;\n+static struct list_objects_filter_options filter_options = LIST_OBJECTS_FILTER_INIT;\n static int option_filter_submodules = -1;    /* unspecified */\n static int config_filter_submodules = -1;    /* unspecified */\n static struct string_list server_options = STRING_LIST_INIT_NODUP;\ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex f045bbbe94..afe679368d 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -62,6 +62,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n \tpacket_trace_identity(\"fetch-pack\");\n \n \tmemset(&args, 0, sizeof(args));\n+\tlist_objects_filter_init(&args.filter_options);\n \targs.uploadpack = \"git-upload-pack\";\n \n \tfor (i = 1; i < argc && *argv[i] == '-'; i++) {\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 368a0f5329..d2c0c65de4 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -80,7 +80,7 @@ static int recurse_submodules_cli = RECURSE_SUBMODULES_DEFAULT;\n static int recurse_submodules_default = RECURSE_SUBMODULES_ON_DEMAND;\n static int shown_url = 0;\n static struct refspec refmap = REFSPEC_INIT_FETCH;\n-static struct list_objects_filter_options filter_options;\n+static struct list_objects_filter_options filter_options = LIST_OBJECTS_FILTER_INIT;\n static struct string_list server_options = STRING_LIST_INIT_DUP;\n static struct string_list negotiation_tip = STRING_LIST_INIT_NODUP;\n static int fetch_write_commit_graph = -1;\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex b63f420ece..4b958e3cf8 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -1747,7 +1747,8 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n {\n \tint dissociate = 0, quiet = 0, progress = 0, require_init = 0;\n \tstruct module_clone_data clone_data = MODULE_CLONE_DATA_INIT;\n-\tstruct list_objects_filter_options filter_options;\n+\tstruct list_objects_filter_options filter_options =\n+\t\tLIST_OBJECTS_FILTER_INIT;\n \n \tstruct option module_clone_options[] = {\n \t\tOPT_STRING(0, \"prefix\", &clone_data.prefix,\n@@ -1789,7 +1790,6 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n \t\tNULL\n \t};\n \n-\tmemset(&filter_options, 0, sizeof(filter_options));\n \targc = parse_options(argc, argv, prefix, module_clone_options,\n \t\t\t     git_submodule_helper_usage, 0);\n \n@@ -2566,7 +2566,8 @@ static int module_update(int argc, const char **argv, const char *prefix)\n {\n \tstruct pathspec pathspec;\n \tstruct update_data opt = UPDATE_DATA_INIT;\n-\tstruct list_objects_filter_options filter_options;\n+\tstruct list_objects_filter_options filter_options =\n+\t\tLIST_OBJECTS_FILTER_INIT;\n \tint ret;\n \n \tstruct option module_update_options[] = {\n@@ -2626,7 +2627,6 @@ static int module_update(int argc, const char **argv, const char *prefix)\n \tupdate_clone_config_from_gitmodules(&opt.max_jobs);\n \tgit_config(git_update_clone_config, &opt.max_jobs);\n \n-\tmemset(&filter_options, 0, sizeof(filter_options));\n \targc = parse_options(argc, argv, prefix, module_update_options,\n \t\t\t     git_submodule_helper_usage, 0);\n \ndiff --git a/bundle.h b/bundle.h\nindex 0c052f5496..68ff39a0a7 100644\n--- a/bundle.h\n+++ b/bundle.h\n@@ -18,6 +18,7 @@ struct bundle_header {\n { \\\n \t.prerequisites = STRING_LIST_INIT_DUP, \\\n \t.references = STRING_LIST_INIT_DUP, \\\n+\t.filter = LIST_OBJECTS_FILTER_INIT, \\\n }\n void bundle_header_init(struct bundle_header *header);\n void bundle_header_release(struct bundle_header *header);\ndiff --git a/list-objects-filter-options.c b/list-objects-filter-options.c\nindex 18c51001dc..56a1933a50 100644\n--- a/list-objects-filter-options.c\n+++ b/list-objects-filter-options.c\n@@ -108,7 +108,7 @@ int gently_parse_list_objects_filter(\n \n \tstrbuf_addf(errbuf, _(\"invalid filter-spec '%s'\"), arg);\n \n-\tmemset(filter_options, 0, sizeof(*filter_options));\n+\tlist_objects_filter_init(filter_options);\n \treturn 1;\n }\n \n@@ -223,8 +223,7 @@ static void transform_to_combine_type(\n \t\tstruct list_objects_filter_options *sub_array =\n \t\t\txcalloc(initial_sub_alloc, sizeof(*sub_array));\n \t\tsub_array[0] = *filter_options;\n-\t\tmemset(filter_options, 0, sizeof(*filter_options));\n-\t\tstring_list_init_dup(&filter_options->filter_spec);\n+\t\tlist_objects_filter_init(filter_options);\n \t\tfilter_options->sub = sub_array;\n \t\tfilter_options->sub_alloc = initial_sub_alloc;\n \t}\n@@ -255,11 +254,8 @@ void parse_list_objects_filter(\n \tstruct strbuf errbuf = STRBUF_INIT;\n \tint parse_error;\n \n-\tif (!filter_options->filter_spec.strdup_strings) {\n-\t\tif (filter_options->filter_spec.nr)\n-\t\t\tBUG(\"unexpected non-allocated string in filter_spec\");\n-\t\tfilter_options->filter_spec.strdup_strings = 1;\n-\t}\n+\tif (!filter_options->filter_spec.strdup_strings)\n+\t\tBUG(\"filter_options not properly initialized\");\n \n \tif (!filter_options->choice) {\n \t\tstring_list_append(&filter_options->filter_spec, arg);\n@@ -346,7 +342,7 @@ void list_objects_filter_release(\n \tfor (sub = 0; sub < filter_options->sub_nr; sub++)\n \t\tlist_objects_filter_release(&filter_options->sub[sub]);\n \tfree(filter_options->sub);\n-\tmemset(filter_options, 0, sizeof(*filter_options));\n+\tlist_objects_filter_init(filter_options);\n }\n \n void partial_clone_register(\n@@ -429,3 +425,9 @@ void list_objects_filter_copy(\n \tfor (i = 0; i < src->sub_nr; i++)\n \t\tlist_objects_filter_copy(&dest->sub[i], &src->sub[i]);\n }\n+\n+void list_objects_filter_init(struct list_objects_filter_options *filter_options)\n+{\n+\tstruct list_objects_filter_options blank = LIST_OBJECTS_FILTER_INIT;\n+\tmemcpy(filter_options, &blank, sizeof(*filter_options));\n+}\ndiff --git a/list-objects-filter-options.h b/list-objects-filter-options.h\nindex ffc02d77e7..2720f7dba8 100644\n--- a/list-objects-filter-options.h\n+++ b/list-objects-filter-options.h\n@@ -69,6 +69,9 @@ struct list_objects_filter_options {\n \t */\n };\n \n+#define LIST_OBJECTS_FILTER_INIT { .filter_spec = STRING_LIST_INIT_DUP }\n+void list_objects_filter_init(struct list_objects_filter_options *filter_options);\n+\n /*\n  * Parse value of the argument to the \"filter\" keyword.\n  * On the command line this looks like:\ndiff --git a/revision.c b/revision.c\nindex a04ebd6139..7fc0e16bdf 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1903,6 +1903,7 @@ void repo_init_revisions(struct repository *r,\n \t}\n \n \tinit_display_notes(&revs->notes_opt);\n+\tlist_objects_filter_init(&revs->filter);\n }\n \n static void add_pending_commit_list(struct rev_info *revs,\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 322c722478..e95267a4ab 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -1286,6 +1286,8 @@ int transport_helper_init(struct transport *transport, const char *name)\n \tif (getenv(\"GIT_TRANSPORT_HELPER_DEBUG\"))\n \t\tdebug = 1;\n \n+\tlist_objects_filter_init(&data->transport_options.filter_options);\n+\n \ttransport->data = data;\n \ttransport->vtable = &vtable;\n \ttransport->smart_options = &(data->transport_options);\ndiff --git a/transport.c b/transport.c\nindex 6ec6130852..a14179684b 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -1113,6 +1113,7 @@ struct transport *transport_get(struct remote *remote, const char *url)\n \t\t * will be checked individually in git_connect.\n \t\t */\n \t\tstruct git_transport_data *data = xcalloc(1, sizeof(*data));\n+\t\tlist_objects_filter_init(&data->options.filter_options);\n \t\tret->data = data;\n \t\tret->vtable = &builtin_smart_vtable;\n \t\tret->smart_options = &(data->options);\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 35fe1a3dbb..54583b71c6 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -141,6 +141,7 @@ static void upload_pack_data_init(struct upload_pack_data *data)\n \tdata->allow_filter_fallback = 1;\n \tdata->tree_filter_max_depth = ULONG_MAX;\n \tpacket_writer_init(&data->writer, 1);\n+\tlist_objects_filter_init(&data->filter_options);\n \n \tdata->keepalive = 5;\n \tdata->advertise_sid = 0;\n-- \n2.37.3.1242.g835d375f85\n\n"},{"id":"462907","messageId":"Yx1sI8cjU/SWUKVy@coredump.intra.peff.net","threadId":"58414","inReplyTo":"Yx1qwlpNbxGEiUgc@coredump.intra.peff.net","subject":"[PATCH 4/4] list-objects-filter: convert filter_spec to a strbuf","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-09-11T05:03:31Z","receivedAt":"2022-09-11T05:03:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Originally, the filter_spec field was just a string pointer. In\ncf9ceb5a12 (list-objects-filter-options: make filter_spec a string_list,\n2019-06-27) it became a string_list, but that commit notes:\n\n    A strbuf would seem to be a more natural choice for this object, but\n    it unfortunately requires initialization besides just zero'ing out\n    the memory.  This results in all container structs, and all\n    containers of those structs, etc., to also require initialization.\n    Initializing them all would be more cumbersome that simply using a\n    string_list, which behaves properly when its contents are zero'd.\n\nNow that we've changed the struct to require non-zero initialization\nanyway (ironically, because string_list also needed non-zero\ninitialization to avoid leaks), we can now convert to that more natural\ntype.\n\nThis makes the list_objects_filter_spec() function much less awkward, as\nit had to collapse the string_list to a single-entry list on the fly.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n list-objects-filter-options.c | 51 +++++++++++++----------------------\n list-objects-filter-options.h |  4 +--\n 2 files changed, 20 insertions(+), 35 deletions(-)\n\ndiff --git a/list-objects-filter-options.c b/list-objects-filter-options.c\nindex 56a1933a50..d46ce4acc4 100644\n--- a/list-objects-filter-options.c\n+++ b/list-objects-filter-options.c\n@@ -202,10 +202,10 @@ static int allow_unencoded(char ch)\n static void filter_spec_append_urlencode(\n \tstruct list_objects_filter_options *filter, const char *raw)\n {\n-\tstruct strbuf buf = STRBUF_INIT;\n-\tstrbuf_addstr_urlencode(&buf, raw, allow_unencoded);\n-\ttrace_printf(\"Add to combine filter-spec: %s\\n\", buf.buf);\n-\tstring_list_append_nodup(&filter->filter_spec, strbuf_detach(&buf, NULL));\n+\tsize_t orig_len = filter->filter_spec.len;\n+\tstrbuf_addstr_urlencode(&filter->filter_spec, raw, allow_unencoded);\n+\ttrace_printf(\"Add to combine filter-spec: %s\\n\",\n+\t\t     filter->filter_spec.buf + orig_len);\n }\n \n /*\n@@ -229,15 +229,15 @@ static void transform_to_combine_type(\n \t}\n \tfilter_options->sub_nr = 1;\n \tfilter_options->choice = LOFC_COMBINE;\n-\tstring_list_append(&filter_options->filter_spec, \"combine:\");\n+\tstrbuf_addstr(&filter_options->filter_spec, \"combine:\");\n \tfilter_spec_append_urlencode(\n \t\tfilter_options,\n \t\tlist_objects_filter_spec(&filter_options->sub[0]));\n \t/*\n \t * We don't need the filter_spec strings for subfilter specs, only the\n \t * top level.\n \t */\n-\tstring_list_clear(&filter_options->sub[0].filter_spec, /*free_util=*/0);\n+\tstrbuf_release(&filter_options->sub[0].filter_spec);\n }\n \n void list_objects_filter_die_if_populated(\n@@ -254,11 +254,11 @@ void parse_list_objects_filter(\n \tstruct strbuf errbuf = STRBUF_INIT;\n \tint parse_error;\n \n-\tif (!filter_options->filter_spec.strdup_strings)\n+\tif (!filter_options->filter_spec.buf)\n \t\tBUG(\"filter_options not properly initialized\");\n \n \tif (!filter_options->choice) {\n-\t\tstring_list_append(&filter_options->filter_spec, arg);\n+\t\tstrbuf_addstr(&filter_options->filter_spec, arg);\n \n \t\tparse_error = gently_parse_list_objects_filter(\n \t\t\tfilter_options, arg, &errbuf);\n@@ -269,7 +269,7 @@ void parse_list_objects_filter(\n \t\t */\n \t\ttransform_to_combine_type(filter_options);\n \n-\t\tstring_list_append(&filter_options->filter_spec, \"+\");\n+\t\tstrbuf_addch(&filter_options->filter_spec, '+');\n \t\tfilter_spec_append_urlencode(filter_options, arg);\n \t\tALLOC_GROW_BY(filter_options->sub, filter_options->sub_nr, 1,\n \t\t\t      filter_options->sub_alloc);\n@@ -300,31 +300,18 @@ int opt_parse_list_objects_filter(const struct option *opt,\n \n const char *list_objects_filter_spec(struct list_objects_filter_options *filter)\n {\n-\tif (!filter->filter_spec.nr)\n+\tif (!filter->filter_spec.len)\n \t\tBUG(\"no filter_spec available for this filter\");\n-\tif (filter->filter_spec.nr != 1) {\n-\t\tstruct strbuf concatted = STRBUF_INIT;\n-\t\tstrbuf_add_separated_string_list(\n-\t\t\t&concatted, \"\", &filter->filter_spec);\n-\t\tstring_list_clear(&filter->filter_spec, /*free_util=*/0);\n-\t\tstring_list_append_nodup(\n-\t\t\t&filter->filter_spec, strbuf_detach(&concatted, NULL));\n-\t}\n-\n-\treturn filter->filter_spec.items[0].string;\n+\treturn filter->filter_spec.buf;\n }\n \n const char *expand_list_objects_filter_spec(\n \tstruct list_objects_filter_options *filter)\n {\n \tif (filter->choice == LOFC_BLOB_LIMIT) {\n-\t\tstruct strbuf expanded_spec = STRBUF_INIT;\n-\t\tstrbuf_addf(&expanded_spec, \"blob:limit=%lu\",\n+\t\tstrbuf_release(&filter->filter_spec);\n+\t\tstrbuf_addf(&filter->filter_spec, \"blob:limit=%lu\",\n \t\t\t    filter->blob_limit_value);\n-\t\tstring_list_clear(&filter->filter_spec, /*free_util=*/0);\n-\t\tstring_list_append_nodup(\n-\t\t\t&filter->filter_spec,\n-\t\t\tstrbuf_detach(&expanded_spec, NULL));\n \t}\n \n \treturn list_objects_filter_spec(filter);\n@@ -337,7 +324,7 @@ void list_objects_filter_release(\n \n \tif (!filter_options)\n \t\treturn;\n-\tstring_list_clear(&filter_options->filter_spec, /*free_util=*/0);\n+\tstrbuf_release(&filter_options->filter_spec);\n \tfree(filter_options->sparse_oid_name);\n \tfor (sub = 0; sub < filter_options->sub_nr; sub++)\n \t\tlist_objects_filter_release(&filter_options->sub[sub]);\n@@ -398,8 +385,8 @@ void partial_clone_get_default_filter_spec(\n \tif (!promisor || !promisor->partial_clone_filter)\n \t\treturn;\n \n-\tstring_list_append(&filter_options->filter_spec,\n-\t\t\t   promisor->partial_clone_filter);\n+\tstrbuf_addstr(&filter_options->filter_spec,\n+\t\t      promisor->partial_clone_filter);\n \tgently_parse_list_objects_filter(filter_options,\n \t\t\t\t\t promisor->partial_clone_filter,\n \t\t\t\t\t &errbuf);\n@@ -411,14 +398,12 @@ void list_objects_filter_copy(\n \tconst struct list_objects_filter_options *src)\n {\n \tint i;\n-\tstruct string_list_item *item;\n \n \t/* Copy everything. We will overwrite the pointers shortly. */\n \tmemcpy(dest, src, sizeof(struct list_objects_filter_options));\n \n-\tstring_list_init_dup(&dest->filter_spec);\n-\tfor_each_string_list_item(item, &src->filter_spec)\n-\t\tstring_list_append(&dest->filter_spec, item->string);\n+\tstrbuf_init(&dest->filter_spec, 0);\n+\tstrbuf_addbuf(&dest->filter_spec, &src->filter_spec);\n \tdest->sparse_oid_name = xstrdup_or_null(src->sparse_oid_name);\n \n \tALLOC_ARRAY(dest->sub, dest->sub_alloc);\ndiff --git a/list-objects-filter-options.h b/list-objects-filter-options.h\nindex 2720f7dba8..7eeadab2dd 100644\n--- a/list-objects-filter-options.h\n+++ b/list-objects-filter-options.h\n@@ -35,7 +35,7 @@ struct list_objects_filter_options {\n \t * To get the raw filter spec given by the user, use the result of\n \t * list_objects_filter_spec().\n \t */\n-\tstruct string_list filter_spec;\n+\tstruct strbuf filter_spec;\n \n \t/*\n \t * 'choice' is determined by parsing the filter-spec.  This indicates\n@@ -69,7 +69,7 @@ struct list_objects_filter_options {\n \t */\n };\n \n-#define LIST_OBJECTS_FILTER_INIT { .filter_spec = STRING_LIST_INIT_DUP }\n+#define LIST_OBJECTS_FILTER_INIT { .filter_spec = STRBUF_INIT }\n void list_objects_filter_init(struct list_objects_filter_options *filter_options);\n \n /*\n-- \n2.37.3.1242.g835d375f85\n"},{"id":"462908","messageId":"Yx1tD7MLGZnZ/RHa@coredump.intra.peff.net","threadId":"58414","inReplyTo":"Yx1qwlpNbxGEiUgc@coredump.intra.peff.net","subject":"Re: [PATCH 0/4] list-objects-filter cleanups","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-09-11T05:07:27Z","receivedAt":"2022-09-11T05:07:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Sep 11, 2022 at 12:57:39AM -0400, Jeff King wrote:\n\n> These patches should be applied on top of jk/plug-list-object-filter-leaks,\n> which is currently in next.\n> \n>   [1/4]: list-objects-filter: don't memset after releasing filter struct\n>   [2/4]: list-objects-filter: handle null default filter spec\n>   [3/4]: list-objects-filter: add and use initializers\n>   [4/4]: list-objects-filter: convert filter_spec to a strbuf\n\nBy the way, there are two small conflicts when merging the result to\nnext. In ab/submodule-helper-leakfix, some memset() calls got moved to\n\"{ 0 }\" initializers in the variable declarations. The resolution is to\njust replace the \"{ 0 }\" with LIST_OBJECTS_FILTER_INIT (both sides\nremove the memset calls).\n\n-Peff\n"},{"id":"462935","messageId":"xmqq5yhsha8v.fsf@gitster.g","threadId":"58414","inReplyTo":"Yx1q4crkHGNzwLhh@coredump.intra.peff.net","subject":"Re: [PATCH 1/4] list-objects-filter: don't memset after releasing filter struct","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-12T15:40:16Z","receivedAt":"2022-09-12T15:40:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> If we see an error while parsing a \"combine\" filter, we call\n> list_objects_filter_release() to free any allocated memory,\n> and then use memset() to return the struct to a known state. But the\n> release function already does that reinitializing. Doing it again is\n> pointless.\n\nMakes sense.\n\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  list-objects-filter-options.c | 4 +---\n>  1 file changed, 1 insertion(+), 3 deletions(-)\n>\n> diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c\n> index 6cc4eb8e1c..ea989db260 100644\n> --- a/list-objects-filter-options.c\n> +++ b/list-objects-filter-options.c\n> @@ -187,10 +187,8 @@ static int parse_combine_filter(\n>  \n>  cleanup:\n>  \tstrbuf_list_free(subspecs);\n> -\tif (result) {\n> +\tif (result)\n>  \t\tlist_objects_filter_release(filter_options);\n> -\t\tmemset(filter_options, 0, sizeof(*filter_options));\n> -\t}\n>  \treturn result;\n>  }\n"},{"id":"462939","messageId":"xmqqo7vkfs58.fsf@gitster.g","threadId":"58414","inReplyTo":"Yx1rfYtQAlZGfAak@coredump.intra.peff.net","subject":"Re: [PATCH 2/4] list-objects-filter: handle null default filter spec","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-12T16:56:35Z","receivedAt":"2022-09-12T16:56:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> But it's probably worth avoiding this confused state. It's an accident\n> waiting to happen, and it will be a problem if we replace the lazy\n> initialization from 7e2619d8ff (list_objects_filter_options: plug leak\n> of filter_spec strings, 2022-09-08) with a real initialization function.\n\nTrue.  Thanks for a careful analysis.\n"},{"id":"463211","messageId":"2c6fca96-6b7b-b4c4-526c-2497422d8b8f@github.com","threadId":"58414","inReplyTo":"Yx1sI8cjU/SWUKVy@coredump.intra.peff.net","subject":"Re: [PATCH 4/4] list-objects-filter: convert filter_spec to a strbuf","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-09-19T17:50:32Z","receivedAt":"2022-09-19T17:50:44Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/11/2022 1:03 AM, Jeff King wrote:\n> -\tif (!filter_options->filter_spec.strdup_strings)\n> +\tif (!filter_options->filter_spec.buf)\n>  \t\tBUG(\"filter_options not properly initialized\");\n\nI couldn't figure out why this would work until I dug into\nSTRBUF_INIT and found this:\n\n/*\n * Used as the default ->buf value, so that people can always assume\n * buf is non NULL and ->buf is NUL terminated even for a freshly\n * initialized strbuf.\n */\nchar strbuf_slopbuf[1];\n\nSo, this makes sense now.\n\n> -#define LIST_OBJECTS_FILTER_INIT { .filter_spec = STRING_LIST_INIT_DUP }\n> +#define LIST_OBJECTS_FILTER_INIT { .filter_spec = STRBUF_INIT }\n\nAnd now this macro swap works, too.\n\nThanks,\n-Stolee\n\n"},{"id":"463212","messageId":"a0d692da-9910-9c43-d710-6cfe739ce09e@github.com","threadId":"58414","inReplyTo":"Yx1qwlpNbxGEiUgc@coredump.intra.peff.net","subject":"Re: [PATCH 0/4] list-objects-filter cleanups","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-09-19T17:51:29Z","receivedAt":"2022-09-19T17:52:34Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/11/2022 12:57 AM, Jeff King wrote:\n> In the recent jk/plug-list-object-filter-leaks topic[1], I stopped short\n> of fixing all of the callers to actually initialize the filter struct\n> beyond zero-ing it.\n> \n> This series does the cleanup that I was afraid to do there. :)\n> \n> I think the end result is less confusing and error-prone. And as you can\n> see in patch 4, it matches how the code originally hoped to be written,\n> but the author was also afraid of the zero-initialization thing.\n> \n> It is kind of churny, and carries some risk of regression (if I missed a\n> spot). IMHO it's worth it, but even if we don't take it, we should pick\n> up the first two patches, which are small bug-lets that the conversion\n> turned up.\n\nI agree that it's worth it. Hopefully the BUG() you inserted is sufficient\nto catch any regression in CI.\n\nThanks,\n-Stolee\n"},{"id":"463224","messageId":"Yyi9lkns3OgjiLl0@coredump.intra.peff.net","threadId":"58414","inReplyTo":"2c6fca96-6b7b-b4c4-526c-2497422d8b8f@github.com","subject":"Re: [PATCH 4/4] list-objects-filter: convert filter_spec to a strbuf","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-09-19T19:05:58Z","receivedAt":"2022-09-19T19:06:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 19, 2022 at 01:50:32PM -0400, Derrick Stolee wrote:\n\n> On 9/11/2022 1:03 AM, Jeff King wrote:\n> > -\tif (!filter_options->filter_spec.strdup_strings)\n> > +\tif (!filter_options->filter_spec.buf)\n> >  \t\tBUG(\"filter_options not properly initialized\");\n> \n> I couldn't figure out why this would work until I dug into\n> STRBUF_INIT and found this:\n> \n> /*\n>  * Used as the default ->buf value, so that people can always assume\n>  * buf is non NULL and ->buf is NUL terminated even for a freshly\n>  * initialized strbuf.\n>  */\n> char strbuf_slopbuf[1];\n> \n> So, this makes sense now.\n\nYeah. It's a little intimate with the strbuf implementation, but I think\nthat's OK given the scope of things here. Ironically, if \"buf\" could be\nNULL, then we'd actually be OK with the original zero-initialization. :)\n\n-Peff\n"}]}