{"thread":{"id":"58795","subject":"[PATCH 0/3] pack-objects: fix and simplify --filter handling","startedAt":"2022-11-12T10:42:54Z","lastAt":"2022-11-30T11:24:47Z","messageCount":52,"participants":["René Scharfe","Ævar Arnfjörð Bjarmason","Jeff King","Taylor Blau","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"467192","messageId":"c64e4fa5-62c2-2a93-a4ef-bd84407ea570@web.de","threadId":"58795","inReplyTo":null,"subject":"[PATCH 0/3] pack-objects: fix and simplify --filter handling","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-11-12T10:42:42Z","receivedAt":"2022-11-12T10:42:54Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Fix a regression that prevents using multiple --filter options, simplify\nthe option parsing code and avoid relying on undefined behavior in it.\n\nEffectively almost reverts 5cb28270a1 (pack-objects: lazily set up\n\"struct rev_info\", don't leak, 2022-03-28), which introduced these\nshortcomings.\n\nPatch 2 conflicts with cc/filtered-repack in seen, but not semantically.\n\n pack-objects: fix handling of multiple --filter options\n pack-object: simplify --filter handling\n list-objects-filter: remove OPT_PARSE_LIST_OBJECTS_FILTER_INIT()\n\n builtin/pack-objects.c | 27 ++++++--------------------\n list-objects-filter-options.c | 4 ----\n list-objects-filter-options.h | 18 ++---------------\n t/t5317-pack-objects-filter-objects.sh | 19 ++++++++++++++++++\n 4 files changed, 27 insertions(+), 41 deletions(-)\n\n--\n2.38.1\n"},{"id":"467193","messageId":"53bffbf4-8308-0dd7-bca5-7c85b8334e05@web.de","threadId":"58795","inReplyTo":"c64e4fa5-62c2-2a93-a4ef-bd84407ea570@web.de","subject":"[PATCH 1/3] pack-objects: fix handling of multiple --filter options","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-11-12T10:44:00Z","receivedAt":"2022-11-12T10:44:11Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Since 5cb28270a1 (pack-objects: lazily set up \"struct rev_info\", don't\nleak, 2022-03-28) --filter options given to git pack-objects overrule\nearlier ones, letting only the leftmost win and leaking the memory\nallocated for earlier ones. Fix that by only initializing the rev_info\nstruct once.\n\nSigned-off-by: René Scharfe \n---\n builtin/pack-objects.c | 3 ++-\n t/t5317-pack-objects-filter-objects.sh | 19 +++++++++++++++++++\n 2 files changed, 21 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 573d0b20b7..c702c09dd4 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -4158,7 +4158,8 @@ static struct list_objects_filter_options *po_filter_revs_init(void *value)\n {\n \tstruct po_filter_data *data = value;\n\n-\trepo_init_revisions(the_repository, &data->revs, NULL);\n+\tif (!data->have_revs)\n+\t\trepo_init_revisions(the_repository, &data->revs, NULL);\n \tdata->have_revs = 1;\n\n \treturn &data->revs.filter;\ndiff --git a/t/t5317-pack-objects-filter-objects.sh b/t/t5317-pack-objects-filter-objects.sh\nindex bb633c9b09..bd8983bb56 100755\n--- a/t/t5317-pack-objects-filter-objects.sh\n+++ b/t/t5317-pack-objects-filter-objects.sh\n@@ -178,6 +178,25 @@ test_expect_success 'verify blob:limit=1001' '\n \ttest_cmp expected observed\n '\n\n+test_expect_success 'verify blob:limit=1001+object:type=blob' '\n+\tgit -C r2 ls-files -s large.1000 |\n+\ttest_parse_ls_files_stage_oids |\n+\tsort >expected &&\n+\n+\tgit -C r2 pack-objects --revs --stdout --filter=blob:limit=1001 \\\n+\t\t--filter=object:type=blob >filter.pack <<-EOF &&\n+\tHEAD\n+\tEOF\n+\tgit -C r2 index-pack ../filter.pack &&\n+\n+\tgit -C r2 verify-pack -v ../filter.pack >verify_result &&\n+\tgrep blob verify_result |\n+\tparse_verify_pack_blob_oid |\n+\tsort >observed &&\n+\n+\ttest_cmp expected observed\n+'\n+\n test_expect_success 'verify blob:limit=10001' '\n \tgit -C r2 ls-files -s large.1000 large.10000 |\n \ttest_parse_ls_files_stage_oids |\n--\n2.38.1\n"},{"id":"467194","messageId":"59abb8d2-e8e6-7423-ead8-49382293aaa6@web.de","threadId":"58795","inReplyTo":"c64e4fa5-62c2-2a93-a4ef-bd84407ea570@web.de","subject":"[PATCH 2/3] pack-object: simplify --filter handling","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-11-12T10:44:50Z","receivedAt":"2022-11-12T10:44:58Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"pack-objects uses OPT_PARSE_LIST_OBJECTS_FILTER_INIT() to initialize the\na rev_info struct lazily before populating its filter member using the\n--filter option values. It tracks whether the initialization is needed\nusing the .have_revs member of the callback data.\n\nThere is a better way: Use a stand-alone list_objects_filter_options\nstruct and build a rev_info struct with its .filter member after option\nparsing. This allows using the simpler OPT_PARSE_LIST_OBJECTS_FILTER()\nand getting rid of the extra callback mechanism.\n\nSigned-off-by: René Scharfe \n---\n builtin/pack-objects.c | 28 ++++++----------------------\n 1 file changed, 6 insertions(+), 22 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex c702c09dd4..30023bcebb 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -4149,22 +4149,6 @@ static int option_parse_cruft_expiration(const struct option *opt,\n \treturn 0;\n }\n\n-struct po_filter_data {\n-\tunsigned have_revs:1;\n-\tstruct rev_info revs;\n-};\n-\n-static struct list_objects_filter_options *po_filter_revs_init(void *value)\n-{\n-\tstruct po_filter_data *data = value;\n-\n-\tif (!data->have_revs)\n-\t\trepo_init_revisions(the_repository, &data->revs, NULL);\n-\tdata->have_revs = 1;\n-\n-\treturn &data->revs.filter;\n-}\n-\n int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n {\n \tint use_internal_rev_list = 0;\n@@ -4175,7 +4159,8 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \tint rev_list_index = 0;\n \tint stdin_packs = 0;\n \tstruct string_list keep_pack_list = STRING_LIST_INIT_NODUP;\n-\tstruct po_filter_data pfd = { .have_revs = 0 };\n+\tstruct list_objects_filter_options filter_options =\n+\t\tLIST_OBJECTS_FILTER_INIT;\n\n \tstruct option pack_objects_options[] = {\n \t\tOPT_SET_INT('q', \"quiet\", &progress,\n@@ -4266,7 +4251,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \t\t\t &write_bitmap_index,\n \t\t\t N_(\"write a bitmap index if possible\"),\n \t\t\t WRITE_BITMAP_QUIET, PARSE_OPT_HIDDEN),\n-\t\tOPT_PARSE_LIST_OBJECTS_FILTER_INIT(&pfd, po_filter_revs_init),\n+\t\tOPT_PARSE_LIST_OBJECTS_FILTER(&filter_options),\n \t\tOPT_CALLBACK_F(0, \"missing\", NULL, N_(\"action\"),\n \t\t N_(\"handling for missing objects\"), PARSE_OPT_NONEG,\n \t\t option_parse_missing_action),\n@@ -4386,7 +4371,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \tif (!rev_list_all || !rev_list_reflog || !rev_list_index)\n \t\tunpack_unreachable_expiration = 0;\n\n-\tif (pfd.have_revs && pfd.revs.filter.choice) {\n+\tif (filter_options.choice) {\n \t\tif (!pack_to_stdout)\n \t\t\tdie(_(\"cannot use --filter without --stdout\"));\n \t\tif (stdin_packs)\n@@ -4473,16 +4458,15 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \t\tread_cruft_objects();\n \t} else if (!use_internal_rev_list) {\n \t\tread_object_list_from_stdin();\n-\t} else if (pfd.have_revs) {\n-\t\tget_object_list(&pfd.revs, rp.nr, rp.v);\n-\t\trelease_revisions(&pfd.revs);\n \t} else {\n \t\tstruct rev_info revs;\n\n \t\trepo_init_revisions(the_repository, &revs, NULL);\n+\t\tlist_objects_filter_copy(&revs.filter, &filter_options);\n \t\tget_object_list(&revs, rp.nr, rp.v);\n \t\trelease_revisions(&revs);\n \t}\n+\tlist_objects_filter_release(&filter_options);\n \tcleanup_preferred_base();\n \tif (include_tag && nr_result)\n \t\tfor_each_tag_ref(add_ref_tag, NULL);\n--\n2.38.1\n"},{"id":"467195","messageId":"550dd5c9-f174-5494-70eb-5e217b011dd6@web.de","threadId":"58795","inReplyTo":"c64e4fa5-62c2-2a93-a4ef-bd84407ea570@web.de","subject":"[PATCH 3/3] list-objects-filter: remove OPT_PARSE_LIST_OBJECTS_FILTER_INIT()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-11-12T10:46:36Z","receivedAt":"2022-11-12T10:46:46Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"OPT_PARSE_LIST_OBJECTS_FILTER_INIT() with a non-NULL second argument\npasses a function pointer via an object pointer, which is undefined. It\nmay work fine on platforms that implement C99 extension J.5.7 (Function\npointer casts). Remove the unused macro and avoid the dependency on\nthat extension.\n\nSigned-off-by: René Scharfe \n---\n list-objects-filter-options.c | 4 ----\n list-objects-filter-options.h | 18 ++----------------\n 2 files changed, 2 insertions(+), 20 deletions(-)\n\ndiff --git a/list-objects-filter-options.c b/list-objects-filter-options.c\nindex 5339660238..ee01bcd2cc 100644\n--- a/list-objects-filter-options.c\n+++ b/list-objects-filter-options.c\n@@ -290,10 +290,6 @@ int opt_parse_list_objects_filter(const struct option *opt,\n \t\t\t\t const char *arg, int unset)\n {\n \tstruct list_objects_filter_options *filter_options = opt->value;\n-\topt_lof_init init = (opt_lof_init)opt->defval;\n-\n-\tif (init)\n-\t\tfilter_options = init(opt->value);\n\n \tif (unset || !arg)\n \t\tlist_objects_filter_set_no_filter(filter_options);\ndiff --git a/list-objects-filter-options.h b/list-objects-filter-options.h\nindex 7eeadab2dd..1fe393f447 100644\n--- a/list-objects-filter-options.h\n+++ b/list-objects-filter-options.h\n@@ -111,27 +111,13 @@ void parse_list_objects_filter(\n * The opt->value to opt_parse_list_objects_filter() is either a\n * \"struct list_objects_filter_option *\" when using\n * OPT_PARSE_LIST_OBJECTS_FILTER().\n- *\n- * Or, if using no \"struct option\" field is used by the callback,\n- * except the \"defval\" which is expected to be an \"opt_lof_init\"\n- * function, which is called with the \"opt->value\" and must return a\n- * pointer to the \"\"struct list_objects_filter_option *\" to be used.\n- *\n- * The OPT_PARSE_LIST_OBJECTS_FILTER_INIT() can be used e.g. the\n- * \"struct list_objects_filter_option\" is embedded in a \"struct\n- * rev_info\", which the \"defval\" could be tasked with lazily\n- * initializing. See cmd_pack_objects() for an example.\n */\n int opt_parse_list_objects_filter(const struct option *opt,\n \t\t\t\t const char *arg, int unset);\n-typedef struct list_objects_filter_options *(*opt_lof_init)(void *);\n-#define OPT_PARSE_LIST_OBJECTS_FILTER_INIT(fo, init) \\\n-\t{ OPTION_CALLBACK, 0, \"filter\", (fo), N_(\"args\"), \\\n-\t N_(\"object filtering\"), 0, opt_parse_list_objects_filter, \\\n-\t (intptr_t)(init) }\n\n #define OPT_PARSE_LIST_OBJECTS_FILTER(fo) \\\n-\tOPT_PARSE_LIST_OBJECTS_FILTER_INIT((fo), NULL)\n+\tOPT_CALLBACK(0, \"filter\", (fo), N_(\"args\"), \\\n+\t\t N_(\"object filtering\"), opt_parse_list_objects_filter)\n\n /*\n * Translates abbreviated numbers in the filter's filter_spec into their\n--\n2.38.1\n"},{"id":"467197","messageId":"221112.86bkpcmm6i.gmgdl@evledraar.gmail.com","threadId":"58795","inReplyTo":"53bffbf4-8308-0dd7-bca5-7c85b8334e05@web.de","subject":"Re: [PATCH 1/3] pack-objects: fix handling of multiple --filter options","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-12T11:41:13Z","receivedAt":"2022-11-12T11:44:59Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sat, Nov 12 2022, René Scharfe wrote:\n\n> Since 5cb28270a1 (pack-objects: lazily set up \"struct rev_info\", don't\n> leak, 2022-03-28) --filter options given to git pack-objects overrule\n> earlier ones, letting only the leftmost win and leaking the memory\n> allocated for earlier ones. Fix that by only initializing the rev_info\n> struct once.\n\nIf I do e.g. this with SANITIZE=leak:\n\n\techo e83c5163316f89bfbde7d9ab23ca2e25604af290 | ./git pack-objects --revs --filter=blob:limit=1001 --filter=object:type=blob --stdout >/dev/null\n\nI see one leak that wasn't there, but two that are gone now. I haven't\nlooked into it, but I think the commit message should discuss what leaks\nare fixed, which remain/are new etc.\n\n> @@ -4158,7 +4158,8 @@ static struct list_objects_filter_options *po_filter_revs_init(void *value)\n> {\n> \tstruct po_filter_data *data = value;\n>\n> -\trepo_init_revisions(the_repository, &data->revs, NULL);\n> +\tif (!data->have_revs)\n> +\t\trepo_init_revisions(the_repository, &data->revs, NULL);\n> \tdata->have_revs = 1;\n>\n> \treturn &data->revs.filter;\n\nFWIW as this goes away in your 2/3 I think just squashing the two with a\nleak fix would be nice, if...\n\n> diff --git a/t/t5317-pack-objects-filter-objects.sh b/t/t5317-pack-objects-filter-objects.sh\n> index bb633c9b09..bd8983bb56 100755\n> --- a/t/t5317-pack-objects-filter-objects.sh\n> +++ b/t/t5317-pack-objects-filter-objects.sh\n> @@ -178,6 +178,25 @@ test_expect_success 'verify blob:limit=1001' '\n> \ttest_cmp expected observed\n> '\n>\n> +test_expect_success 'verify blob:limit=1001+object:type=blob' '\n> +\tgit -C r2 ls-files -s large.1000 |\n\nAside: Should do \"git >tmp && test_parse... +\ttest_parse_ls_files_stage_oids |\n> +\tsort >expected &&\n> +\n> +\tgit -C r2 pack-objects --revs --stdout --filter=blob:limit=1001 \\\n> +\t\t--filter=object:type=blob >filter.pack <<-EOF &&\n> +\tHEAD\n> +\tEOF\n> +\tgit -C r2 index-pack ../filter.pack &&\n> +\n> +\tgit -C r2 verify-pack -v ../filter.pack >verify_result &&\n> +\tgrep blob verify_result |\n> +\tparse_verify_pack_blob_oid |\n\nWhereas this one's not a problem, no \"git\".\n\n> +\tsort >observed &&\n> +\n> +\ttest_cmp expected observed\n\nAside: It would be nice if we had a \"test_cmp_sort\", but some other day...\n\n> +'\n> +\n> test_expect_success 'verify blob:limit=10001' '\n> \tgit -C r2 ls-files -s large.1000 large.10000 |\n> \ttest_parse_ls_files_stage_oids |\n\n...we can test it, but this test is in a file that's not part of \"linux-leaks\".\n\nIf that one leak I mentioned above can be fixed (or maybe it's not new?)\nthis could be tested if we put it in a file of its own with\nTEST_PASSES_SANITIZE_LEAK=true.\n"},{"id":"467198","messageId":"221112.867d00mlv9.gmgdl@evledraar.gmail.com","threadId":"58795","inReplyTo":"59abb8d2-e8e6-7423-ead8-49382293aaa6@web.de","subject":"Re: [PATCH 2/3] pack-object: simplify --filter handling","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-12T11:45:08Z","receivedAt":"2022-11-12T11:51:45Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sat, Nov 12 2022, René Scharfe wrote:\n\n> pack-objects uses OPT_PARSE_LIST_OBJECTS_FILTER_INIT() to initialize the\n> a rev_info struct lazily before populating its filter member using the\n> --filter option values. It tracks whether the initialization is needed\n> using the .have_revs member of the callback data.\n>\n> There is a better way: Use a stand-alone list_objects_filter_options\n> struct and build a rev_info struct with its .filter member after option\n> parsing. This allows using the simpler OPT_PARSE_LIST_OBJECTS_FILTER()\n> and getting rid of the extra callback mechanism.\n\nI haven't gone through the history again, but as I recall this used to\nbe outside of \"struct rev_info\", then (I think I did, as part of a leak\nfix) it was put there, and now it's still there, but we make a copy of\nit, then copy it into the \"rev_info\" struct again.\n\nMaybe we've finally reached the right trade-off here, but it would help\nto discuss some of those past commits, why those arrangements were\nwrong, and why this is finally the right one...\n"},{"id":"467203","messageId":"Y2/QxOnM/7UI4R8c@coredump.intra.peff.net","threadId":"58795","inReplyTo":"53bffbf4-8308-0dd7-bca5-7c85b8334e05@web.de","subject":"Re: [PATCH 1/3] pack-objects: fix handling of multiple --filter options","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-11-12T16:58:44Z","receivedAt":"2022-11-12T16:58:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Nov 12, 2022 at 11:44:00AM +0100, René Scharfe wrote:\n\n> Since 5cb28270a1 (pack-objects: lazily set up \"struct rev_info\", don't\n> leak, 2022-03-28) --filter options given to git pack-objects overrule\n> earlier ones, letting only the leftmost win and leaking the memory\n> allocated for earlier ones. Fix that by only initializing the rev_info\n> struct once.\n\nYikes. I wondered how we managed to miss this in the tests. Surely we\nhave some examples that use multiple filters?\n\nI think the answer is that we do in t5616, but in practice they didn't\nhit this bug because of the way filter-specs are treated server-sid3.\nEven if we say \"clone --filter=foo --filter=bar\" on the client, the\nserver upload-pack coalesces that into a single \"combine:foo+bar\"\nstring, and that's what its pack-objects sees.\n\nYour test here triggers it by invoking pack-objects directly, which\nmakes sense. That's a little different from how pack-objects is invoked\nby the rest of Git in practice, but it's definitely something that\n_should_ work. It's conceivable that somebody outside of Git could be\ndriving pack-objects directly, but also it's just a trap waiting to\nspring for future developers.\n\n-Peff\n"},{"id":"467204","messageId":"Y2/Rwvb2uiE40rW0@coredump.intra.peff.net","threadId":"58795","inReplyTo":"59abb8d2-e8e6-7423-ead8-49382293aaa6@web.de","subject":"Re: [PATCH 2/3] pack-object: simplify --filter handling","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-11-12T17:02:58Z","receivedAt":"2022-11-12T17:03:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Nov 12, 2022 at 11:44:50AM +0100, René Scharfe wrote:\n\n> pack-objects uses OPT_PARSE_LIST_OBJECTS_FILTER_INIT() to initialize the\n> a rev_info struct lazily before populating its filter member using the\n> --filter option values. It tracks whether the initialization is needed\n> using the .have_revs member of the callback data.\n> \n> There is a better way: Use a stand-alone list_objects_filter_options\n> struct and build a rev_info struct with its .filter member after option\n> parsing. This allows using the simpler OPT_PARSE_LIST_OBJECTS_FILTER()\n> and getting rid of the extra callback mechanism.\n\nThat seems like a reasonable fix (and I think we do it elsewhere). But I\nwonder if it wouldn't be simpler to just unconditionally initialize the\nrev_info in cmd_pack_objects(), and then unconditionally release it.\nTrue, we _might_ not use it if we are receiving objects directly over\nstdin, but I don't think initializing it is particularly costly.\n\nAnd then we don't have to worry about whether it's valid (it always is),\nor about copying between two representations of the filter (there's only\none).\n\n-Peff\n"},{"id":"467214","messageId":"Y3B6PxNaZvmm5OD2@nand.local","threadId":"58795","inReplyTo":"Y2/QxOnM/7UI4R8c@coredump.intra.peff.net","subject":"Re: [PATCH 1/3] pack-objects: fix handling of multiple --filter options","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-11-13T05:01:51Z","receivedAt":"2022-11-13T05:01:57Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Sat, Nov 12, 2022 at 11:58:44AM -0500, Jeff King wrote:\n> On Sat, Nov 12, 2022 at 11:44:00AM +0100, René Scharfe wrote:\n>\n> > Since 5cb28270a1 (pack-objects: lazily set up \"struct rev_info\", don't\n> > leak, 2022-03-28) --filter options given to git pack-objects overrule\n> > earlier ones, letting only the leftmost win and leaking the memory\n> > allocated for earlier ones. Fix that by only initializing the rev_info\n> > struct once.\n>\n> Yikes. I wondered how we managed to miss this in the tests. Surely we\n> have some examples that use multiple filters?\n>\n> I think the answer is that we do in t5616, but in practice they didn't\n> hit this bug because of the way filter-specs are treated server-sid3.\n> Even if we say \"clone --filter=foo --filter=bar\" on the client, the\n> server upload-pack coalesces that into a single \"combine:foo+bar\"\n> string, and that's what its pack-objects sees.\n\n...Or in other words, clients aren't broken here because we always send\n\"combine\" filters when multiple `--filter` arguments are passed to `git\nclone`, `git fetch`, etc.\n\nIs that always the case? I *think* so, but it would be nice to know if\nthere was a way that clients would not send the `combine` filter with >1\nfilter.\n\nThanks,\nTaylor\n"},{"id":"467218","messageId":"Y3Ee1o82JS6Lxr71@coredump.intra.peff.net","threadId":"58795","inReplyTo":"Y3B6PxNaZvmm5OD2@nand.local","subject":"Re: [PATCH 1/3] pack-objects: fix handling of multiple --filter options","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-11-13T16:44:06Z","receivedAt":"2022-11-13T16:44:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Nov 13, 2022 at 12:01:51AM -0500, Taylor Blau wrote:\n\n> > Yikes. I wondered how we managed to miss this in the tests. Surely we\n> > have some examples that use multiple filters?\n> >\n> > I think the answer is that we do in t5616, but in practice they didn't\n> > hit this bug because of the way filter-specs are treated server-sid3.\n> > Even if we say \"clone --filter=foo --filter=bar\" on the client, the\n> > server upload-pack coalesces that into a single \"combine:foo+bar\"\n> > string, and that's what its pack-objects sees.\n> \n> ...Or in other words, clients aren't broken here because we always send\n> \"combine\" filters when multiple `--filter` arguments are passed to `git\n> clone`, `git fetch`, etc.\n\nRight.\n\n> Is that always the case? I *think* so, but it would be nice to know if\n> there was a way that clients would not send the `combine` filter with >1\n> filter.\n\nI think it's always the case at least with Git's code, because the\nclient-side send_filter() sends from list_objects_filter_spec(), which\ntakes the single-string spec of the top-level combine filter. It would\nhave to explicitly iterate over the sub-filters to come up with anything\nelse, and I don't think we even have a function for that (we do when\nevaluating them, of course, but not for generically iterating or\nproducing a multi-string set of specs).\n\nAnd on the server side, even if a client did send multiple \"filter\"\nlines, I think we'd hit list_objects_filter_die_if_populated(), so we'd\ncomplain.\n\nI don't know of any other code paths that could hit this (can you filter\non an outgoing push?). But in the end I'm not sure it even matters. We\nshould be fixing the bug anyway, and this is all just understanding how\nfar-reaching its effects are (and the answer seems to be: not very).\n\n-Peff\n"},{"id":"467219","messageId":"Y3EgEBWoPo2YhYXe@coredump.intra.peff.net","threadId":"58795","inReplyTo":"Y2/Rwvb2uiE40rW0@coredump.intra.peff.net","subject":"Re: [PATCH 2/3] pack-object: simplify --filter handling","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-11-13T16:49:20Z","receivedAt":"2022-11-13T16:49:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Nov 12, 2022 at 12:02:58PM -0500, Jeff King wrote:\n\n> > There is a better way: Use a stand-alone list_objects_filter_options\n> > struct and build a rev_info struct with its .filter member after option\n> > parsing. This allows using the simpler OPT_PARSE_LIST_OBJECTS_FILTER()\n> > and getting rid of the extra callback mechanism.\n> \n> That seems like a reasonable fix (and I think we do it elsewhere). But I\n> wonder if it wouldn't be simpler to just unconditionally initialize the\n> rev_info in cmd_pack_objects(), and then unconditionally release it.\n> True, we _might_ not use it if we are receiving objects directly over\n> stdin, but I don't think initializing it is particularly costly.\n> \n> And then we don't have to worry about whether it's valid (it always is),\n> or about copying between two representations of the filter (there's only\n> one).\n\nJust to make sure I wasn't telling you absolute garbage, I sketched this\nout quickly, so sharing it below (to illustrate, or maybe to save you 5\nminutes if you want to go that direction).\n\nIf you do stick with copying the filter spec, yet another option is that\nthe rev_info variable could move back into get_object_list(), and we\ncould just pass in the filter_options. It was only moved out in\n80f6de4f5b (pack-objects: move revs out of get_object_list(),\n2022-03-22) so that we could parse --filter directly into it.\n\nThat's probably approaching bikeshed territory, and I'm happy enough\neither way, but it keeps the outer cmd_pack_objects() to the absolute\nminimum of what it needs to see.\n\n-Peff\n\n-- >8 --\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex c702c09dd4..cadc2be35f 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -4149,22 +4149,6 @@ static int option_parse_cruft_expiration(const struct option *opt,\n \treturn 0;\n }\n \n-struct po_filter_data {\n-\tunsigned have_revs:1;\n-\tstruct rev_info revs;\n-};\n-\n-static struct list_objects_filter_options *po_filter_revs_init(void *value)\n-{\n-\tstruct po_filter_data *data = value;\n-\n-\tif (!data->have_revs)\n-\t\trepo_init_revisions(the_repository, &data->revs, NULL);\n-\tdata->have_revs = 1;\n-\n-\treturn &data->revs.filter;\n-}\n-\n int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n {\n \tint use_internal_rev_list = 0;\n@@ -4175,7 +4159,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \tint rev_list_index = 0;\n \tint stdin_packs = 0;\n \tstruct string_list keep_pack_list = STRING_LIST_INIT_NODUP;\n-\tstruct po_filter_data pfd = { .have_revs = 0 };\n+\tstruct rev_info revs;\n \n \tstruct option pack_objects_options[] = {\n \t\tOPT_SET_INT('q', \"quiet\", &progress,\n@@ -4266,7 +4250,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \t\t\t &write_bitmap_index,\n \t\t\t N_(\"write a bitmap index if possible\"),\n \t\t\t WRITE_BITMAP_QUIET, PARSE_OPT_HIDDEN),\n-\t\tOPT_PARSE_LIST_OBJECTS_FILTER_INIT(&pfd, po_filter_revs_init),\n+\t\tOPT_PARSE_LIST_OBJECTS_FILTER(&revs.filter),\n \t\tOPT_CALLBACK_F(0, \"missing\", NULL, N_(\"action\"),\n \t\t N_(\"handling for missing objects\"), PARSE_OPT_NONEG,\n \t\t option_parse_missing_action),\n@@ -4297,6 +4281,8 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \tif (git_env_bool(GIT_TEST_WRITE_REV_INDEX, 0))\n \t\tpack_idx_opts.flags |= WRITE_REV;\n \n+\trepo_init_revisions(the_repository, &revs, NULL);\n+\n \tprogress = isatty(2);\n \targc = parse_options(argc, argv, prefix, pack_objects_options,\n \t\t\t pack_usage, 0);\n@@ -4386,7 +4372,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \tif (!rev_list_all || !rev_list_reflog || !rev_list_index)\n \t\tunpack_unreachable_expiration = 0;\n \n-\tif (pfd.have_revs && pfd.revs.filter.choice) {\n+\tif (revs.filter.choice) {\n \t\tif (!pack_to_stdout)\n \t\t\tdie(_(\"cannot use --filter without --stdout\"));\n \t\tif (stdin_packs)\n@@ -4473,16 +4459,10 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \t\tread_cruft_objects();\n \t} else if (!use_internal_rev_list) {\n \t\tread_object_list_from_stdin();\n-\t} else if (pfd.have_revs) {\n-\t\tget_object_list(&pfd.revs, rp.nr, rp.v);\n-\t\trelease_revisions(&pfd.revs);\n \t} else {\n-\t\tstruct rev_info revs;\n-\n-\t\trepo_init_revisions(the_repository, &revs, NULL);\n \t\tget_object_list(&revs, rp.nr, rp.v);\n-\t\trelease_revisions(&revs);\n \t}\n+\trelease_revisions(&revs);\n \tcleanup_preferred_base();\n \tif (include_tag && nr_result)\n \t\tfor_each_tag_ref(add_ref_tag, NULL);\n"},{"id":"467220","messageId":"0222bc92-bbea-6947-3523-cb10edece9bb@web.de","threadId":"58795","inReplyTo":"Y2/Rwvb2uiE40rW0@coredump.intra.peff.net","subject":"Re: [PATCH 2/3] pack-object: simplify --filter handling","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-11-13T17:31:03Z","receivedAt":"2022-11-13T17:31:24Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 12.11.22 um 18:02 schrieb Jeff King:\n> On Sat, Nov 12, 2022 at 11:44:50AM +0100, René Scharfe wrote:\n>\n>> pack-objects uses OPT_PARSE_LIST_OBJECTS_FILTER_INIT() to initialize the\n>> a rev_info struct lazily before populating its filter member using the\n>> --filter option values. It tracks whether the initialization is needed\n>> using the .have_revs member of the callback data.\n>>\n>> There is a better way: Use a stand-alone list_objects_filter_options\n>> struct and build a rev_info struct with its .filter member after option\n>> parsing. This allows using the simpler OPT_PARSE_LIST_OBJECTS_FILTER()\n>> and getting rid of the extra callback mechanism.\n>\n> That seems like a reasonable fix (and I think we do it elsewhere). But I\n> wonder if it wouldn't be simpler to just unconditionally initialize the\n> rev_info in cmd_pack_objects(), and then unconditionally release it.\n> True, we _might_ not use it if we are receiving objects directly over\n> stdin, but I don't think initializing it is particularly costly.\n>\n> And then we don't have to worry about whether it's valid (it always is),\n> or about copying between two representations of the filter (there's only\n> one).\n\nIndeed that's even simpler and what we had before 5cb28270a1\n(pack-objects: lazily set up \"struct rev_info\", don't leak, 2022-03-28).\n\nRené\n"},{"id":"467221","messageId":"be477f52-33e0-b2e8-9bcc-9264f1b50460@web.de","threadId":"58795","inReplyTo":"221112.86bkpcmm6i.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 1/3] pack-objects: fix handling of multiple --filter options","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-11-13T17:31:12Z","receivedAt":"2022-11-13T17:31:30Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 12.11.22 um 12:41 schrieb Ævar Arnfjörð Bjarmason:\n>\n> On Sat, Nov 12 2022, René Scharfe wrote:\n>\n>> Since 5cb28270a1 (pack-objects: lazily set up \"struct rev_info\", don't\n>> leak, 2022-03-28) --filter options given to git pack-objects overrule\n>> earlier ones, letting only the leftmost win and leaking the memory\n>> allocated for earlier ones. Fix that by only initializing the rev_info\n>> struct once.\n>\n> If I do e.g. this with SANITIZE=leak:\n>\n> \techo e83c5163316f89bfbde7d9ab23ca2e25604af290 | ./git pack-objects --revs --filter=blob:limit=1001 --filter=object:type=blob --stdout >/dev/null\n>\n> I see one leak that wasn't there, but two that are gone now. I haven't\n> looked into it, but I think the commit message should discuss what leaks\n> are fixed, which remain/are new etc.\n\nThe leak is insubstantial; I mentioned it just because of the irony. It\nis caused by initializing an already initialized struct rev_info without\nreleasing it in between, as mentioned in the commit message.\n\n.filter_data allocated by filter_combine__init() is not released by\nfilter_combine__init() in list-objects-filter.c. Plugging that leak\nallows your example command to run with SANITIZE=leak. That is a matter\nfor a separate patch, though.\n\n>\n>> @@ -4158,7 +4158,8 @@ static struct list_objects_filter_options *po_filter_revs_init(void *value)\n>> {\n>> \tstruct po_filter_data *data = value;\n>>\n>> -\trepo_init_revisions(the_repository, &data->revs, NULL);\n>> +\tif (!data->have_revs)\n>> +\t\trepo_init_revisions(the_repository, &data->revs, NULL);\n>> \tdata->have_revs = 1;\n>>\n>> \treturn &data->revs.filter;\n>\n> FWIW as this goes away in your 2/3 I think just squashing the two with a\n> leak fix would be nice, if...\n>\n>> diff --git a/t/t5317-pack-objects-filter-objects.sh b/t/t5317-pack-objects-filter-objects.sh\n>> index bb633c9b09..bd8983bb56 100755\n>> --- a/t/t5317-pack-objects-filter-objects.sh\n>> +++ b/t/t5317-pack-objects-filter-objects.sh\n>> @@ -178,6 +178,25 @@ test_expect_success 'verify blob:limit=1001' '\n>> \ttest_cmp expected observed\n>> '\n>>\n>> +test_expect_success 'verify blob:limit=1001+object:type=blob' '\n>> +\tgit -C r2 ls-files -s large.1000 |\n>\n> Aside: Should do \"git >tmp && test_parse... of \"ls-files\" here.\n\nOK. Copied that line from the surrounding tests. They used temporary\nfiles before fb2d0db502 (test-lib-functions: add parsing helpers for\nls-files and ls-tree, 2022-04-04).\n\n>\n>> +\ttest_parse_ls_files_stage_oids |\n>> +\tsort >expected &&\n>> +\n>> +\tgit -C r2 pack-objects --revs --stdout --filter=blob:limit=1001 \\\n>> +\t\t--filter=object:type=blob >filter.pack <<-EOF &&\n>> +\tHEAD\n>> +\tEOF\n>> +\tgit -C r2 index-pack ../filter.pack &&\n>> +\n>> +\tgit -C r2 verify-pack -v ../filter.pack >verify_result &&\n>> +\tgrep blob verify_result |\n>> +\tparse_verify_pack_blob_oid |\n>\n> Whereas this one's not a problem, no \"git\".\n>\n>> +\tsort >observed &&\n>> +\n>> +\ttest_cmp expected observed\n>\n> Aside: It would be nice if we had a \"test_cmp_sort\", but some other day...\n>\n>> +'\n>> +\n>> test_expect_success 'verify blob:limit=10001' '\n>> \tgit -C r2 ls-files -s large.1000 large.10000 |\n>> \ttest_parse_ls_files_stage_oids |\n>\n> ...we can test it, but this test is in a file that's not part of \"linux-leaks\".\n>\n> If that one leak I mentioned above can be fixed (or maybe it's not new?)\n> this could be tested if we put it in a file of its own with\n> TEST_PASSES_SANITIZE_LEAK=true.\n\nPlugging the leak in your example command is not enough to make t5317\nleak free.\n\nRené\n"},{"id":"467222","messageId":"6a6a9410-f854-2288-6d54-0aca115c0bb6@web.de","threadId":"58795","inReplyTo":"Y3B6PxNaZvmm5OD2@nand.local","subject":"Re: [PATCH 1/3] pack-objects: fix handling of multiple --filter options","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-11-13T17:31:09Z","receivedAt":"2022-11-13T17:31:35Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 13.11.22 um 06:01 schrieb Taylor Blau:\n> On Sat, Nov 12, 2022 at 11:58:44AM -0500, Jeff King wrote:\n>> On Sat, Nov 12, 2022 at 11:44:00AM +0100, René Scharfe wrote:\n>>\n>>> Since 5cb28270a1 (pack-objects: lazily set up \"struct rev_info\", don't\n>>> leak, 2022-03-28) --filter options given to git pack-objects overrule\n>>> earlier ones, letting only the leftmost win and leaking the memory\n>>> allocated for earlier ones. Fix that by only initializing the rev_info\n>>> struct once.\n>>\n>> Yikes. I wondered how we managed to miss this in the tests. Surely we\n>> have some examples that use multiple filters?\n>>\n>> I think the answer is that we do in t5616, but in practice they didn't\n>> hit this bug because of the way filter-specs are treated server-sid3.\n>> Even if we say \"clone --filter=foo --filter=bar\" on the client, the\n>> server upload-pack coalesces that into a single \"combine:foo+bar\"\n>> string, and that's what its pack-objects sees.\n>\n> ...Or in other words, clients aren't broken here because we always send\n> \"combine\" filters when multiple `--filter` arguments are passed to `git\n> clone`, `git fetch`, etc.\n>\n> Is that always the case? I *think* so, but it would be nice to know if\n> there was a way that clients would not send the `combine` filter with >1\n> filter.\n\nSearching for pack-objects invocations like this should at least find\nall internal cases, right?\n\n $ git grep -e '\"pack-objects\"' -e --filter --all-match --name-only\n builtin/pack-objects.c\n bundle.c\n upload-pack.c\n\ngit pack-objects handles --filter and doesn't pass it on. bundle.c and\nupload-pack.c only add at most a single --filter option to a strvec.\nbundle.c uses list_objects_filter_spec() to get the filter\nspecification, upload-pack.c uses expand_list_objects_filter_spec().\n\nRené\n"},{"id":"467613","messageId":"d19c6cb4-611f-afea-8a14-5e58d7509113@web.de","threadId":"58795","inReplyTo":"c64e4fa5-62c2-2a93-a4ef-bd84407ea570@web.de","subject":"[PATCH v2 0/3] pack-objects: fix and simplify --filter handling","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-11-20T10:03:35Z","receivedAt":"2022-11-20T10:06:21Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Fix a regression that prevents using multiple --filter options, simplify\nthe option parsing code and avoid relying on undefined behavior in it.\n\nPatch 3 conflicts with cc/filtered-repack in seen, but not semantically.\n\nChanges since v1:\n- Added patch 1 to fix an issue with existing tests.\n- Separate patch 2 for new tests.\n- Test using blob size filters, only, which is a bit simpler.\n- Test both combinations to also catch not just the current\n last-one-wins regression, but also a possible future first-one-wins\n issue.\n- Actually revert 5cb28270a1 (pack-objects: lazily set up\n \"struct rev_info\", don't leak, 2022-03-28) instead of having a\n minimal fix and then adding some kind of middle ground by using a\n separate struct list_objects_filter_options.\n\n t5317: stop losing return codes of git ls-files\n t5317: demonstrate failure to handle multiple --filter options\n Revert \"pack-objects: lazily set up \"struct rev_info\", don't leak\"\n\n builtin/pack-objects.c | 31 ++-------\n list-objects-filter-options.c | 4 --\n list-objects-filter-options.h | 24 +------\n t/t5317-pack-objects-filter-objects.sh | 90 +++++++++++++++++++-------\n 4 files changed, 75 insertions(+), 74 deletions(-)\n\nRange-Diff gegen v1:\n1: 2df1185b5f < -: ---------- pack-objects: fix handling of multiple --filter options\n2: b8e951fb4f < -: ---------- pack-object: simplify --filter handling\n3: 1e4ae7d9f1 < -: ---------- list-objects-filter: remove OPT_PARSE_LIST_OBJECTS_FILTER_INIT()\n-: ---------- > 1: 43af432f5c t5317: stop losing return codes of git ls-files\n-: ---------- > 2: a77b85e71d t5317: demonstrate failure to handle multiple --filter options\n-: ---------- > 3: 63ccb81357 Revert \"pack-objects: lazily set up \"struct rev_info\", don't leak\"\n--\n2.38.1\n"},{"id":"467614","messageId":"0c6722e6-e6bc-0f7d-5f0a-1d242359e37d@web.de","threadId":"58795","inReplyTo":"d19c6cb4-611f-afea-8a14-5e58d7509113@web.de","subject":"[PATCH v2 2/3] t5317: demonstrate failure to handle multiple --filter options","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-11-20T10:07:44Z","receivedAt":"2022-11-20T10:08:01Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"git pack-objects should accept multiple --filter options as documented\nin Documentation/rev-list-options.txt, but currently the last one wins.\nShow that using tests with multiple blob size limits.\n\nSigned-off-by: René Scharfe \n---\n t/t5317-pack-objects-filter-objects.sh | 38 ++++++++++++++++++++++++++\n 1 file changed, 38 insertions(+)\n\ndiff --git a/t/t5317-pack-objects-filter-objects.sh b/t/t5317-pack-objects-filter-objects.sh\nindex 82a22ecaa5..25faebaada 100755\n--- a/t/t5317-pack-objects-filter-objects.sh\n+++ b/t/t5317-pack-objects-filter-objects.sh\n@@ -265,6 +265,44 @@ test_expect_success 'verify normal and blob:limit packfiles have same commits/tr\n \ttest_cmp expected observed\n '\n\n+test_expect_failure 'verify small limit and big limit results in small limit' '\n+\tgit -C r2 ls-files -s large.1000 >ls_files_result &&\n+\ttest_parse_ls_files_stage_oids expected &&\n+\n+\tgit -C r2 pack-objects --revs --stdout --filter=blob:limit=1001 \\\n+\t\t--filter=blob:limit=10001 >filter.pack <<-EOF &&\n+\tHEAD\n+\tEOF\n+\tgit -C r2 index-pack ../filter.pack &&\n+\n+\tgit -C r2 verify-pack -v ../filter.pack >verify_result &&\n+\tgrep blob verify_result |\n+\tparse_verify_pack_blob_oid |\n+\tsort >observed &&\n+\n+\ttest_cmp expected observed\n+'\n+\n+test_expect_success 'verify big limit and small limit results in small limit' '\n+\tgit -C r2 ls-files -s large.1000 >ls_files_result &&\n+\ttest_parse_ls_files_stage_oids expected &&\n+\n+\tgit -C r2 pack-objects --revs --stdout --filter=blob:limit=10001 \\\n+\t\t--filter=blob:limit=1001 >filter.pack <<-EOF &&\n+\tHEAD\n+\tEOF\n+\tgit -C r2 index-pack ../filter.pack &&\n+\n+\tgit -C r2 verify-pack -v ../filter.pack >verify_result &&\n+\tgrep blob verify_result |\n+\tparse_verify_pack_blob_oid |\n+\tsort >observed &&\n+\n+\ttest_cmp expected observed\n+'\n+\n # Test sparse:path= filter.\n # !!!!\n # NOTE: sparse:path filter support has been dropped for security reasons,\n--\n2.38.1\n"},{"id":"467615","messageId":"ad8df33f-cf47-4b1e-e356-d61817ccf810@web.de","threadId":"58795","inReplyTo":"d19c6cb4-611f-afea-8a14-5e58d7509113@web.de","subject":"[PATCH v2 1/3] t5317: stop losing return codes of git ls-files","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-11-20T10:06:05Z","receivedAt":"2022-11-20T10:08:01Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"fb2d0db502 (test-lib-functions: add parsing helpers for ls-files and\nls-tree, 2022-04-04) not only started to use helper functions, it also\nstarted to pipe the output of git ls-files into them directly, without\nusing a temporary file. No explanation was given. This causes the\nreturn code of that git command to be ignored.\n\nRevert that part of the change, use temporary files and check the return\ncode of git ls-files again.\n\nSuggested-by: Ævar Arnfjörð Bjarmason \nSigned-off-by: René Scharfe \n---\n t/t5317-pack-objects-filter-objects.sh | 52 ++++++++++++++------------\n 1 file changed, 28 insertions(+), 24 deletions(-)\n\ndiff --git a/t/t5317-pack-objects-filter-objects.sh b/t/t5317-pack-objects-filter-objects.sh\nindex bb633c9b09..82a22ecaa5 100755\n--- a/t/t5317-pack-objects-filter-objects.sh\n+++ b/t/t5317-pack-objects-filter-objects.sh\n@@ -24,8 +24,9 @@ parse_verify_pack_blob_oid () {\n }\n\n test_expect_success 'verify blob count in normal packfile' '\n-\tgit -C r1 ls-files -s file.1 file.2 file.3 file.4 file.5 |\n-\ttest_parse_ls_files_stage_oids |\n+\tgit -C r1 ls-files -s file.1 file.2 file.3 file.4 file.5 \\\n+\t\t>ls_files_result &&\n+\ttest_parse_ls_files_stage_oids expected &&\n\n \tgit -C r1 pack-objects --revs --stdout >all.pack <<-EOF &&\n@@ -123,8 +124,8 @@ test_expect_success 'setup r2' '\n '\n\n test_expect_success 'verify blob count in normal packfile' '\n-\tgit -C r2 ls-files -s large.1000 large.10000 |\n-\ttest_parse_ls_files_stage_oids |\n+\tgit -C r2 ls-files -s large.1000 large.10000 >ls_files_result &&\n+\ttest_parse_ls_files_stage_oids expected &&\n\n \tgit -C r2 pack-objects --revs --stdout >all.pack <<-EOF &&\n@@ -161,8 +162,8 @@ test_expect_success 'verify blob:limit=1000' '\n '\n\n test_expect_success 'verify blob:limit=1001' '\n-\tgit -C r2 ls-files -s large.1000 |\n-\ttest_parse_ls_files_stage_oids |\n+\tgit -C r2 ls-files -s large.1000 >ls_files_result &&\n+\ttest_parse_ls_files_stage_oids expected &&\n\n \tgit -C r2 pack-objects --revs --stdout --filter=blob:limit=1001 >filter.pack <<-EOF &&\n@@ -179,8 +180,8 @@ test_expect_success 'verify blob:limit=1001' '\n '\n\n test_expect_success 'verify blob:limit=10001' '\n-\tgit -C r2 ls-files -s large.1000 large.10000 |\n-\ttest_parse_ls_files_stage_oids |\n+\tgit -C r2 ls-files -s large.1000 large.10000 >ls_files_result &&\n+\ttest_parse_ls_files_stage_oids expected &&\n\n \tgit -C r2 pack-objects --revs --stdout --filter=blob:limit=10001 >filter.pack <<-EOF &&\n@@ -197,8 +198,8 @@ test_expect_success 'verify blob:limit=10001' '\n '\n\n test_expect_success 'verify blob:limit=1k' '\n-\tgit -C r2 ls-files -s large.1000 |\n-\ttest_parse_ls_files_stage_oids |\n+\tgit -C r2 ls-files -s large.1000 >ls_files_result &&\n+\ttest_parse_ls_files_stage_oids expected &&\n\n \tgit -C r2 pack-objects --revs --stdout --filter=blob:limit=1k >filter.pack <<-EOF &&\n@@ -215,8 +216,8 @@ test_expect_success 'verify blob:limit=1k' '\n '\n\n test_expect_success 'verify explicitly specifying oversized blob in input' '\n-\tgit -C r2 ls-files -s large.1000 large.10000 |\n-\ttest_parse_ls_files_stage_oids |\n+\tgit -C r2 ls-files -s large.1000 large.10000 >ls_files_result &&\n+\ttest_parse_ls_files_stage_oids expected &&\n\n \techo HEAD >objects &&\n@@ -233,8 +234,8 @@ test_expect_success 'verify explicitly specifying oversized blob in input' '\n '\n\n test_expect_success 'verify blob:limit=1m' '\n-\tgit -C r2 ls-files -s large.1000 large.10000 |\n-\ttest_parse_ls_files_stage_oids |\n+\tgit -C r2 ls-files -s large.1000 large.10000 >ls_files_result &&\n+\ttest_parse_ls_files_stage_oids expected &&\n\n \tgit -C r2 pack-objects --revs --stdout --filter=blob:limit=1m >filter.pack <<-EOF &&\n@@ -289,8 +290,9 @@ test_expect_success 'setup r3' '\n '\n\n test_expect_success 'verify blob count in normal packfile' '\n-\tgit -C r3 ls-files -s sparse1 sparse2 dir1/sparse1 dir1/sparse2 |\n-\ttest_parse_ls_files_stage_oids |\n+\tgit -C r3 ls-files -s sparse1 sparse2 dir1/sparse1 dir1/sparse2 \\\n+\t\t>ls_files_result &&\n+\ttest_parse_ls_files_stage_oids expected &&\n\n \tgit -C r3 pack-objects --revs --stdout >all.pack <<-EOF &&\n@@ -341,8 +343,9 @@ test_expect_success 'setup r4' '\n '\n\n test_expect_success 'verify blob count in normal packfile' '\n-\tgit -C r4 ls-files -s pattern sparse1 sparse2 dir1/sparse1 dir1/sparse2 |\n-\ttest_parse_ls_files_stage_oids |\n+\tgit -C r4 ls-files -s pattern sparse1 sparse2 dir1/sparse1 dir1/sparse2 \\\n+\t\t>ls_files_result &&\n+\ttest_parse_ls_files_stage_oids expected &&\n\n \tgit -C r4 pack-objects --revs --stdout >all.pack <<-EOF &&\n@@ -359,8 +362,8 @@ test_expect_success 'verify blob count in normal packfile' '\n '\n\n test_expect_success 'verify sparse:oid=OID' '\n-\tgit -C r4 ls-files -s dir1/sparse1 dir1/sparse2 |\n-\ttest_parse_ls_files_stage_oids |\n+\tgit -C r4 ls-files -s dir1/sparse1 dir1/sparse2 >ls_files_result &&\n+\ttest_parse_ls_files_stage_oids expected &&\n\n \tgit -C r4 ls-files -s pattern >staged &&\n@@ -379,8 +382,8 @@ test_expect_success 'verify sparse:oid=OID' '\n '\n\n test_expect_success 'verify sparse:oid=oid-ish' '\n-\tgit -C r4 ls-files -s dir1/sparse1 dir1/sparse2 |\n-\ttest_parse_ls_files_stage_oids |\n+\tgit -C r4 ls-files -s dir1/sparse1 dir1/sparse2 >ls_files_result &&\n+\ttest_parse_ls_files_stage_oids expected &&\n\n \tgit -C r4 pack-objects --revs --stdout --filter=sparse:oid=main:pattern >filter.pack <<-EOF &&\n@@ -400,8 +403,9 @@ test_expect_success 'verify sparse:oid=oid-ish' '\n # This models previously omitted objects that we did not receive.\n\n test_expect_success 'setup r1 - delete loose blobs' '\n-\tgit -C r1 ls-files -s file.1 file.2 file.3 file.4 file.5 |\n-\ttest_parse_ls_files_stage_oids |\n+\tgit -C r1 ls-files -s file.1 file.2 file.3 file.4 file.5 \\\n+\t\t>ls_files_result &&\n+\ttest_parse_ls_files_stage_oids expected &&\n\n \tfor id in `cat expected | sed \"s|..|&/|\"`\n--\n2.38.1\n"},{"id":"467616","messageId":"f5779e19-813c-cda9-2f84-9fe58f745e89@web.de","threadId":"58795","inReplyTo":"d19c6cb4-611f-afea-8a14-5e58d7509113@web.de","subject":"[PATCH v2 3/3] Revert \"pack-objects: lazily set up \"struct rev_info\", don't leak\"","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-11-20T10:13:48Z","receivedAt":"2022-11-20T10:14:06Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"This reverts commit 5cb28270a1ff94a0a23e67b479bbbec3bc993518.\n\n5cb28270a1 (pack-objects: lazily set up \"struct rev_info\", don't leak,\n2022-03-28) avoided leaking rev_info allocations in many cases by\ncalling repo_init_revisions() only when the .filter member was actually\nneeded, but then still leaking it. That was fixed later by 2108fe4a19\n(revisions API users: add straightforward release_revisions(),\n2022-04-13), making the reverted commit unnecessary.\n\n5cb28270a1 broke support for multiple --filter options by calling\nrepo_init_revisions() for each one, clobbering earlier state and\nleaking rev_info allocations. Reverting it fixes that regression.\nLuckily it only affects users that run pack-objects directly. Internal\ncallers in bundle.c and upload-pack.c combine multiple filters into a\nsingle option using \"+\".\n\n5cb28270a1 introduced OPT_PARSE_LIST_OBJECTS_FILTER_INIT. It relies on\nbeing able to faithfully convert function pointers to object pointers\nand back, which is undefined in C99. The standard mentions this ability\nas a common extension in its annex J (J.5.7 Function pointer casts), but\navoiding that dependency is more portable. The macro hasn't been used\nsince, OPT_PARSE_LIST_OBJECTS_FILTER is enough so far.\n\nSo revert 5cb28270a1 fully to fix support for multiple --filter options\nand avoid reliance on undefined behavior. Its intent is better served\nby the release_revisions() call added by 2108fe4a19 alone -- we just\nneed to do it unconditionally, mirroring the unconditional call of\nrepo_init_revisions().\n\nHelped-by: Jeff King \nSigned-off-by: René Scharfe \n---\n builtin/pack-objects.c | 31 +++++---------------------\n list-objects-filter-options.c | 4 ----\n list-objects-filter-options.h | 24 +++-----------------\n t/t5317-pack-objects-filter-objects.sh | 2 +-\n 4 files changed, 10 insertions(+), 51 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 573d0b20b7..3e74fbb0cd 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -4149,21 +4149,6 @@ static int option_parse_cruft_expiration(const struct option *opt,\n \treturn 0;\n }\n\n-struct po_filter_data {\n-\tunsigned have_revs:1;\n-\tstruct rev_info revs;\n-};\n-\n-static struct list_objects_filter_options *po_filter_revs_init(void *value)\n-{\n-\tstruct po_filter_data *data = value;\n-\n-\trepo_init_revisions(the_repository, &data->revs, NULL);\n-\tdata->have_revs = 1;\n-\n-\treturn &data->revs.filter;\n-}\n-\n int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n {\n \tint use_internal_rev_list = 0;\n@@ -4174,7 +4159,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \tint rev_list_index = 0;\n \tint stdin_packs = 0;\n \tstruct string_list keep_pack_list = STRING_LIST_INIT_NODUP;\n-\tstruct po_filter_data pfd = { .have_revs = 0 };\n+\tstruct rev_info revs;\n\n \tstruct option pack_objects_options[] = {\n \t\tOPT_SET_INT('q', \"quiet\", &progress,\n@@ -4265,7 +4250,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \t\t\t &write_bitmap_index,\n \t\t\t N_(\"write a bitmap index if possible\"),\n \t\t\t WRITE_BITMAP_QUIET, PARSE_OPT_HIDDEN),\n-\t\tOPT_PARSE_LIST_OBJECTS_FILTER_INIT(&pfd, po_filter_revs_init),\n+\t\tOPT_PARSE_LIST_OBJECTS_FILTER(&revs.filter),\n \t\tOPT_CALLBACK_F(0, \"missing\", NULL, N_(\"action\"),\n \t\t N_(\"handling for missing objects\"), PARSE_OPT_NONEG,\n \t\t option_parse_missing_action),\n@@ -4284,6 +4269,8 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n\n \tread_replace_refs = 0;\n\n+\trepo_init_revisions(the_repository, &revs, NULL);\n+\n \tsparse = git_env_bool(\"GIT_TEST_PACK_SPARSE\", -1);\n \tif (the_repository->gitdir) {\n \t\tprepare_repo_settings(the_repository);\n@@ -4385,7 +4372,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \tif (!rev_list_all || !rev_list_reflog || !rev_list_index)\n \t\tunpack_unreachable_expiration = 0;\n\n-\tif (pfd.have_revs && pfd.revs.filter.choice) {\n+\tif (revs.filter.choice) {\n \t\tif (!pack_to_stdout)\n \t\t\tdie(_(\"cannot use --filter without --stdout\"));\n \t\tif (stdin_packs)\n@@ -4472,16 +4459,10 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \t\tread_cruft_objects();\n \t} else if (!use_internal_rev_list) {\n \t\tread_object_list_from_stdin();\n-\t} else if (pfd.have_revs) {\n-\t\tget_object_list(&pfd.revs, rp.nr, rp.v);\n-\t\trelease_revisions(&pfd.revs);\n \t} else {\n-\t\tstruct rev_info revs;\n-\n-\t\trepo_init_revisions(the_repository, &revs, NULL);\n \t\tget_object_list(&revs, rp.nr, rp.v);\n-\t\trelease_revisions(&revs);\n \t}\n+\trelease_revisions(&revs);\n \tcleanup_preferred_base();\n \tif (include_tag && nr_result)\n \t\tfor_each_tag_ref(add_ref_tag, NULL);\ndiff --git a/list-objects-filter-options.c b/list-objects-filter-options.c\nindex 5339660238..ee01bcd2cc 100644\n--- a/list-objects-filter-options.c\n+++ b/list-objects-filter-options.c\n@@ -290,10 +290,6 @@ int opt_parse_list_objects_filter(const struct option *opt,\n \t\t\t\t const char *arg, int unset)\n {\n \tstruct list_objects_filter_options *filter_options = opt->value;\n-\topt_lof_init init = (opt_lof_init)opt->defval;\n-\n-\tif (init)\n-\t\tfilter_options = init(opt->value);\n\n \tif (unset || !arg)\n \t\tlist_objects_filter_set_no_filter(filter_options);\ndiff --git a/list-objects-filter-options.h b/list-objects-filter-options.h\nindex 7eeadab2dd..64af2e29e2 100644\n--- a/list-objects-filter-options.h\n+++ b/list-objects-filter-options.h\n@@ -107,31 +107,13 @@ void parse_list_objects_filter(\n \tstruct list_objects_filter_options *filter_options,\n \tconst char *arg);\n\n-/**\n- * The opt->value to opt_parse_list_objects_filter() is either a\n- * \"struct list_objects_filter_option *\" when using\n- * OPT_PARSE_LIST_OBJECTS_FILTER().\n- *\n- * Or, if using no \"struct option\" field is used by the callback,\n- * except the \"defval\" which is expected to be an \"opt_lof_init\"\n- * function, which is called with the \"opt->value\" and must return a\n- * pointer to the \"\"struct list_objects_filter_option *\" to be used.\n- *\n- * The OPT_PARSE_LIST_OBJECTS_FILTER_INIT() can be used e.g. the\n- * \"struct list_objects_filter_option\" is embedded in a \"struct\n- * rev_info\", which the \"defval\" could be tasked with lazily\n- * initializing. See cmd_pack_objects() for an example.\n- */\n int opt_parse_list_objects_filter(const struct option *opt,\n \t\t\t\t const char *arg, int unset);\n-typedef struct list_objects_filter_options *(*opt_lof_init)(void *);\n-#define OPT_PARSE_LIST_OBJECTS_FILTER_INIT(fo, init) \\\n-\t{ OPTION_CALLBACK, 0, \"filter\", (fo), N_(\"args\"), \\\n-\t N_(\"object filtering\"), 0, opt_parse_list_objects_filter, \\\n-\t (intptr_t)(init) }\n\n #define OPT_PARSE_LIST_OBJECTS_FILTER(fo) \\\n-\tOPT_PARSE_LIST_OBJECTS_FILTER_INIT((fo), NULL)\n+\tOPT_CALLBACK(0, \"filter\", fo, N_(\"args\"), \\\n+\t N_(\"object filtering\"), \\\n+\t opt_parse_list_objects_filter)\n\n /*\n * Translates abbreviated numbers in the filter's filter_spec into their\ndiff --git a/t/t5317-pack-objects-filter-objects.sh b/t/t5317-pack-objects-filter-objects.sh\nindex 25faebaada..5b707d911b 100755\n--- a/t/t5317-pack-objects-filter-objects.sh\n+++ b/t/t5317-pack-objects-filter-objects.sh\n@@ -265,7 +265,7 @@ test_expect_success 'verify normal and blob:limit packfiles have same commits/tr\n \ttest_cmp expected observed\n '\n\n-test_expect_failure 'verify small limit and big limit results in small limit' '\n+test_expect_success 'verify small limit and big limit results in small limit' '\n \tgit -C r2 ls-files -s large.1000 >ls_files_result &&\n \ttest_parse_ls_files_stage_oids expected &&\n--\n2.38.1\n"},{"id":"467800","messageId":"Y30c4YYGb5bOaZae@coredump.intra.peff.net","threadId":"58795","inReplyTo":"d19c6cb4-611f-afea-8a14-5e58d7509113@web.de","subject":"Re: [PATCH v2 0/3] pack-objects: fix and simplify --filter handling","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-11-22T19:02:57Z","receivedAt":"2022-11-22T19:03:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Nov 20, 2022 at 11:03:35AM +0100, René Scharfe wrote:\n\n> Fix a regression that prevents using multiple --filter options, simplify\n> the option parsing code and avoid relying on undefined behavior in it.\n> \n> Patch 3 conflicts with cc/filtered-repack in seen, but not semantically.\n> \n> Changes since v1:\n> - Added patch 1 to fix an issue with existing tests.\n> - Separate patch 2 for new tests.\n> - Test using blob size filters, only, which is a bit simpler.\n> - Test both combinations to also catch not just the current\n> last-one-wins regression, but also a possible future first-one-wins\n> issue.\n> - Actually revert 5cb28270a1 (pack-objects: lazily set up\n> \"struct rev_info\", don't leak, 2022-03-28) instead of having a\n> minimal fix and then adding some kind of middle ground by using a\n> separate struct list_objects_filter_options.\n\nThanks, this looks good to me. The new test is a little funny in that\nit's somewhat-nonsensical input, but I find it unlikely that we'd ever\nadd code to treat two instances of the same filter type specially.\nCompared to something that produces two distinct effects (like a blob\nand tree-depth filter combined), it also requires the \"test in both\ndirections\" trick to cover everything.\n\nBut I do like that it's easier to reason about what the output _should_\nbe. So I don't feel too strongly about it either way.\n\n-Peff\n"},{"id":"468069","messageId":"xmqqv8mz5ras.fsf@gitster.g","threadId":"58795","inReplyTo":"f5779e19-813c-cda9-2f84-9fe58f745e89@web.de","subject":"Re: [PATCH v2 3/3] Revert \"pack-objects: lazily set up \"struct rev_info\", don't leak\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-11-28T10:03:55Z","receivedAt":"2022-11-28T10:04:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> This reverts commit 5cb28270a1ff94a0a23e67b479bbbec3bc993518.\n>\n> 5cb28270a1 (pack-objects: lazily set up \"struct rev_info\", don't leak,\n> 2022-03-28) avoided leaking rev_info allocations in many cases by\n> calling repo_init_revisions() only when the .filter member was actually\n> needed, but then still leaking it. That was fixed later by 2108fe4a19\n> (revisions API users: add straightforward release_revisions(),\n> 2022-04-13), making the reverted commit unnecessary.\n\nHmph, with this merged, 'seen' breaks linux-leaks job in a strange\nway.\n\nhttps://github.com/git/git/actions/runs/3563546608/jobs/5986458300#step:5:3917\n\nDoes anybody want to help looking into it?\n\nThanks.\n"},{"id":"468072","messageId":"d10de9b5-e5ff-18d6-d950-1d090d87b113@web.de","threadId":"58795","inReplyTo":"xmqqv8mz5ras.fsf@gitster.g","subject":"Re: [PATCH v2 3/3] Revert \"pack-objects: lazily set up \"struct rev_info\", don't leak\"","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-11-28T11:26:31Z","receivedAt":"2022-11-28T11:27:40Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 28.11.2022 um 11:03 schrieb Junio C Hamano:\n> René Scharfe writes:\n>\n>> This reverts commit 5cb28270a1ff94a0a23e67b479bbbec3bc993518.\n>>\n>> 5cb28270a1 (pack-objects: lazily set up \"struct rev_info\", don't leak,\n>> 2022-03-28) avoided leaking rev_info allocations in many cases by\n>> calling repo_init_revisions() only when the .filter member was actually\n>> needed, but then still leaking it. That was fixed later by 2108fe4a19\n>> (revisions API users: add straightforward release_revisions(),\n>> 2022-04-13), making the reverted commit unnecessary.\n>\n> Hmph, with this merged, 'seen' breaks linux-leaks job in a strange\n> way.\n>\n> https://github.com/git/git/actions/runs/3563546608/jobs/5986458300#step:5:3917\n>\n> Does anybody want to help looking into it?\nThe patch exposes that release_revisions() leaks the diffopt allocations\nas we're yet to address the TODO added by 54c8a7c379 (revisions API: add\na TODO for diff_free(&revs->diffopt), 2022-04-14).\n\nThe patch below plugs it locally.\n\n--- >8 ---\nSubject: [PATCH 4/3] fixup! revision: free diffopt in release_revisions()\n\nSigned-off-by: René Scharfe \n---\n builtin/pack-objects.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 3e74fbb0cd..a47a3f0fba 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -4462,6 +4462,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \t} else {\n \t\tget_object_list(&revs, rp.nr, rp.v);\n \t}\n+\tdiff_free(&revs.diffopt);\n \trelease_revisions(&revs);\n \tcleanup_preferred_base();\n \tif (include_tag && nr_result)\n--\n2.30.2\n"},{"id":"468073","messageId":"221128.868rjvmi3l.gmgdl@evledraar.gmail.com","threadId":"58795","inReplyTo":"xmqqv8mz5ras.fsf@gitster.g","subject":"Re: [PATCH v2 3/3] Revert \"pack-objects: lazily set up \"struct rev_info\", don't leak\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-28T11:12:23Z","receivedAt":"2022-11-28T11:30:49Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Nov 28 2022, Junio C Hamano wrote:\n\n> René Scharfe writes:\n>\n>> This reverts commit 5cb28270a1ff94a0a23e67b479bbbec3bc993518.\n>>\n>> 5cb28270a1 (pack-objects: lazily set up \"struct rev_info\", don't leak,\n>> 2022-03-28) avoided leaking rev_info allocations in many cases by\n>> calling repo_init_revisions() only when the .filter member was actually\n>> needed, but then still leaking it. That was fixed later by 2108fe4a19\n>> (revisions API users: add straightforward release_revisions(),\n>> 2022-04-13), making the reverted commit unnecessary.\n>\n> Hmph, with this merged, 'seen' breaks linux-leaks job in a strange\n> way.\n>\n> https://github.com/git/git/actions/runs/3563546608/jobs/5986458300#step:5:3917\n>\n> Does anybody want to help looking into it?\n\nIt's because the tip of that branch introduces a new leak, but it's only\nrevealed in our test suite by a \"git\" invocation that we don't check the\nexit code of. So testing with \"GIT_TEST_SANITIZE_LEAK_LOG=true\" (which\nis there to check exactly those edge cases) catches it.\n\nRené, testing locally with e.g.:\n\n\tGIT_TEST_PASSING_SANITIZE_LEAK=check \\\n\tGIT_TEST_SANITIZE_LEAK_LOG=true \\\n\tmake test\n\nShould catch it, but note that we might still have new leaks in tests\nthat are already failing due to other leaks. So diff-ing the resulting\nt/test-results/*.leak/* would be a good sanity check to see if there's\nany other leak we've missed.\n\nAs to the bug itself: This exact edge case is why I went for the\nlazy-setup part of 5cb28270a1f (pack-objects: lazily set up \"struct\nrev_info\", don't leak, 2022-03-28), and indeed the example mentioned in\nits commit message now leaks again, but not on master:\n\n\techo e83c5163316f89bfbde7d9ab23ca2e25604af290 | ./git pack-objects initial\n\nNow, maybe I'm missing something, but it the tip of that branch points\nout a legitimate bug in my 5cb28270a1f.\n\nBut I don't see why the fix for it needs to be a full revert of\n5cb28270a1f. If I replace all the C code changes in that commit with\nthis the test it flipped also passes:\n\t\n\tdiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n\tindex 573d0b20b76..0de53c5a2df 100644\n\t--- a/builtin/pack-objects.c\n\t+++ b/builtin/pack-objects.c\n\t@@ -4158,8 +4158,10 @@ static struct list_objects_filter_options *po_filter_revs_init(void *value)\n\t {\n\t \tstruct po_filter_data *data = value;\n\t \n\t-\trepo_init_revisions(the_repository, &data->revs, NULL);\n\t-\tdata->have_revs = 1;\n\t+\tif (!data->have_revs) {\n\t+\t\trepo_init_revisions(the_repository, &data->revs, NULL);\n\t+\t\tdata->have_revs = 1;\n\t+\t}\n\t \n\t \treturn &data->revs.filter;\n\t }\n\nI.e. the commit is correct that we shouldn't be clobbering the\n\"rev_info\" when we see --filter again, but then let's just stop doing\nthat.\n\nWell, I know why, it's because René initially started poking at this to\nrid the codebase of the reliance of C99's J.5.7. I think our two\nopinions on that are quite clear, so I won't repeat that here (I don't\nper-se mind, I just think it's a nothingburger).\n\nBut in any case, if we're going to also refactor this code to get rd of\nthe J.5.7 reliance I'd think we could agree that it really belongs in\nits own change. That change could then be a pure refactoring change, and\nwouldn't need to also explain how it's fixing a bug while-at-it.\n"},{"id":"468074","messageId":"221128.864jujmhgp.gmgdl@evledraar.gmail.com","threadId":"58795","inReplyTo":"d10de9b5-e5ff-18d6-d950-1d090d87b113@web.de","subject":"Re: [PATCH v2 3/3] Revert \"pack-objects: lazily set up \"struct rev_info\", don't leak\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-28T11:31:17Z","receivedAt":"2022-11-28T11:44:28Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Nov 28 2022, René Scharfe wrote:\n\n> Am 28.11.2022 um 11:03 schrieb Junio C Hamano:\n>> René Scharfe writes:\n>>\n>>> This reverts commit 5cb28270a1ff94a0a23e67b479bbbec3bc993518.\n>>>\n>>> 5cb28270a1 (pack-objects: lazily set up \"struct rev_info\", don't leak,\n>>> 2022-03-28) avoided leaking rev_info allocations in many cases by\n>>> calling repo_init_revisions() only when the .filter member was actually\n>>> needed, but then still leaking it. That was fixed later by 2108fe4a19\n>>> (revisions API users: add straightforward release_revisions(),\n>>> 2022-04-13), making the reverted commit unnecessary.\n>>\n>> Hmph, with this merged, 'seen' breaks linux-leaks job in a strange\n>> way.\n>>\n>> https://github.com/git/git/actions/runs/3563546608/jobs/5986458300#step:5:3917\n>>\n>> Does anybody want to help looking into it?\n\n[I see we crossed E-Mails]:\nhttps://lore.kernel.org/git/221128.868rjvmi3l.gmgdl@evledraar.gmail.com/\n\n> The patch exposes that release_revisions() leaks the diffopt allocations\n> as we're yet to address the TODO added by 54c8a7c379 (revisions API: add\n> a TODO for diff_free(&revs->diffopt), 2022-04-14).\n\nThat's correct, and we have that leak in various places in our codebase,\nbut per the above side-thread I think this is primarily exposing that\nwe're setting up the \"struct rev_info\" with your change when we don't\nneed to. Why can't we just skip it?\n\nYeah, if we do set it up we'll run into an outstanding leak, and that\nshould also be fixed (I have some local patches...), but the other cases\nI know of where we'll leak that data is where we're actually using the\n\"struct rev_info\".\n\nI haven't tried tearing your change apart to poke at it myself, and\nmaybe there's some really good reason for why you can't separate getting\nrid of the J.5.7 dependency and removing the lazy-init.\n\n> The patch below plugs it locally.\n>\n> --- >8 ---\n> Subject: [PATCH 4/3] fixup! revision: free diffopt in release_revisions()\n>\n> Signed-off-by: René Scharfe \n> ---\n> builtin/pack-objects.c | 1 +\n> 1 file changed, 1 insertion(+)\n>\n> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n> index 3e74fbb0cd..a47a3f0fba 100644\n> --- a/builtin/pack-objects.c\n> +++ b/builtin/pack-objects.c\n> @@ -4462,6 +4462,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n> \t} else {\n> \t\tget_object_list(&revs, rp.nr, rp.v);\n> \t}\n> +\tdiff_free(&revs.diffopt);\n> \trelease_revisions(&revs);\n> \tcleanup_preferred_base();\n> \tif (include_tag && nr_result)\n\nSo, the main motivation for the change was paranoia that a compiler or\nplatform might show up without J.5.7 support and that would bite us, but\nwe're now adding a double-free-in-waiting?\n\nI think we're both a bit paranoid, but clearly have different\nparanoia-priorities :)\n\nIf we do end up with some hack like this instead of fixing the\nunderlying problem I'd much prefer that such a hack just be an UNLEAK()\nhere.\n\nI.e. we have a destructor for \"revs.*\" already, let's not bypass it and\nstart freeing things from under it, which will result in a double-free\nif we forget this callsite once the TODO in 54c8a7c379 is addressed.\n\nAs you'd see if you made release_revisions() simply call\ndiff_free(&revs.diffopt) doing so would reveal some really gnarly edge\ncases.\n\nI haven't dug into this one, but offhand I'm not confident in saying\nthat this isn't exposing us to some aspect of that gnarlyness (maybe\nnot, it's been a while since I looked).\n\n(IIRC some of the most gnarly edge cases will only show up as CI\nfailures on Windows, to do with the ordering of when we'll fclose()\nfiles hanging off that \"diffopt\").\n\n"},{"id":"468076","messageId":"patch-1.1-45b240740eb-20221128T115740Z-avarab@gmail.com","threadId":"58795","inReplyTo":"221128.868rjvmi3l.gmgdl@evledraar.gmail.com","subject":"[PATCH] t5314: check exit code of \"rev-parse\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-28T12:00:16Z","receivedAt":"2022-11-28T12:01:37Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Amend the test added in [1] to check the exit code of the \"rev-parse\"\ninvocations. An in-flight change[2] introduced a memory leak in these\ninvocations, which went undetected unless we were running under\n\"GIT_TEST_SANITIZE_LEAK_LOG=true\".\n\nNote that the in-flight change made 8 test files fail, but as far as I\ncan tell only this one would have had its exit code hidden unless\nunder \"GIT_TEST_SANITIZE_LEAK_LOG=true\". The rest would be caught\nwithout it.\n\n1. 4cf2143e029 (pack-objects: break delta cycles before delta-search\n phase, 2016-08-11)\n2. https://lore.kernel.org/git/221128.868rjvmi3l.gmgdl@evledraar.gmail.com/\n3. faececa53f9 (test-lib: have the \"check\" mode for SANITIZE=leak\n consider leak logs, 2022-07-28)\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n\nAside from rs/multi-filter-args, we should be checking the return code\nof \"git\" in these cases.\n\n t/t5314-pack-cycle-detection.sh | 17 ++++++++++-------\n 1 file changed, 10 insertions(+), 7 deletions(-)\n\ndiff --git a/t/t5314-pack-cycle-detection.sh b/t/t5314-pack-cycle-detection.sh\nindex 73a241743aa..169d8198641 100755\n--- a/t/t5314-pack-cycle-detection.sh\n+++ b/t/t5314-pack-cycle-detection.sh\n@@ -63,13 +63,16 @@ TEST_PASSES_SANITIZE_LEAK=true\n # Note that the two variants of \"file\" must be similar enough to convince git\n # to create the delta.\n make_pack () {\n-\t{\n-\t\tprintf '%s\\n' \"-$(git rev-parse $2)\"\n-\t\tprintf '%s dummy\\n' \"$(git rev-parse $1:dummy)\"\n-\t\tprintf '%s file\\n' \"$(git rev-parse $1:file)\"\n-\t} |\n-\tgit pack-objects --stdout |\n-\tgit index-pack --stdin --fix-thin\n+\tgrp1=$(git rev-parse \"$2\") &&\n+\tgrp2=$(git rev-parse \"$1:dummy\") &&\n+\tgrp3=$(git rev-parse \"$1:file\") &&\n+\tcat >list <<-EOF\n+\t-$grp1\n+\t$grp2 dummy\n+\t$grp3 file\n+\tEOF\n+\tgit pack-objects --stdout pack &&\n+\tgit index-pack --stdin --fix-thin On Mon, Nov 28 2022, René Scharfe wrote:\n>\n>> Am 28.11.2022 um 11:03 schrieb Junio C Hamano:\n>>> René Scharfe writes:\n>>>\n>>>> This reverts commit 5cb28270a1ff94a0a23e67b479bbbec3bc993518.\n>>>>\n>>>> 5cb28270a1 (pack-objects: lazily set up \"struct rev_info\", don't leak,\n>>>> 2022-03-28) avoided leaking rev_info allocations in many cases by\n>>>> calling repo_init_revisions() only when the .filter member was actually\n>>>> needed, but then still leaking it. That was fixed later by 2108fe4a19\n>>>> (revisions API users: add straightforward release_revisions(),\n>>>> 2022-04-13), making the reverted commit unnecessary.\n>>>\n>>> Hmph, with this merged, 'seen' breaks linux-leaks job in a strange\n>>> way.\n>>>\n>>> https://github.com/git/git/actions/runs/3563546608/jobs/5986458300#step:5:3917\n>>>\n>>> Does anybody want to help looking into it?\n>\n> [I see we crossed E-Mails]:\n> https://lore.kernel.org/git/221128.868rjvmi3l.gmgdl@evledraar.gmail.com/\n>\n>> The patch exposes that release_revisions() leaks the diffopt allocations\n>> as we're yet to address the TODO added by 54c8a7c379 (revisions API: add\n>> a TODO for diff_free(&revs->diffopt), 2022-04-14).\n>\n> That's correct, and we have that leak in various places in our codebase,\n> but per the above side-thread I think this is primarily exposing that\n> we're setting up the \"struct rev_info\" with your change when we don't\n> need to. Why can't we just skip it?\n>\n> Yeah, if we do set it up we'll run into an outstanding leak, and that\n> should also be fixed (I have some local patches...), but the other cases\n> I know of where we'll leak that data is where we're actually using the\n> \"struct rev_info\".\n>\n> I haven't tried tearing your change apart to poke at it myself, and\n> maybe there's some really good reason for why you can't separate getting\n> rid of the J.5.7 dependency and removing the lazy-init.\n>\n>> The patch below plugs it locally.\n>>\n>> --- >8 ---\n>> Subject: [PATCH 4/3] fixup! revision: free diffopt in release_revisions()\n>>\n>> Signed-off-by: René Scharfe \n>> ---\n>> builtin/pack-objects.c | 1 +\n>> 1 file changed, 1 insertion(+)\n>>\n>> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n>> index 3e74fbb0cd..a47a3f0fba 100644\n>> --- a/builtin/pack-objects.c\n>> +++ b/builtin/pack-objects.c\n>> @@ -4462,6 +4462,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n>> \t} else {\n>> \t\tget_object_list(&revs, rp.nr, rp.v);\n>> \t}\n>> +\tdiff_free(&revs.diffopt);\n>> \trelease_revisions(&revs);\n>> \tcleanup_preferred_base();\n>> \tif (include_tag && nr_result)\n>\n> So, the main motivation for the change was paranoia that a compiler or\n> platform might show up without J.5.7 support and that would bite us, but\n> we're now adding a double-free-in-waiting?\n>\n> I think we're both a bit paranoid, but clearly have different\n> paranoia-priorities :)\n>\n> If we do end up with some hack like this instead of fixing the\n> underlying problem I'd much prefer that such a hack just be an UNLEAK()\n> here.\n>\n> I.e. we have a destructor for \"revs.*\" already, let's not bypass it and\n> start freeing things from under it, which will result in a double-free\n> if we forget this callsite once the TODO in 54c8a7c379 is addressed.\n>\n> As you'd see if you made release_revisions() simply call\n> diff_free(&revs.diffopt) doing so would reveal some really gnarly edge\n> cases.\n>\n> I haven't dug into this one, but offhand I'm not confident in saying\n> that this isn't exposing us to some aspect of that gnarlyness (maybe\n> not, it's been a while since I looked).\n>\n> (IIRC some of the most gnarly edge cases will only show up as CI\n> failures on Windows, to do with the ordering of when we'll fclose()\n> files hanging off that \"diffopt\").\n\nThis squashed into 3/3 seems to me to be a proper fix to a change that\nwants to refactor the code for non-J.5.7 compatibility. I.e. this just\ndoes the data<->fp casting part of the change, without refactoring the\n\"lazy init\".\n\nBut I think you should check this a bit more carefully. Your 3/3 says\nthat your change \"mak[es] the reverted commit unnecessary\", but as I\nnoted if you'd run the command that commit shows, you'd have seen you're\nre-introducing the leak it fixed. So I wonder what else has been missed\nhere.\n\nI vaguely recall that one reason I ended up with that J.5.7 dependency\nwas because there was an objection to mocking up the \"struct option\" as\nI'm doing here. I.e. here we assume that the\nopt_parse_list_objects_filter() is only ever going to care about the\n\"value\" member.\n\nI think that's probably fine, but I may be misrecalling, or missing some\ncrucial detail. I'll leave digging that up & convincing us that it's\nfine to the person pushing for refactoring all of this :)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 3e74fbb0cd5..faf210bfe8c 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -4149,6 +4149,27 @@ static int option_parse_cruft_expiration(const struct option *opt,\n \treturn 0;\n }\n \n+struct po_filter_data {\n+\tunsigned have_revs:1;\n+\tstruct rev_info revs;\n+};\n+\n+static int opt_parse_list_objects_filter_init(const struct option *opt,\n+\t\t\t\t\t const char *arg, int unset)\n+{\n+\tstruct po_filter_data *data = opt->value;\n+\tstruct rev_info *revs = &data->revs;\n+\tconst struct option opt_rev = {\n+\t\t.value = (void *)&revs->filter,\n+\t};\n+\n+\tif (!data->have_revs)\n+\t\trepo_init_revisions(the_repository, revs, NULL);\n+\tdata->have_revs = 1;\n+\n+\treturn opt_parse_list_objects_filter(&opt_rev, arg, unset);\n+}\n+\n int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n {\n \tint use_internal_rev_list = 0;\n@@ -4159,7 +4180,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \tint rev_list_index = 0;\n \tint stdin_packs = 0;\n \tstruct string_list keep_pack_list = STRING_LIST_INIT_NODUP;\n-\tstruct rev_info revs;\n+\tstruct po_filter_data pfd = { .have_revs = 0 };\n \n \tstruct option pack_objects_options[] = {\n \t\tOPT_SET_INT('q', \"quiet\", &progress,\n@@ -4250,7 +4271,8 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \t\t\t &write_bitmap_index,\n \t\t\t N_(\"write a bitmap index if possible\"),\n \t\t\t WRITE_BITMAP_QUIET, PARSE_OPT_HIDDEN),\n-\t\tOPT_PARSE_LIST_OBJECTS_FILTER(&revs.filter),\n+\t\tOPT_CALLBACK(0, \"filter\", &pfd, N_(\"args\"), N_(\"object filtering\"),\n+\t\t\t opt_parse_list_objects_filter_init),\n \t\tOPT_CALLBACK_F(0, \"missing\", NULL, N_(\"action\"),\n \t\t N_(\"handling for missing objects\"), PARSE_OPT_NONEG,\n \t\t option_parse_missing_action),\n@@ -4269,8 +4291,6 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \n \tread_replace_refs = 0;\n \n-\trepo_init_revisions(the_repository, &revs, NULL);\n-\n \tsparse = git_env_bool(\"GIT_TEST_PACK_SPARSE\", -1);\n \tif (the_repository->gitdir) {\n \t\tprepare_repo_settings(the_repository);\n@@ -4372,7 +4392,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \tif (!rev_list_all || !rev_list_reflog || !rev_list_index)\n \t\tunpack_unreachable_expiration = 0;\n \n-\tif (revs.filter.choice) {\n+\tif (pfd.have_revs && pfd.revs.filter.choice) {\n \t\tif (!pack_to_stdout)\n \t\t\tdie(_(\"cannot use --filter without --stdout\"));\n \t\tif (stdin_packs)\n@@ -4459,10 +4479,16 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \t\tread_cruft_objects();\n \t} else if (!use_internal_rev_list) {\n \t\tread_object_list_from_stdin();\n+\t} else if (pfd.have_revs) {\n+\t\tget_object_list(&pfd.revs, rp.nr, rp.v);\n+\t\trelease_revisions(&pfd.revs);\n \t} else {\n+\t\tstruct rev_info revs;\n+\n+\t\trepo_init_revisions(the_repository, &revs, NULL);\n \t\tget_object_list(&revs, rp.nr, rp.v);\n+\t\trelease_revisions(&revs);\n \t}\n-\trelease_revisions(&revs);\n \tcleanup_preferred_base();\n \tif (include_tag && nr_result)\n \t\tfor_each_tag_ref(add_ref_tag, NULL);\n"},{"id":"468094","messageId":"96a2f70d-c338-a992-a3e4-3e6b82b644ca@web.de","threadId":"58795","inReplyTo":"patch-1.1-45b240740eb-20221128T115740Z-avarab@gmail.com","subject":"Re: [PATCH] t5314: check exit code of \"rev-parse\"","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-11-28T13:51:29Z","receivedAt":"2022-11-28T13:51:49Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 28.11.2022 um 13:00 schrieb Ævar Arnfjörð Bjarmason:\n> Amend the test added in [1] to check the exit code of the \"rev-parse\"\n> invocations. An in-flight change[2] introduced a memory leak in these\n> invocations, which went undetected unless we were running under\n> \"GIT_TEST_SANITIZE_LEAK_LOG=true\".\n\nChecking the return code of rev-parse might be a good idea, but AFAICS\nnone of the patches in the thread around [2] add a leak to it; they are\nonly about pack-objects. So that's not how this patch works.\n\n> Note that the in-flight change made 8 test files fail, but as far as I\n> can tell only this one would have had its exit code hidden unless\n> under \"GIT_TEST_SANITIZE_LEAK_LOG=true\". The rest would be caught\n> without it.\n>\n> 1. 4cf2143e029 (pack-objects: break delta cycles before delta-search\n> phase, 2016-08-11)\n> 2. https://lore.kernel.org/git/221128.868rjvmi3l.gmgdl@evledraar.gmail.com/\n> 3. faececa53f9 (test-lib: have the \"check\" mode for SANITIZE=leak\n> consider leak logs, 2022-07-28)\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n>\n> Aside from rs/multi-filter-args, we should be checking the return code\n> of \"git\" in these cases.\n>\n> t/t5314-pack-cycle-detection.sh | 17 ++++++++++-------\n> 1 file changed, 10 insertions(+), 7 deletions(-)\n>\n> diff --git a/t/t5314-pack-cycle-detection.sh b/t/t5314-pack-cycle-detection.sh\n> index 73a241743aa..169d8198641 100755\n> --- a/t/t5314-pack-cycle-detection.sh\n> +++ b/t/t5314-pack-cycle-detection.sh\n> @@ -63,13 +63,16 @@ TEST_PASSES_SANITIZE_LEAK=true\n> # Note that the two variants of \"file\" must be similar enough to convince git\n> # to create the delta.\n> make_pack () {\n> -\t{\n> -\t\tprintf '%s\\n' \"-$(git rev-parse $2)\"\n> -\t\tprintf '%s dummy\\n' \"$(git rev-parse $1:dummy)\"\n> -\t\tprintf '%s file\\n' \"$(git rev-parse $1:file)\"\n> -\t} |\n> -\tgit pack-objects --stdout |\n\nTHIS is how it works: pack-objects with the patch in the\ngrandparent of [2] and built with SANITIZE=leak fails here, but\nthe test marches on, ignoring its return value.\n\n> -\tgit index-pack --stdin --fix-thin\n> +\tgrp1=$(git rev-parse \"$2\") &&\n> +\tgrp2=$(git rev-parse \"$1:dummy\") &&\n> +\tgrp3=$(git rev-parse \"$1:file\") &&\n\nNit: \"grp\" expands to \"group\" in my mind. Here it stands for \"git\nrev-parse\", hmm. \"commit\", \"dummy_blob\" and \"file_blob\" might be\nbetter names.\n\n> +\tcat >list <<-EOF\n> +\t-$grp1\n> +\t$grp2 dummy\n> +\t$grp3 file\n> +\tEOF\n> +\tgit pack-objects --stdout pack &&\n\nHere's the new return code check that makes the test detect the\ndiffopt leak in question.\n\n> +\tgit index-pack --stdin --fix-thin }\n>\n> test_expect_success 'setup' '\n"},{"id":"468101","messageId":"patch-v2-1.1-ca77a7249e6-20221128T141818Z-avarab@gmail.com","threadId":"58795","inReplyTo":"patch-1.1-45b240740eb-20221128T115740Z-avarab@gmail.com","subject":"[PATCH v2] t5314: check exit code of \"git\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-28T14:18:55Z","receivedAt":"2022-11-28T14:19:14Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Amend the test added in [1] to check the exit code of the \"git\"\ninvocations. An in-flight change[2] introduced a memory leak in these\ninvocations, which went undetected unless we were running under\n\"GIT_TEST_SANITIZE_LEAK_LOG=true\".\n\nNote that the in-flight change made 8 test files fail, but as far as I\ncan tell only this one would have had its exit code hidden unless\nunder \"GIT_TEST_SANITIZE_LEAK_LOG=true\". The rest would be caught\nwithout it.\n\nWe could pick other variable names here than \"ln%d\", e.g. \"commit\",\n\"dummy_blob\" and \"file_blob\", but having the \"rev-parse\" invocations\naligned makes the difference between them more readable, so let's pick\n\"ln%d\".\n\n1. 4cf2143e029 (pack-objects: break delta cycles before delta-search\n phase, 2016-08-11)\n2. https://lore.kernel.org/git/221128.868rjvmi3l.gmgdl@evledraar.gmail.com/\n3. faececa53f9 (test-lib: have the \"check\" mode for SANITIZE=leak\n consider leak logs, 2022-07-28)\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\nRange-diff against v1:\n1: 45b240740eb ! 1: ca77a7249e6 t5314: check exit code of \"rev-parse\"\n @@ Metadata\n Author: Ævar Arnfjörð Bjarmason \n \n ## Commit message ##\n - t5314: check exit code of \"rev-parse\"\n + t5314: check exit code of \"git\"\n \n - Amend the test added in [1] to check the exit code of the \"rev-parse\"\n + Amend the test added in [1] to check the exit code of the \"git\"\n invocations. An in-flight change[2] introduced a memory leak in these\n invocations, which went undetected unless we were running under\n \"GIT_TEST_SANITIZE_LEAK_LOG=true\".\n @@ Commit message\n under \"GIT_TEST_SANITIZE_LEAK_LOG=true\". The rest would be caught\n without it.\n \n + We could pick other variable names here than \"ln%d\", e.g. \"commit\",\n + \"dummy_blob\" and \"file_blob\", but having the \"rev-parse\" invocations\n + aligned makes the difference between them more readable, so let's pick\n + \"ln%d\".\n +\n 1. 4cf2143e029 (pack-objects: break delta cycles before delta-search\n phase, 2016-08-11)\n 2. https://lore.kernel.org/git/221128.868rjvmi3l.gmgdl@evledraar.gmail.com/\n @@ t/t5314-pack-cycle-detection.sh: TEST_PASSES_SANITIZE_LEAK=true\n -\t} |\n -\tgit pack-objects --stdout |\n -\tgit index-pack --stdin --fix-thin\n -+\tgrp1=$(git rev-parse \"$2\") &&\n -+\tgrp2=$(git rev-parse \"$1:dummy\") &&\n -+\tgrp3=$(git rev-parse \"$1:file\") &&\n ++\tln1=$(git rev-parse \"$2\") &&\n ++\tln2=$(git rev-parse \"$1:dummy\") &&\n ++\tln3=$(git rev-parse \"$1:file\") &&\n +\tcat >list <<-EOF\n -+\t-$grp1\n -+\t$grp2 dummy\n -+\t$grp3 file\n ++\t-$ln1\n ++\t$ln2 dummy\n ++\t$ln3 file\n +\tEOF\n +\tgit pack-objects --stdout pack &&\n +\tgit index-pack --stdin --fix-thin list <<-EOF\n+\t-$ln1\n+\t$ln2 dummy\n+\t$ln3 file\n+\tEOF\n+\tgit pack-objects --stdout pack &&\n+\tgit index-pack --stdin --fix-thin \n> On Mon, Nov 28 2022, René Scharfe wrote:\n>\n>> Am 28.11.2022 um 11:03 schrieb Junio C Hamano:\n>>> René Scharfe writes:\n>>>\n>>>> This reverts commit 5cb28270a1ff94a0a23e67b479bbbec3bc993518.\n>>>>\n>>>> 5cb28270a1 (pack-objects: lazily set up \"struct rev_info\", don't leak,\n>>>> 2022-03-28) avoided leaking rev_info allocations in many cases by\n>>>> calling repo_init_revisions() only when the .filter member was actually\n>>>> needed, but then still leaking it. That was fixed later by 2108fe4a19\n>>>> (revisions API users: add straightforward release_revisions(),\n>>>> 2022-04-13), making the reverted commit unnecessary.\n>>>\n>>> Hmph, with this merged, 'seen' breaks linux-leaks job in a strange\n>>> way.\n>>>\n>>> https://github.com/git/git/actions/runs/3563546608/jobs/5986458300#step:5:3917\n>>>\n>>> Does anybody want to help looking into it?\n>\n> [I see we crossed E-Mails]:\n> https://lore.kernel.org/git/221128.868rjvmi3l.gmgdl@evledraar.gmail.com/\n>\n>> The patch exposes that release_revisions() leaks the diffopt allocations\n>> as we're yet to address the TODO added by 54c8a7c379 (revisions API: add\n>> a TODO for diff_free(&revs->diffopt), 2022-04-14).\n>\n> That's correct, and we have that leak in various places in our codebase,\n> but per the above side-thread I think this is primarily exposing that\n> we're setting up the \"struct rev_info\" with your change when we don't\n> need to. Why can't we just skip it?\n\nI have no idea how to stop get_object_list() from using struct rev_info.\nWe could let it take a struct list_objects_filter_options pointer\ninstead and have it build a struct rev_info internally, but that would\njust move the problem, not solve it.\n\n> Yeah, if we do set it up we'll run into an outstanding leak, and that\n> should also be fixed (I have some local patches...), but the other cases\n> I know of where we'll leak that data is where we're actually using the\n> \"struct rev_info\".\n>\n> I haven't tried tearing your change apart to poke at it myself, and\n> maybe there's some really good reason for why you can't separate getting\n> rid of the J.5.7 dependency and removing the lazy-init.\n>\n>> The patch below plugs it locally.\n>>\n>> --- >8 ---\n>> Subject: [PATCH 4/3] fixup! revision: free diffopt in release_revisions()\n>>\n>> Signed-off-by: René Scharfe \n>> ---\n>> builtin/pack-objects.c | 1 +\n>> 1 file changed, 1 insertion(+)\n>>\n>> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n>> index 3e74fbb0cd..a47a3f0fba 100644\n>> --- a/builtin/pack-objects.c\n>> +++ b/builtin/pack-objects.c\n>> @@ -4462,6 +4462,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n>> \t} else {\n>> \t\tget_object_list(&revs, rp.nr, rp.v);\n>> \t}\n>> +\tdiff_free(&revs.diffopt);\n>> \trelease_revisions(&revs);\n>> \tcleanup_preferred_base();\n>> \tif (include_tag && nr_result)\n>\n> So, the main motivation for the change was paranoia that a compiler or\n> platform might show up without J.5.7 support and that would bite us, but\n> we're now adding a double-free-in-waiting?\n>\n> I think we're both a bit paranoid, but clearly have different\n> paranoia-priorities :)\n>\n> If we do end up with some hack like this instead of fixing the\n> underlying problem I'd much prefer that such a hack just be an UNLEAK()\n> here.\n>\n> I.e. we have a destructor for \"revs.*\" already, let's not bypass it and\n> start freeing things from under it, which will result in a double-free\n> if we forget this callsite once the TODO in 54c8a7c379 is addressed.\n\nWell, that TODO fix should remove this new diff_free() call, but I\nagree that this is fragile.\n\nRemoving the \"TEST_PASSES_SANITIZE_LEAK=true\" line from affected tests\nis probably better.\n\n> As you'd see if you made release_revisions() simply call\n> diff_free(&revs.diffopt) doing so would reveal some really gnarly edge\n> cases.\n\nThat was my first attempt; it breaks lots of tests due to double frees.\n\n> I haven't dug into this one, but offhand I'm not confident in saying\n> that this isn't exposing us to some aspect of that gnarlyness (maybe\n> not, it's been a while since I looked).\n\nI saw it as the way towards a release_revisions() that calls diff_free()\nitself: Add such calls to each of them, fix the \"gnarlyness\"\nindividually, finally move them all into release_revisions(). The only\nproblem is that there are 60+ callsites.\n\n> (IIRC some of the most gnarly edge cases will only show up as CI\n> failures on Windows, to do with the ordering of when we'll fclose()\n> files hanging off that \"diffopt\").\n\nFun.\n\nRené\n"},{"id":"468108","messageId":"221128.865yezkule.gmgdl@evledraar.gmail.com","threadId":"58795","inReplyTo":"c5aeb93c-763d-3eae-0150-15f6ca675319@web.de","subject":"Re: [PATCH v2 3/3] Revert \"pack-objects: lazily set up \"struct rev_info\", don't leak\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-28T14:34:25Z","receivedAt":"2022-11-28T14:45:09Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Nov 28 2022, René Scharfe wrote:\n\n> Am 28.11.2022 um 12:31 schrieb Ævar Arnfjörð Bjarmason:\n>>\n>> On Mon, Nov 28 2022, René Scharfe wrote:\n>>\n>>> Am 28.11.2022 um 11:03 schrieb Junio C Hamano:\n>>>> René Scharfe writes:\n>>>>\n>>>>> This reverts commit 5cb28270a1ff94a0a23e67b479bbbec3bc993518.\n>>>>>\n>>>>> 5cb28270a1 (pack-objects: lazily set up \"struct rev_info\", don't leak,\n>>>>> 2022-03-28) avoided leaking rev_info allocations in many cases by\n>>>>> calling repo_init_revisions() only when the .filter member was actually\n>>>>> needed, but then still leaking it. That was fixed later by 2108fe4a19\n>>>>> (revisions API users: add straightforward release_revisions(),\n>>>>> 2022-04-13), making the reverted commit unnecessary.\n>>>>\n>>>> Hmph, with this merged, 'seen' breaks linux-leaks job in a strange\n>>>> way.\n>>>>\n>>>> https://github.com/git/git/actions/runs/3563546608/jobs/5986458300#step:5:3917\n>>>>\n>>>> Does anybody want to help looking into it?\n>>\n>> [I see we crossed E-Mails]:\n>> https://lore.kernel.org/git/221128.868rjvmi3l.gmgdl@evledraar.gmail.com/\n>>\n>>> The patch exposes that release_revisions() leaks the diffopt allocations\n>>> as we're yet to address the TODO added by 54c8a7c379 (revisions API: add\n>>> a TODO for diff_free(&revs->diffopt), 2022-04-14).\n>>\n>> That's correct, and we have that leak in various places in our codebase,\n>> but per the above side-thread I think this is primarily exposing that\n>> we're setting up the \"struct rev_info\" with your change when we don't\n>> need to. Why can't we just skip it?\n>\n> I have no idea how to stop get_object_list() from using struct rev_info.\n> We could let it take a struct list_objects_filter_options pointer\n> instead and have it build a struct rev_info internally, but that would\n> just move the problem, not solve it.\n\nI mean skip it when it's not needed, it's needed when we call\nget_object_list().\n\nBut what \"problem\" is being caused by get_object_list()? That there's\nsome other case(s) where it'll leak still? I haven't checked, I think we\nshould leave that for some other time if there's such leaks, and just\nnot introduce any new leaks in this topic.\n\n>> Yeah, if we do set it up we'll run into an outstanding leak, and that\n>> should also be fixed (I have some local patches...), but the other cases\n>> I know of where we'll leak that data is where we're actually using the\n>> \"struct rev_info\".\n>>\n>> I haven't tried tearing your change apart to poke at it myself, and\n>> maybe there's some really good reason for why you can't separate getting\n>> rid of the J.5.7 dependency and removing the lazy-init.\n>>\n>>> The patch below plugs it locally.\n>>>\n>>> --- >8 ---\n>>> Subject: [PATCH 4/3] fixup! revision: free diffopt in release_revisions()\n>>>\n>>> Signed-off-by: René Scharfe \n>>> ---\n>>> builtin/pack-objects.c | 1 +\n>>> 1 file changed, 1 insertion(+)\n>>>\n>>> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n>>> index 3e74fbb0cd..a47a3f0fba 100644\n>>> --- a/builtin/pack-objects.c\n>>> +++ b/builtin/pack-objects.c\n>>> @@ -4462,6 +4462,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n>>> \t} else {\n>>> \t\tget_object_list(&revs, rp.nr, rp.v);\n>>> \t}\n>>> +\tdiff_free(&revs.diffopt);\n>>> \trelease_revisions(&revs);\n>>> \tcleanup_preferred_base();\n>>> \tif (include_tag && nr_result)\n>>\n>> So, the main motivation for the change was paranoia that a compiler or\n>> platform might show up without J.5.7 support and that would bite us, but\n>> we're now adding a double-free-in-waiting?\n>>\n>> I think we're both a bit paranoid, but clearly have different\n>> paranoia-priorities :)\n>>\n>> If we do end up with some hack like this instead of fixing the\n>> underlying problem I'd much prefer that such a hack just be an UNLEAK()\n>> here.\n>>\n>> I.e. we have a destructor for \"revs.*\" already, let's not bypass it and\n>> start freeing things from under it, which will result in a double-free\n>> if we forget this callsite once the TODO in 54c8a7c379 is addressed.\n>\n> Well, that TODO fix should remove this new diff_free() call, but I\n> agree that this is fragile.\n>\n> Removing the \"TEST_PASSES_SANITIZE_LEAK=true\" line from affected tests\n> is probably better.\n\nOr just not introduce new leaks, per my suggested fix-up at\nhttps://lore.kernel.org/git/221128.86zgcbl0pe.gmgdl@evledraar.gmail.com/\n(which it looks like you haven't seen when this E-Mail is composed...).\n\n>> As you'd see if you made release_revisions() simply call\n>> diff_free(&revs.diffopt) doing so would reveal some really gnarly edge\n>> cases.\n>\n> That was my first attempt; it breaks lots of tests due to double frees.\n\nRight, to be clear I'm saying that none of this is needed right now,\ni.e. I don't get why we'd want the scope-creep past the hunk I noted in\nhttps://lore.kernel.org/git/221128.868rjvmi3l.gmgdl@evledraar.gmail.com/\nfor the --filter bug fix (plus the tests you're adding).\n\n>> I haven't dug into this one, but offhand I'm not confident in saying\n>> that this isn't exposing us to some aspect of that gnarlyness (maybe\n>> not, it's been a while since I looked).\n>\n> I saw it as the way towards a release_revisions() that calls diff_free()\n> itself: Add such calls to each of them, fix the \"gnarlyness\"\n> individually, finally move them all into release_revisions(). The only\n> problem is that there are 60+ callsites.\n\nI think this is a really bad approach in general.\n\nYes, it may happen to work to free() some data from under an API, but\nit's just as likely that we'll miss that this one caller is screwing\nwith its internal state, and e.g. when some new revision.c code is used\nit'll go boom.\n\nIf we wanted to phase in such a free() of \"foo\" I think the right way\nwould be to add some \"revs.free_foo = 1\" flag, giving the API a chance\nto treat that sanely, not to start poking at members of the struct, and\nassuming that its release() won't be free()-ing them.\n\nBut as noted above & in the linked I think we can defer all of that. The\nonly reason we're discussing this is because you're changing the\nlazy-init to be not-lazy, and introducing new leaks as a result.\n\nI've shown a couple of approaches in this thread of fixing the issue(s)\nat hand without introducing such leaks, so ...\n"},{"id":"468110","messageId":"9bea523d-93d1-953b-a136-3f00844c880a@web.de","threadId":"58795","inReplyTo":"221128.86zgcbl0pe.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2 3/3] Revert \"pack-objects: lazily set up \"struct rev_info\", don't leak\"","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-11-28T15:16:46Z","receivedAt":"2022-11-28T15:17:08Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 28.11.2022 um 13:24 schrieb Ævar Arnfjörð Bjarmason:\n>\n> On Mon, Nov 28 2022, Ævar Arnfjörð Bjarmason wrote:\n>\n> René:\n>\n>> On Mon, Nov 28 2022, René Scharfe wrote:\n>>\n>>> Am 28.11.2022 um 11:03 schrieb Junio C Hamano:\n>>>> René Scharfe writes:\n>>>>\n>>>>> This reverts commit 5cb28270a1ff94a0a23e67b479bbbec3bc993518.\n>>>>>\n>>>>> 5cb28270a1 (pack-objects: lazily set up \"struct rev_info\", don't leak,\n>>>>> 2022-03-28) avoided leaking rev_info allocations in many cases by\n>>>>> calling repo_init_revisions() only when the .filter member was actually\n>>>>> needed, but then still leaking it. That was fixed later by 2108fe4a19\n>>>>> (revisions API users: add straightforward release_revisions(),\n>>>>> 2022-04-13), making the reverted commit unnecessary.\n>>>>\n>>>> Hmph, with this merged, 'seen' breaks linux-leaks job in a strange\n>>>> way.\n>>>>\n>>>> https://github.com/git/git/actions/runs/3563546608/jobs/5986458300#step:5:3917\n>>>>\n>>>> Does anybody want to help looking into it?\n>>\n>> [I see we crossed E-Mails]:\n>> https://lore.kernel.org/git/221128.868rjvmi3l.gmgdl@evledraar.gmail.com/\n>>\n>>> The patch exposes that release_revisions() leaks the diffopt allocations\n>>> as we're yet to address the TODO added by 54c8a7c379 (revisions API: add\n>>> a TODO for diff_free(&revs->diffopt), 2022-04-14).\n>>\n>> That's correct, and we have that leak in various places in our codebase,\n>> but per the above side-thread I think this is primarily exposing that\n>> we're setting up the \"struct rev_info\" with your change when we don't\n>> need to. Why can't we just skip it?\n>>\n>> Yeah, if we do set it up we'll run into an outstanding leak, and that\n>> should also be fixed (I have some local patches...), but the other cases\n>> I know of where we'll leak that data is where we're actually using the\n>> \"struct rev_info\".\n>>\n>> I haven't tried tearing your change apart to poke at it myself, and\n>> maybe there's some really good reason for why you can't separate getting\n>> rid of the J.5.7 dependency and removing the lazy-init.\n>>\n>>> The patch below plugs it locally.\n>>>\n>>> --- >8 ---\n>>> Subject: [PATCH 4/3] fixup! revision: free diffopt in release_revisions()\n>>>\n>>> Signed-off-by: René Scharfe \n>>> ---\n>>> builtin/pack-objects.c | 1 +\n>>> 1 file changed, 1 insertion(+)\n>>>\n>>> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n>>> index 3e74fbb0cd..a47a3f0fba 100644\n>>> --- a/builtin/pack-objects.c\n>>> +++ b/builtin/pack-objects.c\n>>> @@ -4462,6 +4462,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n>>> \t} else {\n>>> \t\tget_object_list(&revs, rp.nr, rp.v);\n>>> \t}\n>>> +\tdiff_free(&revs.diffopt);\n>>> \trelease_revisions(&revs);\n>>> \tcleanup_preferred_base();\n>>> \tif (include_tag && nr_result)\n>>\n>> So, the main motivation for the change was paranoia that a compiler or\n>> platform might show up without J.5.7 support and that would bite us, but\n>> we're now adding a double-free-in-waiting?\n>>\n>> I think we're both a bit paranoid, but clearly have different\n>> paranoia-priorities :)\n>>\n>> If we do end up with some hack like this instead of fixing the\n>> underlying problem I'd much prefer that such a hack just be an UNLEAK()\n>> here.\n>>\n>> I.e. we have a destructor for \"revs.*\" already, let's not bypass it and\n>> start freeing things from under it, which will result in a double-free\n>> if we forget this callsite once the TODO in 54c8a7c379 is addressed.\n>>\n>> As you'd see if you made release_revisions() simply call\n>> diff_free(&revs.diffopt) doing so would reveal some really gnarly edge\n>> cases.\n>>\n>> I haven't dug into this one, but offhand I'm not confident in saying\n>> that this isn't exposing us to some aspect of that gnarlyness (maybe\n>> not, it's been a while since I looked).\n>>\n>> (IIRC some of the most gnarly edge cases will only show up as CI\n>> failures on Windows, to do with the ordering of when we'll fclose()\n>> files hanging off that \"diffopt\").\n>\n> This squashed into 3/3 seems to me to be a proper fix to a change that\n> wants to refactor the code for non-J.5.7 compatibility. I.e. this just\n> does the data<->fp casting part of the change, without refactoring the\n> \"lazy init\".\n\nThat works, but lazy code is more complicated and there is no benefit\nhere -- eager allocations are not noticably slow or big. Laziness\nhides leaks in corners, i.e. requiring invocations with uncommon\noptions to trigger them.\n\n>\n> But I think you should check this a bit more carefully. Your 3/3 says\n> that your change \"mak[es] the reverted commit unnecessary\"\n\nNo, it says that _your_ change 2108fe4a19 (revisions API users: add\nstraightforward release_revisions(), 2022-04-13) made it unnecessary.\n\n> , but as I\n> noted if you'd run the command that commit shows, you'd have seen you're\n> re-introducing the leak it fixed. So I wonder what else has been missed\n> here.\n\n5cb28270a1 (pack-objects: lazily set up \"struct rev_info\", don't leak,\n2022-03-28) did not plug the leak. It only moved it to the corner that\nhandles the --filter option.\n\nThat leak is only interesting to Git developers and harmless for users.\nBut if the goal is to become free of trivial leaks in order to allow\nusing tools like LeakSanitizer to find real ones then pushing them into\nthe shadows not yet reached by our test coverage won't help for long.\n\n> I vaguely recall that one reason I ended up with that J.5.7 dependency\n> was because there was an objection to mocking up the \"struct option\" as\n> I'm doing here. I.e. here we assume that the\n> opt_parse_list_objects_filter() is only ever going to care about the\n> \"value\" member.\n\nIt's probably fine, but unnecessarily complicated compared to calling\nrepo_init_revisions() eagerly.\n\n>\n> I think that's probably fine, but I may be misrecalling, or missing some\n> crucial detail. I'll leave digging that up & convincing us that it's\n> fine to the person pushing for refactoring all of this :)\n>\n> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n> index 3e74fbb0cd5..faf210bfe8c 100644\n> --- a/builtin/pack-objects.c\n> +++ b/builtin/pack-objects.c\n> @@ -4149,6 +4149,27 @@ static int option_parse_cruft_expiration(const struct option *opt,\n> \treturn 0;\n> }\n>\n> +struct po_filter_data {\n> +\tunsigned have_revs:1;\n> +\tstruct rev_info revs;\n> +};\n> +\n> +static int opt_parse_list_objects_filter_init(const struct option *opt,\n> +\t\t\t\t\t const char *arg, int unset)\n> +{\n> +\tstruct po_filter_data *data = opt->value;\n> +\tstruct rev_info *revs = &data->revs;\n> +\tconst struct option opt_rev = {\n> +\t\t.value = (void *)&revs->filter,\n> +\t};\n> +\n> +\tif (!data->have_revs)\n> +\t\trepo_init_revisions(the_repository, revs, NULL);\n> +\tdata->have_revs = 1;\n> +\n> +\treturn opt_parse_list_objects_filter(&opt_rev, arg, unset);\n> +}\n> +\n> int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n> {\n> \tint use_internal_rev_list = 0;\n> @@ -4159,7 +4180,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n> \tint rev_list_index = 0;\n> \tint stdin_packs = 0;\n> \tstruct string_list keep_pack_list = STRING_LIST_INIT_NODUP;\n> -\tstruct rev_info revs;\n> +\tstruct po_filter_data pfd = { .have_revs = 0 };\n>\n> \tstruct option pack_objects_options[] = {\n> \t\tOPT_SET_INT('q', \"quiet\", &progress,\n> @@ -4250,7 +4271,8 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n> \t\t\t &write_bitmap_index,\n> \t\t\t N_(\"write a bitmap index if possible\"),\n> \t\t\t WRITE_BITMAP_QUIET, PARSE_OPT_HIDDEN),\n> -\t\tOPT_PARSE_LIST_OBJECTS_FILTER(&revs.filter),\n> +\t\tOPT_CALLBACK(0, \"filter\", &pfd, N_(\"args\"), N_(\"object filtering\"),\n> +\t\t\t opt_parse_list_objects_filter_init),\n> \t\tOPT_CALLBACK_F(0, \"missing\", NULL, N_(\"action\"),\n> \t\t N_(\"handling for missing objects\"), PARSE_OPT_NONEG,\n> \t\t option_parse_missing_action),\n> @@ -4269,8 +4291,6 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n>\n> \tread_replace_refs = 0;\n>\n> -\trepo_init_revisions(the_repository, &revs, NULL);\n> -\n> \tsparse = git_env_bool(\"GIT_TEST_PACK_SPARSE\", -1);\n> \tif (the_repository->gitdir) {\n> \t\tprepare_repo_settings(the_repository);\n> @@ -4372,7 +4392,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n> \tif (!rev_list_all || !rev_list_reflog || !rev_list_index)\n> \t\tunpack_unreachable_expiration = 0;\n>\n> -\tif (revs.filter.choice) {\n> +\tif (pfd.have_revs && pfd.revs.filter.choice) {\n> \t\tif (!pack_to_stdout)\n> \t\t\tdie(_(\"cannot use --filter without --stdout\"));\n> \t\tif (stdin_packs)\n> @@ -4459,10 +4479,16 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n> \t\tread_cruft_objects();\n> \t} else if (!use_internal_rev_list) {\n> \t\tread_object_list_from_stdin();\n> +\t} else if (pfd.have_revs) {\n> +\t\tget_object_list(&pfd.revs, rp.nr, rp.v);\n> +\t\trelease_revisions(&pfd.revs);\n> \t} else {\n> +\t\tstruct rev_info revs;\n> +\n> +\t\trepo_init_revisions(the_repository, &revs, NULL);\n> \t\tget_object_list(&revs, rp.nr, rp.v);\n> +\t\trelease_revisions(&revs);\n> \t}\n> -\trelease_revisions(&revs);\n> \tcleanup_preferred_base();\n> \tif (include_tag && nr_result)\n> \t\tfor_each_tag_ref(add_ref_tag, NULL);\n"},{"id":"468112","messageId":"221128.861qpnkrd3.gmgdl@evledraar.gmail.com","threadId":"58795","inReplyTo":"9bea523d-93d1-953b-a136-3f00844c880a@web.de","subject":"Re: [PATCH v2 3/3] Revert \"pack-objects: lazily set up \"struct rev_info\", don't leak\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-28T15:27:57Z","receivedAt":"2022-11-28T15:53:36Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Nov 28 2022, René Scharfe wrote:\n\n> Am 28.11.2022 um 13:24 schrieb Ævar Arnfjörð Bjarmason:\n>>\n>> On Mon, Nov 28 2022, Ævar Arnfjörð Bjarmason wrote:\n>>\n>> René:\n>>\n>>> On Mon, Nov 28 2022, René Scharfe wrote:\n>>>\n>>>> Am 28.11.2022 um 11:03 schrieb Junio C Hamano:\n>>>>> René Scharfe writes:\n>>>>>\n>>>>>> This reverts commit 5cb28270a1ff94a0a23e67b479bbbec3bc993518.\n>>>>>>\n>>>>>> 5cb28270a1 (pack-objects: lazily set up \"struct rev_info\", don't leak,\n>>>>>> 2022-03-28) avoided leaking rev_info allocations in many cases by\n>>>>>> calling repo_init_revisions() only when the .filter member was actually\n>>>>>> needed, but then still leaking it. That was fixed later by 2108fe4a19\n>>>>>> (revisions API users: add straightforward release_revisions(),\n>>>>>> 2022-04-13), making the reverted commit unnecessary.\n>>>>>\n>>>>> Hmph, with this merged, 'seen' breaks linux-leaks job in a strange\n>>>>> way.\n>>>>>\n>>>>> https://github.com/git/git/actions/runs/3563546608/jobs/5986458300#step:5:3917\n>>>>>\n>>>>> Does anybody want to help looking into it?\n>>>\n>>> [I see we crossed E-Mails]:\n>>> https://lore.kernel.org/git/221128.868rjvmi3l.gmgdl@evledraar.gmail.com/\n>>>\n>>>> The patch exposes that release_revisions() leaks the diffopt allocations\n>>>> as we're yet to address the TODO added by 54c8a7c379 (revisions API: add\n>>>> a TODO for diff_free(&revs->diffopt), 2022-04-14).\n>>>\n>>> That's correct, and we have that leak in various places in our codebase,\n>>> but per the above side-thread I think this is primarily exposing that\n>>> we're setting up the \"struct rev_info\" with your change when we don't\n>>> need to. Why can't we just skip it?\n>>>\n>>> Yeah, if we do set it up we'll run into an outstanding leak, and that\n>>> should also be fixed (I have some local patches...), but the other cases\n>>> I know of where we'll leak that data is where we're actually using the\n>>> \"struct rev_info\".\n>>>\n>>> I haven't tried tearing your change apart to poke at it myself, and\n>>> maybe there's some really good reason for why you can't separate getting\n>>> rid of the J.5.7 dependency and removing the lazy-init.\n>>>\n>>>> The patch below plugs it locally.\n>>>>\n>>>> --- >8 ---\n>>>> Subject: [PATCH 4/3] fixup! revision: free diffopt in release_revisions()\n>>>>\n>>>> Signed-off-by: René Scharfe \n>>>> ---\n>>>> builtin/pack-objects.c | 1 +\n>>>> 1 file changed, 1 insertion(+)\n>>>>\n>>>> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n>>>> index 3e74fbb0cd..a47a3f0fba 100644\n>>>> --- a/builtin/pack-objects.c\n>>>> +++ b/builtin/pack-objects.c\n>>>> @@ -4462,6 +4462,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n>>>> \t} else {\n>>>> \t\tget_object_list(&revs, rp.nr, rp.v);\n>>>> \t}\n>>>> +\tdiff_free(&revs.diffopt);\n>>>> \trelease_revisions(&revs);\n>>>> \tcleanup_preferred_base();\n>>>> \tif (include_tag && nr_result)\n>>>\n>>> So, the main motivation for the change was paranoia that a compiler or\n>>> platform might show up without J.5.7 support and that would bite us, but\n>>> we're now adding a double-free-in-waiting?\n>>>\n>>> I think we're both a bit paranoid, but clearly have different\n>>> paranoia-priorities :)\n>>>\n>>> If we do end up with some hack like this instead of fixing the\n>>> underlying problem I'd much prefer that such a hack just be an UNLEAK()\n>>> here.\n>>>\n>>> I.e. we have a destructor for \"revs.*\" already, let's not bypass it and\n>>> start freeing things from under it, which will result in a double-free\n>>> if we forget this callsite once the TODO in 54c8a7c379 is addressed.\n>>>\n>>> As you'd see if you made release_revisions() simply call\n>>> diff_free(&revs.diffopt) doing so would reveal some really gnarly edge\n>>> cases.\n>>>\n>>> I haven't dug into this one, but offhand I'm not confident in saying\n>>> that this isn't exposing us to some aspect of that gnarlyness (maybe\n>>> not, it's been a while since I looked).\n>>>\n>>> (IIRC some of the most gnarly edge cases will only show up as CI\n>>> failures on Windows, to do with the ordering of when we'll fclose()\n>>> files hanging off that \"diffopt\").\n>>\n>> This squashed into 3/3 seems to me to be a proper fix to a change that\n>> wants to refactor the code for non-J.5.7 compatibility. I.e. this just\n>> does the data<->fp casting part of the change, without refactoring the\n>> \"lazy init\".\n>\n> That works, but lazy code is more complicated and there is no benefit\n> here -- eager allocations are not noticably slow or big. Laziness\n> hides leaks in corners, i.e. requiring invocations with uncommon\n> options to trigger them.\n\nYes, sometimes it's easier to just set everything up at the\nbeginning. As for hiding leaks I think the empirical data here is going\nagainst that, i.e. your change introduced a leak.\n\nI don't think it's realistic that we'll have the side that assigns to\n\"have_revs\" drift from the corresponding code in cmd_pack_objects().\n\n>> But I think you should check this a bit more carefully. Your 3/3 says\n>> that your change \"mak[es] the reverted commit unnecessary\"\n>\n> No, it says that _your_ change 2108fe4a19 (revisions API users: add\n> straightforward release_revisions(), 2022-04-13) made it unnecessary.\n\nYes, I'm saying that's not correct, because if you run the command that\n5cb28270a1 prominently notes we'll now leak with this revert:\n\n\techo e83c5163316f89bfbde7d9ab23ca2e25604af290 | ./git pack-objects initial\n\nBut yes with just 5cb28270a1 didn't add release_revisions(), that came\nshortly afterwards in 2108fe4a19.\n\n>> , but as I\n>> noted if you'd run the command that commit shows, you'd have seen you're\n>> re-introducing the leak it fixed. So I wonder what else has been missed\n>> here.\n>\n> 5cb28270a1 (pack-objects: lazily set up \"struct rev_info\", don't leak,\n> 2022-03-28) did not plug the leak. It only moved it to the corner that\n> handles the --filter option.\n\nI think we're using \"the leak\" here differently. I mean callstacks that\nLeakSanitizer emits & tests we have that do & don't pass with\nSANITIZE=leak.\n\nBut yes, there may be multiple paths through a function, some of which\nleak, some of which don't. I'm not saying that the entire set of API\nfeatures that builtin/pack-objects.c uses in the revision API is\nleak-free.\n\n> That leak is only interesting to Git developers and harmless for users.\n> But if the goal is to become free of trivial leaks in order to allow\n> using tools like LeakSanitizer to find real ones then pushing them into\n> the shadows not yet reached by our test coverage won't help for long.\n\nIt's clearly helping in this case, as our CI had multiple failing tests.\n\n>> I vaguely recall that one reason I ended up with that J.5.7 dependency\n>> was because there was an objection to mocking up the \"struct option\" as\n>> I'm doing here. I.e. here we assume that the\n>> opt_parse_list_objects_filter() is only ever going to care about the\n>> \"value\" member.\n>\n> It's probably fine, but unnecessarily complicated compared to calling\n> repo_init_revisions() eagerly.\n\nI'm leaving aside the question of whether we should go for some version\nof the refactoring in your 3/3.\n\nWhat I am saying is that such refactoring should be split up from the\nmore narrow bug fix to the existing code. I.e. this as a replacement for\nyour 3/3 is all that's needed to pass the test you're adding in 2/3.\n\n-- >8 --\nFrom: =?UTF-8?q?Ren=C3=A9=20Scharfe?= \nSubject: [PATCH] pack-objects: support multiple --filter options again\n\n5cb28270a1f (pack-objects: lazily set up \"struct rev_info\", don't\nleak, 2022-03-28) broke support for multiple --filter options by\ncalling repo_init_revisions() every time \"--filter\" was seen. Instead\nwe should only do so the first time, and subsequently append to the\nexisting filter data.\n\nHelped-by: Jeff King \nSigned-off-by: René Scharfe \nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/pack-objects.c | 3 ++-\n t/t5317-pack-objects-filter-objects.sh | 2 +-\n 2 files changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 573d0b20b76..c702c09dd45 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -4158,7 +4158,8 @@ static struct list_objects_filter_options *po_filter_revs_init(void *value)\n {\n \tstruct po_filter_data *data = value;\n \n-\trepo_init_revisions(the_repository, &data->revs, NULL);\n+\tif (!data->have_revs)\n+\t\trepo_init_revisions(the_repository, &data->revs, NULL);\n \tdata->have_revs = 1;\n \n \treturn &data->revs.filter;\ndiff --git a/t/t5317-pack-objects-filter-objects.sh b/t/t5317-pack-objects-filter-objects.sh\nindex 25faebaada8..5b707d911b5 100755\n--- a/t/t5317-pack-objects-filter-objects.sh\n+++ b/t/t5317-pack-objects-filter-objects.sh\n@@ -265,7 +265,7 @@ test_expect_success 'verify normal and blob:limit packfiles have same commits/tr\n \ttest_cmp expected observed\n '\n \n-test_expect_failure 'verify small limit and big limit results in small limit' '\n+test_expect_success 'verify small limit and big limit results in small limit' '\n \tgit -C r2 ls-files -s large.1000 >ls_files_result &&\n \ttest_parse_ls_files_stage_oids expected &&\n-- \n2.39.0.rc0.993.g0c499e58e3b\n\n"},{"id":"468113","messageId":"59431916-9f55-d0f4-da54-e7369803eb4c@web.de","threadId":"58795","inReplyTo":"221128.865yezkule.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2 3/3] Revert \"pack-objects: lazily set up \"struct rev_info\", don't leak\"","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-11-28T15:56:04Z","receivedAt":"2022-11-28T15:56:23Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 28.11.2022 um 15:34 schrieb Ævar Arnfjörð Bjarmason:\n>\n> On Mon, Nov 28 2022, René Scharfe wrote:\n>\n>> Am 28.11.2022 um 12:31 schrieb Ævar Arnfjörð Bjarmason:\n>>>\n>>> On Mon, Nov 28 2022, René Scharfe wrote:\n>>>\n>>>> Am 28.11.2022 um 11:03 schrieb Junio C Hamano:\n>>>>> René Scharfe writes:\n>>>>>\n>>>>>> This reverts commit 5cb28270a1ff94a0a23e67b479bbbec3bc993518.\n>>>>>>\n>>>>>> 5cb28270a1 (pack-objects: lazily set up \"struct rev_info\", don't leak,\n>>>>>> 2022-03-28) avoided leaking rev_info allocations in many cases by\n>>>>>> calling repo_init_revisions() only when the .filter member was actually\n>>>>>> needed, but then still leaking it. That was fixed later by 2108fe4a19\n>>>>>> (revisions API users: add straightforward release_revisions(),\n>>>>>> 2022-04-13), making the reverted commit unnecessary.\n>>>>>\n>>>>> Hmph, with this merged, 'seen' breaks linux-leaks job in a strange\n>>>>> way.\n>>>>>\n>>>>> https://github.com/git/git/actions/runs/3563546608/jobs/5986458300#step:5:3917\n>>>>>\n>>>>> Does anybody want to help looking into it?\n>>>\n>>> [I see we crossed E-Mails]:\n>>> https://lore.kernel.org/git/221128.868rjvmi3l.gmgdl@evledraar.gmail.com/\n>>>\n>>>> The patch exposes that release_revisions() leaks the diffopt allocations\n>>>> as we're yet to address the TODO added by 54c8a7c379 (revisions API: add\n>>>> a TODO for diff_free(&revs->diffopt), 2022-04-14).\n>>>\n>>> That's correct, and we have that leak in various places in our codebase,\n>>> but per the above side-thread I think this is primarily exposing that\n>>> we're setting up the \"struct rev_info\" with your change when we don't\n>>> need to. Why can't we just skip it?\n>>\n>> I have no idea how to stop get_object_list() from using struct rev_info.\n>> We could let it take a struct list_objects_filter_options pointer\n>> instead and have it build a struct rev_info internally, but that would\n>> just move the problem, not solve it.\n>\n> I mean skip it when it's not needed, it's needed when we call\n> get_object_list().\n>\n> But what \"problem\" is being caused by get_object_list()? That there's\n> some other case(s) where it'll leak still? I haven't checked, I think we\n> should leave that for some other time if there's such leaks, and just\n> not introduce any new leaks in this topic.\n\nThe problem is \"How to use struct rev_info without leaks?\". No matter\nwhere you move it, the leak will be present until the TODO in\nrelease_revisions() is done.\n\n>\n>>> Yeah, if we do set it up we'll run into an outstanding leak, and that\n>>> should also be fixed (I have some local patches...), but the other cases\n>>> I know of where we'll leak that data is where we're actually using the\n>>> \"struct rev_info\".\n>>>\n>>> I haven't tried tearing your change apart to poke at it myself, and\n>>> maybe there's some really good reason for why you can't separate getting\n>>> rid of the J.5.7 dependency and removing the lazy-init.\n>>>\n>>>> The patch below plugs it locally.\n>>>>\n>>>> --- >8 ---\n>>>> Subject: [PATCH 4/3] fixup! revision: free diffopt in release_revisions()\n>>>>\n>>>> Signed-off-by: René Scharfe \n>>>> ---\n>>>> builtin/pack-objects.c | 1 +\n>>>> 1 file changed, 1 insertion(+)\n>>>>\n>>>> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n>>>> index 3e74fbb0cd..a47a3f0fba 100644\n>>>> --- a/builtin/pack-objects.c\n>>>> +++ b/builtin/pack-objects.c\n>>>> @@ -4462,6 +4462,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n>>>> \t} else {\n>>>> \t\tget_object_list(&revs, rp.nr, rp.v);\n>>>> \t}\n>>>> +\tdiff_free(&revs.diffopt);\n>>>> \trelease_revisions(&revs);\n>>>> \tcleanup_preferred_base();\n>>>> \tif (include_tag && nr_result)\n>>>\n>>> So, the main motivation for the change was paranoia that a compiler or\n>>> platform might show up without J.5.7 support and that would bite us, but\n>>> we're now adding a double-free-in-waiting?\n>>>\n>>> I think we're both a bit paranoid, but clearly have different\n>>> paranoia-priorities :)\n>>>\n>>> If we do end up with some hack like this instead of fixing the\n>>> underlying problem I'd much prefer that such a hack just be an UNLEAK()\n>>> here.\n>>>\n>>> I.e. we have a destructor for \"revs.*\" already, let's not bypass it and\n>>> start freeing things from under it, which will result in a double-free\n>>> if we forget this callsite once the TODO in 54c8a7c379 is addressed.\n>>\n>> Well, that TODO fix should remove this new diff_free() call, but I\n>> agree that this is fragile.\n>>\n>> Removing the \"TEST_PASSES_SANITIZE_LEAK=true\" line from affected tests\n>> is probably better.\n>\n> Or just not introduce new leaks, per my suggested fix-up at\n> https://lore.kernel.org/git/221128.86zgcbl0pe.gmgdl@evledraar.gmail.com/\n> (which it looks like you haven't seen when this E-Mail is composed...).\n\nNot adding leaks is a good idea. AFAICS none of my patches so far add\nany. Patch 3 of v2 exposes an existing one that was only triggered by\nthe --filter option before. Which is also not ideal, of course, but\ngiving it more visibility hopefully will motivate a proper fix.\n\n>>> As you'd see if you made release_revisions() simply call\n>>> diff_free(&revs.diffopt) doing so would reveal some really gnarly edge\n>>> cases.\n>>\n>> That was my first attempt; it breaks lots of tests due to double frees.\n>\n> Right, to be clear I'm saying that none of this is needed right now,\n> i.e. I don't get why we'd want the scope-creep past the hunk I noted in\n> https://lore.kernel.org/git/221128.868rjvmi3l.gmgdl@evledraar.gmail.com/\n> for the --filter bug fix (plus the tests you're adding).\n\nWell, you asked to squash the minimal fix into the laziness removal in\nhttps://lore.kernel.org/git/221112.86bkpcmm6i.gmgdl@evledraar.gmail.com/\n\nReverting 5cb28270a1 (pack-objects: lazily set up \"struct rev_info\",\ndon't leak, 2022-03-28) wholesale is the simplest way to reach the goals\nof regression fix, simplification and standard compliance. Except that\nthe leak check tests have come to depend on the leak being hidden in the\n--filter corner. So going back to v1 sure seems attractive.\n\n>\n>>> I haven't dug into this one, but offhand I'm not confident in saying\n>>> that this isn't exposing us to some aspect of that gnarlyness (maybe\n>>> not, it's been a while since I looked).\n>>\n>> I saw it as the way towards a release_revisions() that calls diff_free()\n>> itself: Add such calls to each of them, fix the \"gnarlyness\"\n>> individually, finally move them all into release_revisions(). The only\n>> problem is that there are 60+ callsites.\n>\n> I think this is a really bad approach in general.\n>\n> Yes, it may happen to work to free() some data from under an API, but\n> it's just as likely that we'll miss that this one caller is screwing\n> with its internal state, and e.g. when some new revision.c code is used\n> it'll go boom.\n>\n> If we wanted to phase in such a free() of \"foo\" I think the right way\n> would be to add some \"revs.free_foo = 1\" flag, giving the API a chance\n> to treat that sanely, not to start poking at members of the struct, and\n> assuming that its release() won't be free()-ing them.\n\nAnd that's why you added no_free to struct diff_options. We could use\nit here by setting it in repo_init_revisions() and unsetting in\ncmd_pack_objects() and elsewhere, until it is set everywhere.\n\n> But as noted above & in the linked I think we can defer all of that. The\n> only reason we're discussing this is because you're changing the\n> lazy-init to be not-lazy, and introducing new leaks as a result.>\n> I've shown a couple of approaches in this thread of fixing the issue(s)\n> at hand without introducing such leaks, so ...\n\nAs noted above: These leaks are not new, they are just moved into\ntest coverage.\n\nRené\n"},{"id":"468117","messageId":"2488058d-dc59-e8c1-0611-fbcaeb083d73@web.de","threadId":"58795","inReplyTo":"59431916-9f55-d0f4-da54-e7369803eb4c@web.de","subject":"Re: [PATCH v2 3/3] Revert \"pack-objects: lazily set up \"struct rev_info\", don't leak\"","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-11-28T17:57:31Z","receivedAt":"2022-11-28T18:15:36Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 28.11.2022 um 16:56 schrieb René Scharfe:>\n> The problem is \"How to use struct rev_info without leaks?\". No matter\n> where you move it, the leak will be present until the TODO in\n> release_revisions() is done.\n\nWrong. The following sequence leaks:\n\n\tstruct rev_info revs;\n\trepo_init_revisions(the_repository, &revs, NULL);\n\trelease_revisions(&revs);\n\n... and this here doesn't:\n\n\tstruct rev_info revs;\n\trepo_init_revisions(the_repository, &revs, NULL);\n\tsetup_revisions(0, NULL, &revs, NULL); // leak plugger\n\trelease_revisions(&revs);\n\nThat's because setup_revisions() calls diff_setup_done(), which frees\nrevs->diffopt.parseopts, and release_revisions() doesn't.\n\nAnd since builtin/pack-objects.c::get_object_list() calls\nsetup_revisions(), it really frees that memory, as you claimed from the\nstart. Sorry, I was somehow assuming that a setup function wouldn't\nclean up. D'oh!\n\nThe first sequence is used in some other places. e.g. builtin/prune.c.\nBut there LeakSanitizer doesn't complain for some reason; Valgrind\nreports the parseopts allocation as \"possibly lost\".\n\nI still think the assumption that \"init_x(x); release_x(x);\" doesn't\nleak is reasonable. Let's make it true. How about this? It's safe\nin the sense that we don't risk double frees and it's close to the\nTODO comment so we probably won't forget removing it once diff_free()\nbecomes used.\n\n---\n revision.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/revision.c b/revision.c\nindex 439e34a7c5..6a51ef9418 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -3055,6 +3055,7 @@ void release_revisions(struct rev_info *revs)\n \trelease_revisions_mailmap(revs->mailmap);\n \tfree_grep_patterns(&revs->grep_filter);\n \t/* TODO (need to handle \"no_free\"): diff_free(&revs->diffopt) */\n+\tFREE_AND_NULL(revs->diffopt.parseopts);\n \tdiff_free(&revs->pruning);\n \treflog_walk_info_release(revs->reflog_info);\n \trelease_revisions_topo_walk_info(revs->topo_walk_info);\n--\n2.30.2\n"},{"id":"468118","messageId":"221128.86sfi3j5uy.gmgdl@evledraar.gmail.com","threadId":"58795","inReplyTo":"59431916-9f55-d0f4-da54-e7369803eb4c@web.de","subject":"Re: [PATCH v2 3/3] Revert \"pack-objects: lazily set up \"struct rev_info\", don't leak\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-28T17:57:34Z","receivedAt":"2022-11-28T18:30:23Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Nov 28 2022, René Scharfe wrote:\n\n> Am 28.11.2022 um 15:34 schrieb Ævar Arnfjörð Bjarmason:\n> [...]\n>> I mean skip it when it's not needed, it's needed when we call\n>> get_object_list().\n>>\n>> But what \"problem\" is being caused by get_object_list()? That there's\n>> some other case(s) where it'll leak still? I haven't checked, I think we\n>> should leave that for some other time if there's such leaks, and just\n>> not introduce any new leaks in this topic.\n>\n> The problem is \"How to use struct rev_info without leaks?\". No matter\n> where you move it, the leak will be present until the TODO in\n> release_revisions() is done.\n\nNo, the problem at hand is that \"--filter\" doesn't accept N arguments,\nwhich is easily fixed.\n\nJunio then tested the patch, and noted CI failing. Yes we have various\noutstanding issues with the revisions API etc. leaking, but the\nscope-creep of conflating those questions with a regression fix doesn't\nseem necessary.\n\n>>> [...]\n>>> Well, that TODO fix should remove this new diff_free() call, but I\n>>> agree that this is fragile.\n>>>\n>>> Removing the \"TEST_PASSES_SANITIZE_LEAK=true\" line from affected tests\n>>> is probably better.\n>>\n>> Or just not introduce new leaks, per my suggested fix-up at\n>> https://lore.kernel.org/git/221128.86zgcbl0pe.gmgdl@evledraar.gmail.com/\n>> (which it looks like you haven't seen when this E-Mail is composed...).\n>\n> Not adding leaks is a good idea. AFAICS none of my patches so far add\n> any.\n\nI don't see where debating what constitutes a \"new\" or \"added\" memory\nleak is going to get us.\n\nBut to clarify I'm not saying you're responsible for the\nrevisions/filter APIs you're running into, and some of those are\nexisting leaks.\n\nI'm also saying that if you run:\n\n\tmake SANITIZE=leak test T=...\n\nFor some values of T=... (and the whole test suite with\n\"GIT_TEST_PASSING_SANITIZE_LEAK=true\") your 2/3 passes certain tests,\nand your 3/3 doesn't.\n\n> Patch 3 of v2 exposes an existing one that was only triggered by\n> the --filter option before. Which is also not ideal, of course, but\n> giving it more visibility hopefully will motivate a proper fix.\n\nWe try not to break CI. Only finding out that existing CI breaks once a\npatch is queued & Junio & others start testing it to hunt down issues\nisn't a good use of anyone's time.\n\nSo, maybe you think this area of CI is useless etc., but again, if\nyou're submitting a change to advocate for that can we peel that away\nfrom a regression we can fix relatively easily?\n\n>>>> As you'd see if you made release_revisions() simply call\n>>>> diff_free(&revs.diffopt) doing so would reveal some really gnarly edge\n>>>> cases.\n>>>\n>>> That was my first attempt; it breaks lots of tests due to double frees.\n>>\n>> Right, to be clear I'm saying that none of this is needed right now,\n>> i.e. I don't get why we'd want the scope-creep past the hunk I noted in\n>> https://lore.kernel.org/git/221128.868rjvmi3l.gmgdl@evledraar.gmail.com/\n>> for the --filter bug fix (plus the tests you're adding).\n>\n> Well, you asked to squash the minimal fix into the laziness removal in\n> https://lore.kernel.org/git/221112.86bkpcmm6i.gmgdl@evledraar.gmail.com/\n\nThe second part of the relevant request being so that we could\nregression test the memory leak(s).\n\nSo, thanks for trying to address that in some way, but proceeding to\nbreak linux-leaks, and then arguing that that's OK seems like an odd way\nto go about that...\n\n> Reverting 5cb28270a1 (pack-objects: lazily set up \"struct rev_info\",\n> don't leak, 2022-03-28) wholesale is the simplest way to reach the goals\n> of regression fix, simplification and standard compliance. Except that\n> the leak check tests have come to depend on the leak being hidden in the\n> --filter corner. So going back to v1 sure seems attractive.\n\nYeah.\n\nHonestly I'd mostly forgotten about the v1 review, and only skimmed\nthings then. So, I've just noticed that the \"squash\" I'm suggesting is\nbasically equivalent to your v1 now.\n\nBut yeah I think it's fine to just skip testing the regression fix with\nSANITIZE=leak for now, we can do it some other time.\n\nI just brought it up in the v1 feedback because that patch prominently\nnotes the leak fix, so if we can get regression test it relatively\neasily that seemed like a good change to make.\n\n>>> [...]\n>>> I saw it as the way towards a release_revisions() that calls diff_free()\n>>> itself: Add such calls to each of them, fix the \"gnarlyness\"\n>>> individually, finally move them all into release_revisions(). The only\n>>> problem is that there are 60+ callsites.\n>>\n>> I think this is a really bad approach in general.\n>>\n>> Yes, it may happen to work to free() some data from under an API, but\n>> it's just as likely that we'll miss that this one caller is screwing\n>> with its internal state, and e.g. when some new revision.c code is used\n>> it'll go boom.\n>>\n>> If we wanted to phase in such a free() of \"foo\" I think the right way\n>> would be to add some \"revs.free_foo = 1\" flag, giving the API a chance\n>> to treat that sanely, not to start poking at members of the struct, and\n>> assuming that its release() won't be free()-ing them.\n>\n> And that's why you added no_free to struct diff_options. We could use\n> it here by setting it in repo_init_revisions() and unsetting in\n> cmd_pack_objects() and elsewhere, until it is set everywhere.\n\nFWIW I think the \"no_free\" isn't all that good of an approach in\nretrospect either, but at least it's (mostly) self-contained in the\nstruct that owns it.\n\nBut it's rather messy, because some of the time it's embedded in a\n\"struct rev_info\" that should really own it, and sometimes now. At the\ntime release_revisions() didn't exist yet, so making some forward\nprogress with the diff leaks was easiest with the \"no_free\".\n\n>> But as noted above & in the linked I think we can defer all of that. The\n>> only reason we're discussing this is because you're changing the\n>> lazy-init to be not-lazy, and introducing new leaks as a result.>\n>> I've shown a couple of approaches in this thread of fixing the issue(s)\n>> at hand without introducing such leaks, so ...\n>\n> As noted above: These leaks are not new, they are just moved into\n> test coverage.\n\nRight, and in some cases we should definitely just un-mark a test as\nbeing tested under linux-leaks to make forward progress.\n\nBut in this case I just don't see how that's a good trade-off. We can\nfix the --filter bug without that, and yes we'll have some new/existing\nleaks, but those are all contained in tests that are not tested by\nlinux-leaks now.\n\nWhereas the larger rewrite to make the init non-lazy would lose us test\ncoverage.\n\n"},{"id":"468120","messageId":"221128.86o7sqkjcj.gmgdl@evledraar.gmail.com","threadId":"58795","inReplyTo":"2488058d-dc59-e8c1-0611-fbcaeb083d73@web.de","subject":"Re: [PATCH v2 3/3] Revert \"pack-objects: lazily set up \"struct rev_info\", don't leak\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-28T18:32:29Z","receivedAt":"2022-11-28T18:46:42Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Nov 28 2022, René Scharfe wrote:\n\n> Am 28.11.2022 um 16:56 schrieb René Scharfe:>\n>> The problem is \"How to use struct rev_info without leaks?\". No matter\n>> where you move it, the leak will be present until the TODO in\n>> release_revisions() is done.\n>\n> Wrong. The following sequence leaks:\n>\n> \tstruct rev_info revs;\n> \trepo_init_revisions(the_repository, &revs, NULL);\n> \trelease_revisions(&revs);\n>\n> ... and this here doesn't:\n>\n> \tstruct rev_info revs;\n> \trepo_init_revisions(the_repository, &revs, NULL);\n> \tsetup_revisions(0, NULL, &revs, NULL); // leak plugger\n> \trelease_revisions(&revs);\n>\n> That's because setup_revisions() calls diff_setup_done(), which frees\n> revs->diffopt.parseopts, and release_revisions() doesn't.\n>\n> And since builtin/pack-objects.c::get_object_list() calls\n> setup_revisions(), it really frees that memory, as you claimed from the\n> start. Sorry, I was somehow assuming that a setup function wouldn't\n> clean up. D'oh!\n>\n> The first sequence is used in some other places. e.g. builtin/prune.c.\n> But there LeakSanitizer doesn't complain for some reason; Valgrind\n> reports the parseopts allocation as \"possibly lost\".\n\nYes, some of the interactions are tricky. It's really useful to run the\ntests with GIT_TEST_PASSING_SANITIZE_LEAK=[true|check] (see t/README) to\ncheck these sorts of assumptions for sanity.\n\n> I still think the assumption that \"init_x(x); release_x(x);\" doesn't\n> leak is reasonable. Let's make it true. How about this? It's safe\n> in the sense that we don't risk double frees and it's close to the\n> TODO comment so we probably won't forget removing it once diff_free()\n> becomes used.\n>\n> ---\n> revision.c | 1 +\n> 1 file changed, 1 insertion(+)\n>\n> diff --git a/revision.c b/revision.c\n> index 439e34a7c5..6a51ef9418 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -3055,6 +3055,7 @@ void release_revisions(struct rev_info *revs)\n> \trelease_revisions_mailmap(revs->mailmap);\n> \tfree_grep_patterns(&revs->grep_filter);\n> \t/* TODO (need to handle \"no_free\"): diff_free(&revs->diffopt) */\n> +\tFREE_AND_NULL(revs->diffopt.parseopts);\n> \tdiff_free(&revs->pruning);\n> \treflog_walk_info_release(revs->reflog_info);\n> \trelease_revisions_topo_walk_info(revs->topo_walk_info);\n\nAt this point I'm unclear on what & why this is needed? I.e. once we\nnarrowly fix the >1 \"--filter\" options what still fails?\n\nBut in general: I don't really think this sort of thing is worth\nit. Here we're reaching into a member of \"revs->diffopt\" behind its back\nrather than calling diff_free(). I think we should just focus on being\nable to do do that safely.\n\nWIP patches I have in that direction, partially based on your previous\n\"is_dead\" suggestion:\n\n\thttps://github.com/avar/git/commit/e02a15f6206\n\thttps://github.com/avar/git/commit/c718f36566a\n\nI haven't poked at that in a while, I think the only outstanding issue\nwith it is that fclose() interaction.\n\nI think for this particular thing there aren't going to be any bad\nside-effects in practice, but I also think convincing oneself of that\nbasically means putting the same amount of work in as just fixing some\nof these properly.\n"},{"id":"468128","messageId":"0b86ae8b-5523-3857-cdba-12275f727cde@web.de","threadId":"58795","inReplyTo":"221128.86o7sqkjcj.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2 3/3] Revert \"pack-objects: lazily set up \"struct rev_info\", don't leak\"","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-11-28T21:57:02Z","receivedAt":"2022-11-28T21:57:20Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 28.11.2022 um 19:32 schrieb Ævar Arnfjörð Bjarmason:\n>\n> On Mon, Nov 28 2022, René Scharfe wrote:\n>\n>> Am 28.11.2022 um 16:56 schrieb René Scharfe:>\n>>> The problem is \"How to use struct rev_info without leaks?\". No matter\n>>> where you move it, the leak will be present until the TODO in\n>>> release_revisions() is done.\n>>\n>> Wrong. The following sequence leaks:\n>>\n>> \tstruct rev_info revs;\n>> \trepo_init_revisions(the_repository, &revs, NULL);\n>> \trelease_revisions(&revs);\n>>\n>> ... and this here doesn't:\n>>\n>> \tstruct rev_info revs;\n>> \trepo_init_revisions(the_repository, &revs, NULL);\n>> \tsetup_revisions(0, NULL, &revs, NULL); // leak plugger\n>> \trelease_revisions(&revs);\n>>\n>> That's because setup_revisions() calls diff_setup_done(), which frees\n>> revs->diffopt.parseopts, and release_revisions() doesn't.\n>>\n>> And since builtin/pack-objects.c::get_object_list() calls\n>> setup_revisions(), it really frees that memory, as you claimed from the\n>> start. Sorry, I was somehow assuming that a setup function wouldn't\n>> clean up. D'oh!\n>>\n>> The first sequence is used in some other places. e.g. builtin/prune.c.\n>> But there LeakSanitizer doesn't complain for some reason; Valgrind\n>> reports the parseopts allocation as \"possibly lost\".\n>\n> Yes, some of the interactions are tricky. It's really useful to run the\n> tests with GIT_TEST_PASSING_SANITIZE_LEAK=[true|check] (see t/README) to\n> check these sorts of assumptions for sanity.\n\nThat may be true, and looks even useful -- I didn't know the check\nvalue. I only get a strange error message, though:\n\n $ GIT_TEST_PASSING_SANITIZE_LEAK=check ./t0001-init.sh\n Bail out! GIT_TEST_PASSING_SANITIZE_LEAK=true has no effect except when compiled with SANITIZE=leak\n\nSame with make test and prove, of course. And of course I compiled\nwith SANITIZE=leak beforehand.\n\nBut I don't see a connection between my comment and yours. I was\nnot running any tests, just the above sequences of function calls,\ne.g. in git prune.\n\n>\n>> I still think the assumption that \"init_x(x); release_x(x);\" doesn't\n>> leak is reasonable. Let's make it true. How about this? It's safe\n>> in the sense that we don't risk double frees and it's close to the\n>> TODO comment so we probably won't forget removing it once diff_free()\n>> becomes used.\n>>\n>> ---\n>> revision.c | 1 +\n>> 1 file changed, 1 insertion(+)\n>>\n>> diff --git a/revision.c b/revision.c\n>> index 439e34a7c5..6a51ef9418 100644\n>> --- a/revision.c\n>> +++ b/revision.c\n>> @@ -3055,6 +3055,7 @@ void release_revisions(struct rev_info *revs)\n>> \trelease_revisions_mailmap(revs->mailmap);\n>> \tfree_grep_patterns(&revs->grep_filter);\n>> \t/* TODO (need to handle \"no_free\"): diff_free(&revs->diffopt) */\n>> +\tFREE_AND_NULL(revs->diffopt.parseopts);\n>> \tdiff_free(&revs->pruning);\n>> \treflog_walk_info_release(revs->reflog_info);\n>> \trelease_revisions_topo_walk_info(revs->topo_walk_info);\n>\n> At this point I'm unclear on what & why this is needed? I.e. once we\n> narrowly fix the >1 \"--filter\" options what still fails?\n\nAs I wrote: A call to an initialization function followed by a call to a\ncleanup function and nothing else shouldn't leak. There are examples of\nrepo_init_revisions()+release_revisions() without setup_revisions() or\ndiff_setup_done() beyond pack-objects. I mentioned prune, but there are\nmore, e.g. in sequencer.c.\n\n> But in general: I don't really think this sort of thing is worth\n> it. Here we're reaching into a member of \"revs->diffopt\" behind its back\n> rather than calling diff_free(). I think we should just focus on being\n> able to do do that safely.\n\nSure, but the FREE_AND_NULL call is simple and safe, while diff_free()\nis complicated and calling it one time too many can hurt.\n\n> WIP patches I have in that direction, partially based on your previous\n> \"is_dead\" suggestion:\n>\n> \thttps://github.com/avar/git/commit/e02a15f6206\n> \thttps://github.com/avar/git/commit/c718f36566a\n\nCopy-typed the interesting parts of the first patch like a medieval monk\nbecause there doesn't seem to be a download option. :-|\n\n> I haven't poked at that in a while, I think the only outstanding issue\n> with it is that fclose() interaction.\n\nYou mean the t3702-add-edit.sh failure on Windows mentioned in the\ncommit message of e02a15f6206? That's caused by the file being kept\nopen and thus locked during the call of the editor. Moving the\nrelease_revisions() call in builtin/add.c::edit_patch() before the\nlaunch_editor() call fixes that by closing the file.\n\n> I think for this particular thing there aren't going to be any bad\n> side-effects in practice, but I also think convincing oneself of that\n> basically means putting the same amount of work in as just fixing some\n> of these properly.\n\nNot to me, but perhaps that TODO is easier solved that I expected.\nIn any case, with the mentioned edit_patch() change described above\ne02a15f6206 passes the test suite on Windows for me.\n\nRené\n"},{"id":"468145","messageId":"Y4VfrsYwWkestQ05@coredump.intra.peff.net","threadId":"58795","inReplyTo":"0b86ae8b-5523-3857-cdba-12275f727cde@web.de","subject":"Re: [PATCH v2 3/3] Revert \"pack-objects: lazily set up \"struct rev_info\", don't leak\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-11-29T01:26:06Z","receivedAt":"2022-11-29T01:28:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 28, 2022 at 10:57:02PM +0100, René Scharfe wrote:\n\n> >> diff --git a/revision.c b/revision.c\n> >> index 439e34a7c5..6a51ef9418 100644\n> >> --- a/revision.c\n> >> +++ b/revision.c\n> >> @@ -3055,6 +3055,7 @@ void release_revisions(struct rev_info *revs)\n> >> \trelease_revisions_mailmap(revs->mailmap);\n> >> \tfree_grep_patterns(&revs->grep_filter);\n> >> \t/* TODO (need to handle \"no_free\"): diff_free(&revs->diffopt) */\n> >> +\tFREE_AND_NULL(revs->diffopt.parseopts);\n> >> \tdiff_free(&revs->pruning);\n> >> \treflog_walk_info_release(revs->reflog_info);\n> >> \trelease_revisions_topo_walk_info(revs->topo_walk_info);\n> >\n> > At this point I'm unclear on what & why this is needed? I.e. once we\n> > narrowly fix the >1 \"--filter\" options what still fails?\n> \n> As I wrote: A call to an initialization function followed by a call to a\n> cleanup function and nothing else shouldn't leak. There are examples of\n> repo_init_revisions()+release_revisions() without setup_revisions() or\n> diff_setup_done() beyond pack-objects. I mentioned prune, but there are\n> more, e.g. in sequencer.c.\n\nI tend to agree with you; an init+release combo should release all\nmemory. We _can_ work around it in the caller here, but I think we are\nbetter to correct the root cause.\n\nI do think what Ævar is saying is: once we have fixed the bug, why are\nmore changes needed? I.e., why would we get rid of the lazy-init. IMHO\nthe answer is that the lazy-init is a more complex pattern, and requires\nmore boilerplate code (which can lead to more bugs, as we saw). So once\nthe bug is fixed, this is purely about cleanup/simplification (if one\nignores the C-standard issues), but I tend to think it is one worth\ndoing.\n\n(Apologies to Ævar if I'm mis-stating your position).\n\n> > But in general: I don't really think this sort of thing is worth\n> > it. Here we're reaching into a member of \"revs->diffopt\" behind its back\n> > rather than calling diff_free(). I think we should just focus on being\n> > able to do do that safely.\n> \n> Sure, but the FREE_AND_NULL call is simple and safe, while diff_free()\n> is complicated and calling it one time too many can hurt.\n\nAgain, I'd agree with you here. In the long run, yes, we want\ndiff_free(). But if that is not ready yet, the FREE_AND_NULL() is a\nstepping stone that takes us in the right direction and which we can do\nimmediately.\n\n> > WIP patches I have in that direction, partially based on your previous\n> > \"is_dead\" suggestion:\n> >\n> > \thttps://github.com/avar/git/commit/e02a15f6206\n> > \thttps://github.com/avar/git/commit/c718f36566a\n> \n> Copy-typed the interesting parts of the first patch like a medieval monk\n> because there doesn't seem to be a download option. :-|\n\nThis is the actual reason I responded to your message. ;) Try:\n\n https://github.com/avar/git/commit/e02a15f6206.patch\n\netc. I don't think there's a \"raw patch\" link or similar in the\ninterface, though. You have to just know about it.\n\n-Peff\n"},{"id":"468147","messageId":"xmqqk03e4jnv.fsf@gitster.g","threadId":"58795","inReplyTo":"Y4VfrsYwWkestQ05@coredump.intra.peff.net","subject":"Re: [PATCH v2 3/3] Revert \"pack-objects: lazily set up \"struct rev_info\", don't leak\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-11-29T01:46:28Z","receivedAt":"2022-11-29T01:46:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> This is the actual reason I responded to your message. ;) Try:\n>\n> https://github.com/avar/git/commit/e02a15f6206.patch\n>\n> etc. I don't think there's a \"raw patch\" link or similar in the\n> interface, though. You have to just know about it.\n\n;-)\n"},{"id":"468159","messageId":"221129.86fse2ji6c.gmgdl@evledraar.gmail.com","threadId":"58795","inReplyTo":"0b86ae8b-5523-3857-cdba-12275f727cde@web.de","subject":"Re: [PATCH v2 3/3] Revert \"pack-objects: lazily set up \"struct rev_info\", don't leak\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-29T07:12:25Z","receivedAt":"2022-11-29T08:09:38Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Nov 28 2022, René Scharfe wrote:\n\n> Am 28.11.2022 um 19:32 schrieb Ævar Arnfjörð Bjarmason:\n>>\n>> On Mon, Nov 28 2022, René Scharfe wrote:\n>>\n>>> Am 28.11.2022 um 16:56 schrieb René Scharfe:>\n>>>> The problem is \"How to use struct rev_info without leaks?\". No matter\n>>>> where you move it, the leak will be present until the TODO in\n>>>> release_revisions() is done.\n>>>\n>>> Wrong. The following sequence leaks:\n>>>\n>>> \tstruct rev_info revs;\n>>> \trepo_init_revisions(the_repository, &revs, NULL);\n>>> \trelease_revisions(&revs);\n>>>\n>>> ... and this here doesn't:\n>>>\n>>> \tstruct rev_info revs;\n>>> \trepo_init_revisions(the_repository, &revs, NULL);\n>>> \tsetup_revisions(0, NULL, &revs, NULL); // leak plugger\n>>> \trelease_revisions(&revs);\n>>>\n>>> That's because setup_revisions() calls diff_setup_done(), which frees\n>>> revs->diffopt.parseopts, and release_revisions() doesn't.\n>>>\n>>> And since builtin/pack-objects.c::get_object_list() calls\n>>> setup_revisions(), it really frees that memory, as you claimed from the\n>>> start. Sorry, I was somehow assuming that a setup function wouldn't\n>>> clean up. D'oh!\n>>>\n>>> The first sequence is used in some other places. e.g. builtin/prune.c.\n>>> But there LeakSanitizer doesn't complain for some reason; Valgrind\n>>> reports the parseopts allocation as \"possibly lost\".\n>>\n>> Yes, some of the interactions are tricky. It's really useful to run the\n>> tests with GIT_TEST_PASSING_SANITIZE_LEAK=[true|check] (see t/README) to\n>> check these sorts of assumptions for sanity.\n>\n> That may be true, and looks even useful -- I didn't know the check\n> value. I only get a strange error message, though:\n>\n> $ GIT_TEST_PASSING_SANITIZE_LEAK=check ./t0001-init.sh\n> Bail out! GIT_TEST_PASSING_SANITIZE_LEAK=true has no effect except when compiled with SANITIZE=leak\n>\n> Same with make test and prove, of course. And of course I compiled\n> with SANITIZE=leak beforehand.\n\nThe \"=true\" part of the message is unfortunately incorrect (it pre-dates\n\"check\" being a possible value), but I don't see how you could have\ncompiled with \"SANITIZE=leak\" and get that message.\n\nIt's unreachable if 'test -n \"$SANITIZE_LEAK\"', and that'll be non-empty\nin GIT-BUILD-OPTIONS if compiled with it. Perhaps you gave SANITIZE=leak\nto t/Makefile, not the top-level Makefile?\n\nTry this at the top-level:\n\n\tGIT_TEST_PASSING_SANITIZE_LEAK=check make SANITIZE= test T=t0001-init.sh\n\n> But I don't see a connection between my comment and yours. I was\n> not running any tests, just the above sequences of function calls,\n> e.g. in git prune.\n\nThe connection is that you submitted patches that fail the CI. I don't\nthink you use GitHub, so in particular if you're submitting patches that\nclaim to do something with leak checking it's useful to run those modes\nagainst them to check your assumptions.\n\n>>\n>>> I still think the assumption that \"init_x(x); release_x(x);\" doesn't\n>>> leak is reasonable. Let's make it true. How about this? It's safe\n>>> in the sense that we don't risk double frees and it's close to the\n>>> TODO comment so we probably won't forget removing it once diff_free()\n>>> becomes used.\n>>>\n>>> ---\n>>> revision.c | 1 +\n>>> 1 file changed, 1 insertion(+)\n>>>\n>>> diff --git a/revision.c b/revision.c\n>>> index 439e34a7c5..6a51ef9418 100644\n>>> --- a/revision.c\n>>> +++ b/revision.c\n>>> @@ -3055,6 +3055,7 @@ void release_revisions(struct rev_info *revs)\n>>> \trelease_revisions_mailmap(revs->mailmap);\n>>> \tfree_grep_patterns(&revs->grep_filter);\n>>> \t/* TODO (need to handle \"no_free\"): diff_free(&revs->diffopt) */\n>>> +\tFREE_AND_NULL(revs->diffopt.parseopts);\n>>> \tdiff_free(&revs->pruning);\n>>> \treflog_walk_info_release(revs->reflog_info);\n>>> \trelease_revisions_topo_walk_info(revs->topo_walk_info);\n>>\n>> At this point I'm unclear on what & why this is needed? I.e. once we\n>> narrowly fix the >1 \"--filter\" options what still fails?\n>\n> As I wrote: A call to an initialization function followed by a call to a\n> cleanup function and nothing else shouldn't leak. There are examples of\n> repo_init_revisions()+release_revisions() without setup_revisions() or\n> diff_setup_done() beyond pack-objects. I mentioned prune, but there are\n> more, e.g. in sequencer.c.\n\nYes, I agree it shouldn't leak. And we should definitely fix those\nleaks. I just don't see why a series fixing bugs in --filter needs to\nexpand the scope to fix those.\n\n>> But in general: I don't really think this sort of thing is worth\n>> it. Here we're reaching into a member of \"revs->diffopt\" behind its back\n>> rather than calling diff_free(). I think we should just focus on being\n>> able to do do that safely.\n>\n> Sure, but the FREE_AND_NULL call is simple and safe, while diff_free()\n> is complicated and calling it one time too many can hurt.\n\nIt's \"safe\" because you've read the internals of it, and know that it\nisn't assuming a non-NULL there once it's past initialization?\n\nOr is it like the revisions init()+release() in this thread, where\nyou're assuming it works one way based on the function names etc., only\nfor the CI to fail?\n\nIn either case, I'm saying that if someone's confident enough to reach\ninto the internals of a structure and tweak it they should be confident\nenough to just patch diff_free() or the like.\n\n>> WIP patches I have in that direction, partially based on your previous\n>> \"is_dead\" suggestion:\n>>\n>> \thttps://github.com/avar/git/commit/e02a15f6206\n>> \thttps://github.com/avar/git/commit/c718f36566a\n>\n> Copy-typed the interesting parts of the first patch like a medieval monk\n> because there doesn't seem to be a download option. :-|\n\nJeff pointed out the \".patch\" (there's also \".diff\"), but also: Git has\nthis well-known transport protocol it uses, which typically maps to the\nweb URL on public hosting sites ... :)\n\n\tgit remote add avar https://github.com/avar/git.git\n\tgit fetch avar\n\tgit show \n\n>> I haven't poked at that in a while, I think the only outstanding issue\n>> with it is that fclose() interaction.\n>\n> You mean the t3702-add-edit.sh failure on Windows mentioned in the\n> commit message of e02a15f6206? That's caused by the file being kept\n> open and thus locked during the call of the editor. Moving the\n> release_revisions() call in builtin/add.c::edit_patch() before the\n> launch_editor() call fixes that by closing the file.\n\nYeah, I haven't had time to poke at it in a while, and I had it queued\nbehind various other leak fixes.\n\nI'm sure it's not that complicated in the end, just that that\ninteraction needs to be dealt with.\n\n>> I think for this particular thing there aren't going to be any bad\n>> side-effects in practice, but I also think convincing oneself of that\n>> basically means putting the same amount of work in as just fixing some\n>> of these properly.\n>\n> Not to me, but perhaps that TODO is easier solved that I expected.\n> In any case, with the mentioned edit_patch() change described above\n> e02a15f6206 passes the test suite on Windows for me.\n\nNice! I'll try that out.\n"},{"id":"468164","messageId":"221129.86bkoqjb8v.gmgdl@evledraar.gmail.com","threadId":"58795","inReplyTo":"Y4VfrsYwWkestQ05@coredump.intra.peff.net","subject":"Re: [PATCH v2 3/3] Revert \"pack-objects: lazily set up \"struct rev_info\", don't leak\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-29T10:25:12Z","receivedAt":"2022-11-29T10:39:21Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Nov 28 2022, Jeff King wrote:\n\n> On Mon, Nov 28, 2022 at 10:57:02PM +0100, René Scharfe wrote:\n>\n>> >> diff --git a/revision.c b/revision.c\n>> >> index 439e34a7c5..6a51ef9418 100644\n>> >> --- a/revision.c\n>> >> +++ b/revision.c\n>> >> @@ -3055,6 +3055,7 @@ void release_revisions(struct rev_info *revs)\n>> >> \trelease_revisions_mailmap(revs->mailmap);\n>> >> \tfree_grep_patterns(&revs->grep_filter);\n>> >> \t/* TODO (need to handle \"no_free\"): diff_free(&revs->diffopt) */\n>> >> +\tFREE_AND_NULL(revs->diffopt.parseopts);\n>> >> \tdiff_free(&revs->pruning);\n>> >> \treflog_walk_info_release(revs->reflog_info);\n>> >> \trelease_revisions_topo_walk_info(revs->topo_walk_info);\n>> >\n>> > At this point I'm unclear on what & why this is needed? I.e. once we\n>> > narrowly fix the >1 \"--filter\" options what still fails?\n>> \n>> As I wrote: A call to an initialization function followed by a call to a\n>> cleanup function and nothing else shouldn't leak. There are examples of\n>> repo_init_revisions()+release_revisions() without setup_revisions() or\n>> diff_setup_done() beyond pack-objects. I mentioned prune, but there are\n>> more, e.g. in sequencer.c.\n>\n> I tend to agree with you; an init+release combo should release all\n> memory. We _can_ work around it in the caller here, but I think we are\n> better to correct the root cause.\n>\n> I do think what Ævar is saying is: once we have fixed the bug, why are\n> more changes needed? I.e., why would we get rid of the lazy-init. IMHO\n> the answer is that the lazy-init is a more complex pattern, and requires\n> more boilerplate code (which can lead to more bugs, as we saw). So once\n> the bug is fixed, this is purely about cleanup/simplification (if one\n> ignores the C-standard issues), but I tend to think it is one worth\n> doing.\n>\n> (Apologies to Ævar if I'm mis-stating your position).\n\nMy position isn't that it isn't worth doing, but that we can clearly\nproceed in smaller steps here, and changing one thing at a time is\nbetter.\n\nSo far in this thread we've had, in relation to 3/3 (and I may have lost\ntrack of some points made):\n\n1. It's fixing the bug where you provide --filter N times\n2. It's refactoring the \"lazy init\" to \"non-lazy init\"\n3. It's refactoring the code to avoid relying on the J.5.7 extension in C99.\n4. Due to #2 we run into more leaks\n5. A dozen or so test files fail due to #2. Are we digressing to fix the\n root cause of the leak(s), or un-marking those tests as passing\n leak-free & losing test coverage?\n\nAs\nhttps://lore.kernel.org/git/221128.868rjvmi3l.gmgdl@evledraar.gmail.com/\nshows we can do #1 as a stand-alone change.\n\nI'm not saying the rest of this isn't worth pursuing, except to point\nout in\nhttps://lore.kernel.org/git/221128.86zgcbl0pe.gmgdl@evledraar.gmail.com/\nthat #2 can be split off from #3.\n"},{"id":"468170","messageId":"c2d3e7a3-599f-dae5-8dde-dc969df39f02@web.de","threadId":"58795","inReplyTo":"c64e4fa5-62c2-2a93-a4ef-bd84407ea570@web.de","subject":"[PATCH v3 0/5] pack-objects: fix and simplify --filter handling","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-11-29T12:19:16Z","receivedAt":"2022-11-29T12:19:29Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Fix a regression that prevents using multiple --filter options, simplify\nthe option parsing code and avoid relying on undefined behavior in it.\n\nPatch 4 conflicts with cc/filtered-repack in seen, but not semantically.\n\nChanges since v2:\n- Split the code changes up again like in v1; keep the separate test\n patches. Reverting 5cb28270a1 wholesale is too cumbersome.\n- Bring the dedicated list_objects_filter_options struct back.\n- Move the list_objects_filter_release() call to the cleanup section.\n\nChanges since v1:\n- Added patch 1 to fix an issue with existing tests.\n- Separate patch 2 for new tests.\n- Test using blob size filters, only, which is a bit simpler.\n- Test both combinations to also catch not just the current\n last-one-wins regression, but also a possible future first-one-wins\n issue.\n- Actually revert 5cb28270a1 (pack-objects: lazily set up\n \"struct rev_info\", don't leak, 2022-03-28) instead of having a\n minimal fix and then adding some kind of middle ground by using a\n separate struct list_objects_filter_options.\n\n t5317: stop losing return codes of git ls-files\n t5317: demonstrate failure to handle multiple --filter options\n pack-objects: fix handling of multiple --filter options\n pack-objects: simplify --filter handling\n list-objects-filter: remove OPT_PARSE_LIST_OBJECTS_FILTER_INIT()\n\n builtin/pack-objects.c | 27 ++------\n list-objects-filter-options.c | 4 --\n list-objects-filter-options.h | 18 +-----\n t/t5317-pack-objects-filter-objects.sh | 90 +++++++++++++++++++-------\n 4 files changed, 74 insertions(+), 65 deletions(-)\n\nRange-Diff gegen v2:\n1: 955ec33c30 = 1: 955ec33c30 t5317: stop losing return codes of git ls-files\n2: 966094ef98 = 2: 966094ef98 t5317: demonstrate failure to handle multiple --filter options\n3: f5ba2a2f5e < -: ---------- Revert \"pack-objects: lazily set up \"struct rev_info\", don't leak\"\n-: ---------- > 3: d51424e8d1 pack-objects: fix handling of multiple --filter options\n-: ---------- > 4: e1fa0fcb1a pack-objects: simplify --filter handling\n-: ---------- > 5: 5865e24c04 list-objects-filter: remove OPT_PARSE_LIST_OBJECTS_FILTER_INIT()\n--\n2.38.1\n"},{"id":"468171","messageId":"05f162e2-0687-af8e-d4f8-4ab2ea003518@web.de","threadId":"58795","inReplyTo":"c2d3e7a3-599f-dae5-8dde-dc969df39f02@web.de","subject":"[PATCH v3 1/5] t5317: stop losing return codes of git ls-files","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-11-29T12:21:17Z","receivedAt":"2022-11-29T12:21:40Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"fb2d0db502 (test-lib-functions: add parsing helpers for ls-files and\nls-tree, 2022-04-04) not only started to use helper functions, it also\nstarted to pipe the output of git ls-files into them directly, without\nusing a temporary file. No explanation was given. This causes the\nreturn code of that git command to be ignored.\n\nRevert that part of the change, use temporary files and check the return\ncode of git ls-files again.\n\nSuggested-by: Ævar Arnfjörð Bjarmason \nSigned-off-by: René Scharfe \n---\n t/t5317-pack-objects-filter-objects.sh | 52 ++++++++++++++------------\n 1 file changed, 28 insertions(+), 24 deletions(-)\n\ndiff --git a/t/t5317-pack-objects-filter-objects.sh b/t/t5317-pack-objects-filter-objects.sh\nindex bb633c9b09..82a22ecaa5 100755\n--- a/t/t5317-pack-objects-filter-objects.sh\n+++ b/t/t5317-pack-objects-filter-objects.sh\n@@ -24,8 +24,9 @@ parse_verify_pack_blob_oid () {\n }\n\n test_expect_success 'verify blob count in normal packfile' '\n-\tgit -C r1 ls-files -s file.1 file.2 file.3 file.4 file.5 |\n-\ttest_parse_ls_files_stage_oids |\n+\tgit -C r1 ls-files -s file.1 file.2 file.3 file.4 file.5 \\\n+\t\t>ls_files_result &&\n+\ttest_parse_ls_files_stage_oids expected &&\n\n \tgit -C r1 pack-objects --revs --stdout >all.pack <<-EOF &&\n@@ -123,8 +124,8 @@ test_expect_success 'setup r2' '\n '\n\n test_expect_success 'verify blob count in normal packfile' '\n-\tgit -C r2 ls-files -s large.1000 large.10000 |\n-\ttest_parse_ls_files_stage_oids |\n+\tgit -C r2 ls-files -s large.1000 large.10000 >ls_files_result &&\n+\ttest_parse_ls_files_stage_oids expected &&\n\n \tgit -C r2 pack-objects --revs --stdout >all.pack <<-EOF &&\n@@ -161,8 +162,8 @@ test_expect_success 'verify blob:limit=1000' '\n '\n\n test_expect_success 'verify blob:limit=1001' '\n-\tgit -C r2 ls-files -s large.1000 |\n-\ttest_parse_ls_files_stage_oids |\n+\tgit -C r2 ls-files -s large.1000 >ls_files_result &&\n+\ttest_parse_ls_files_stage_oids expected &&\n\n \tgit -C r2 pack-objects --revs --stdout --filter=blob:limit=1001 >filter.pack <<-EOF &&\n@@ -179,8 +180,8 @@ test_expect_success 'verify blob:limit=1001' '\n '\n\n test_expect_success 'verify blob:limit=10001' '\n-\tgit -C r2 ls-files -s large.1000 large.10000 |\n-\ttest_parse_ls_files_stage_oids |\n+\tgit -C r2 ls-files -s large.1000 large.10000 >ls_files_result &&\n+\ttest_parse_ls_files_stage_oids expected &&\n\n \tgit -C r2 pack-objects --revs --stdout --filter=blob:limit=10001 >filter.pack <<-EOF &&\n@@ -197,8 +198,8 @@ test_expect_success 'verify blob:limit=10001' '\n '\n\n test_expect_success 'verify blob:limit=1k' '\n-\tgit -C r2 ls-files -s large.1000 |\n-\ttest_parse_ls_files_stage_oids |\n+\tgit -C r2 ls-files -s large.1000 >ls_files_result &&\n+\ttest_parse_ls_files_stage_oids expected &&\n\n \tgit -C r2 pack-objects --revs --stdout --filter=blob:limit=1k >filter.pack <<-EOF &&\n@@ -215,8 +216,8 @@ test_expect_success 'verify blob:limit=1k' '\n '\n\n test_expect_success 'verify explicitly specifying oversized blob in input' '\n-\tgit -C r2 ls-files -s large.1000 large.10000 |\n-\ttest_parse_ls_files_stage_oids |\n+\tgit -C r2 ls-files -s large.1000 large.10000 >ls_files_result &&\n+\ttest_parse_ls_files_stage_oids expected &&\n\n \techo HEAD >objects &&\n@@ -233,8 +234,8 @@ test_expect_success 'verify explicitly specifying oversized blob in input' '\n '\n\n test_expect_success 'verify blob:limit=1m' '\n-\tgit -C r2 ls-files -s large.1000 large.10000 |\n-\ttest_parse_ls_files_stage_oids |\n+\tgit -C r2 ls-files -s large.1000 large.10000 >ls_files_result &&\n+\ttest_parse_ls_files_stage_oids expected &&\n\n \tgit -C r2 pack-objects --revs --stdout --filter=blob:limit=1m >filter.pack <<-EOF &&\n@@ -289,8 +290,9 @@ test_expect_success 'setup r3' '\n '\n\n test_expect_success 'verify blob count in normal packfile' '\n-\tgit -C r3 ls-files -s sparse1 sparse2 dir1/sparse1 dir1/sparse2 |\n-\ttest_parse_ls_files_stage_oids |\n+\tgit -C r3 ls-files -s sparse1 sparse2 dir1/sparse1 dir1/sparse2 \\\n+\t\t>ls_files_result &&\n+\ttest_parse_ls_files_stage_oids expected &&\n\n \tgit -C r3 pack-objects --revs --stdout >all.pack <<-EOF &&\n@@ -341,8 +343,9 @@ test_expect_success 'setup r4' '\n '\n\n test_expect_success 'verify blob count in normal packfile' '\n-\tgit -C r4 ls-files -s pattern sparse1 sparse2 dir1/sparse1 dir1/sparse2 |\n-\ttest_parse_ls_files_stage_oids |\n+\tgit -C r4 ls-files -s pattern sparse1 sparse2 dir1/sparse1 dir1/sparse2 \\\n+\t\t>ls_files_result &&\n+\ttest_parse_ls_files_stage_oids expected &&\n\n \tgit -C r4 pack-objects --revs --stdout >all.pack <<-EOF &&\n@@ -359,8 +362,8 @@ test_expect_success 'verify blob count in normal packfile' '\n '\n\n test_expect_success 'verify sparse:oid=OID' '\n-\tgit -C r4 ls-files -s dir1/sparse1 dir1/sparse2 |\n-\ttest_parse_ls_files_stage_oids |\n+\tgit -C r4 ls-files -s dir1/sparse1 dir1/sparse2 >ls_files_result &&\n+\ttest_parse_ls_files_stage_oids expected &&\n\n \tgit -C r4 ls-files -s pattern >staged &&\n@@ -379,8 +382,8 @@ test_expect_success 'verify sparse:oid=OID' '\n '\n\n test_expect_success 'verify sparse:oid=oid-ish' '\n-\tgit -C r4 ls-files -s dir1/sparse1 dir1/sparse2 |\n-\ttest_parse_ls_files_stage_oids |\n+\tgit -C r4 ls-files -s dir1/sparse1 dir1/sparse2 >ls_files_result &&\n+\ttest_parse_ls_files_stage_oids expected &&\n\n \tgit -C r4 pack-objects --revs --stdout --filter=sparse:oid=main:pattern >filter.pack <<-EOF &&\n@@ -400,8 +403,9 @@ test_expect_success 'verify sparse:oid=oid-ish' '\n # This models previously omitted objects that we did not receive.\n\n test_expect_success 'setup r1 - delete loose blobs' '\n-\tgit -C r1 ls-files -s file.1 file.2 file.3 file.4 file.5 |\n-\ttest_parse_ls_files_stage_oids |\n+\tgit -C r1 ls-files -s file.1 file.2 file.3 file.4 file.5 \\\n+\t\t>ls_files_result &&\n+\ttest_parse_ls_files_stage_oids expected &&\n\n \tfor id in `cat expected | sed \"s|..|&/|\"`\n--\n2.38.1\n"},{"id":"468172","messageId":"4d5a63e2-e4dc-1f51-cbfa-621c7bba0d9a@web.de","threadId":"58795","inReplyTo":"c2d3e7a3-599f-dae5-8dde-dc969df39f02@web.de","subject":"[PATCH v3 2/5] t5317: demonstrate failure to handle multiple --filter options","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-11-29T12:22:26Z","receivedAt":"2022-11-29T12:22:45Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"git pack-objects should accept multiple --filter options as documented\nin Documentation/rev-list-options.txt, but currently the last one wins.\nShow that using tests with multiple blob size limits\n\nSigned-off-by: René Scharfe \n---\n t/t5317-pack-objects-filter-objects.sh | 38 ++++++++++++++++++++++++++\n 1 file changed, 38 insertions(+)\n\ndiff --git a/t/t5317-pack-objects-filter-objects.sh b/t/t5317-pack-objects-filter-objects.sh\nindex 82a22ecaa5..25faebaada 100755\n--- a/t/t5317-pack-objects-filter-objects.sh\n+++ b/t/t5317-pack-objects-filter-objects.sh\n@@ -265,6 +265,44 @@ test_expect_success 'verify normal and blob:limit packfiles have same commits/tr\n \ttest_cmp expected observed\n '\n\n+test_expect_failure 'verify small limit and big limit results in small limit' '\n+\tgit -C r2 ls-files -s large.1000 >ls_files_result &&\n+\ttest_parse_ls_files_stage_oids expected &&\n+\n+\tgit -C r2 pack-objects --revs --stdout --filter=blob:limit=1001 \\\n+\t\t--filter=blob:limit=10001 >filter.pack <<-EOF &&\n+\tHEAD\n+\tEOF\n+\tgit -C r2 index-pack ../filter.pack &&\n+\n+\tgit -C r2 verify-pack -v ../filter.pack >verify_result &&\n+\tgrep blob verify_result |\n+\tparse_verify_pack_blob_oid |\n+\tsort >observed &&\n+\n+\ttest_cmp expected observed\n+'\n+\n+test_expect_success 'verify big limit and small limit results in small limit' '\n+\tgit -C r2 ls-files -s large.1000 >ls_files_result &&\n+\ttest_parse_ls_files_stage_oids expected &&\n+\n+\tgit -C r2 pack-objects --revs --stdout --filter=blob:limit=10001 \\\n+\t\t--filter=blob:limit=1001 >filter.pack <<-EOF &&\n+\tHEAD\n+\tEOF\n+\tgit -C r2 index-pack ../filter.pack &&\n+\n+\tgit -C r2 verify-pack -v ../filter.pack >verify_result &&\n+\tgrep blob verify_result |\n+\tparse_verify_pack_blob_oid |\n+\tsort >observed &&\n+\n+\ttest_cmp expected observed\n+'\n+\n # Test sparse:path= filter.\n # !!!!\n # NOTE: sparse:path filter support has been dropped for security reasons,\n--\n2.38.1\n"},{"id":"468173","messageId":"6c7ef5fb-59b5-df16-036d-9a9a3d987cb0@web.de","threadId":"58795","inReplyTo":"c2d3e7a3-599f-dae5-8dde-dc969df39f02@web.de","subject":"[PATCH v3 3/5] pack-objects: fix handling of multiple --filter options","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-11-29T12:23:53Z","receivedAt":"2022-11-29T12:24:13Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Since 5cb28270a1 (pack-objects: lazily set up \"struct rev_info\", don't\nleak, 2022-03-28) --filter options given to git pack-objects overrule\nearlier ones, letting only the leftmost win and leaking the memory\nallocated for earlier ones. Fix that by only initializing the rev_info\nstruct once.\n\nSigned-off-by: René Scharfe \n---\n builtin/pack-objects.c | 3 ++-\n t/t5317-pack-objects-filter-objects.sh | 2 +-\n 2 files changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 573d0b20b7..c702c09dd4 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -4158,7 +4158,8 @@ static struct list_objects_filter_options *po_filter_revs_init(void *value)\n {\n \tstruct po_filter_data *data = value;\n\n-\trepo_init_revisions(the_repository, &data->revs, NULL);\n+\tif (!data->have_revs)\n+\t\trepo_init_revisions(the_repository, &data->revs, NULL);\n \tdata->have_revs = 1;\n\n \treturn &data->revs.filter;\ndiff --git a/t/t5317-pack-objects-filter-objects.sh b/t/t5317-pack-objects-filter-objects.sh\nindex 25faebaada..5b707d911b 100755\n--- a/t/t5317-pack-objects-filter-objects.sh\n+++ b/t/t5317-pack-objects-filter-objects.sh\n@@ -265,7 +265,7 @@ test_expect_success 'verify normal and blob:limit packfiles have same commits/tr\n \ttest_cmp expected observed\n '\n\n-test_expect_failure 'verify small limit and big limit results in small limit' '\n+test_expect_success 'verify small limit and big limit results in small limit' '\n \tgit -C r2 ls-files -s large.1000 >ls_files_result &&\n \ttest_parse_ls_files_stage_oids expected &&\n--\n2.38.1\n"},{"id":"468174","messageId":"d7a86408-da5c-10ca-90e8-01bc7e268d3e@web.de","threadId":"58795","inReplyTo":"c2d3e7a3-599f-dae5-8dde-dc969df39f02@web.de","subject":"[PATCH v3 4/5] pack-objects: simplify --filter handling","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-11-29T12:25:05Z","receivedAt":"2022-11-29T12:25:19Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"pack-objects uses OPT_PARSE_LIST_OBJECTS_FILTER_INIT() to initialize the\na rev_info struct lazily before populating its filter member using the\n--filter option values. It tracks whether the initialization is needed\nusing the .have_revs member of the callback data.\n\nThere is a better way: Use a stand-alone list_objects_filter_options\nstruct and build a rev_info struct with its .filter member after option\nparsing. This allows using the simpler OPT_PARSE_LIST_OBJECTS_FILTER()\nand getting rid of the extra callback mechanism.\n\nEven simpler would be using a struct rev_info as before 5cb28270a1\n(pack-objects: lazily set up \"struct rev_info\", don't leak, 2022-03-28),\nbut that would expose a memory leak caused by repo_init_revisions()\nfollowed by release_revisions() without a setup_revisions() call in\nbetween.\n\nUsing list_objects_filter_options also allows pushing the rev_info\nstruct into get_object_list(), where it arguably belongs. Either way,\nthis is all left for later.\n\nHelped-by: Ævar Arnfjörð Bjarmason \nSigned-off-by: René Scharfe \n---\n builtin/pack-objects.c | 28 ++++++----------------------\n 1 file changed, 6 insertions(+), 22 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex c702c09dd4..2193f80b89 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -4149,22 +4149,6 @@ static int option_parse_cruft_expiration(const struct option *opt,\n \treturn 0;\n }\n\n-struct po_filter_data {\n-\tunsigned have_revs:1;\n-\tstruct rev_info revs;\n-};\n-\n-static struct list_objects_filter_options *po_filter_revs_init(void *value)\n-{\n-\tstruct po_filter_data *data = value;\n-\n-\tif (!data->have_revs)\n-\t\trepo_init_revisions(the_repository, &data->revs, NULL);\n-\tdata->have_revs = 1;\n-\n-\treturn &data->revs.filter;\n-}\n-\n int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n {\n \tint use_internal_rev_list = 0;\n@@ -4175,7 +4159,8 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \tint rev_list_index = 0;\n \tint stdin_packs = 0;\n \tstruct string_list keep_pack_list = STRING_LIST_INIT_NODUP;\n-\tstruct po_filter_data pfd = { .have_revs = 0 };\n+\tstruct list_objects_filter_options filter_options =\n+\t\tLIST_OBJECTS_FILTER_INIT;\n\n \tstruct option pack_objects_options[] = {\n \t\tOPT_SET_INT('q', \"quiet\", &progress,\n@@ -4266,7 +4251,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \t\t\t &write_bitmap_index,\n \t\t\t N_(\"write a bitmap index if possible\"),\n \t\t\t WRITE_BITMAP_QUIET, PARSE_OPT_HIDDEN),\n-\t\tOPT_PARSE_LIST_OBJECTS_FILTER_INIT(&pfd, po_filter_revs_init),\n+\t\tOPT_PARSE_LIST_OBJECTS_FILTER(&filter_options),\n \t\tOPT_CALLBACK_F(0, \"missing\", NULL, N_(\"action\"),\n \t\t N_(\"handling for missing objects\"), PARSE_OPT_NONEG,\n \t\t option_parse_missing_action),\n@@ -4386,7 +4371,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \tif (!rev_list_all || !rev_list_reflog || !rev_list_index)\n \t\tunpack_unreachable_expiration = 0;\n\n-\tif (pfd.have_revs && pfd.revs.filter.choice) {\n+\tif (filter_options.choice) {\n \t\tif (!pack_to_stdout)\n \t\t\tdie(_(\"cannot use --filter without --stdout\"));\n \t\tif (stdin_packs)\n@@ -4473,13 +4458,11 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \t\tread_cruft_objects();\n \t} else if (!use_internal_rev_list) {\n \t\tread_object_list_from_stdin();\n-\t} else if (pfd.have_revs) {\n-\t\tget_object_list(&pfd.revs, rp.nr, rp.v);\n-\t\trelease_revisions(&pfd.revs);\n \t} else {\n \t\tstruct rev_info revs;\n\n \t\trepo_init_revisions(the_repository, &revs, NULL);\n+\t\tlist_objects_filter_copy(&revs.filter, &filter_options);\n \t\tget_object_list(&revs, rp.nr, rp.v);\n \t\trelease_revisions(&revs);\n \t}\n@@ -4514,6 +4497,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \t\t\t reuse_packfile_objects);\n\n cleanup:\n+\tlist_objects_filter_release(&filter_options);\n \tstrvec_clear(&rp);\n\n \treturn 0;\n--\n2.38.1\n"},{"id":"468175","messageId":"accc4f29-8e8e-5b8b-9409-6f2ac51c2936@web.de","threadId":"58795","inReplyTo":"c2d3e7a3-599f-dae5-8dde-dc969df39f02@web.de","subject":"[PATCH v3 5/5] list-objects-filter: remove OPT_PARSE_LIST_OBJECTS_FILTER_INIT()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-11-29T12:26:44Z","receivedAt":"2022-11-29T12:26:59Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"OPT_PARSE_LIST_OBJECTS_FILTER_INIT() with a non-NULL second argument\npasses a function pointer via an object pointer, which is undefined. It\nmay work fine on platforms that implement C99 extension J.5.7 (Function\npointer casts). Remove the unused macro and avoid the dependency on\nthat extension.\n\nSigned-off-by: René Scharfe \n---\n list-objects-filter-options.c | 4 ----\n list-objects-filter-options.h | 18 ++----------------\n 2 files changed, 2 insertions(+), 20 deletions(-)\n\ndiff --git a/list-objects-filter-options.c b/list-objects-filter-options.c\nindex 5339660238..ee01bcd2cc 100644\n--- a/list-objects-filter-options.c\n+++ b/list-objects-filter-options.c\n@@ -290,10 +290,6 @@ int opt_parse_list_objects_filter(const struct option *opt,\n \t\t\t\t const char *arg, int unset)\n {\n \tstruct list_objects_filter_options *filter_options = opt->value;\n-\topt_lof_init init = (opt_lof_init)opt->defval;\n-\n-\tif (init)\n-\t\tfilter_options = init(opt->value);\n\n \tif (unset || !arg)\n \t\tlist_objects_filter_set_no_filter(filter_options);\ndiff --git a/list-objects-filter-options.h b/list-objects-filter-options.h\nindex 7eeadab2dd..1fe393f447 100644\n--- a/list-objects-filter-options.h\n+++ b/list-objects-filter-options.h\n@@ -111,27 +111,13 @@ void parse_list_objects_filter(\n * The opt->value to opt_parse_list_objects_filter() is either a\n * \"struct list_objects_filter_option *\" when using\n * OPT_PARSE_LIST_OBJECTS_FILTER().\n- *\n- * Or, if using no \"struct option\" field is used by the callback,\n- * except the \"defval\" which is expected to be an \"opt_lof_init\"\n- * function, which is called with the \"opt->value\" and must return a\n- * pointer to the \"\"struct list_objects_filter_option *\" to be used.\n- *\n- * The OPT_PARSE_LIST_OBJECTS_FILTER_INIT() can be used e.g. the\n- * \"struct list_objects_filter_option\" is embedded in a \"struct\n- * rev_info\", which the \"defval\" could be tasked with lazily\n- * initializing. See cmd_pack_objects() for an example.\n */\n int opt_parse_list_objects_filter(const struct option *opt,\n \t\t\t\t const char *arg, int unset);\n-typedef struct list_objects_filter_options *(*opt_lof_init)(void *);\n-#define OPT_PARSE_LIST_OBJECTS_FILTER_INIT(fo, init) \\\n-\t{ OPTION_CALLBACK, 0, \"filter\", (fo), N_(\"args\"), \\\n-\t N_(\"object filtering\"), 0, opt_parse_list_objects_filter, \\\n-\t (intptr_t)(init) }\n\n #define OPT_PARSE_LIST_OBJECTS_FILTER(fo) \\\n-\tOPT_PARSE_LIST_OBJECTS_FILTER_INIT((fo), NULL)\n+\tOPT_CALLBACK(0, \"filter\", (fo), N_(\"args\"), \\\n+\t\t N_(\"object filtering\"), opt_parse_list_objects_filter)\n\n /*\n * Translates abbreviated numbers in the filter's filter_spec into their\n--\n2.38.1\n"},{"id":"468186","messageId":"221129.8635a1kglk.gmgdl@evledraar.gmail.com","threadId":"58795","inReplyTo":"d7a86408-da5c-10ca-90e8-01bc7e268d3e@web.de","subject":"Re: [PATCH v3 4/5] pack-objects: simplify --filter handling","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-29T13:27:49Z","receivedAt":"2022-11-29T13:58:22Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Nov 29 2022, René Scharfe wrote:\n\nFirst, I think 3/5 in this series looks really good in this iteration.\n\nI think this is also worth cleaning up, but depending on when we expect\nthis to be queued maybe splitting these post-cleanups into their own\nseries would make sense?\n\n> pack-objects uses OPT_PARSE_LIST_OBJECTS_FILTER_INIT() to initialize the\n> a rev_info struct lazily before populating its filter member using the\n> --filter option values. It tracks whether the initialization is needed\n> using the .have_revs member of the callback data.\n>\n> There is a better way: Use a stand-alone list_objects_filter_options\n> struct and build a rev_info struct with its .filter member after option\n> parsing. This allows using the simpler OPT_PARSE_LIST_OBJECTS_FILTER()\n> and getting rid of the extra callback mechanism.\n>\n> Even simpler would be using a struct rev_info as before 5cb28270a1\n> (pack-objects: lazily set up \"struct rev_info\", don't leak, 2022-03-28),\n> but that would expose a memory leak caused by repo_init_revisions()\n> followed by release_revisions() without a setup_revisions() call in\n> between.\n>\n> Using list_objects_filter_options also allows pushing the rev_info\n> struct into get_object_list(), where it arguably belongs. Either way,\n> this is all left for later.\n\nI think what you're missing here (or not explaining) is that *the\nreason* for us not being able to return to a pre-5cb28270a1 state is\nthat if we initialize with REV_INFO_INIT we'll first of all find that\nthe \"filter\" isn't initialized, because it's not in that macro.\n\nAnd even if it were there, the repo_init_revisions() will wipe away\nanything we add to it once we call repo_init_revisions(), which we'll\nneed to do any \"real\" work with it.\n\nBut if we're really going to refactor this properly let's stop beating\naround the bush and just connect those dots. So first, the only reason\nfor why \"filter\" isn't there yet is because a recent topic would have\nconflicted with it, so if this is cherry-picked after 3/5 we'll have\nthat: https://github.com/avar/git/commit/6b29069d72f.patch\n\nThen the below change on top of that could replace this 4/5 and\n5/5. I.e. also the 5/5 because the only reason we need the\n\"opt_lof_init\" is because of this init-ing edge case.\n\n\t builtin/pack-objects.c | 32 +++++++-------------------------\n\t list-objects-filter-options.c | 4 ----\n\t list-objects-filter-options.h | 29 ++++++++++-------------------\n\t revision.c | 19 +++++++++++++------\n\t revision.h | 9 +++++++++\n\t 5 files changed, 39 insertions(+), 54 deletions(-)\n\t\n\tdiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n\tindex c702c09dd45..d0c4c26f4e9 100644\n\t--- a/builtin/pack-objects.c\n\t+++ b/builtin/pack-objects.c\n\t@@ -4149,22 +4149,6 @@ static int option_parse_cruft_expiration(const struct option *opt,\n\t \treturn 0;\n\t }\n\t \n\t-struct po_filter_data {\n\t-\tunsigned have_revs:1;\n\t-\tstruct rev_info revs;\n\t-};\n\t-\n\t-static struct list_objects_filter_options *po_filter_revs_init(void *value)\n\t-{\n\t-\tstruct po_filter_data *data = value;\n\t-\n\t-\tif (!data->have_revs)\n\t-\t\trepo_init_revisions(the_repository, &data->revs, NULL);\n\t-\tdata->have_revs = 1;\n\t-\n\t-\treturn &data->revs.filter;\n\t-}\n\t-\n\t int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n\t {\n\t \tint use_internal_rev_list = 0;\n\t@@ -4175,7 +4159,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n\t \tint rev_list_index = 0;\n\t \tint stdin_packs = 0;\n\t \tstruct string_list keep_pack_list = STRING_LIST_INIT_NODUP;\n\t-\tstruct po_filter_data pfd = { .have_revs = 0 };\n\t+\tstruct rev_info revs = REV_INFO_INIT;\n\t \n\t \tstruct option pack_objects_options[] = {\n\t \t\tOPT_SET_INT('q', \"quiet\", &progress,\n\t@@ -4266,7 +4250,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n\t \t\t\t &write_bitmap_index,\n\t \t\t\t N_(\"write a bitmap index if possible\"),\n\t \t\t\t WRITE_BITMAP_QUIET, PARSE_OPT_HIDDEN),\n\t-\t\tOPT_PARSE_LIST_OBJECTS_FILTER_INIT(&pfd, po_filter_revs_init),\n\t+\t\tOPT_PARSE_LIST_OBJECTS_FILTER(&revs.filter),\n\t \t\tOPT_CALLBACK_F(0, \"missing\", NULL, N_(\"action\"),\n\t \t\t N_(\"handling for missing objects\"), PARSE_OPT_NONEG,\n\t \t\t option_parse_missing_action),\n\t@@ -4386,7 +4370,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n\t \tif (!rev_list_all || !rev_list_reflog || !rev_list_index)\n\t \t\tunpack_unreachable_expiration = 0;\n\t \n\t-\tif (pfd.have_revs && pfd.revs.filter.choice) {\n\t+\tif (revs.filter.choice) {\n\t \t\tif (!pack_to_stdout)\n\t \t\t\tdie(_(\"cannot use --filter without --stdout\"));\n\t \t\tif (stdin_packs)\n\t@@ -4473,16 +4457,14 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n\t \t\tread_cruft_objects();\n\t \t} else if (!use_internal_rev_list) {\n\t \t\tread_object_list_from_stdin();\n\t-\t} else if (pfd.have_revs) {\n\t-\t\tget_object_list(&pfd.revs, rp.nr, rp.v);\n\t-\t\trelease_revisions(&pfd.revs);\n\t+\t} else if (revs.filter.choice) {\n\t+\t\trepo_dynamic_init_revisions(the_repository, &revs, NULL);\n\t+\t\tget_object_list(&revs, rp.nr, rp.v);\n\t \t} else {\n\t-\t\tstruct rev_info revs;\n\t-\n\t \t\trepo_init_revisions(the_repository, &revs, NULL);\n\t \t\tget_object_list(&revs, rp.nr, rp.v);\n\t-\t\trelease_revisions(&revs);\n\t \t}\n\t+\trelease_revisions(&revs);\n\t \tcleanup_preferred_base();\n\t \tif (include_tag && nr_result)\n\t \t\tfor_each_tag_ref(add_ref_tag, NULL);\n\tdiff --git a/list-objects-filter-options.c b/list-objects-filter-options.c\n\tindex 53396602380..ee01bcd2cc3 100644\n\t--- a/list-objects-filter-options.c\n\t+++ b/list-objects-filter-options.c\n\t@@ -290,10 +290,6 @@ int opt_parse_list_objects_filter(const struct option *opt,\n\t \t\t\t\t const char *arg, int unset)\n\t {\n\t \tstruct list_objects_filter_options *filter_options = opt->value;\n\t-\topt_lof_init init = (opt_lof_init)opt->defval;\n\t-\n\t-\tif (init)\n\t-\t\tfilter_options = init(opt->value);\n\t \n\t \tif (unset || !arg)\n\t \t\tlist_objects_filter_set_no_filter(filter_options);\n\tdiff --git a/list-objects-filter-options.h b/list-objects-filter-options.h\n\tindex 7eeadab2dd0..470b86113ac 100644\n\t--- a/list-objects-filter-options.h\n\t+++ b/list-objects-filter-options.h\n\t@@ -108,30 +108,21 @@ void parse_list_objects_filter(\n\t \tconst char *arg);\n\t \n\t /**\n\t- * The opt->value to opt_parse_list_objects_filter() is either a\n\t- * \"struct list_objects_filter_option *\" when using\n\t- * OPT_PARSE_LIST_OBJECTS_FILTER().\n\t+ * The opt->value to opt_parse_list_objects_filter() is a \"struct\n\t+ * list_objects_filter_option *\".\n\t *\n\t- * Or, if using no \"struct option\" field is used by the callback,\n\t- * except the \"defval\" which is expected to be an \"opt_lof_init\"\n\t- * function, which is called with the \"opt->value\" and must return a\n\t- * pointer to the \"\"struct list_objects_filter_option *\" to be used.\n\t- *\n\t- * The OPT_PARSE_LIST_OBJECTS_FILTER_INIT() can be used e.g. the\n\t- * \"struct list_objects_filter_option\" is embedded in a \"struct\n\t- * rev_info\", which the \"defval\" could be tasked with lazily\n\t- * initializing. See cmd_pack_objects() for an example.\n\t+ * When it's the \"filter\" member of a \"struct rev_info\" make sure to\n\t+ * initialize it first with \"REV_INFO_INIT\" or repo_init_revisions()\n\t+ * before manipulating the \"filter\". When using \"REV_INFO_INIT\" to\n\t+ * lazily initialize a \"struct rev_info\" use\n\t+ * repo_dynamic_init_revisions() before using that \"struct rev_info\"\n\t+ * with the revisions API.\n\t */\n\t int opt_parse_list_objects_filter(const struct option *opt,\n\t \t\t\t\t const char *arg, int unset);\n\t-typedef struct list_objects_filter_options *(*opt_lof_init)(void *);\n\t-#define OPT_PARSE_LIST_OBJECTS_FILTER_INIT(fo, init) \\\n\t-\t{ OPTION_CALLBACK, 0, \"filter\", (fo), N_(\"args\"), \\\n\t-\t N_(\"object filtering\"), 0, opt_parse_list_objects_filter, \\\n\t-\t (intptr_t)(init) }\n\t-\n\t #define OPT_PARSE_LIST_OBJECTS_FILTER(fo) \\\n\t-\tOPT_PARSE_LIST_OBJECTS_FILTER_INIT((fo), NULL)\n\t+\t{ OPTION_CALLBACK, 0, \"filter\", (fo), N_(\"args\"), \\\n\t+\t N_(\"object filtering\"), 0, opt_parse_list_objects_filter }\n\t \n\t /*\n\t * Translates abbreviated numbers in the filter's filter_spec into their\n\tdiff --git a/revision.c b/revision.c\n\tindex 47817da209a..435c864d7cd 100644\n\t--- a/revision.c\n\t+++ b/revision.c\n\t@@ -1888,13 +1888,10 @@ static int add_parents_only(struct rev_info *revs, const char *arg_, int flags,\n\t \treturn 1;\n\t }\n\t \n\t-void repo_init_revisions(struct repository *r,\n\t-\t\t\t struct rev_info *revs,\n\t-\t\t\t const char *prefix)\n\t+void repo_dynamic_init_revisions(struct repository *r,\n\t+\t\t\t\t struct rev_info *revs,\n\t+\t\t\t\t const char *prefix)\n\t {\n\t-\tstruct rev_info blank = REV_INFO_INIT;\n\t-\tmemcpy(revs, &blank, sizeof(*revs));\n\t-\n\t \trevs->repo = r;\n\t \trevs->pruning.repo = r;\n\t \trevs->pruning.add_remove = file_add_remove;\n\t@@ -1914,6 +1911,16 @@ void repo_init_revisions(struct repository *r,\n\t \tinit_display_notes(&revs->notes_opt);\n\t }\n\t \n\t+void repo_init_revisions(struct repository *r,\n\t+\t\t\t struct rev_info *revs,\n\t+\t\t\t const char *prefix)\n\t+{\n\t+\tstruct rev_info blank = REV_INFO_INIT;\n\t+\tmemcpy(revs, &blank, sizeof(*revs));\n\t+\n\t+\trepo_dynamic_init_revisions(r, revs, prefix);\n\t+}\n\t+\n\t static void add_pending_commit_list(struct rev_info *revs,\n\t \t\t\t\t struct commit_list *commit_list,\n\t \t\t\t\t unsigned int flags)\n\tdiff --git a/revision.h b/revision.h\n\tindex 56c6b012abb..75982288db8 100644\n\t--- a/revision.h\n\t+++ b/revision.h\n\t@@ -417,6 +417,15 @@ struct rev_info {\n\t void repo_init_revisions(struct repository *r,\n\t \t\t\t struct rev_info *revs,\n\t \t\t\t const char *prefix);\n\t+\n\t+/**\n\t+ * A subset of repo_init_revisions(), assumes that we've already\n\t+ * initialized with REV_INFO_INIT, and possibly manipulated the\n\t+ * members initialized therein.\n\t+ */\n\t+void repo_dynamic_init_revisions(struct repository *r,\n\t+\t\t\t\t struct rev_info *revs,\n\t+\t\t\t\t const char *prefix);\n\t #ifndef NO_THE_REPOSITORY_COMPATIBILITY_MACROS\n\t #define init_revisions(revs, prefix) repo_init_revisions(the_repository, revs, prefix)\n\t #endif\n\nNow, that repo_dynamic_init_revisions() is rather nasty, but why is it\nthat we need it in the first place? It turns out that this whole\n\"dynamic init\" business is only needed because we need to set\n\"revs.repo\" to \"the_repository\". So this on top passes all tests:\n\t\n\tdiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n\tindex d0c4c26f4e9..190e2f8ee0e 100644\n\t--- a/builtin/pack-objects.c\n\t+++ b/builtin/pack-objects.c\n\t@@ -4458,10 +4458,12 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n\t \t} else if (!use_internal_rev_list) {\n\t \t\tread_object_list_from_stdin();\n\t \t} else if (revs.filter.choice) {\n\t+\t\txsetenv(\"GIT_TEST_RI\", \"1\", 1);\n\t \t\trepo_dynamic_init_revisions(the_repository, &revs, NULL);\n\t \t\tget_object_list(&revs, rp.nr, rp.v);\n\t \t} else {\n\t-\t\trepo_init_revisions(the_repository, &revs, NULL);\n\t+\t\txsetenv(\"GIT_TEST_RI\", \"1\", 1);\n\t+\t\trepo_dynamic_init_revisions(the_repository, &revs, NULL);\n\t \t\tget_object_list(&revs, rp.nr, rp.v);\n\t \t}\n\t \trelease_revisions(&revs);\n\tdiff --git a/revision.c b/revision.c\n\tindex 435c864d7cd..cd832499d22 100644\n\t--- a/revision.c\n\t+++ b/revision.c\n\t@@ -1893,6 +1893,9 @@ void repo_dynamic_init_revisions(struct repository *r,\n\t \t\t\t\t const char *prefix)\n\t {\n\t \trevs->repo = r;\n\t+\tif (getenv(\"GIT_TEST_RI\"))\n\t+\t\treturn;\n\t+\n\t \trevs->pruning.repo = r;\n\t \trevs->pruning.add_remove = file_add_remove;\n\t \trevs->pruning.change = file_change;\n\nAnd of course we can in turn simplify that to something like the\nCHILD_PROCESS_INIT macro we discussed the other day in another thread,\ni.e. just:\n\n\tstruct rev_info = REV_INFO_INIT_VA(\n\t\t.repo = the_repository,\n\t};\n\nSo then the whole intermediate repo_dynamic_init_revisions() will also\ngo away, and I suspect many current users of repo_init_revisions() can\nbe made to just use the macro.\n\nRegarding 5/5: I know I asked in the previous iteration if we couldn't\nsplit that J.5.7 change from the rest, but the 4/5 here just leaves it\nas a loose end. I.e. we've already made the macro unused, so in 5/5\nwe're just cleaning up already dead code.\n\nOn reflection, I don't know if splitting it up is worth it after all,\nbut if it's going to be made an atomic change it'll surely have to\nhappen the other way around.\n\nE.g. now we take an arbtirary \"intptr_t\", but once you get rid of the\ncallback mechanism we stop using it, so we've already done the J.5.7\nchange.\n\nMaybe it's not possible to split it up after all. In any case the real\nvaluable thing is to split it from the bug fix in 3/5. If it then goes\naway as part of some general refactoring...\n\nI'd also be fine with just having this series as-is at this point,\ndepending on your appetite to continue hacking on this.\n\nI've been slowly working towards making the \"struct rev_info\"\ninitialization less crappy, so the changes above could also be made in\nsome eventual REV_INFO_INIT series...\n\n"},{"id":"468205","messageId":"6eaa4178-ad38-c02d-a4c6-7c666cc87bba@web.de","threadId":"58795","inReplyTo":"221129.86fse2ji6c.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2 3/3] Revert \"pack-objects: lazily set up \"struct rev_info\", don't leak\"","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-11-29T19:18:04Z","receivedAt":"2022-11-29T19:18:30Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 29.11.22 um 08:12 schrieb Ævar Arnfjörð Bjarmason:\n>\n> On Mon, Nov 28 2022, René Scharfe wrote:\n>\n>> That may be true, and looks even useful -- I didn't know the check\n>> value. I only get a strange error message, though:\n>>\n>> $ GIT_TEST_PASSING_SANITIZE_LEAK=check ./t0001-init.sh\n>> Bail out! GIT_TEST_PASSING_SANITIZE_LEAK=true has no effect except when compiled with SANITIZE=leak\n>>\n>> Same with make test and prove, of course. And of course I compiled\n>> with SANITIZE=leak beforehand.\n>\n> The \"=true\" part of the message is unfortunately incorrect (it pre-dates\n> \"check\" being a possible value), but I don't see how you could have\n> compiled with \"SANITIZE=leak\" and get that message.\n>\n> It's unreachable if 'test -n \"$SANITIZE_LEAK\"', and that'll be non-empty\n> in GIT-BUILD-OPTIONS if compiled with it. Perhaps you gave SANITIZE=leak\n> to t/Makefile, not the top-level Makefile?\n>\n> Try this at the top-level:\n>\n> \tGIT_TEST_PASSING_SANITIZE_LEAK=check make SANITIZE= test T=t0001-init.sh\n\nIt works today, no idea what I did yesterday. Did reboot and make\nclean in the meantime, shell history is inconclusive. *shrug*\n\n>> As I wrote: A call to an initialization function followed by a call to a\n>> cleanup function and nothing else shouldn't leak. There are examples of\n>> repo_init_revisions()+release_revisions() without setup_revisions() or\n>> diff_setup_done() beyond pack-objects. I mentioned prune, but there are\n>> more, e.g. in sequencer.c.\n>\n> Yes, I agree it shouldn't leak. And we should definitely fix those\n> leaks. I just don't see why a series fixing bugs in --filter needs to\n> expand the scope to fix those.\n\nThe connection is that this is the very leak that 5cb28270a1\n(pack-objects: lazily set up \"struct rev_info\", don't leak, 2022-03-28)\nplugged locally. Took me a while to see that. Anyway, I'm also not\nkeen on scope creep.\n\n>>> But in general: I don't really think this sort of thing is worth\n>>> it. Here we're reaching into a member of \"revs->diffopt\" behind its back\n>>> rather than calling diff_free(). I think we should just focus on being\n>>> able to do do that safely.\n>>\n>> Sure, but the FREE_AND_NULL call is simple and safe, while diff_free()\n>> is complicated and calling it one time too many can hurt.\n>\n> It's \"safe\" because you've read the internals of it, and know that it\n> isn't assuming a non-NULL there once it's past initialization?\n>\n> Or is it like the revisions init()+release() in this thread, where\n> you're assuming it works one way based on the function names etc., only\n> for the CI to fail?\n\nOuch.\n\n> In either case, I'm saying that if someone's confident enough to reach\n> into the internals of a structure and tweak it they should be confident\n> enough to just patch diff_free() or the like.\n\ndiff_free() is more complicated; it does that FREE_AND_NULL plus several\nthings that are not idempotent.\n\n>>> WIP patches I have in that direction, partially based on your previous\n>>> \"is_dead\" suggestion:\n>>>\n>>> \thttps://github.com/avar/git/commit/e02a15f6206\n>>> \thttps://github.com/avar/git/commit/c718f36566a\n>>\n>> Copy-typed the interesting parts of the first patch like a medieval monk\n>> because there doesn't seem to be a download option. :-|\n>\n> Jeff pointed out the \".patch\" (there's also \".diff\"), but also: Git has\n> this well-known transport protocol it uses, which typically maps to the\n> web URL on public hosting sites ... :)\n>\n> \tgit remote add avar https://github.com/avar/git.git\n> \tgit fetch avar\n> \tgit show \n\nYes, I probably should have downloaded everything like that. Just did\nit for the heck of it and got 43.37 MiB at 598.00 KiB/s. Typing wasn't\nthat much slower.\n\nRené\n"},{"id":"468218","messageId":"xmqq7czd2qq6.fsf@gitster.g","threadId":"58795","inReplyTo":"6c7ef5fb-59b5-df16-036d-9a9a3d987cb0@web.de","subject":"Re: [PATCH v3 3/5] pack-objects: fix handling of multiple --filter options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-11-30T01:09:05Z","receivedAt":"2022-11-30T01:09:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> Since 5cb28270a1 (pack-objects: lazily set up \"struct rev_info\", don't\n> leak, 2022-03-28) --filter options given to git pack-objects overrule\n> earlier ones, letting only the leftmost win and leaking the memory\n> allocated for earlier ones. Fix that by only initializing the rev_info\n> struct once.\n\nI think \"leftmost\" -> \"rightmost\", if your command line goes from\nleft to right? Or \"leftmost\" -> \"last\" (e.g. \"last one wins\" over\n\"earlier ones\")?\n\n> Signed-off-by: René Scharfe \n> ---\n> builtin/pack-objects.c | 3 ++-\n> t/t5317-pack-objects-filter-objects.sh | 2 +-\n> 2 files changed, 3 insertions(+), 2 deletions(-)\n>\n> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n> index 573d0b20b7..c702c09dd4 100644\n> --- a/builtin/pack-objects.c\n> +++ b/builtin/pack-objects.c\n> @@ -4158,7 +4158,8 @@ static struct list_objects_filter_options *po_filter_revs_init(void *value)\n> {\n> \tstruct po_filter_data *data = value;\n>\n> -\trepo_init_revisions(the_repository, &data->revs, NULL);\n> +\tif (!data->have_revs)\n> +\t\trepo_init_revisions(the_repository, &data->revs, NULL);\n> \tdata->have_revs = 1;\n>\n> \treturn &data->revs.filter;\n> diff --git a/t/t5317-pack-objects-filter-objects.sh b/t/t5317-pack-objects-filter-objects.sh\n> index 25faebaada..5b707d911b 100755\n> --- a/t/t5317-pack-objects-filter-objects.sh\n> +++ b/t/t5317-pack-objects-filter-objects.sh\n> @@ -265,7 +265,7 @@ test_expect_success 'verify normal and blob:limit packfiles have same commits/tr\n> \ttest_cmp expected observed\n> '\n>\n> -test_expect_failure 'verify small limit and big limit results in small limit' '\n> +test_expect_success 'verify small limit and big limit results in small limit' '\n> \tgit -C r2 ls-files -s large.1000 >ls_files_result &&\n> \ttest_parse_ls_files_stage_oids \tsort >expected &&\n> --\n> 2.38.1\n"},{"id":"468219","messageId":"xmqqzgc91bn7.fsf@gitster.g","threadId":"58795","inReplyTo":"accc4f29-8e8e-5b8b-9409-6f2ac51c2936@web.de","subject":"Re: [PATCH v3 5/5] list-objects-filter: remove OPT_PARSE_LIST_OBJECTS_FILTER_INIT()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-11-30T01:20:12Z","receivedAt":"2022-11-30T01:20:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> OPT_PARSE_LIST_OBJECTS_FILTER_INIT() with a non-NULL second argument\n> passes a function pointer via an object pointer, which is undefined. It\n> may work fine on platforms that implement C99 extension J.5.7 (Function\n> pointer casts). Remove the unused macro and avoid the dependency on\n> that extension.\n\nMakes sense. Some of us may consider that no platform of importance\nexists in practice that breaks with such a cast, but our opinion\ndoes not matter at all, while opinion of those who supply compilers\nand checkers to us do---they may decide to flag such a use with a\nwarning and that would break our build with the -Werror option. If\nwe do not have to rely on the extension, we should.\n\nThanks.\n\n"},{"id":"468230","messageId":"3eefcd8c-3011-0317-c6d2-d554e03d8e6c@web.de","threadId":"58795","inReplyTo":"xmqq7czd2qq6.fsf@gitster.g","subject":"Re: [PATCH v3 3/5] pack-objects: fix handling of multiple --filter options","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-11-30T07:11:46Z","receivedAt":"2022-11-30T07:12:08Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 30.11.22 um 02:09 schrieb Junio C Hamano:\n> René Scharfe writes:\n>\n>> Since 5cb28270a1 (pack-objects: lazily set up \"struct rev_info\", don't\n>> leak, 2022-03-28) --filter options given to git pack-objects overrule\n>> earlier ones, letting only the leftmost win and leaking the memory\n>> allocated for earlier ones. Fix that by only initializing the rev_info\n>> struct once.\n>\n> I think \"leftmost\" -> \"rightmost\", if your command line goes from\n> left to right? Or \"leftmost\" -> \"last\" (e.g. \"last one wins\" over\n> \"earlier ones\")?\n\nYes, indeed, I mixed up left and right again.\n\nRené\n"},{"id":"468243","messageId":"4fd82dc6-e0f8-0638-5b10-16bfef39a171@web.de","threadId":"58795","inReplyTo":"221129.8635a1kglk.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v3 4/5] pack-objects: simplify --filter handling","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-11-30T11:23:17Z","receivedAt":"2022-11-30T11:24:47Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 29.11.22 um 14:27 schrieb Ævar Arnfjörð Bjarmason:\n>\n> On Tue, Nov 29 2022, René Scharfe wrote:\n>\n> First, I think 3/5 in this series looks really good in this iteration.\n>\n> I think this is also worth cleaning up, but depending on when we expect\n> this to be queued maybe splitting these post-cleanups into their own\n> series would make sense?\n>\n>> pack-objects uses OPT_PARSE_LIST_OBJECTS_FILTER_INIT() to initialize the\n>> a rev_info struct lazily before populating its filter member using the\n>> --filter option values. It tracks whether the initialization is needed\n>> using the .have_revs member of the callback data.\n>>\n>> There is a better way: Use a stand-alone list_objects_filter_options\n>> struct and build a rev_info struct with its .filter member after option\n>> parsing. This allows using the simpler OPT_PARSE_LIST_OBJECTS_FILTER()\n>> and getting rid of the extra callback mechanism.\n>>\n>> Even simpler would be using a struct rev_info as before 5cb28270a1\n>> (pack-objects: lazily set up \"struct rev_info\", don't leak, 2022-03-28),\n>> but that would expose a memory leak caused by repo_init_revisions()\n>> followed by release_revisions() without a setup_revisions() call in\n>> between.\n>>\n>> Using list_objects_filter_options also allows pushing the rev_info\n>> struct into get_object_list(), where it arguably belongs. Either way,\n>> this is all left for later.\n>\n> I think what you're missing here (or not explaining) is that *the\n> reason* for us not being able to return to a pre-5cb28270a1 state is\n> that if we initialize with REV_INFO_INIT we'll first of all find that\n> the \"filter\" isn't initialized, because it's not in that macro.\n\nThe parent of 5cb28270a1 used repo_init_revisions(), not REV_INFO_INIT\ndirectly, so that macro is not a direct reason.\n\nBut you mean that in a world where struct rev_info can be sufficiently\ninitialized with a macro we could use it without allocations, avoiding\nleaks? And for pack-objects just the filter member is missing to reach\nthat goal?\n\n> And even if it were there, the repo_init_revisions() will wipe away\n> anything we add to it once we call repo_init_revisions(), which we'll\n> need to do any \"real\" work with it.\n>\n> But if we're really going to refactor this properly let's stop beating\n> around the bush and just connect those dots. So first, the only reason\n> for why \"filter\" isn't there yet is because a recent topic would have\n> conflicted with it, so if this is cherry-picked after 3/5 we'll have\n> that: https://github.com/avar/git/commit/6b29069d72f.patch\n\nThat patch looks good.\n\n> Then the below change on top of that could replace this 4/5 and\n> 5/5. I.e. also the 5/5 because the only reason we need the\n> \"opt_lof_init\" is because of this init-ing edge case.\n>\n> \t builtin/pack-objects.c | 32 +++++++-------------------------\n> \t list-objects-filter-options.c | 4 ----\n> \t list-objects-filter-options.h | 29 ++++++++++-------------------\n> \t revision.c | 19 +++++++++++++------\n> \t revision.h | 9 +++++++++\n> \t 5 files changed, 39 insertions(+), 54 deletions(-)\n>\n> \tdiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n> \tindex c702c09dd45..d0c4c26f4e9 100644\n> \t--- a/builtin/pack-objects.c\n> \t+++ b/builtin/pack-objects.c\n> \t@@ -4149,22 +4149,6 @@ static int option_parse_cruft_expiration(const struct option *opt,\n> \t \treturn 0;\n> \t }\n>\n> \t-struct po_filter_data {\n> \t-\tunsigned have_revs:1;\n> \t-\tstruct rev_info revs;\n> \t-};\n> \t-\n> \t-static struct list_objects_filter_options *po_filter_revs_init(void *value)\n> \t-{\n> \t-\tstruct po_filter_data *data = value;\n> \t-\n> \t-\tif (!data->have_revs)\n> \t-\t\trepo_init_revisions(the_repository, &data->revs, NULL);\n> \t-\tdata->have_revs = 1;\n> \t-\n> \t-\treturn &data->revs.filter;\n> \t-}\n> \t-\n> \t int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n> \t {\n> \t \tint use_internal_rev_list = 0;\n> \t@@ -4175,7 +4159,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n> \t \tint rev_list_index = 0;\n> \t \tint stdin_packs = 0;\n> \t \tstruct string_list keep_pack_list = STRING_LIST_INIT_NODUP;\n> \t-\tstruct po_filter_data pfd = { .have_revs = 0 };\n> \t+\tstruct rev_info revs = REV_INFO_INIT;\n>\n> \t \tstruct option pack_objects_options[] = {\n> \t \t\tOPT_SET_INT('q', \"quiet\", &progress,\n> \t@@ -4266,7 +4250,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n> \t \t\t\t &write_bitmap_index,\n> \t \t\t\t N_(\"write a bitmap index if possible\"),\n> \t \t\t\t WRITE_BITMAP_QUIET, PARSE_OPT_HIDDEN),\n> \t-\t\tOPT_PARSE_LIST_OBJECTS_FILTER_INIT(&pfd, po_filter_revs_init),\n> \t+\t\tOPT_PARSE_LIST_OBJECTS_FILTER(&revs.filter),\n> \t \t\tOPT_CALLBACK_F(0, \"missing\", NULL, N_(\"action\"),\n> \t \t\t N_(\"handling for missing objects\"), PARSE_OPT_NONEG,\n> \t \t\t option_parse_missing_action),\n> \t@@ -4386,7 +4370,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n> \t \tif (!rev_list_all || !rev_list_reflog || !rev_list_index)\n> \t \t\tunpack_unreachable_expiration = 0;\n>\n> \t-\tif (pfd.have_revs && pfd.revs.filter.choice) {\n> \t+\tif (revs.filter.choice) {\n> \t \t\tif (!pack_to_stdout)\n> \t \t\t\tdie(_(\"cannot use --filter without --stdout\"));\n> \t \t\tif (stdin_packs)\n> \t@@ -4473,16 +4457,14 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n> \t \t\tread_cruft_objects();\n> \t \t} else if (!use_internal_rev_list) {\n> \t \t\tread_object_list_from_stdin();\n> \t-\t} else if (pfd.have_revs) {\n> \t-\t\tget_object_list(&pfd.revs, rp.nr, rp.v);\n> \t-\t\trelease_revisions(&pfd.revs);\n> \t+\t} else if (revs.filter.choice) {\n> \t+\t\trepo_dynamic_init_revisions(the_repository, &revs, NULL);\n> \t+\t\tget_object_list(&revs, rp.nr, rp.v);\n> \t \t} else {\n> \t-\t\tstruct rev_info revs;\n> \t-\n> \t \t\trepo_init_revisions(the_repository, &revs, NULL);\n> \t \t\tget_object_list(&revs, rp.nr, rp.v);\n> \t-\t\trelease_revisions(&revs);\n> \t \t}\n> \t+\trelease_revisions(&revs);\n> \t \tcleanup_preferred_base();\n> \t \tif (include_tag && nr_result)\n> \t \t\tfor_each_tag_ref(add_ref_tag, NULL);\n> \tdiff --git a/list-objects-filter-options.c b/list-objects-filter-options.c\n> \tindex 53396602380..ee01bcd2cc3 100644\n> \t--- a/list-objects-filter-options.c\n> \t+++ b/list-objects-filter-options.c\n> \t@@ -290,10 +290,6 @@ int opt_parse_list_objects_filter(const struct option *opt,\n> \t \t\t\t\t const char *arg, int unset)\n> \t {\n> \t \tstruct list_objects_filter_options *filter_options = opt->value;\n> \t-\topt_lof_init init = (opt_lof_init)opt->defval;\n> \t-\n> \t-\tif (init)\n> \t-\t\tfilter_options = init(opt->value);\n>\n> \t \tif (unset || !arg)\n> \t \t\tlist_objects_filter_set_no_filter(filter_options);\n> \tdiff --git a/list-objects-filter-options.h b/list-objects-filter-options.h\n> \tindex 7eeadab2dd0..470b86113ac 100644\n> \t--- a/list-objects-filter-options.h\n> \t+++ b/list-objects-filter-options.h\n> \t@@ -108,30 +108,21 @@ void parse_list_objects_filter(\n> \t \tconst char *arg);\n>\n> \t /**\n> \t- * The opt->value to opt_parse_list_objects_filter() is either a\n> \t- * \"struct list_objects_filter_option *\" when using\n> \t- * OPT_PARSE_LIST_OBJECTS_FILTER().\n> \t+ * The opt->value to opt_parse_list_objects_filter() is a \"struct\n> \t+ * list_objects_filter_option *\".\n> \t *\n> \t- * Or, if using no \"struct option\" field is used by the callback,\n> \t- * except the \"defval\" which is expected to be an \"opt_lof_init\"\n> \t- * function, which is called with the \"opt->value\" and must return a\n> \t- * pointer to the \"\"struct list_objects_filter_option *\" to be used.\n> \t- *\n> \t- * The OPT_PARSE_LIST_OBJECTS_FILTER_INIT() can be used e.g. the\n> \t- * \"struct list_objects_filter_option\" is embedded in a \"struct\n> \t- * rev_info\", which the \"defval\" could be tasked with lazily\n> \t- * initializing. See cmd_pack_objects() for an example.\n> \t+ * When it's the \"filter\" member of a \"struct rev_info\" make sure to\n> \t+ * initialize it first with \"REV_INFO_INIT\" or repo_init_revisions()\n> \t+ * before manipulating the \"filter\". When using \"REV_INFO_INIT\" to\n> \t+ * lazily initialize a \"struct rev_info\" use\n> \t+ * repo_dynamic_init_revisions() before using that \"struct rev_info\"\n> \t+ * with the revisions API.\n> \t */\n> \t int opt_parse_list_objects_filter(const struct option *opt,\n> \t \t\t\t\t const char *arg, int unset);\n> \t-typedef struct list_objects_filter_options *(*opt_lof_init)(void *);\n> \t-#define OPT_PARSE_LIST_OBJECTS_FILTER_INIT(fo, init) \\\n> \t-\t{ OPTION_CALLBACK, 0, \"filter\", (fo), N_(\"args\"), \\\n> \t-\t N_(\"object filtering\"), 0, opt_parse_list_objects_filter, \\\n> \t-\t (intptr_t)(init) }\n> \t-\n> \t #define OPT_PARSE_LIST_OBJECTS_FILTER(fo) \\\n> \t-\tOPT_PARSE_LIST_OBJECTS_FILTER_INIT((fo), NULL)\n> \t+\t{ OPTION_CALLBACK, 0, \"filter\", (fo), N_(\"args\"), \\\n> \t+\t N_(\"object filtering\"), 0, opt_parse_list_objects_filter }\n>\n> \t /*\n> \t * Translates abbreviated numbers in the filter's filter_spec into their\n> \tdiff --git a/revision.c b/revision.c\n> \tindex 47817da209a..435c864d7cd 100644\n> \t--- a/revision.c\n> \t+++ b/revision.c\n> \t@@ -1888,13 +1888,10 @@ static int add_parents_only(struct rev_info *revs, const char *arg_, int flags,\n> \t \treturn 1;\n> \t }\n>\n> \t-void repo_init_revisions(struct repository *r,\n> \t-\t\t\t struct rev_info *revs,\n> \t-\t\t\t const char *prefix)\n> \t+void repo_dynamic_init_revisions(struct repository *r,\n> \t+\t\t\t\t struct rev_info *revs,\n> \t+\t\t\t\t const char *prefix)\n> \t {\n> \t-\tstruct rev_info blank = REV_INFO_INIT;\n> \t-\tmemcpy(revs, &blank, sizeof(*revs));\n> \t-\n> \t \trevs->repo = r;\n> \t \trevs->pruning.repo = r;\n> \t \trevs->pruning.add_remove = file_add_remove;\n> \t@@ -1914,6 +1911,16 @@ void repo_init_revisions(struct repository *r,\n> \t \tinit_display_notes(&revs->notes_opt);\n> \t }\n>\n> \t+void repo_init_revisions(struct repository *r,\n> \t+\t\t\t struct rev_info *revs,\n> \t+\t\t\t const char *prefix)\n> \t+{\n> \t+\tstruct rev_info blank = REV_INFO_INIT;\n> \t+\tmemcpy(revs, &blank, sizeof(*revs));\n> \t+\n> \t+\trepo_dynamic_init_revisions(r, revs, prefix);\n> \t+}\n> \t+\n> \t static void add_pending_commit_list(struct rev_info *revs,\n> \t \t\t\t\t struct commit_list *commit_list,\n> \t \t\t\t\t unsigned int flags)\n> \tdiff --git a/revision.h b/revision.h\n> \tindex 56c6b012abb..75982288db8 100644\n> \t--- a/revision.h\n> \t+++ b/revision.h\n> \t@@ -417,6 +417,15 @@ struct rev_info {\n> \t void repo_init_revisions(struct repository *r,\n> \t \t\t\t struct rev_info *revs,\n> \t \t\t\t const char *prefix);\n> \t+\n> \t+/**\n> \t+ * A subset of repo_init_revisions(), assumes that we've already\n> \t+ * initialized with REV_INFO_INIT, and possibly manipulated the\n> \t+ * members initialized therein.\n> \t+ */\n> \t+void repo_dynamic_init_revisions(struct repository *r,\n> \t+\t\t\t\t struct rev_info *revs,\n> \t+\t\t\t\t const char *prefix);\n> \t #ifndef NO_THE_REPOSITORY_COMPATIBILITY_MACROS\n> \t #define init_revisions(revs, prefix) repo_init_revisions(the_repository, revs, prefix)\n> \t #endif\n>\n> Now, that repo_dynamic_init_revisions() is rather nasty, but why is it\n> that we need it in the first place? It turns out that this whole\n> \"dynamic init\" business is only needed because we need to set\n> \"revs.repo\" to \"the_repository\". So this on top passes all tests:\n>\n> \tdiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n> \tindex d0c4c26f4e9..190e2f8ee0e 100644\n> \t--- a/builtin/pack-objects.c\n> \t+++ b/builtin/pack-objects.c\n> \t@@ -4458,10 +4458,12 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n> \t \t} else if (!use_internal_rev_list) {\n> \t \t\tread_object_list_from_stdin();\n> \t \t} else if (revs.filter.choice) {\n> \t+\t\txsetenv(\"GIT_TEST_RI\", \"1\", 1);\n> \t \t\trepo_dynamic_init_revisions(the_repository, &revs, NULL);\n> \t \t\tget_object_list(&revs, rp.nr, rp.v);\n> \t \t} else {\n> \t-\t\trepo_init_revisions(the_repository, &revs, NULL);\n> \t+\t\txsetenv(\"GIT_TEST_RI\", \"1\", 1);\n> \t+\t\trepo_dynamic_init_revisions(the_repository, &revs, NULL);\n> \t \t\tget_object_list(&revs, rp.nr, rp.v);\n> \t \t}\n> \t \trelease_revisions(&revs);\n> \tdiff --git a/revision.c b/revision.c\n> \tindex 435c864d7cd..cd832499d22 100644\n> \t--- a/revision.c\n> \t+++ b/revision.c\n> \t@@ -1893,6 +1893,9 @@ void repo_dynamic_init_revisions(struct repository *r,\n> \t \t\t\t\t const char *prefix)\n> \t {\n> \t \trevs->repo = r;\n> \t+\tif (getenv(\"GIT_TEST_RI\"))\n> \t+\t\treturn;\n> \t+\n> \t \trevs->pruning.repo = r;\n> \t \trevs->pruning.add_remove = file_add_remove;\n> \t \trevs->pruning.change = file_change;\n>\n> And of course we can in turn simplify that to something like the\n> CHILD_PROCESS_INIT macro we discussed the other day in another thread,\n> i.e. just:\n>\n> \tstruct rev_info = REV_INFO_INIT_VA(\n> \t\t.repo = the_repository,\n> \t};\n>\n> So then the whole intermediate repo_dynamic_init_revisions() will also\n> go away, and I suspect many current users of repo_init_revisions() can\n> be made to just use the macro.\n\nConverting existing users of repo_init_revisions() to partial\ninitialization using a macro seems like a lot of work to determine if\nthat subset is sufficient. Testing alone won't be enough.\n\nA macro that initializes the struct fully would be easier to use. One\nobstacle is that repo and prefix are duplicated into embedded structs\n(.repo, .pruning.repo, .grep_filter.repo, .diffopt.repo; .prefix,\n.diffopt.prefix). Another is .diffopt.parseopts, which includes\npointers to other .diffopt members, whose initialization requires the\nname of the rev_info variable.\n\nWhy do we want to get rid of repo_init_revisions()? Because it\nallocates .diffopt.parseopts and we sometimes forget to release it,\nespecially in cases where we don't actually need it. We can avoid\nthat by leaving the allocations to the code paths that parse diff\noptions. That's actually easy (for now?) because only two places\ncustomize the option list and they do it by concatenation. Patch\nbelow.\n\n> Regarding 5/5: I know I asked in the previous iteration if we couldn't\n> split that J.5.7 change from the rest, but the 4/5 here just leaves it\n> as a loose end. I.e. we've already made the macro unused, so in 5/5\n> we're just cleaning up already dead code.\n>\n> On reflection, I don't know if splitting it up is worth it after all,\n> but if it's going to be made an atomic change it'll surely have to\n> happen the other way around.\n>\n> E.g. now we take an arbtirary \"intptr_t\", but once you get rid of the\n> callback mechanism we stop using it, so we've already done the J.5.7\n> change.\n>\n> Maybe it's not possible to split it up after all. In any case the real\n> valuable thing is to split it from the bug fix in 3/5. If it then goes\n> away as part of some general refactoring...\n>\n> I'd also be fine with just having this series as-is at this point,\n> depending on your appetite to continue hacking on this.\n>\n> I've been slowly working towards making the \"struct rev_info\"\n> initialization less crappy, so the changes above could also be made in\n> some eventual REV_INFO_INIT series...\n\nMerging the introduction or removal of infrastructure with the first or\nlast use is sometimes nicer, e.g. if one part is small. Here I slightly\nprefer to keep the different concerns separate, i.e. to not merge patch\n4 and 5, because 4 is big enough already and 5 being a trivial dead code\nremoval is a good thing in my eyes.\n\n\ndiff --git a/builtin/range-diff.c b/builtin/range-diff.c\nindex e2a74efb42..757010069f 100644\n--- a/builtin/range-diff.c\n+++ b/builtin/range-diff.c\n@@ -47,7 +47,7 @@ int cmd_range_diff(int argc, const char **argv, const char *prefix)\n\n \trepo_diff_setup(the_repository, &diffopt);\n\n-\toptions = parse_options_concat(range_diff_options, diffopt.parseopts);\n+\toptions = diff_parse_options_concat(range_diff_options, &diffopt);\n \targc = parse_options(argc, argv, prefix, options,\n \t\t\t builtin_range_diff_usage, PARSE_OPT_KEEP_DASHDASH);\n\ndiff --git a/diff-no-index.c b/diff-no-index.c\nindex 18edbdf4b5..0a240039a0 100644\n--- a/diff-no-index.c\n+++ b/diff-no-index.c\n@@ -255,8 +255,7 @@ int diff_no_index(struct rev_info *revs,\n \t};\n \tstruct option *options;\n\n-\toptions = parse_options_concat(no_index_options,\n-\t\t\t\t revs->diffopt.parseopts);\n+\toptions = diff_parse_options_concat(no_index_options, &revs->diffopt);\n \targc = parse_options(argc, argv, revs->prefix, options,\n \t\t\t diff_no_index_usage, 0);\n \tif (argc != 2) {\ndiff --git a/diff.c b/diff.c\nindex 1054a4b732..ebf9154eaf 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4615,8 +4615,6 @@ static void run_checkdiff(struct diff_filepair *p, struct diff_options *o)\n \tbuiltin_checkdiff(name, other, attr_path, p->one, p->two, o);\n }\n\n-static void prep_parse_options(struct diff_options *options);\n-\n void repo_diff_setup(struct repository *r, struct diff_options *options)\n {\n \tmemcpy(options, &default_diff_options, sizeof(*options));\n@@ -4662,8 +4660,6 @@ void repo_diff_setup(struct repository *r, struct diff_options *options)\n\n \toptions->color_moved = diff_color_moved_default;\n \toptions->color_moved_ws_handling = diff_color_moved_ws_default;\n-\n-\tprep_parse_options(options);\n }\n\n static const char diff_status_letters[] = {\n@@ -4821,8 +4817,6 @@ void diff_setup_done(struct diff_options *options)\n \t\t\toptions->filter = ~filter_bit[DIFF_STATUS_FILTER_AON];\n \t\toptions->filter &= ~options->filter_not;\n \t}\n-\n-\tFREE_AND_NULL(options->parseopts);\n }\n\n int parse_long_opt(const char *opt, const char **argv,\n@@ -5419,7 +5413,8 @@ static int diff_opt_rotate_to(const struct option *opt, const char *arg, int uns\n \treturn 0;\n }\n\n-static void prep_parse_options(struct diff_options *options)\n+struct option *diff_parse_options_concat(const struct option *extra,\n+\t\t\t\t\t struct diff_options *options)\n {\n \tstruct option parseopts[] = {\n \t\tOPT_GROUP(N_(\"Diff output format options\")),\n@@ -5689,22 +5684,24 @@ static void prep_parse_options(struct diff_options *options)\n \t\tOPT_END()\n \t};\n\n-\tALLOC_ARRAY(options->parseopts, ARRAY_SIZE(parseopts));\n-\tmemcpy(options->parseopts, parseopts, sizeof(parseopts));\n+\treturn parse_options_concat(extra, parseopts);\n }\n\n int diff_opt_parse(struct diff_options *options,\n \t\t const char **av, int ac, const char *prefix)\n {\n+\tstruct option *opts = diff_parse_options_concat(NULL, options);\n+\n \tif (!prefix)\n \t\tprefix = \"\";\n\n-\tac = parse_options(ac, av, prefix, options->parseopts, NULL,\n+\tac = parse_options(ac, av, prefix, opts, NULL,\n \t\t\t PARSE_OPT_KEEP_DASHDASH |\n \t\t\t PARSE_OPT_KEEP_UNKNOWN_OPT |\n \t\t\t PARSE_OPT_NO_INTERNAL_HELP |\n \t\t\t PARSE_OPT_ONE_SHOT |\n \t\t\t PARSE_OPT_STOP_AT_NON_OPTION);\n+\tfree(opts);\n\n \treturn ac;\n }\n@@ -6513,7 +6510,6 @@ void diff_free(struct diff_options *options)\n \tdiff_free_file(options);\n \tdiff_free_ignore_regex(options);\n \tclear_pathspec(&options->pathspec);\n-\tFREE_AND_NULL(options->parseopts);\n }\n\n void diff_flush(struct diff_options *options)\ndiff --git a/diff.h b/diff.h\nindex fd33caeb25..f2ab97b8b4 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -394,7 +394,6 @@ struct diff_options {\n \tunsigned color_moved_ws_handling;\n\n \tstruct repository *repo;\n-\tstruct option *parseopts;\n \tstruct strmap *additional_path_headers;\n\n \tint no_free;\n@@ -539,6 +538,8 @@ int git_diff_ui_config(const char *var, const char *value, void *cb);\n #define diff_setup(diffopts) repo_diff_setup(the_repository, diffopts)\n #endif\n void repo_diff_setup(struct repository *, struct diff_options *);\n+struct option *diff_parse_options_concat(const struct option *extra,\n+\t\t\t\t\t struct diff_options *options);\n int diff_opt_parse(struct diff_options *, const char **, int, const char *);\n void diff_setup_done(struct diff_options *);\n int git_config_rename(const char *var, const char *value);\n\n\n"}]}