{"thread":{"id":"57600","subject":"[PATCH 2/5] pack-objects: move revs out of get_object_list()","startedAt":"2022-03-22T17:28:48Z","lastAt":"2022-03-28T17:10:41Z","messageCount":25,"participants":["Derrick Stolee via GitGitGadget","Ramsay Jones","Ævar Arnfjörð Bjarmason","Bagas Sanjaya","Derrick Stolee","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"451852","messageId":"619b757d98465dbc4995bdc11a5282fbfcbd3daa.1647970119.git.gitgitgadget@gmail.com","threadId":"57600","inReplyTo":"pull.1186.git.1647970119.gitgitgadget@gmail.com","subject":"[PATCH 2/5] pack-objects: move revs out of get_object_list()","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-03-22T17:28:36Z","receivedAt":"2022-03-22T17:28:48Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nWe intend to parse the --filter option directly into revs.filter, but we\nfirst need to move the 'revs' variable out of get_object_list() and pass\nit as a pointer instead. This change only deals with the issues of\nmaking that work.\n\nSigned-off-by: Derrick Stolee \n---\n builtin/pack-objects.c | 33 ++++++++++++++++++---------------\n 1 file changed, 18 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 829ca359cf9..f29bef9d0b6 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -3714,9 +3714,8 @@ static void mark_bitmap_preferred_tips(void)\n \t}\n }\n \n-static void get_object_list(int ac, const char **av)\n+static void get_object_list(struct rev_info *revs, int ac, const char **av)\n {\n-\tstruct rev_info revs;\n \tstruct setup_revision_opt s_r_opt = {\n \t\t.allow_exclude_promisor_objects = 1,\n \t};\n@@ -3724,10 +3723,10 @@ static void get_object_list(int ac, const char **av)\n \tint flags = 0;\n \tint save_warning;\n \n-\trepo_init_revisions(the_repository, &revs, NULL);\n+\trepo_init_revisions(the_repository, revs, NULL);\n \tsave_commit_buffer = 0;\n-\tsetup_revisions(ac, av, &revs, &s_r_opt);\n-\tlist_objects_filter_copy(&revs.filter, &filter_options);\n+\tsetup_revisions(ac, av, revs, &s_r_opt);\n+\tlist_objects_filter_copy(&revs->filter, &filter_options);\n \n \t/* make sure shallows are read */\n \tis_repository_shallow(the_repository);\n@@ -3757,13 +3756,13 @@ static void get_object_list(int ac, const char **av)\n \t\t\t}\n \t\t\tdie(_(\"not a rev '%s'\"), line);\n \t\t}\n-\t\tif (handle_revision_arg(line, &revs, flags, REVARG_CANNOT_BE_FILENAME))\n+\t\tif (handle_revision_arg(line, revs, flags, REVARG_CANNOT_BE_FILENAME))\n \t\t\tdie(_(\"bad revision '%s'\"), line);\n \t}\n \n \twarn_on_object_refname_ambiguity = save_warning;\n \n-\tif (use_bitmap_index && !get_object_list_from_bitmap(&revs))\n+\tif (use_bitmap_index && !get_object_list_from_bitmap(revs))\n \t\treturn;\n \n \tif (use_delta_islands)\n@@ -3772,24 +3771,24 @@ static void get_object_list(int ac, const char **av)\n \tif (write_bitmap_index)\n \t\tmark_bitmap_preferred_tips();\n \n-\tif (prepare_revision_walk(&revs))\n+\tif (prepare_revision_walk(revs))\n \t\tdie(_(\"revision walk setup failed\"));\n-\tmark_edges_uninteresting(&revs, show_edge, sparse);\n+\tmark_edges_uninteresting(revs, show_edge, sparse);\n \n \tif (!fn_show_object)\n \t\tfn_show_object = show_object;\n-\ttraverse_commit_list(&revs,\n+\ttraverse_commit_list(revs,\n \t\t\t show_commit, fn_show_object,\n \t\t\t NULL);\n \n \tif (unpack_unreachable_expiration) {\n-\t\trevs.ignore_missing_links = 1;\n-\t\tif (add_unseen_recent_objects_to_traversal(&revs,\n+\t\trevs->ignore_missing_links = 1;\n+\t\tif (add_unseen_recent_objects_to_traversal(revs,\n \t\t\t\tunpack_unreachable_expiration))\n \t\t\tdie(_(\"unable to add recent objects\"));\n-\t\tif (prepare_revision_walk(&revs))\n+\t\tif (prepare_revision_walk(revs))\n \t\t\tdie(_(\"revision walk setup failed\"));\n-\t\ttraverse_commit_list(&revs, record_recent_commit,\n+\t\ttraverse_commit_list(revs, record_recent_commit,\n \t\t\t\t record_recent_object, NULL);\n \t}\n \n@@ -3872,6 +3871,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 rev_info revs;\n+\n \tstruct option pack_objects_options[] = {\n \t\tOPT_SET_INT('q', \"quiet\", &progress,\n \t\t\t N_(\"do not show progress meter\"), 0),\n@@ -3976,6 +3977,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@@ -4154,7 +4157,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \t} else if (!use_internal_rev_list) {\n \t\tread_object_list_from_stdin();\n \t} else {\n-\t\tget_object_list(rp.nr, rp.v);\n+\t\tget_object_list(&revs, rp.nr, rp.v);\n \t}\n \tcleanup_preferred_base();\n \tif (include_tag && nr_result)\n-- \ngitgitgadget\n\n"},{"id":"451853","messageId":"pull.1186.git.1647970119.gitgitgadget@gmail.com","threadId":"57600","inReplyTo":null,"subject":"[PATCH 0/5] Partial bundle follow ups","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-03-22T17:28:34Z","receivedAt":"2022-03-22T17:28:49Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"\nFor Junio\n=========\n\nThis is based on master, which recently took ds/partial-bundles.\n\nThere are a couple conflicts with 'seen':\n\n * rc/fetch-refetch adds the \"--refetch\" option right next to where I remove\n an instance of CL_ARG__FILTER.\n * tb/cruft-packs updates the add_unseen_recent_objects_to_traversal()\n method, but this series changes one call from using \"&revs\" to using\n \"revs\".\n\nHopefully these are easy conflicts to resolve.\n\n\nSeries Goals\n============\n\nThese are a few cleanups that were discussed as part of ds/partial-bundles.\n\n * Patch 1 removes the CL_ARG__FILTER macro.\n * Patches 2-3 help parse --filter directly into a revs.filter member\n instead of copying it from another filter_options.\n * Patches 4-5 add output of the hash function capability in 'git bundle\n verify'. It also moves the capability output to the end, since it looks a\n bit strange in the current location when there are boundary commits.\n\n\nSeries Non-Goals\n================\n\nSome items were discussed that I either attempted and dropped, or delayed\nfor another series:\n\n * create_bundle() in bundle.c does two commit walks: first to discover the\n boundary commits to write into the bundle header, and then another that\n happens when constructing the pack-file. It looks like this could be\n avoided if there will not be any boundary, but there are additional\n checks in write_bundle_refs() that look for the SHOWN bit on the commit\n objects in order to determine if a requested ref would be excluded by the\n rev-list options. This behavior seems important, so I did not remove it.\n\n * 'git clone --bare partial.bdl\" should work when partial.bdl is a partial\n bundle. However, this requires changing the way we configure partial\n cloned repositories to not rely on a remote in order to understand an\n object filter. I'm working on this as a parallel series that will likely\n require more discussion than these small cleanups.\n\n * Ævar pointed out some newly-visible memory leaks due to moving the filter\n out of a static global. After looking at the recommended change, it seems\n that we actually need to be more careful about freeing the revs and not\n just revs.filter.\n\nThanks, -Stolee\n\nDerrick Stolee (5):\n list-objects-filter: remove CL_ARG__FILTER\n pack-objects: move revs out of get_object_list()\n pack-objects: parse --filter directly into revs.filter\n bundle: move capabilities to end of 'verify'\n bundle: output hash information in 'verify'\n\n Documentation/git-bundle.txt | 10 +++++-----\n builtin/fetch-pack.c | 4 ++--\n builtin/pack-objects.c | 37 +++++++++++++++++------------------\n bundle.c | 11 ++++++-----\n list-objects-filter-options.h | 5 +----\n revision.c | 4 ++--\n t/t6020-bundle-misc.sh | 26 ++++++++++++++++--------\n 7 files changed, 52 insertions(+), 45 deletions(-)\n\n\nbase-commit: f01e51a7cfd75131b7266131b1f7540ce0a8e5c1\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1186%2Fderrickstolee%2Fpartial-bundle-follow-up-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1186/derrickstolee/partial-bundle-follow-up-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1186\n-- \ngitgitgadget\n"},{"id":"451854","messageId":"166d86fd7009a8a1b82a4157cb718a6a4a527a0c.1647970119.git.gitgitgadget@gmail.com","threadId":"57600","inReplyTo":"pull.1186.git.1647970119.gitgitgadget@gmail.com","subject":"[PATCH 1/5] list-objects-filter: remove CL_ARG__FILTER","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-03-22T17:28:35Z","receivedAt":"2022-03-22T17:28:52Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nWe have established the command-line interface for the --[no-]filter\noptions for a while now, so we do not need a helper to make this\neditable in the future.\n\nSigned-off-by: Derrick Stolee \n---\n builtin/fetch-pack.c | 4 ++--\n list-objects-filter-options.h | 5 +----\n revision.c | 4 ++--\n 3 files changed, 5 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex c2d96f4c89a..c4b9104f9b5 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -153,11 +153,11 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n \t\t\targs.from_promisor = 1;\n \t\t\tcontinue;\n \t\t}\n-\t\tif (skip_prefix(arg, (\"--\" CL_ARG__FILTER \"=\"), &arg)) {\n+\t\tif (skip_prefix(arg, (\"--filter=\"), &arg)) {\n \t\t\tparse_list_objects_filter(&args.filter_options, arg);\n \t\t\tcontinue;\n \t\t}\n-\t\tif (!strcmp(arg, (\"--no-\" CL_ARG__FILTER))) {\n+\t\tif (!strcmp(arg, (\"--no-filter\"))) {\n \t\t\tlist_objects_filter_set_no_filter(&args.filter_options);\n \t\t\tcontinue;\n \t\t}\ndiff --git a/list-objects-filter-options.h b/list-objects-filter-options.h\nindex 2eb6c983949..90e4bc96252 100644\n--- a/list-objects-filter-options.h\n+++ b/list-objects-filter-options.h\n@@ -69,9 +69,6 @@ struct list_objects_filter_options {\n \t */\n };\n \n-/* Normalized command line arguments */\n-#define CL_ARG__FILTER \"filter\"\n-\n /*\n * Parse value of the argument to the \"filter\" keyword.\n * On the command line this looks like:\n@@ -111,7 +108,7 @@ int opt_parse_list_objects_filter(const struct option *opt,\n \t\t\t\t const char *arg, int unset);\n \n #define OPT_PARSE_LIST_OBJECTS_FILTER(fo) \\\n-\tOPT_CALLBACK(0, CL_ARG__FILTER, fo, N_(\"args\"), \\\n+\tOPT_CALLBACK(0, \"filter\", fo, N_(\"args\"), \\\n \t N_(\"object filtering\"), \\\n \t opt_parse_list_objects_filter)\n \ndiff --git a/revision.c b/revision.c\nindex 2646b78990e..7d435f80480 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2691,9 +2691,9 @@ static int handle_revision_pseudo_opt(struct rev_info *revs,\n \t\trevs->no_walk = 0;\n \t} else if (!strcmp(arg, \"--single-worktree\")) {\n \t\trevs->single_worktree = 1;\n-\t} else if (skip_prefix(arg, (\"--\" CL_ARG__FILTER \"=\"), &arg)) {\n+\t} else if (skip_prefix(arg, (\"--filter=\"), &arg)) {\n \t\tparse_list_objects_filter(&revs->filter, arg);\n-\t} else if (!strcmp(arg, (\"--no-\" CL_ARG__FILTER))) {\n+\t} else if (!strcmp(arg, (\"--no-filter\"))) {\n \t\tlist_objects_filter_set_no_filter(&revs->filter);\n \t} else {\n \t\treturn 0;\n-- \ngitgitgadget\n\n"},{"id":"451855","messageId":"97de926904988b89b5663bd4c59c011a1723a8f5.1647970119.git.gitgitgadget@gmail.com","threadId":"57600","inReplyTo":"pull.1186.git.1647970119.gitgitgadget@gmail.com","subject":"[PATCH 3/5] pack-objects: parse --filter directly into revs.filter","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-03-22T17:28:37Z","receivedAt":"2022-03-22T17:28:53Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe previous change moved the 'revs' variable into cmd_pack_objects()\nand now we can remote the global filter_options in favor of revs.filter.\n\nSigned-off-by: Derrick Stolee \n---\n builtin/pack-objects.c | 8 ++------\n 1 file changed, 2 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex f29bef9d0b6..d39f668ad56 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -237,8 +237,6 @@ static unsigned long cache_max_small_delta_size = 1000;\n \n static unsigned long window_memory_limit = 0;\n \n-static struct list_objects_filter_options filter_options;\n-\n static struct string_list uri_protocols = STRING_LIST_INIT_NODUP;\n \n enum missing_action {\n@@ -3723,10 +3721,8 @@ static void get_object_list(struct rev_info *revs, int ac, const char **av)\n \tint flags = 0;\n \tint save_warning;\n \n-\trepo_init_revisions(the_repository, revs, NULL);\n \tsave_commit_buffer = 0;\n \tsetup_revisions(ac, av, revs, &s_r_opt);\n-\tlist_objects_filter_copy(&revs->filter, &filter_options);\n \n \t/* make sure shallows are read */\n \tis_repository_shallow(the_repository);\n@@ -3958,7 +3954,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(&filter_options),\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@@ -4080,7 +4076,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 (filter_options.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-- \ngitgitgadget\n\n"},{"id":"451856","messageId":"b3c9e6523a5466bd8949c77b480c74881edade2f.1647970119.git.gitgitgadget@gmail.com","threadId":"57600","inReplyTo":"pull.1186.git.1647970119.gitgitgadget@gmail.com","subject":"[PATCH 5/5] bundle: output hash information in 'verify'","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-03-22T17:28:39Z","receivedAt":"2022-03-22T17:28:58Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe previous change moved the 'filter' capability to the end of the 'git\nbundle verify' output. Now, add the 'object-format' capability to the\noutput, when it exists.\n\nThis change makes 'git bundle verify' output the hash used in all cases,\neven if the capability is not in the bundle. This means that v2 bundles\nwill always output that they use \"sha1\". This might look noisy to some\nusers, but it does simplify the implementation and the test strategy for\nthis feature.\n\nSince 'verify' ends early when a prerequisite commit is missing, we need\nto insert this hash message carefully into our expected test output\nthroughout t6020.\n\nSigned-off-by: Derrick Stolee \n---\n bundle.c | 2 ++\n t/t6020-bundle-misc.sh | 24 +++++++++++++++++-------\n 2 files changed, 19 insertions(+), 7 deletions(-)\n\ndiff --git a/bundle.c b/bundle.c\nindex 276b55f8ce2..d50cfb5aa7e 100644\n--- a/bundle.c\n+++ b/bundle.c\n@@ -278,6 +278,8 @@ int verify_bundle(struct repository *r,\n \t\t\tlist_refs(r, 0, NULL);\n \t\t}\n \n+\t\tprintf_ln(\"The bundle uses this hash algorithm: %s\",\n+\t\t\t header->hash_algo->name);\n \t\tif (header->filter.choice)\n \t\t\tprintf_ln(\"The bundle uses this filter: %s\",\n \t\t\t\t list_objects_filter_spec(&header->filter));\ndiff --git a/t/t6020-bundle-misc.sh b/t/t6020-bundle-misc.sh\nindex c4ab1367afc..833205125ab 100755\n--- a/t/t6020-bundle-misc.sh\n+++ b/t/t6020-bundle-misc.sh\n@@ -122,6 +122,8 @@ format_and_save_expect () {\n \tsed -e 's/Z$//' >expect\n }\n \n+HASH_MESSAGE=\"The bundle uses this hash algorithm: $GIT_DEFAULT_HASH\"\n+\n # (C) (D, pull/1/head, topic/1)\n # o --- o\n # / \\ (L)\n@@ -194,11 +196,12 @@ test_expect_success 'create bundle from special rev: main^!' '\n \n \tgit bundle verify special-rev.bdl |\n \t\tmake_user_friendly_and_stable_output >actual &&\n-\tformat_and_save_expect <<-\\EOF &&\n+\tformat_and_save_expect <<-EOF &&\n \tThe bundle contains this ref:\n \t refs/heads/main\n \tThe bundle requires this ref:\n \t Z\n+\t$HASH_MESSAGE\n \tEOF\n \ttest_cmp expect actual &&\n \n@@ -215,12 +218,13 @@ test_expect_success 'create bundle with --max-count option' '\n \n \tgit bundle verify max-count.bdl |\n \t\tmake_user_friendly_and_stable_output >actual &&\n-\tformat_and_save_expect <<-\\EOF &&\n+\tformat_and_save_expect <<-EOF &&\n \tThe bundle contains these 2 refs:\n \t refs/heads/main\n \t refs/tags/v1\n \tThe bundle requires this ref:\n \t Z\n+\t$HASH_MESSAGE\n \tEOF\n \ttest_cmp expect actual &&\n \n@@ -240,7 +244,7 @@ test_expect_success 'create bundle with --since option' '\n \n \tgit bundle verify since.bdl |\n \t\tmake_user_friendly_and_stable_output >actual &&\n-\tformat_and_save_expect <<-\\EOF &&\n+\tformat_and_save_expect <<-EOF &&\n \tThe bundle contains these 5 refs:\n \t refs/heads/main\n \t refs/heads/release\n@@ -250,6 +254,7 @@ test_expect_success 'create bundle with --since option' '\n \tThe bundle requires these 2 refs:\n \t Z\n \t Z\n+\t$HASH_MESSAGE\n \tEOF\n \ttest_cmp expect actual &&\n \n@@ -267,11 +272,12 @@ test_expect_success 'create bundle 1 - no prerequisites' '\n \tEOF\n \tgit bundle create stdin-1.bdl --stdin expect <<-\\EOF &&\n+\tformat_and_save_expect <<-EOF &&\n \tThe bundle contains these 2 refs:\n \t refs/heads/topic/1\n \t refs/heads/topic/2\n \tThe bundle records a complete history.\n+\t$HASH_MESSAGE\n \tEOF\n \n \t# verify bundle, which has no prerequisites\n@@ -308,13 +314,14 @@ test_expect_success 'create bundle 2 - has prerequisites' '\n \t\t--stdin \\\n \t\trelease refs/heads/release\n \tThe bundle requires these 3 refs:\n \t Z\n \t Z\n \t Z\n+\t$HASH_MESSAGE\n \tEOF\n \n \tgit bundle verify 2.bdl |\n@@ -367,13 +374,14 @@ test_expect_success 'create bundle 3 - two refs, same object' '\n \t\t--stdin \\\n \t\tmain HEAD refs/heads/main\n \t HEAD\n \tThe bundle requires these 2 refs:\n \t Z\n \t Z\n+\t$HASH_MESSAGE\n \tEOF\n \n \tgit bundle verify 3.bdl |\n@@ -409,12 +417,13 @@ test_expect_success 'create bundle 4 - with tags' '\n \t\t--stdin \\\n \t\t--all expect <<-\\EOF &&\n+\tcat >expect <<-EOF &&\n \tThe bundle contains these 3 refs:\n \t refs/tags/v1\n \t refs/tags/v2\n \t refs/tags/v3\n \tThe bundle records a complete history.\n+\t$HASH_MESSAGE\n \tEOF\n \n \tgit bundle verify 4.bdl |\n@@ -511,6 +520,7 @@ do\n \t\t refs/tags/v3\n \t\t HEAD\n \t\tThe bundle records a complete history.\n+\t\t$HASH_MESSAGE\n \t\tThe bundle uses this filter: $filter\n \t\tEOF\n \t\ttest_cmp expect actual &&\n-- \ngitgitgadget\n"},{"id":"451857","messageId":"1b2130426bd7bd6c0bf5c56be2bf66a4d81f0b27.1647970119.git.gitgitgadget@gmail.com","threadId":"57600","inReplyTo":"pull.1186.git.1647970119.gitgitgadget@gmail.com","subject":"[PATCH 4/5] bundle: move capabilities to end of 'verify'","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-03-22T17:28:38Z","receivedAt":"2022-03-22T17:29:01Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe 'filter' capability was added in 105c6f14a (bundle: parse filter\ncapability, 2022-03-09), but was added in a strange place in the 'git\nbundle verify' output.\n\nThe tests for this show output like the following:\n\n\tThe bundle contains these 2 refs:\n\t \n\t \n\tThe bundle uses this filter: blob:none\n\tThe bundle records a complete history.\n\nThis looks very odd if we have a thin bundle that contains boundary\ncommits instead of a complete history:\n\n\tThe bundle contains these 2 refs:\n\t \n\t \n\tThe bundle uses this filter: blob:none\n\tThe bundle requires these 2 refs:\n\t\n\t\n\nThis separation between tip refs and boundary refs is unfortunate. Move\nthe filter capability output to the end of the output. Update the\ndocumentation to match.\n\nSigned-off-by: Derrick Stolee \n---\n Documentation/git-bundle.txt | 10 +++++-----\n bundle.c | 9 ++++-----\n t/t6020-bundle-misc.sh | 2 +-\n 3 files changed, 10 insertions(+), 11 deletions(-)\n\ndiff --git a/Documentation/git-bundle.txt b/Documentation/git-bundle.txt\nindex ac4c4352aae..7685b570455 100644\n--- a/Documentation/git-bundle.txt\n+++ b/Documentation/git-bundle.txt\n@@ -75,11 +75,11 @@ verify ::\n \tcleanly to the current repository. This includes checks on the\n \tbundle format itself as well as checking that the prerequisite\n \tcommits exist and are fully linked in the current repository.\n-\tInformation about additional capabilities, such as \"object filter\",\n-\tis printed. See \"Capabilities\" in link:technical/bundle-format.html\n-\tfor more information. Finally, 'git bundle' prints a list of\n-\tmissing commits, if any. The exit code is zero for success, but\n-\twill be nonzero if the bundle file is invalid.\n+\tThen, 'git bundle' prints a list of missing commits, if any.\n+\tFinally, information about additional capabilities, such as \"object\n+\tfilter\", is printed. See \"Capabilities\" in link:technical/bundle-format.html\n+\tfor more information. The exit code is zero for success, but will\n+\tbe nonzero if the bundle file is invalid.\n \n list-heads ::\n \tLists the references defined in the bundle. If followed by a\ndiff --git a/bundle.c b/bundle.c\nindex e359370cfcd..276b55f8ce2 100644\n--- a/bundle.c\n+++ b/bundle.c\n@@ -267,11 +267,6 @@ int verify_bundle(struct repository *r,\n \t\t\t (uintmax_t)r->nr);\n \t\tlist_refs(r, 0, NULL);\n \n-\t\tif (header->filter.choice) {\n-\t\t\tprintf_ln(\"The bundle uses this filter: %s\",\n-\t\t\t\t list_objects_filter_spec(&header->filter));\n-\t\t}\n-\n \t\tr = &header->prerequisites;\n \t\tif (!r->nr) {\n \t\t\tprintf_ln(_(\"The bundle records a complete history.\"));\n@@ -282,6 +277,10 @@ int verify_bundle(struct repository *r,\n \t\t\t\t (uintmax_t)r->nr);\n \t\t\tlist_refs(r, 0, NULL);\n \t\t}\n+\n+\t\tif (header->filter.choice)\n+\t\t\tprintf_ln(\"The bundle uses this filter: %s\",\n+\t\t\t\t list_objects_filter_spec(&header->filter));\n \t}\n \treturn ret;\n }\ndiff --git a/t/t6020-bundle-misc.sh b/t/t6020-bundle-misc.sh\nindex ed95d195427..c4ab1367afc 100755\n--- a/t/t6020-bundle-misc.sh\n+++ b/t/t6020-bundle-misc.sh\n@@ -510,8 +510,8 @@ do\n \t\t refs/tags/v2\n \t\t refs/tags/v3\n \t\t HEAD\n-\t\tThe bundle uses this filter: $filter\n \t\tThe bundle records a complete history.\n+\t\tThe bundle uses this filter: $filter\n \t\tEOF\n \t\ttest_cmp expect actual &&\n \n-- \ngitgitgadget\n\n"},{"id":"451923","messageId":"b5685cca-2fb9-dabf-fe4e-4a718817820b@ramsayjones.plus.com","threadId":"57600","inReplyTo":"97de926904988b89b5663bd4c59c011a1723a8f5.1647970119.git.gitgitgadget@gmail.com","subject":"Re: [-SPAM-] [PATCH 3/5] pack-objects: parse --filter directly into revs.filter","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2022-03-22T19:37:17Z","receivedAt":"2022-03-22T19:37:28Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 22/03/2022 17:28, Derrick Stolee via GitGitGadget wrote:\n> From: Derrick Stolee \n> \n> The previous change moved the 'revs' variable into cmd_pack_objects()\n> and now we can remote the global filter_options in favor of revs.filter.\n\ns/remote/remove/\n\nATB,\nRamsay Jones\n\n"},{"id":"451930","messageId":"220322.861qytpu31.gmgdl@evledraar.gmail.com","threadId":"57600","inReplyTo":"97de926904988b89b5663bd4c59c011a1723a8f5.1647970119.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 3/5] pack-objects: parse --filter directly into revs.filter","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-03-22T21:15:14Z","receivedAt":"2022-03-22T21:18:27Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Mar 22 2022, Derrick Stolee via GitGitGadget wrote:\n\n> From: Derrick Stolee \n>\n> The previous change moved the 'revs' variable into cmd_pack_objects()\n> and now we can remote the global filter_options in favor of revs.filter.\n\ngrammar: s/remote/remove/\n"},{"id":"451952","messageId":"29e64add-59ca-df77-a717-58c09c686336@gmail.com","threadId":"57600","inReplyTo":"1b2130426bd7bd6c0bf5c56be2bf66a4d81f0b27.1647970119.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 4/5] bundle: move capabilities to end of 'verify'","fromName":"Bagas Sanjaya","fromEmail":"bagasdotme@gmail.com","sentAt":"2022-03-23T07:08:47Z","receivedAt":"2022-03-23T07:08:56Z","isPatch":true,"sender":{"key":"bagasdotme@gmail.com","avatar":"https://avatars.githubusercontent.com/u/40219486?v=4"},"body":"On 23/03/22 00.28, Derrick Stolee via GitGitGadget wrote:\n\n> -\tInformation about additional capabilities, such as \"object filter\",\n> -\tis printed. See \"Capabilities\" in link:technical/bundle-format.html\n> -\tfor more information. Finally, 'git bundle' prints a list of\n> -\tmissing commits, if any. The exit code is zero for success, but\n> -\twill be nonzero if the bundle file is invalid.\n> +\tThen, 'git bundle' prints a list of missing commits, if any.\n> +\tFinally, information about additional capabilities, such as \"object\n> +\tfilter\", is printed. See \"Capabilities\" in link:technical/bundle-format.html\n> +\tfor more information. The exit code is zero for success, but will\n> +\tbe nonzero if the bundle file is invalid.\n\nThat means, nonzero exit code for corrupted bundle files, right?\n\n-- \nAn old man doll... just what I always wanted! - Clara\n"},{"id":"451980","messageId":"d0f5f912-c566-fc1d-b0b3-41669e34210a@github.com","threadId":"57600","inReplyTo":"29e64add-59ca-df77-a717-58c09c686336@gmail.com","subject":"Re: [PATCH 4/5] bundle: move capabilities to end of 'verify'","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-03-23T13:39:54Z","receivedAt":"2022-03-23T13:40:01Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 3/23/2022 3:08 AM, Bagas Sanjaya wrote:\n> On 23/03/22 00.28, Derrick Stolee via GitGitGadget wrote:\n> \n>> -    Information about additional capabilities, such as \"object filter\",\n>> -    is printed. See \"Capabilities\" in link:technical/bundle-format.html\n>> -    for more information. Finally, 'git bundle' prints a list of\n>> -    missing commits, if any. The exit code is zero for success, but\n>> -    will be nonzero if the bundle file is invalid.\n>> +    Then, 'git bundle' prints a list of missing commits, if any.\n>> +    Finally, information about additional capabilities, such as \"object\n>> +    filter\", is printed. See \"Capabilities\" in link:technical/bundle-format.html\n>> +    for more information. The exit code is zero for success, but will\n>> +    be nonzero if the bundle file is invalid.\n> \n> That means, nonzero exit code for corrupted bundle files, right?\n\nYes. That last sentence is unchanged from the earlier version\n(modulo line wrapping).\n\nThanks,\n-Stolee\n"},{"id":"451981","messageId":"0b827ba1-5870-dfc0-8fdb-455e2f23a048@github.com","threadId":"57600","inReplyTo":"b5685cca-2fb9-dabf-fe4e-4a718817820b@ramsayjones.plus.com","subject":"Re: [-SPAM-] [PATCH 3/5] pack-objects: parse --filter directly into revs.filter","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-03-23T13:48:54Z","receivedAt":"2022-03-23T13:48:59Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 3/22/2022 3:37 PM, Ramsay Jones wrote:> On 22/03/2022 17:28, Derrick Stolee via GitGitGadget wrote:\n>> From: Derrick Stolee \n>>\n>> The previous change moved the 'revs' variable into cmd_pack_objects()\n>> and now we can remote the global filter_options in favor of revs.filter.\n> \n> s/remote/remove/\n\nThanks (you and Ævar both).\n-Stolee\n"},{"id":"452069","messageId":"xmqqlex0qrz7.fsf@gitster.g","threadId":"57600","inReplyTo":"pull.1186.git.1647970119.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/5] Partial bundle follow ups","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-23T21:27:56Z","receivedAt":"2022-03-23T21:28:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" writes:\n\n> This is based on master, which recently took ds/partial-bundles.\n>\n> There are a couple conflicts with 'seen':\n>\n> * rc/fetch-refetch adds the \"--refetch\" option right next to where I remove\n> an instance of CL_ARG__FILTER.\n> * tb/cruft-packs updates the add_unseen_recent_objects_to_traversal()\n> method, but this series changes one call from using \"&revs\" to using\n> \"revs\".\n\nDemonstrating that the submitter has already made an effort to make\nsure the new topic plays well with other topics in flight this way\nis very much appreciated.\n\n> These are a few cleanups that were discussed as part of ds/partial-bundles.\n>\n> * Patch 1 removes the CL_ARG__FILTER macro.\n> * Patches 2-3 help parse --filter directly into a revs.filter member\n> instead of copying it from another filter_options.\n> * Patches 4-5 add output of the hash function capability in 'git bundle\n> verify'. It also moves the capability output to the end, since it looks a\n> bit strange in the current location when there are boundary commits.\n\nOK.\n\n> * create_bundle() in bundle.c does two commit walks: first to discover the\n> boundary commits to write into the bundle header, and then another that\n> happens when constructing the pack-file. It looks like this could be\n> avoided if there will not be any boundary, but there are additional\n> checks in write_bundle_refs() that look for the SHOWN bit on the commit\n> objects in order to determine if a requested ref would be excluded by the\n> rev-list options. This behavior seems important, so I did not remove it.\n\nGood thinking. Thanks.\n\n> * 'git clone --bare partial.bdl\" should work when partial.bdl is a partial\n> bundle. However, this requires changing the way we configure partial\n> cloned repositories to not rely on a remote in order to understand an\n> object filter. I'm working on this as a parallel series that will likely\n> require more discussion than these small cleanups.\n\nLeaving it outside the topic, saying that you cannot yet clone from\nsuch a bundle, is perfectly fine, I would think. Thanks.\n\n> * Ævar pointed out some newly-visible memory leaks due to moving the filter\n> out of a static global. After looking at the recommended change, it seems\n> that we actually need to be more careful about freeing the revs and not\n> just revs.filter.\n\nOK.\n\n"},{"id":"452266","messageId":"patch-1.1-193534b0f07-20220325T121715Z-avarab@gmail.com","threadId":"57600","inReplyTo":"pull.1186.git.1647970119.gitgitgadget@gmail.com","subject":"[PATCH] pack-objects: lazily set up \"struct rev_info\", don't leak","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-03-25T14:25:55Z","receivedAt":"2022-03-25T14:26:24Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"In the preceding [1] (pack-objects: move revs out of\nget_object_list(), 2022-03-22) the \"repo_init_revisions()\" was moved\nto cmd_pack_objects() so that it unconditionally took place for all\ninvocations of \"git pack-objects\".\n\nWe'd thus start leaking memory, which is easily reproduced in\ne.g. git.git by feeding e83c5163316 (Initial revision of \"git\", the\ninformation manager from hell, 2005-04-07) to \"git pack-objects\";\n\n $ echo e83c5163316f89bfbde7d9ab23ca2e25604af290 | ./git pack-objects initial\n [...]\n\t==19130==ERROR: LeakSanitizer: detected memory leaks\n\n\tDirect leak of 7120 byte(s) in 1 object(s) allocated from:\n\t #0 0x455308 in __interceptor_malloc (/home/avar/g/git/git+0x455308)\n\t #1 0x75b399 in do_xmalloc /home/avar/g/git/wrapper.c:41:8\n\t #2 0x75b356 in xmalloc /home/avar/g/git/wrapper.c:62:9\n\t #3 0x5d7609 in prep_parse_options /home/avar/g/git/diff.c:5647:2\n\t #4 0x5d415a in repo_diff_setup /home/avar/g/git/diff.c:4621:2\n\t #5 0x6dffbb in repo_init_revisions /home/avar/g/git/revision.c:1853:2\n\t #6 0x4f599d in cmd_pack_objects /home/avar/g/git/builtin/pack-objects.c:3980:2\n\t #7 0x4592ca in run_builtin /home/avar/g/git/git.c:465:11\n\t #8 0x457d81 in handle_builtin /home/avar/g/git/git.c:718:3\n\t #9 0x458ca5 in run_argv /home/avar/g/git/git.c:785:4\n\t #10 0x457b40 in cmd_main /home/avar/g/git/git.c:916:19\n\t #11 0x562259 in main /home/avar/g/git/common-main.c:56:11\n\t #12 0x7fce792ac7ec in __libc_start_main csu/../csu/libc-start.c:332:16\n\t #13 0x4300f9 in _start (/home/avar/g/git/git+0x4300f9)\n\n\tSUMMARY: LeakSanitizer: 7120 byte(s) leaked in 1 allocation(s).\n\tAborted\n\nNarrowly fixing that commit would have been easy, just add call\nrepo_init_revisions() right before get_object_list(), which is\neffectively what was done before that commit.\n\nBut an unstated constraint when setting it up early is that it was\nneeded for the subsequent [2] (pack-objects: parse --filter directly\ninto revs.filter, 2022-03-22), i.e. we might have a --filter\ncommand-line option, and need to either have the \"struct rev_info\"\nsetup when we encounter that option, or later.\n\nLet's just change the control flow so that we'll instead set up the\n\"struct rev_info\" only when we need it. Doing so leads to a bit more\nverbosity, but it's a lot clearer what we're doing and why.\n\nWe could furthermore combine the two get_object_list() invocations\nhere by having repo_init_revisions() invoked on &pfd.revs, but I think\nclearly separating the two makes the flow clearer. Likewise\nredundantly but explicitly (i.e. redundant v.s. a \"{ 0 }\") \"0\" to\n\"have_revs\" early in cmd_pack_objects().\n\nThis does add the future constraint to opt_parse_list_objects_filter()\nthat we'll need to adjust this wrapper code if it looks at any other\nvalue of the \"struct option\" than the \"value\" member.\n\nBut that regression should be relatively easy to spot. I'm\nintentionally not initializing the \"struct wrap\" with e.g. \"{ 0 }\" so\nthat various memory sanity checkers would spot that, we just\ninitialize the \"value\" in po_filter_cb(). By doing this e.g. we'll die\non e.g. this test if we were to use another member of \"opt\" in\nopt_parse_list_objects_filter()>\n\n ./t5317-pack-objects-filter-objects.sh -vixd --valgrind-only=3\n\nWhile we're at it add parentheses around the arguments to the OPT_*\nmacros in in list-objects-filter-options.h, as we need to change those\nlines anyway. It doesn't matter in this case, but is good general\npractice.\n\n1. https://lore.kernel.org/git/619b757d98465dbc4995bdc11a5282fbfcbd3daa.1647970119.git.gitgitgadget@gmail.com\n2. https://lore.kernel.org/git/97de926904988b89b5663bd4c59c011a1723a8f5.1647970119.git.gitgitgadget@gmail.com/\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n\nThis is on top of ds/partial-bundle-more: I thought the fix for this\nnew leak was involved enough to propose it as a commit-on-top rather\nthan a fixup for a re-roll, especially since aside from the newly\nleaked memory I don't think ds/partial-bundle-more is breaking\nanything by doing that.\n\nExcept that is, interacting badly with my release_revisions() series\nin \"seen\", which currently causes the \"linux-leaks\" job to fail there:\nhttps://lore.kernel.org/git/cover-v2-00.27-00000000000-20220323T203149Z-avarab@gmail.com/\n\nThis is proper fix for the issue the interaction with my topic\nrevealed (not caused, we just started testing for this leak there),\ni.e. it obsoletes the suggestion of adding an UNLEAK() there.\n\n builtin/pack-objects.c | 30 +++++++++++++++++++++++++-----\n list-objects-filter-options.h | 8 +++++---\n 2 files changed, 30 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex d39f668ad56..735080a4a95 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -3857,6 +3857,23 @@ static int option_parse_unpack_unreachable(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 po_filter_cb(const struct option *opt, const char *arg, int unset)\n+{\n+\tstruct po_filter_data *data = opt->value;\n+\tstruct option wrap; /* don't initialize! */\n+\n+\trepo_init_revisions(the_repository, &data->revs, NULL);\n+\twrap.value = &data->revs.filter;\n+\tdata->have_revs = 1;\n+\n+\treturn opt_parse_list_objects_filter(&wrap, 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@@ -3867,7 +3884,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@@ -3954,7 +3971,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(&revs.filter),\n+\t\tOPT_PARSE_LIST_OBJECTS_FILTER_CB(&pfd, po_filter_cb),\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@@ -3973,8 +3990,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@@ -4076,7 +4091,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@@ -4152,7 +4167,12 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \t\t\tadd_unreachable_loose_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} 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}\n \tcleanup_preferred_base();\ndiff --git a/list-objects-filter-options.h b/list-objects-filter-options.h\nindex 90e4bc96252..910f4a46e91 100644\n--- a/list-objects-filter-options.h\n+++ b/list-objects-filter-options.h\n@@ -107,10 +107,12 @@ void parse_list_objects_filter(\n int opt_parse_list_objects_filter(const struct option *opt,\n \t\t\t\t const char *arg, int unset);\n \n+#define OPT_PARSE_LIST_OBJECTS_FILTER_CB(fo, cb) \\\n+\tOPT_CALLBACK(0, \"filter\", (fo), N_(\"args\"), \\\n+\t\t N_(\"object filtering\"), (cb))\n+\n #define OPT_PARSE_LIST_OBJECTS_FILTER(fo) \\\n-\tOPT_CALLBACK(0, \"filter\", fo, N_(\"args\"), \\\n-\t N_(\"object filtering\"), \\\n-\t opt_parse_list_objects_filter)\n+\tOPT_PARSE_LIST_OBJECTS_FILTER_CB((fo), opt_parse_list_objects_filter)\n \n /*\n * Translates abbreviated numbers in the filter's filter_spec into their\n-- \n2.35.1.1501.g363d33e530b\n\n"},{"id":"452272","messageId":"d71fa968-be75-f4ad-ea6c-644f9d2b52d9@github.com","threadId":"57600","inReplyTo":"patch-1.1-193534b0f07-20220325T121715Z-avarab@gmail.com","subject":"Re: [PATCH] pack-objects: lazily set up \"struct rev_info\", don't leak","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-03-25T14:57:04Z","receivedAt":"2022-03-25T14:57:09Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 3/25/2022 10:25 AM, Ævar Arnfjörð Bjarmason wrote:\n> In the preceding [1] (pack-objects: move revs out of\n> get_object_list(), 2022-03-22) the \"repo_init_revisions()\" was moved\n> to cmd_pack_objects() so that it unconditionally took place for all\n> invocations of \"git pack-objects\".\n> \n> We'd thus start leaking memory, which is easily reproduced in\n> e.g. git.git by feeding e83c5163316 (Initial revision of \"git\", the\n> information manager from hell, 2005-04-07) to \"git pack-objects\";\n> \n> $ echo e83c5163316f89bfbde7d9ab23ca2e25604af290 | ./git pack-objects initial\n> [...]\n> \t==19130==ERROR: LeakSanitizer: detected memory leaks\n> \n> \tDirect leak of 7120 byte(s) in 1 object(s) allocated from:\n> \t #0 0x455308 in __interceptor_malloc (/home/avar/g/git/git+0x455308)\n> \t #1 0x75b399 in do_xmalloc /home/avar/g/git/wrapper.c:41:8\n> \t #2 0x75b356 in xmalloc /home/avar/g/git/wrapper.c:62:9\n> \t #3 0x5d7609 in prep_parse_options /home/avar/g/git/diff.c:5647:2\n> \t #4 0x5d415a in repo_diff_setup /home/avar/g/git/diff.c:4621:2\n> \t #5 0x6dffbb in repo_init_revisions /home/avar/g/git/revision.c:1853:2\n> \t #6 0x4f599d in cmd_pack_objects /home/avar/g/git/builtin/pack-objects.c:3980:2\n> \t #7 0x4592ca in run_builtin /home/avar/g/git/git.c:465:11\n> \t #8 0x457d81 in handle_builtin /home/avar/g/git/git.c:718:3\n> \t #9 0x458ca5 in run_argv /home/avar/g/git/git.c:785:4\n> \t #10 0x457b40 in cmd_main /home/avar/g/git/git.c:916:19\n> \t #11 0x562259 in main /home/avar/g/git/common-main.c:56:11\n> \t #12 0x7fce792ac7ec in __libc_start_main csu/../csu/libc-start.c:332:16\n> \t #13 0x4300f9 in _start (/home/avar/g/git/git+0x4300f9)\n> \n> \tSUMMARY: LeakSanitizer: 7120 byte(s) leaked in 1 allocation(s).\n> \tAborted\n> \n> Narrowly fixing that commit would have been easy, just add call\n> repo_init_revisions() right before get_object_list(), which is\n> effectively what was done before that commit.\n> \n> But an unstated constraint when setting it up early is that it was\n> needed for the subsequent [2] (pack-objects: parse --filter directly\n> into revs.filter, 2022-03-22), i.e. we might have a --filter\n> command-line option, and need to either have the \"struct rev_info\"\n> setup when we encounter that option, or later.\n> \n> Let's just change the control flow so that we'll instead set up the\n> \"struct rev_info\" only when we need it. Doing so leads to a bit more\n> verbosity, but it's a lot clearer what we're doing and why.\n\nThis makes sense.\n\n> We could furthermore combine the two get_object_list() invocations\n> here by having repo_init_revisions() invoked on &pfd.revs, but I think\n> clearly separating the two makes the flow clearer. Likewise\n> redundantly but explicitly (i.e. redundant v.s. a \"{ 0 }\") \"0\" to\n> \"have_revs\" early in cmd_pack_objects().\n\nI disagree, especially when you later want to make sure we free\nthe data from revs using your release_revisions().\n\n> This does add the future constraint to opt_parse_list_objects_filter()\n> that we'll need to adjust this wrapper code if it looks at any other\n> value of the \"struct option\" than the \"value\" member.\n\nSo we are coupling ourselves to the implementation of this method.\n\n> But that regression should be relatively easy to spot. I'm\n> intentionally not initializing the \"struct wrap\" with e.g. \"{ 0 }\" so\n> that various memory sanity checkers would spot that, we just\n> initialize the \"value\" in po_filter_cb(). By doing this e.g. we'll die\n> on e.g. this test if we were to use another member of \"opt\" in\n> opt_parse_list_objects_filter()>\n\nSo you are using uninitialized memory as a way to discover any\nnecessary changes to that coupling. I'm not sure this is super-safe\nbecause we don't necessarily run memory checkers during CI builds.\n\nI'd rather have a consistently initialized chunk of data that would\nbehave predictably (and hopefully we'd discover it is behaving\nincorrectly with that predictable behavior).\n\n\n> While we're at it add parentheses around the arguments to the OPT_*\n> macros in in list-objects-filter-options.h, as we need to change those\n> lines anyway. It doesn't matter in this case, but is good general\n> practice.\n> \n> 1. https://lore.kernel.org/git/619b757d98465dbc4995bdc11a5282fbfcbd3daa.1647970119.git.gitgitgadget@gmail.com\n> 2. https://lore.kernel.org/git/97de926904988b89b5663bd4c59c011a1723a8f5.1647970119.git.gitgitgadget@gmail.com/\n> \n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> \n> This is on top of ds/partial-bundle-more: I thought the fix for this\n> new leak was involved enough to propose it as a commit-on-top rather\n> than a fixup for a re-roll, especially since aside from the newly\n> leaked memory I don't think ds/partial-bundle-more is breaking\n> anything by doing that.\n> \n> Except that is, interacting badly with my release_revisions() series\n> in \"seen\", which currently causes the \"linux-leaks\" job to fail there:\n> https://lore.kernel.org/git/cover-v2-00.27-00000000000-20220323T203149Z-avarab@gmail.com/\n> \n> This is proper fix for the issue the interaction with my topic\n> revealed (not caused, we just started testing for this leak there),\n> i.e. it obsoletes the suggestion of adding an UNLEAK() there.\n> \n> builtin/pack-objects.c | 30 +++++++++++++++++++++++++-----\n> list-objects-filter-options.h | 8 +++++---\n> 2 files changed, 30 insertions(+), 8 deletions(-)\n> \n> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n> index d39f668ad56..735080a4a95 100644\n> --- a/builtin/pack-objects.c\n> +++ b/builtin/pack-objects.c\n> @@ -3857,6 +3857,23 @@ static int option_parse_unpack_unreachable(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 po_filter_cb(const struct option *opt, const char *arg, int unset)\n> +{\n> +\tstruct po_filter_data *data = opt->value;\n> +\tstruct option wrap; /* don't initialize! */\n> +\n> +\trepo_init_revisions(the_repository, &data->revs, NULL);\n> +\twrap.value = &data->revs.filter;\n> +\tdata->have_revs = 1;\n> +\n> +\treturn opt_parse_list_objects_filter(&wrap, arg, unset);\n> +}\n\nThe coupling here is unfortunate, but unavoidable. The future-proof\nway to do it would be to modify opt->value and pass the rest of its\nmembers as-is, but it's marked const so that's not an option.\n\nOne way to help make this coupling more obvious would be to move\nthis method into list-filter-options.c so we can have their\nimplementations adjacent and even refer to them.\n\nHere is a potential version that looks like that:\n\n--- >8 ---\n\ndiff --git a/list-objects-filter-options.c b/list-objects-filter-options.c\nindex f02d8df142..55c7131814 100644\n--- a/list-objects-filter-options.c\n+++ b/list-objects-filter-options.c\n@@ -281,6 +281,10 @@ void parse_list_objects_filter(\n \t\tdie(\"%s\", errbuf.buf);\n }\n \n+/*\n+ * If you change this to use any member in 'opt' other than 'value',\n+ * then be sure to update opt_parse_list_objects_filter_in_revs().\n+ */\n int opt_parse_list_objects_filter(const struct option *opt,\n \t\t\t\t const char *arg, int unset)\n {\n@@ -293,6 +297,18 @@ int opt_parse_list_objects_filter(const struct option *opt,\n \treturn 0;\n }\n \n+int opt_parse_list_objects_filter_in_revs(const struct option *opt,\n+\t\t\t\t\t const char *arg, int unset)\n+{\n+\tstruct rev_info_maybe_empty *ri = opt->value;\n+\tstruct option wrap = { .value = &ri->revs.filter };\n+\n+\trepo_init_revisions(the_repository, &ri->revs, NULL);\n+\tri->has_revs = 1;\n+\n+\treturn opt_parse_list_objects_filter(&wrap, arg, unset);\n+}\n+\n const char *list_objects_filter_spec(struct list_objects_filter_options *filter)\n {\n \tif (!filter->filter_spec.nr)\ndiff --git a/list-objects-filter-options.h b/list-objects-filter-options.h\nindex 90e4bc9625..cf8b710a76 100644\n--- a/list-objects-filter-options.h\n+++ b/list-objects-filter-options.h\n@@ -106,6 +106,8 @@ void parse_list_objects_filter(\n \n int opt_parse_list_objects_filter(const struct option *opt,\n \t\t\t\t const char *arg, int unset);\n+int opt_parse_list_objects_filter_in_revs(const struct option *opt,\n+\t\t\t\t\t const char *arg, int unset);\n \n #define OPT_PARSE_LIST_OBJECTS_FILTER(fo) \\\n \tOPT_CALLBACK(0, \"filter\", fo, N_(\"args\"), \\\ndiff --git a/revision.h b/revision.h\nindex 5bc59c7bfe..95aa3ee891 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -329,6 +329,11 @@ struct rev_info {\n \tstruct tmp_objdir *remerge_objdir;\n };\n \n+struct rev_info_maybe_empty {\n+\tint has_revs;\n+\tstruct rev_info revs;\n+};\n+\n int ref_excluded(struct string_list *, const char *path);\n void clear_ref_exclusion(struct string_list **);\n void add_ref_exclusion(struct string_list **, const char *exclude);\n\n--- >8 ---\n\n\n> +\t} else if (pfd.have_revs) {\n> +\t\tget_object_list(&pfd.revs, rp.nr, rp.v);\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}\n\nHere, I think it would be better to have\n\n\telse {\n\t\tif (!pfd.have_revs) {\n\t\t\trepo_init_revisions(the_repository, &pfd.revs, NULL);\n\t\t\tpfd.have_revs = 1;\n\t\t}\n\t\tget_object_list(&pfd.revs, rp.nr, rp.v);\n\t}\n\nand then later you can add\n\n\tif (pfd.have_revs)\n\t\trelease_revisions(&pfd.revs);\n\nto clear the memory in exactly one place.\n\nThanks,\n-Stolee\n"},{"id":"452274","messageId":"220325.86r16qkodl.gmgdl@evledraar.gmail.com","threadId":"57600","inReplyTo":"d71fa968-be75-f4ad-ea6c-644f9d2b52d9@github.com","subject":"Re: [PATCH] pack-objects: lazily set up \"struct rev_info\", don't leak","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-03-25T16:00:14Z","receivedAt":"2022-03-25T16:07:01Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Mar 25 2022, Derrick Stolee wrote:\n\n> On 3/25/2022 10:25 AM, Ævar Arnfjörð Bjarmason wrote:\n>> In the preceding [1] (pack-objects: move revs out of\n>> get_object_list(), 2022-03-22) the \"repo_init_revisions()\" was moved\n>> to cmd_pack_objects() so that it unconditionally took place for all\n>> invocations of \"git pack-objects\".\n>> \n>> We'd thus start leaking memory, which is easily reproduced in\n>> e.g. git.git by feeding e83c5163316 (Initial revision of \"git\", the\n>> information manager from hell, 2005-04-07) to \"git pack-objects\";\n>> \n>> $ echo e83c5163316f89bfbde7d9ab23ca2e25604af290 | ./git pack-objects initial\n>> [...]\n>> \t==19130==ERROR: LeakSanitizer: detected memory leaks\n>> \n>> \tDirect leak of 7120 byte(s) in 1 object(s) allocated from:\n>> \t #0 0x455308 in __interceptor_malloc (/home/avar/g/git/git+0x455308)\n>> \t #1 0x75b399 in do_xmalloc /home/avar/g/git/wrapper.c:41:8\n>> \t #2 0x75b356 in xmalloc /home/avar/g/git/wrapper.c:62:9\n>> \t #3 0x5d7609 in prep_parse_options /home/avar/g/git/diff.c:5647:2\n>> \t #4 0x5d415a in repo_diff_setup /home/avar/g/git/diff.c:4621:2\n>> \t #5 0x6dffbb in repo_init_revisions /home/avar/g/git/revision.c:1853:2\n>> \t #6 0x4f599d in cmd_pack_objects /home/avar/g/git/builtin/pack-objects.c:3980:2\n>> \t #7 0x4592ca in run_builtin /home/avar/g/git/git.c:465:11\n>> \t #8 0x457d81 in handle_builtin /home/avar/g/git/git.c:718:3\n>> \t #9 0x458ca5 in run_argv /home/avar/g/git/git.c:785:4\n>> \t #10 0x457b40 in cmd_main /home/avar/g/git/git.c:916:19\n>> \t #11 0x562259 in main /home/avar/g/git/common-main.c:56:11\n>> \t #12 0x7fce792ac7ec in __libc_start_main csu/../csu/libc-start.c:332:16\n>> \t #13 0x4300f9 in _start (/home/avar/g/git/git+0x4300f9)\n>> \n>> \tSUMMARY: LeakSanitizer: 7120 byte(s) leaked in 1 allocation(s).\n>> \tAborted\n>> \n>> Narrowly fixing that commit would have been easy, just add call\n>> repo_init_revisions() right before get_object_list(), which is\n>> effectively what was done before that commit.\n>> \n>> But an unstated constraint when setting it up early is that it was\n>> needed for the subsequent [2] (pack-objects: parse --filter directly\n>> into revs.filter, 2022-03-22), i.e. we might have a --filter\n>> command-line option, and need to either have the \"struct rev_info\"\n>> setup when we encounter that option, or later.\n>> \n>> Let's just change the control flow so that we'll instead set up the\n>> \"struct rev_info\" only when we need it. Doing so leads to a bit more\n>> verbosity, but it's a lot clearer what we're doing and why.\n>\n> This makes sense.\n>\n>> We could furthermore combine the two get_object_list() invocations\n>> here by having repo_init_revisions() invoked on &pfd.revs, but I think\n>> clearly separating the two makes the flow clearer. Likewise\n>> redundantly but explicitly (i.e. redundant v.s. a \"{ 0 }\") \"0\" to\n>> \"have_revs\" early in cmd_pack_objects().\n>\n> I disagree, especially when you later want to make sure we free\n> the data from revs using your release_revisions().\n\nBecause we'll need two release_revisions() instead of one?\n\n>> This does add the future constraint to opt_parse_list_objects_filter()\n>> that we'll need to adjust this wrapper code if it looks at any other\n>> value of the \"struct option\" than the \"value\" member.\n>\n> So we are coupling ourselves to the implementation of this method.\n\nSure, but aren't all OPT_* where the macro is providing some custom\ncallback coupling themselves to how it's implemented?\n\n>> But that regression should be relatively easy to spot. I'm\n>> intentionally not initializing the \"struct wrap\" with e.g. \"{ 0 }\" so\n>> that various memory sanity checkers would spot that, we just\n>> initialize the \"value\" in po_filter_cb(). By doing this e.g. we'll die\n>> on e.g. this test if we were to use another member of \"opt\" in\n>> opt_parse_list_objects_filter()>\n>\n> So you are using uninitialized memory as a way to discover any\n> necessary changes to that coupling. I'm not sure this is super-safe\n> because we don't necessarily run memory checkers during CI builds.\n>\n> I'd rather have a consistently initialized chunk of data that would\n> behave predictably (and hopefully we'd discover it is behaving\n> incorrectly with that predictable behavior).\n\nI'd like to keep this as-is, i.e. if you zero it out it might also\nbehave unpredictably or even in undefined ways (e.g. a NULL ptr\ndereference). Likewise in my version, but it has the benefit of being\ncaught by some tooling we have.\n\n>> +struct po_filter_data {\n>> +\tunsigned have_revs:1;\n>> +\tstruct rev_info revs;\n>> +};\n>> +\n>> +static int po_filter_cb(const struct option *opt, const char *arg, int unset)\n>> +{\n>> +\tstruct po_filter_data *data = opt->value;\n>> +\tstruct option wrap; /* don't initialize! */\n>> +\n>> +\trepo_init_revisions(the_repository, &data->revs, NULL);\n>> +\twrap.value = &data->revs.filter;\n>> +\tdata->have_revs = 1;\n>> +\n>> +\treturn opt_parse_list_objects_filter(&wrap, arg, unset);\n>> +}\n>\n> The coupling here is unfortunate, but unavoidable. The future-proof\n> way to do it would be to modify opt->value and pass the rest of its\n> members as-is, but it's marked const so that's not an option.\n\nWe could always cast it..., but that would probably be more nasty.\n\n> One way to help make this coupling more obvious would be to move\n> this method into list-filter-options.c so we can have their\n> implementations adjacent and even refer to them.\n>\n> Here is a potential version that looks like that:\n>\n> --- >8 ---\n>\n> diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c\n> index f02d8df142..55c7131814 100644\n> --- a/list-objects-filter-options.c\n> +++ b/list-objects-filter-options.c\n> @@ -281,6 +281,10 @@ void parse_list_objects_filter(\n> \t\tdie(\"%s\", errbuf.buf);\n> }\n> \n> +/*\n> + * If you change this to use any member in 'opt' other than 'value',\n> + * then be sure to update opt_parse_list_objects_filter_in_revs().\n> + */\n> int opt_parse_list_objects_filter(const struct option *opt,\n> \t\t\t\t const char *arg, int unset)\n> {\n> @@ -293,6 +297,18 @@ int opt_parse_list_objects_filter(const struct option *opt,\n> \treturn 0;\n> }\n> \n> +int opt_parse_list_objects_filter_in_revs(const struct option *opt,\n> +\t\t\t\t\t const char *arg, int unset)\n> +{\n> +\tstruct rev_info_maybe_empty *ri = opt->value;\n> +\tstruct option wrap = { .value = &ri->revs.filter };\n> +\n> +\trepo_init_revisions(the_repository, &ri->revs, NULL);\n> +\tri->has_revs = 1;\n> +\n> +\treturn opt_parse_list_objects_filter(&wrap, arg, unset);\n> +}\n> +\n> const char *list_objects_filter_spec(struct list_objects_filter_options *filter)\n> {\n> \tif (!filter->filter_spec.nr)\n> diff --git a/list-objects-filter-options.h b/list-objects-filter-options.h\n> index 90e4bc9625..cf8b710a76 100644\n> --- a/list-objects-filter-options.h\n> +++ b/list-objects-filter-options.h\n> @@ -106,6 +106,8 @@ void parse_list_objects_filter(\n> \n> int opt_parse_list_objects_filter(const struct option *opt,\n> \t\t\t\t const char *arg, int unset);\n> +int opt_parse_list_objects_filter_in_revs(const struct option *opt,\n> +\t\t\t\t\t const char *arg, int unset);\n> \n> #define OPT_PARSE_LIST_OBJECTS_FILTER(fo) \\\n> \tOPT_CALLBACK(0, \"filter\", fo, N_(\"args\"), \\\n> diff --git a/revision.h b/revision.h\n> index 5bc59c7bfe..95aa3ee891 100644\n> --- a/revision.h\n> +++ b/revision.h\n> @@ -329,6 +329,11 @@ struct rev_info {\n> \tstruct tmp_objdir *remerge_objdir;\n> };\n> \n> +struct rev_info_maybe_empty {\n> +\tint has_revs;\n> +\tstruct rev_info revs;\n> +};\n> +\n> int ref_excluded(struct string_list *, const char *path);\n> void clear_ref_exclusion(struct string_list **);\n> void add_ref_exclusion(struct string_list **, const char *exclude);\n>\n> --- >8 ---\n>\n>\n>> +\t} else if (pfd.have_revs) {\n>> +\t\tget_object_list(&pfd.revs, rp.nr, rp.v);\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}\n>\n> Here, I think it would be better to have\n>\n> \telse {\n> \t\tif (!pfd.have_revs) {\n> \t\t\trepo_init_revisions(the_repository, &pfd.revs, NULL);\n> \t\t\tpfd.have_revs = 1;\n> \t\t}\n> \t\tget_object_list(&pfd.revs, rp.nr, rp.v);\n> \t}\n\nConceptually I think the saving of that one line isn't worth it to the\nreader.\n\nThen you'd need to read up and see exactly how pfd.revs gets mutated,\nand its callback etc., only to see we're just doing this to save\nourselves a variable deceleration and a call to release_revisions().\n\n> and then later you can add\n>\n> \tif (pfd.have_revs)\n> \t\trelease_revisions(&pfd.revs);\n>\n> to clear the memory in exactly one place.\n\nYeah, it would work. I'd just prefer control flow that's trivial to\nreason about over saving a couple of lines here.\n"},{"id":"452276","messageId":"8d368240-dae5-7a66-6c0c-9e0a960ca34c@github.com","threadId":"57600","inReplyTo":"220325.86r16qkodl.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] pack-objects: lazily set up \"struct rev_info\", don't leak","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-03-25T16:41:35Z","receivedAt":"2022-03-25T16:41:44Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 3/25/2022 12:00 PM, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Fri, Mar 25 2022, Derrick Stolee wrote:\n> \n>> On 3/25/2022 10:25 AM, Ævar Arnfjörð Bjarmason wrote:\n>>> In the preceding [1] (pack-objects: move revs out of\n>>> get_object_list(), 2022-03-22) the \"repo_init_revisions()\" was moved\n>>> to cmd_pack_objects() so that it unconditionally took place for all\n>>> invocations of \"git pack-objects\".\n>>>\n>>> We'd thus start leaking memory, which is easily reproduced in\n>>> e.g. git.git by feeding e83c5163316 (Initial revision of \"git\", the\n>>> information manager from hell, 2005-04-07) to \"git pack-objects\";\n>>>\n>>> $ echo e83c5163316f89bfbde7d9ab23ca2e25604af290 | ./git pack-objects initial\n>>> [...]\n>>> \t==19130==ERROR: LeakSanitizer: detected memory leaks\n>>>\n>>> \tDirect leak of 7120 byte(s) in 1 object(s) allocated from:\n>>> \t #0 0x455308 in __interceptor_malloc (/home/avar/g/git/git+0x455308)\n>>> \t #1 0x75b399 in do_xmalloc /home/avar/g/git/wrapper.c:41:8\n>>> \t #2 0x75b356 in xmalloc /home/avar/g/git/wrapper.c:62:9\n>>> \t #3 0x5d7609 in prep_parse_options /home/avar/g/git/diff.c:5647:2\n>>> \t #4 0x5d415a in repo_diff_setup /home/avar/g/git/diff.c:4621:2\n>>> \t #5 0x6dffbb in repo_init_revisions /home/avar/g/git/revision.c:1853:2\n>>> \t #6 0x4f599d in cmd_pack_objects /home/avar/g/git/builtin/pack-objects.c:3980:2\n>>> \t #7 0x4592ca in run_builtin /home/avar/g/git/git.c:465:11\n>>> \t #8 0x457d81 in handle_builtin /home/avar/g/git/git.c:718:3\n>>> \t #9 0x458ca5 in run_argv /home/avar/g/git/git.c:785:4\n>>> \t #10 0x457b40 in cmd_main /home/avar/g/git/git.c:916:19\n>>> \t #11 0x562259 in main /home/avar/g/git/common-main.c:56:11\n>>> \t #12 0x7fce792ac7ec in __libc_start_main csu/../csu/libc-start.c:332:16\n>>> \t #13 0x4300f9 in _start (/home/avar/g/git/git+0x4300f9)\n>>>\n>>> \tSUMMARY: LeakSanitizer: 7120 byte(s) leaked in 1 allocation(s).\n>>> \tAborted\n>>>\n>>> Narrowly fixing that commit would have been easy, just add call\n>>> repo_init_revisions() right before get_object_list(), which is\n>>> effectively what was done before that commit.\n>>>\n>>> But an unstated constraint when setting it up early is that it was\n>>> needed for the subsequent [2] (pack-objects: parse --filter directly\n>>> into revs.filter, 2022-03-22), i.e. we might have a --filter\n>>> command-line option, and need to either have the \"struct rev_info\"\n>>> setup when we encounter that option, or later.\n>>>\n>>> Let's just change the control flow so that we'll instead set up the\n>>> \"struct rev_info\" only when we need it. Doing so leads to a bit more\n>>> verbosity, but it's a lot clearer what we're doing and why.\n>>\n>> This makes sense.\n>>\n>>> We could furthermore combine the two get_object_list() invocations\n>>> here by having repo_init_revisions() invoked on &pfd.revs, but I think\n>>> clearly separating the two makes the flow clearer. Likewise\n>>> redundantly but explicitly (i.e. redundant v.s. a \"{ 0 }\") \"0\" to\n>>> \"have_revs\" early in cmd_pack_objects().\n>>\n>> I disagree, especially when you later want to make sure we free\n>> the data from revs using your release_revisions().\n> \n> Because we'll need two release_revisions() instead of one?\n\nAnd it would be easy to miss one of the two if you are only testing\ncertain paths with leak-check on.\n\n>>> This does add the future constraint to opt_parse_list_objects_filter()\n>>> that we'll need to adjust this wrapper code if it looks at any other\n>>> value of the \"struct option\" than the \"value\" member.\n>>\n>> So we are coupling ourselves to the implementation of this method.\n> \n> Sure, but aren't all OPT_* where the macro is providing some custom\n> callback coupling themselves to how it's implemented?\n\nNo, I mean that the callback function you are making here is coupling\nitself to the existing callback function in a novel way.\n\nI tried to find another example of a nested callback, but I didn't\neven find another instance of creating a new 'struct option'.\n\n>>> But that regression should be relatively easy to spot. I'm\n>>> intentionally not initializing the \"struct wrap\" with e.g. \"{ 0 }\" so\n>>> that various memory sanity checkers would spot that, we just\n>>> initialize the \"value\" in po_filter_cb(). By doing this e.g. we'll die\n>>> on e.g. this test if we were to use another member of \"opt\" in\n>>> opt_parse_list_objects_filter()>\n>>\n>> So you are using uninitialized memory as a way to discover any\n>> necessary changes to that coupling. I'm not sure this is super-safe\n>> because we don't necessarily run memory checkers during CI builds.\n>>\n>> I'd rather have a consistently initialized chunk of data that would\n>> behave predictably (and hopefully we'd discover it is behaving\n>> incorrectly with that predictable behavior).\n> \n> I'd like to keep this as-is, i.e. if you zero it out it might also\n> behave unpredictably or even in undefined ways (e.g. a NULL ptr\n> dereference). Likewise in my version, but it has the benefit of being\n> caught by some tooling we have.\n\nI guess by \"predictable\" I mean \"well-defined\" or \"deterministic\".\n\nI don't want the build options to change how this works (for instance,\ndebug builds sometimes initialize data to zero).\n\n>> +struct rev_info_maybe_empty {\n>> +\tint has_revs;\n>> +\tstruct rev_info revs;\n>> +};\n\nThinking about this a second time, perhaps it would be best to add\nan \"unsigned initialized:1;\" to struct rev_info so we can look at\nsuch a struct and know whether or not repo_init_revisions() has\nbeen run or not. Avoids the custom struct and unifies a few things.\n\nIn particular, release_revisions() could choose to do nothing if\nrevs->initialized is false.\n\nFurther, a second repo_init_revisions() could do nothing if\nrevs->initialized is true. This allows us to safely \"re-init\"\nwithout our own \"if (has_revs)\" checks...\n>> \telse {\n>> \t\tif (!pfd.have_revs) {\n>> \t\t\trepo_init_revisions(the_repository, &pfd.revs, NULL);\n>> \t\t\tpfd.have_revs = 1;\n>> \t\t}\n>> \t\tget_object_list(&pfd.revs, rp.nr, rp.v);\n>> \t}\n\n...making this just\n\n\telse {\n\t\trepo_init_revisions(the_repository, &revs, NULL);\n\t\tget_object_list(&revs. rp.nr, rp.v);\n\t}\n\n> Conceptually I think the saving of that one line isn't worth it to the\n> reader.\n> \n> Then you'd need to read up and see exactly how pfd.revs gets mutated,\n> and its callback etc., only to see we're just doing this to save\n> ourselves a variable deceleration and a call to release_revisions().\n> \n>> and then later you can add\n>>\n>> \tif (pfd.have_revs)\n>> \t\trelease_revisions(&pfd.revs);\n\nAnd this would just be\n\n\trelease_revisions(&revs);\n\n>> to clear the memory in exactly one place.\n> \n> Yeah, it would work. I'd just prefer control flow that's trivial to\n> reason about over saving a couple of lines here.\n\nI think having multiple revision things that can live at different\nlevels (one embedded in a custom struct, one not) is not trivial to\nreason about. If we change the \"is this initialized\" indicator to be\nwithin 'struct rev_info', then this gets simpler.\n\nIt seems to me that it is easier to track a single struct and release\nthe memory in one place based on its lifespan.\n\nThanks,\n-Stolee\n"},{"id":"452305","messageId":"xmqqmthdampa.fsf@gitster.g","threadId":"57600","inReplyTo":"patch-1.1-193534b0f07-20220325T121715Z-avarab@gmail.com","subject":"Re: [PATCH] pack-objects: lazily set up \"struct rev_info\", don't leak","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-25T18:53:05Z","receivedAt":"2022-03-25T19:25:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> In the preceding [1] (pack-objects: move revs out of\n> get_object_list(), 2022-03-22) the \"repo_init_revisions()\" was moved\n> to cmd_pack_objects() so that it unconditionally took place for all\n> invocations of \"git pack-objects\".\n>\n> We'd thus start leaking memory, which is easily reproduced in\n> e.g. git.git by feeding e83c5163316 (Initial revision of \"git\", the\n> information manager from hell, 2005-04-07) to \"git pack-objects\";\n> ...\n> Narrowly fixing that commit would have been easy, just add call\n> repo_init_revisions() right before get_object_list(), which is\n> effectively what was done before that commit.\n>\n> But an unstated constraint when setting it up early is that it was\n> needed for the subsequent [2] (pack-objects: parse --filter directly\n> into revs.filter, 2022-03-22), i.e. we might have a --filter\n> command-line option, and need to either have the \"struct rev_info\"\n> setup when we encounter that option, or later.\n>\n> Let's just change the control flow so that we'll instead set up the\n> \"struct rev_info\" only when we need it. Doing so leads to a bit more\n> verbosity, but it's a lot clearer what we're doing and why.\n\nIs this about \"we take it as given that the use of rev_info leaks\nuntil we fix revisions API, so let's keep its use limited to avoid\nunnecessary leaks\"?\n\nIf so, it sort-of makes sense, but smells like a roundabout way to\naddress the issue. An obvious alternative is to wait until both the\ntopic and the \"plug revision API\" topic graduate and then add a\n\"release\" call to release the resource in the same sope as the\nunconditional call to init_revisions at the end. I do not quite get\nwhat on-demand lazy set-up buys us. What we need to lazily set-up,\nwhen we do lazily set-up, needs to be released either way, no?\n\n"},{"id":"452337","messageId":"d90bb9c8-3155-ca5f-8363-154876a7ad0a@github.com","threadId":"57600","inReplyTo":"220325.86mthdlx59.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] pack-objects: lazily set up \"struct rev_info\", don't leak","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-03-25T19:08:23Z","receivedAt":"2022-03-25T19:32:18Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 3/25/2022 1:34 PM, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Fri, Mar 25 2022, Derrick Stolee wrote:\n> \n>> On 3/25/2022 12:00 PM, Ævar Arnfjörð Bjarmason wrote:\n>>>> +struct rev_info_maybe_empty {\n>>>> +\tint has_revs;\n>>>> +\tstruct rev_info revs;\n>>>> +};\n>>\n>> Thinking about this a second time, perhaps it would be best to add\n>> an \"unsigned initialized:1;\" to struct rev_info so we can look at\n>> such a struct and know whether or not repo_init_revisions() has\n>> been run or not. Avoids the custom struct and unifies a few things.\n>>\n>> In particular, release_revisions() could choose to do nothing if\n>> revs->initialized is false.\n> \n> This plan won't work because that behavior is both undefined per the\n> standard, and something that's wildly undefined in practice.\n> \n> I.e. we initialize it on the stack, so it'll point to uninitialized\n> memory, sometimes that bit will be 0, sometimes 1...\n> \n> If you mean just initialize it to { 0 } or whatever that would work,\n> yes, but if we're going to refactor all the callers to do that we might\n> as well refactor the few missing bits that would be needed to initialize\n> it statically, and drop the dynamic by default initialization...\n\nYes, I was assuming that we initialize all structs to all-zero,\nbut the existing failure to do this will cause such a change too\nlarge for this issue.\n\n> But FWIW I think a much more obvious thing to do overall would be to\n> skip the whole \"filter bust me in rev_info\" refactoring part of your\n> series and just add a trivial list_objects_filter_copy_attach() method,\n> or do it inline with memcpy/memset.\n> \n> I.e. to not touch the \"filter\" etc. callback stuff at all, still pass it\n> to get_object_list(). Can't 2/5 and 3/5 in your series be replaced by\n> this simpler and smaller change?:\n\n> \t-\tlist_objects_filter_copy(&revs.filter, &filter_options);\n> \t+\t/* attach our CLI --filter to rev_info's filter */\n> \t+\tmemcpy(&revs.filter, filter, sizeof(*filter));\n> \t+\tmemset(filter, 0, sizeof(*filter));\n\nHere, you are removing a deep copy with a shallow copy. After this,\nfreeing the arrays within revs.filter would cause a double-free when\nfreeing the arrays in the original filter_options.\n\nIf you went this way, then you could do a s/&filter_options/filter/\nin the existing line.\n\n> \t \t/* make sure shallows are read */\n> \t \tis_repository_shallow(the_repository);\n> \t@@ -3872,6 +3873,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 list_objects_filter_options filter_options = { 0 };\n> \t \tstruct option pack_objects_options[] = {\n> \t \t\tOPT_SET_INT('q', \"quiet\", &progress,\n> \t \t\t\t N_(\"do not show progress meter\"), 0),\n> \t@@ -4154,7 +4156,7 @@ 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 {\n> \t-\t\tget_object_list(rp.nr, rp.v);\n> \t+\t\tget_object_list(rp.nr, rp.v, &filter_options);\n> \t \t}\n> \t \tcleanup_preferred_base();\n> \t \tif (include_tag && nr_result)\n> \n> And even most of that could be omitted by not removing the global\n> \"static struct\" since pack-objects is a one-off anyway ... :)\n\nEven if you fix the deep/shallow copy above, you still need to\nclean up the filter in two places.\n\nThanks,\n-Stolee\n"},{"id":"452361","messageId":"220325.86mthdlx59.gmgdl@evledraar.gmail.com","threadId":"57600","inReplyTo":"8d368240-dae5-7a66-6c0c-9e0a960ca34c@github.com","subject":"Re: [PATCH] pack-objects: lazily set up \"struct rev_info\", don't leak","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-03-25T17:34:11Z","receivedAt":"2022-03-25T19:38:59Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Mar 25 2022, Derrick Stolee wrote:\n\n> On 3/25/2022 12:00 PM, Ævar Arnfjörð Bjarmason wrote:\n>> \n>> On Fri, Mar 25 2022, Derrick Stolee wrote:\n>> [...]\n>>>> We could furthermore combine the two get_object_list() invocations\n>>>> here by having repo_init_revisions() invoked on &pfd.revs, but I think\n>>>> clearly separating the two makes the flow clearer. Likewise\n>>>> redundantly but explicitly (i.e. redundant v.s. a \"{ 0 }\") \"0\" to\n>>>> \"have_revs\" early in cmd_pack_objects().\n>>>\n>>> I disagree, especially when you later want to make sure we free\n>>> the data from revs using your release_revisions().\n>> \n>> Because we'll need two release_revisions() instead of one?\n>\n> And it would be easy to miss one of the two if you are only testing\n> certain paths with leak-check on.\n>\n>>>> This does add the future constraint to opt_parse_list_objects_filter()\n>>>> that we'll need to adjust this wrapper code if it looks at any other\n>>>> value of the \"struct option\" than the \"value\" member.\n>>>\n>>> So we are coupling ourselves to the implementation of this method.\n>> \n>> Sure, but aren't all OPT_* where the macro is providing some custom\n>> callback coupling themselves to how it's implemented?\n>\n> No, I mean that the callback function you are making here is coupling\n> itself to the existing callback function in a novel way.\n\nYeah, I did start by just copy/pasting its contents, or I could have\nboth of them call a semi-\"private\" wrapper, would you prefer one of\nthose instead?\n\n> I tried to find another example of a nested callback, but I didn't\n> even find another instance of creating a new 'struct option'.\n\nI haven't seen a nested one, FWIW OPT_ALIAS() creates a new \"struct\noption\", but in a completely unrelated way, what I'm doing here is\nprobably novel within the codebase...\n\n>>>> But that regression should be relatively easy to spot. I'm\n>>>> intentionally not initializing the \"struct wrap\" with e.g. \"{ 0 }\" so\n>>>> that various memory sanity checkers would spot that, we just\n>>>> initialize the \"value\" in po_filter_cb(). By doing this e.g. we'll die\n>>>> on e.g. this test if we were to use another member of \"opt\" in\n>>>> opt_parse_list_objects_filter()>\n>>>\n>>> So you are using uninitialized memory as a way to discover any\n>>> necessary changes to that coupling. I'm not sure this is super-safe\n>>> because we don't necessarily run memory checkers during CI builds.\n>>>\n>>> I'd rather have a consistently initialized chunk of data that would\n>>> behave predictably (and hopefully we'd discover it is behaving\n>>> incorrectly with that predictable behavior).\n>> \n>> I'd like to keep this as-is, i.e. if you zero it out it might also\n>> behave unpredictably or even in undefined ways (e.g. a NULL ptr\n>> dereference). Likewise in my version, but it has the benefit of being\n>> caught by some tooling we have.\n>\n> I guess by \"predictable\" I mean \"well-defined\" or \"deterministic\".\n>\n> I don't want the build options to change how this works (for instance,\n> debug builds sometimes initialize data to zero).\n\nYeah I agree that does suck.\n\nHonestly I don't care that much in this case, I just wanted to resolve\nthe topic conflict.\n\nIn the general case though I think it's much better to have a flaky\nfailure caught on some platforms, via valgrind or whatever because we\nused uninitialized memory than to have a silent failure (e.g. because\nit's always an int=0, but we expected something else...)\n\n>>> +struct rev_info_maybe_empty {\n>>> +\tint has_revs;\n>>> +\tstruct rev_info revs;\n>>> +};\n>\n> Thinking about this a second time, perhaps it would be best to add\n> an \"unsigned initialized:1;\" to struct rev_info so we can look at\n> such a struct and know whether or not repo_init_revisions() has\n> been run or not. Avoids the custom struct and unifies a few things.\n>\n> In particular, release_revisions() could choose to do nothing if\n> revs->initialized is false.\n\nThis plan won't work because that behavior is both undefined per the\nstandard, and something that's wildly undefined in practice.\n\nI.e. we initialize it on the stack, so it'll point to uninitialized\nmemory, sometimes that bit will be 0, sometimes 1...\n\nIf you mean just initialize it to { 0 } or whatever that would work,\nyes, but if we're going to refactor all the callers to do that we might\nas well refactor the few missing bits that would be needed to initialize\nit statically, and drop the dynamic by default initialization...\n\n> Further, a second repo_init_revisions() could do nothing if\n> revs->initialized is true. This allows us to safely \"re-init\"\n> without our own \"if (has_revs)\" checks...\n\nYeah if you had a previous repo_init_revisions() you could rely on that.\n\n>>> \telse {\n>>> \t\tif (!pfd.have_revs) {\n>>> \t\t\trepo_init_revisions(the_repository, &pfd.revs, NULL);\n>>> \t\t\tpfd.have_revs = 1;\n>>> \t\t}\n>>> \t\tget_object_list(&pfd.revs, rp.nr, rp.v);\n>>> \t}\n>\n> ...making this just\n>\n> \telse {\n> \t\trepo_init_revisions(the_repository, &revs, NULL);\n> \t\tget_object_list(&revs. rp.nr, rp.v);\n> \t}\n>\n>> Conceptually I think the saving of that one line isn't worth it to the\n>> reader.\n>> \n>> Then you'd need to read up and see exactly how pfd.revs gets mutated,\n>> and its callback etc., only to see we're just doing this to save\n>> ourselves a variable deceleration and a call to release_revisions().\n>> \n>>> and then later you can add\n>>>\n>>> \tif (pfd.have_revs)\n>>> \t\trelease_revisions(&pfd.revs);\n>\n> And this would just be\n>\n> \trelease_revisions(&revs);\n>\n>>> to clear the memory in exactly one place.\n>> \n>> Yeah, it would work. I'd just prefer control flow that's trivial to\n>> reason about over saving a couple of lines here.\n>\n> I think having multiple revision things that can live at different\n> levels (one embedded in a custom struct, one not) is not trivial to\n> reason about. If we change the \"is this initialized\" indicator to be\n> within 'struct rev_info', then this gets simpler.\n\nI meant: if you want to read that code through without considering the\n\"filter\" case it's obvious that you can skip the whole \"pfd.revs\" part\nin that case, and that it's only something to do with \"filter\".\n\n> It seems to me that it is easier to track a single struct and release\n> the memory in one place based on its lifespan.\n\nI submitted a patch on top because it looked like your topic was going\nto be merged to \"next\" soon, and I really didn't care much in this case.\n\nBut FWIW I think a much more obvious thing to do overall would be to\nskip the whole \"filter bust me in rev_info\" refactoring part of your\nseries and just add a trivial list_objects_filter_copy_attach() method,\nor do it inline with memcpy/memset.\n\nI.e. to not touch the \"filter\" etc. callback stuff at all, still pass it\nto get_object_list(). Can't 2/5 and 3/5 in your series be replaced by\nthis simpler and smaller change?:\n\n\tdiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n\tindex 829ca359cf9..4ce76a378c8 100644\n\t--- a/builtin/pack-objects.c\n\t+++ b/builtin/pack-objects.c\n\t@@ -237,8 +237,6 @@ static unsigned long cache_max_small_delta_size = 1000;\n\t \n\t static unsigned long window_memory_limit = 0;\n\t \n\t-static struct list_objects_filter_options filter_options;\n\t-\n\t static struct string_list uri_protocols = STRING_LIST_INIT_NODUP;\n\t \n\t enum missing_action {\n\t@@ -3714,7 +3712,8 @@ static void mark_bitmap_preferred_tips(void)\n\t \t}\n\t }\n\t \n\t-static void get_object_list(int ac, const char **av)\n\t+static void get_object_list(int ac, const char **av,\n\t+\t\t\t struct list_objects_filter_options *filter)\n\t {\n\t \tstruct rev_info revs;\n\t \tstruct setup_revision_opt s_r_opt = {\n\t@@ -3727,7 +3726,9 @@ static void get_object_list(int ac, const char **av)\n\t \trepo_init_revisions(the_repository, &revs, NULL);\n\t \tsave_commit_buffer = 0;\n\t \tsetup_revisions(ac, av, &revs, &s_r_opt);\n\t-\tlist_objects_filter_copy(&revs.filter, &filter_options);\n\t+\t/* attach our CLI --filter to rev_info's filter */\n\t+\tmemcpy(&revs.filter, filter, sizeof(*filter));\n\t+\tmemset(filter, 0, sizeof(*filter));\n\t \n\t \t/* make sure shallows are read */\n\t \tis_repository_shallow(the_repository);\n\t@@ -3872,6 +3873,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 list_objects_filter_options filter_options = { 0 };\n\t \tstruct option pack_objects_options[] = {\n\t \t\tOPT_SET_INT('q', \"quiet\", &progress,\n\t \t\t\t N_(\"do not show progress meter\"), 0),\n\t@@ -4154,7 +4156,7 @@ 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 {\n\t-\t\tget_object_list(rp.nr, rp.v);\n\t+\t\tget_object_list(rp.nr, rp.v, &filter_options);\n\t \t}\n\t \tcleanup_preferred_base();\n\t \tif (include_tag && nr_result)\n\nAnd even most of that could be omitted by not removing the global\n\"static struct\" since pack-objects is a one-off anyway ... :)\n"},{"id":"452399","messageId":"220326.865yo1leay.gmgdl@evledraar.gmail.com","threadId":"57600","inReplyTo":"d90bb9c8-3155-ca5f-8363-154876a7ad0a@github.com","subject":"Re: [PATCH] pack-objects: lazily set up \"struct rev_info\", don't leak","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-03-26T00:52:42Z","receivedAt":"2022-03-26T00:59:15Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Mar 25 2022, Derrick Stolee wrote:\n\n> On 3/25/2022 1:34 PM, Ævar Arnfjörð Bjarmason wrote:\n>> \n>> On Fri, Mar 25 2022, Derrick Stolee wrote:\n>> \n>>> On 3/25/2022 12:00 PM, Ævar Arnfjörð Bjarmason wrote:\n>>>>> +struct rev_info_maybe_empty {\n>>>>> +\tint has_revs;\n>>>>> +\tstruct rev_info revs;\n>>>>> +};\n>>>\n>>> Thinking about this a second time, perhaps it would be best to add\n>>> an \"unsigned initialized:1;\" to struct rev_info so we can look at\n>>> such a struct and know whether or not repo_init_revisions() has\n>>> been run or not. Avoids the custom struct and unifies a few things.\n>>>\n>>> In particular, release_revisions() could choose to do nothing if\n>>> revs->initialized is false.\n>> \n>> This plan won't work because that behavior is both undefined per the\n>> standard, and something that's wildly undefined in practice.\n>> \n>> I.e. we initialize it on the stack, so it'll point to uninitialized\n>> memory, sometimes that bit will be 0, sometimes 1...\n>> \n>> If you mean just initialize it to { 0 } or whatever that would work,\n>> yes, but if we're going to refactor all the callers to do that we might\n>> as well refactor the few missing bits that would be needed to initialize\n>> it statically, and drop the dynamic by default initialization...\n>\n> Yes, I was assuming that we initialize all structs to all-zero,\n> but the existing failure to do this will cause such a change too\n> large for this issue.\n\nI don't see how that wouldn't be a regression on the upthread patch in\nthe sense that yes, we could of course initialize it, but the whole\npoint of not doing so was to have our tooling detect if the downstream\ncode assumed it could start using a struct member we hadn't filled in.\n\nBy initializing it we'll never know.\n\nBut yes, if you consider that a non-goal then init to \"{ 0 }\" makes the\nmost sense.\n\n>> But FWIW I think a much more obvious thing to do overall would be to\n>> skip the whole \"filter bust me in rev_info\" refactoring part of your\n>> series and just add a trivial list_objects_filter_copy_attach() method,\n>> or do it inline with memcpy/memset.\n>> \n>> I.e. to not touch the \"filter\" etc. callback stuff at all, still pass it\n>> to get_object_list(). Can't 2/5 and 3/5 in your series be replaced by\n>> this simpler and smaller change?:\n>\n>> \t-\tlist_objects_filter_copy(&revs.filter, &filter_options);\n>> \t+\t/* attach our CLI --filter to rev_info's filter */\n>> \t+\tmemcpy(&revs.filter, filter, sizeof(*filter));\n>> \t+\tmemset(filter, 0, sizeof(*filter));\n>\n> Here, you are removing a deep copy with a shallow copy. After this,\n> freeing the arrays within revs.filter would cause a double-free when\n> freeing the arrays in the original filter_options.\n\nYes, and that's what we want, right? I.e. we don't want a copy, but to\nuse the &filter for parse_options(), then once that's populated we\nshallow-copy that to \"struct rev_info\"'s \"filter\", and forget about our\nown copy (i.e. the memset there is redundant, but just a \"let's not use\nthis again) marker.\n\nOf course this will leak now, but once merged with my\nrelease_revisions() patch will work, and we'll free what we allocated\n(once!).\n\n> If you went this way, then you could do a s/&filter_options/filter/\n> in the existing line.\n>\n>> \t \t/* make sure shallows are read */\n>> \t \tis_repository_shallow(the_repository);\n>> \t@@ -3872,6 +3873,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 list_objects_filter_options filter_options = { 0 };\n>> \t \tstruct option pack_objects_options[] = {\n>> \t \t\tOPT_SET_INT('q', \"quiet\", &progress,\n>> \t \t\t\t N_(\"do not show progress meter\"), 0),\n>> \t@@ -4154,7 +4156,7 @@ 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 {\n>> \t-\t\tget_object_list(rp.nr, rp.v);\n>> \t+\t\tget_object_list(rp.nr, rp.v, &filter_options);\n>> \t \t}\n>> \t \tcleanup_preferred_base();\n>> \t \tif (include_tag && nr_result)\n>> \n>> And even most of that could be omitted by not removing the global\n>> \"static struct\" since pack-objects is a one-off anyway ... :)\n>\n> Even if you fix the deep/shallow copy above, you still need to\n> clean up the filter in two places.\n\nIf you \"fix\" the shallow copying you need to free it twice, but if you\ndon't you free it once.\n\nI.e. this is conceptually the same as strbuf_detach() + strbuf_attach().\n\nBut maybe I'm missing something...\n\n(If I am it's rather worrying that it passed all our tests, both in your\nseries + merged with the release_revisions() series).\n"},{"id":"452401","messageId":"220326.86wnghjz2s.gmgdl@evledraar.gmail.com","threadId":"57600","inReplyTo":"xmqqmthdampa.fsf@gitster.g","subject":"Re: [PATCH] pack-objects: lazily set up \"struct rev_info\", don't leak","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-03-26T01:09:08Z","receivedAt":"2022-03-26T01:13:24Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Mar 25 2022, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> In the preceding [1] (pack-objects: move revs out of\n>> get_object_list(), 2022-03-22) the \"repo_init_revisions()\" was moved\n>> to cmd_pack_objects() so that it unconditionally took place for all\n>> invocations of \"git pack-objects\".\n>>\n>> We'd thus start leaking memory, which is easily reproduced in\n>> e.g. git.git by feeding e83c5163316 (Initial revision of \"git\", the\n>> information manager from hell, 2005-04-07) to \"git pack-objects\";\n>> ...\n>> Narrowly fixing that commit would have been easy, just add call\n>> repo_init_revisions() right before get_object_list(), which is\n>> effectively what was done before that commit.\n>>\n>> But an unstated constraint when setting it up early is that it was\n>> needed for the subsequent [2] (pack-objects: parse --filter directly\n>> into revs.filter, 2022-03-22), i.e. we might have a --filter\n>> command-line option, and need to either have the \"struct rev_info\"\n>> setup when we encounter that option, or later.\n>>\n>> Let's just change the control flow so that we'll instead set up the\n>> \"struct rev_info\" only when we need it. Doing so leads to a bit more\n>> verbosity, but it's a lot clearer what we're doing and why.\n>\n> Is this about \"we take it as given that the use of rev_info leaks\n> until we fix revisions API, so let's keep its use limited to avoid\n> unnecessary leaks\"?\n\nNot exactly,. When you use the revisions API to do \"filter\" stuff in\nthis codepath it leaks both before & after Derrick's patches, so nothing\nhas changed in that case, but...\n\n> If so, it sort-of makes sense, but smells like a roundabout way to\n> address the issue. An obvious alternative is to wait until both the\n> topic and the \"plug revision API\" topic graduate and then add a\n> \"release\" call to release the resource in the same sope as the\n> unconditional call to init_revisions at the end. I do not quite get\n> what on-demand lazy set-up buys us. What we need to lazily set-up,\n> when we do lazily set-up, needs to be released either way, no?\n\n...We were doing lazy setup of \"struct rev_info\" before the parent\nseries, and as a result it introduces a new memory leak. We do a\nmalloc() for some diff.c code that revisions.c uses unconditionally,\nwhich then don't use at all in some common cases.\n\nThe patch I've submitted on top just restores the previous state of the\ninitialization being lazy, but in a way that has to be adapted for other\ncode changes the series made.\n"},{"id":"452476","messageId":"6511a6b4-79da-ffe8-8000-982018a7e701@github.com","threadId":"57600","inReplyTo":"220326.865yo1leay.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] pack-objects: lazily set up \"struct rev_info\", don't leak","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-03-28T14:04:23Z","receivedAt":"2022-03-28T14:04:30Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 3/25/2022 8:52 PM, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Fri, Mar 25 2022, Derrick Stolee wrote:\n> \n>> On 3/25/2022 1:34 PM, Ævar Arnfjörð Bjarmason wrote:>>> \t-\tlist_objects_filter_copy(&revs.filter, &filter_options);\n>>> \t+\t/* attach our CLI --filter to rev_info's filter */\n>>> \t+\tmemcpy(&revs.filter, filter, sizeof(*filter));\n>>> \t+\tmemset(filter, 0, sizeof(*filter));\n>>\n>> Here, you are removing a deep copy with a shallow copy. After this,\n>> freeing the arrays within revs.filter would cause a double-free when\n>> freeing the arrays in the original filter_options.\n> \n> Yes, and that's what we want, right? I.e. we don't want a copy, but to\n> use the &filter for parse_options(), then once that's populated we\n> shallow-copy that to \"struct rev_info\"'s \"filter\", and forget about our\n> own copy (i.e. the memset there is redundant, but just a \"let's not use\n> this again) marker.\n> \n> Of course this will leak now, but once merged with my\n> release_revisions() patch will work, and we'll free what we allocated\n> (once!).\n\n\n>> Even if you fix the deep/shallow copy above, you still need to\n>> clean up the filter in two places.\n> \n> If you \"fix\" the shallow copying you need to free it twice, but if you\n> don't you free it once.\n> \n> I.e. this is conceptually the same as strbuf_detach() + strbuf_attach().\n> \n> But maybe I'm missing something...\n> \n> (If I am it's rather worrying that it passed all our tests, both in your\n> series + merged with the release_revisions() series).\n\nMy problem is that you need to know that the filter data was\n\"detached\" in a different scope than it is defined.\n\n * filter_options is defined in cmd_pack_objects()\n * The detach and reattach to revs is in get_object_list()\n\nYour model requires internal information from get_object_list()\nto know that you shouldn't release filter_options within\ncmd_pack_objects(), which I think is a code smell. Better to\nhave something allocated in cmd_pack_objects() be freed in that\nsame method so it is visually detectable that we are freeing\ncorrectly.\n\nThanks,\n-Stolee\n\n"},{"id":"452479","messageId":"patch-v2-1.1-9951d92176e-20220328T154049Z-avarab@gmail.com","threadId":"57600","inReplyTo":"patch-1.1-193534b0f07-20220325T121715Z-avarab@gmail.com","subject":"[PATCH v2] pack-objects: lazily set up \"struct rev_info\", don't leak","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-03-28T15:43:18Z","receivedAt":"2022-03-28T15:43:28Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"In the preceding [1] (pack-objects: move revs out of\nget_object_list(), 2022-03-22) the \"repo_init_revisions()\" was moved\nto cmd_pack_objects() so that it unconditionally took place for all\ninvocations of \"git pack-objects\".\n\nWe'd thus start leaking memory, which is easily reproduced in\ne.g. git.git by feeding e83c5163316 (Initial revision of \"git\", the\ninformation manager from hell, 2005-04-07) to \"git pack-objects\";\n\n $ echo e83c5163316f89bfbde7d9ab23ca2e25604af290 | ./git pack-objects initial\n [...]\n\t==19130==ERROR: LeakSanitizer: detected memory leaks\n\n\tDirect leak of 7120 byte(s) in 1 object(s) allocated from:\n\t #0 0x455308 in __interceptor_malloc (/home/avar/g/git/git+0x455308)\n\t #1 0x75b399 in do_xmalloc /home/avar/g/git/wrapper.c:41:8\n\t #2 0x75b356 in xmalloc /home/avar/g/git/wrapper.c:62:9\n\t #3 0x5d7609 in prep_parse_options /home/avar/g/git/diff.c:5647:2\n\t #4 0x5d415a in repo_diff_setup /home/avar/g/git/diff.c:4621:2\n\t #5 0x6dffbb in repo_init_revisions /home/avar/g/git/revision.c:1853:2\n\t #6 0x4f599d in cmd_pack_objects /home/avar/g/git/builtin/pack-objects.c:3980:2\n\t #7 0x4592ca in run_builtin /home/avar/g/git/git.c:465:11\n\t #8 0x457d81 in handle_builtin /home/avar/g/git/git.c:718:3\n\t #9 0x458ca5 in run_argv /home/avar/g/git/git.c:785:4\n\t #10 0x457b40 in cmd_main /home/avar/g/git/git.c:916:19\n\t #11 0x562259 in main /home/avar/g/git/common-main.c:56:11\n\t #12 0x7fce792ac7ec in __libc_start_main csu/../csu/libc-start.c:332:16\n\t #13 0x4300f9 in _start (/home/avar/g/git/git+0x4300f9)\n\n\tSUMMARY: LeakSanitizer: 7120 byte(s) leaked in 1 allocation(s).\n\tAborted\n\nNarrowly fixing that commit would have been easy, just add call\nrepo_init_revisions() right before get_object_list(), which is\neffectively what was done before that commit.\n\nBut an unstated constraint when setting it up early is that it was\nneeded for the subsequent [2] (pack-objects: parse --filter directly\ninto revs.filter, 2022-03-22), i.e. we might have a --filter\ncommand-line option, and need to either have the \"struct rev_info\"\nsetup when we encounter that option, or later.\n\nLet's just change the control flow so that we'll instead set up the\n\"struct rev_info\" only when we need it. Doing so leads to a bit more\nverbosity, but it's a lot clearer what we're doing and why.\n\nAn earlier version of this commit[3] went behind\nopt_parse_list_objects_filter()'s back by faking up a \"struct option\"\nbefore calling it. Let's avoid that and instead create a blessed API\nfor this pattern.\n\nWe could furthermore combine the two get_object_list() invocations\nhere by having repo_init_revisions() invoked on &pfd.revs, but I think\nclearly separating the two makes the flow clearer. Likewise\nredundantly but explicitly (i.e. redundant v.s. a \"{ 0 }\") \"0\" to\n\"have_revs\" early in cmd_pack_objects().\n\nWhile we're at it add parentheses around the arguments to the OPT_*\nmacros in in list-objects-filter-options.h, as we need to change those\nlines anyway. It doesn't matter in this case, but is good general\npractice.\n\n1. https://lore.kernel.org/git/619b757d98465dbc4995bdc11a5282fbfcbd3daa.1647970119.git.gitgitgadget@gmail.com\n2. https://lore.kernel.org/git/97de926904988b89b5663bd4c59c011a1723a8f5.1647970119.git.gitgitgadget@gmail.com\n3. https://lore.kernel.org/git/patch-1.1-193534b0f07-20220325T121715Z-avarab@gmail.com/\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n\nHopefully this v2 addresses enough of the concern on v1 to move\nforward with squashing the linux-leaks failure in \"seen\" currently.\n\nThere's still \"struct rev_info\" in play here, which I still think\nmakes sense, but this gets entirely rid of the evil magic trickery\nwith faking up a \"struct option\" in favor of a defined API for doing\nthe lazy initialization.\n\nRange-diff against v1:\n1: 193534b0f07 ! 1: 9951d92176e pack-objects: lazily set up \"struct rev_info\", don't leak\n @@ Commit message\n \"struct rev_info\" only when we need it. Doing so leads to a bit more\n verbosity, but it's a lot clearer what we're doing and why.\n \n + An earlier version of this commit[3] went behind\n + opt_parse_list_objects_filter()'s back by faking up a \"struct option\"\n + before calling it. Let's avoid that and instead create a blessed API\n + for this pattern.\n +\n We could furthermore combine the two get_object_list() invocations\n here by having repo_init_revisions() invoked on &pfd.revs, but I think\n clearly separating the two makes the flow clearer. Likewise\n redundantly but explicitly (i.e. redundant v.s. a \"{ 0 }\") \"0\" to\n \"have_revs\" early in cmd_pack_objects().\n \n - This does add the future constraint to opt_parse_list_objects_filter()\n - that we'll need to adjust this wrapper code if it looks at any other\n - value of the \"struct option\" than the \"value\" member.\n -\n - But that regression should be relatively easy to spot. I'm\n - intentionally not initializing the \"struct wrap\" with e.g. \"{ 0 }\" so\n - that various memory sanity checkers would spot that, we just\n - initialize the \"value\" in po_filter_cb(). By doing this e.g. we'll die\n - on e.g. this test if we were to use another member of \"opt\" in\n - opt_parse_list_objects_filter()>\n -\n - ./t5317-pack-objects-filter-objects.sh -vixd --valgrind-only=3\n -\n While we're at it add parentheses around the arguments to the OPT_*\n macros in in list-objects-filter-options.h, as we need to change those\n lines anyway. It doesn't matter in this case, but is good general\n practice.\n \n 1. https://lore.kernel.org/git/619b757d98465dbc4995bdc11a5282fbfcbd3daa.1647970119.git.gitgitgadget@gmail.com\n - 2. https://lore.kernel.org/git/97de926904988b89b5663bd4c59c011a1723a8f5.1647970119.git.gitgitgadget@gmail.com/\n + 2. https://lore.kernel.org/git/97de926904988b89b5663bd4c59c011a1723a8f5.1647970119.git.gitgitgadget@gmail.com\n + 3. https://lore.kernel.org/git/patch-1.1-193534b0f07-20220325T121715Z-avarab@gmail.com/\n \n Signed-off-by: Ævar Arnfjörð Bjarmason \n \n @@ builtin/pack-objects.c: static int option_parse_unpack_unreachable(const struct\n +\tstruct rev_info revs;\n +};\n +\n -+static int po_filter_cb(const struct option *opt, const char *arg, int unset)\n ++static struct list_objects_filter_options *po_filter_revs_init(void *value)\n +{\n -+\tstruct po_filter_data *data = opt->value;\n -+\tstruct option wrap; /* don't initialize! */\n ++\tstruct po_filter_data *data = value;\n +\n +\trepo_init_revisions(the_repository, &data->revs, NULL);\n -+\twrap.value = &data->revs.filter;\n +\tdata->have_revs = 1;\n +\n -+\treturn opt_parse_list_objects_filter(&wrap, arg, unset);\n ++\treturn &data->revs.filter;\n +}\n +\n int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n @@ builtin/pack-objects.c: int cmd_pack_objects(int argc, const char **argv, const\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_PARSE_LIST_OBJECTS_FILTER_CB(&pfd, po_filter_cb),\n ++\t\tOPT_PARSE_LIST_OBJECTS_FILTER_INIT(&pfd, po_filter_revs_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 @@ builtin/pack-objects.c: int cmd_pack_objects(int argc, const char **argv, const\n \t}\n \tcleanup_preferred_base();\n \n + ## list-objects-filter-options.c ##\n +@@ list-objects-filter-options.c: 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);\n +\n ## list-objects-filter-options.h ##\n @@ list-objects-filter-options.h: 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_CB(fo, cb) \\\n -+\tOPT_CALLBACK(0, \"filter\", (fo), N_(\"args\"), \\\n -+\t\t N_(\"object filtering\"), (cb))\n -+\n #define OPT_PARSE_LIST_OBJECTS_FILTER(fo) \\\n -\tOPT_CALLBACK(0, \"filter\", fo, N_(\"args\"), \\\n -\t N_(\"object filtering\"), \\\n -\t opt_parse_list_objects_filter)\n -+\tOPT_PARSE_LIST_OBJECTS_FILTER_CB((fo), opt_parse_list_objects_filter)\n ++\tOPT_PARSE_LIST_OBJECTS_FILTER_INIT((fo), NULL)\n \n /*\n * Translates abbreviated numbers in the filter's filter_spec into their\n\n builtin/pack-objects.c | 28 +++++++++++++++++++++++-----\n list-objects-filter-options.c | 4 ++++\n list-objects-filter-options.h | 24 +++++++++++++++++++++---\n 3 files changed, 48 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex d39f668ad56..09680fb6a8b 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -3857,6 +3857,21 @@ static int option_parse_unpack_unreachable(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@@ -3867,7 +3882,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@@ -3954,7 +3969,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(&revs.filter),\n+\t\tOPT_PARSE_LIST_OBJECTS_FILTER_INIT(&pfd, po_filter_revs_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@@ -3973,8 +3988,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@@ -4076,7 +4089,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@@ -4152,7 +4165,12 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \t\t\tadd_unreachable_loose_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} 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}\n \tcleanup_preferred_base();\ndiff --git a/list-objects-filter-options.c b/list-objects-filter-options.c\nindex f02d8df1422..4b25287886d 100644\n--- a/list-objects-filter-options.c\n+++ b/list-objects-filter-options.c\n@@ -285,6 +285,10 @@ 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 90e4bc96252..ffc02d77e76 100644\n--- a/list-objects-filter-options.h\n+++ b/list-objects-filter-options.h\n@@ -104,13 +104,31 @@ 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_CALLBACK(0, \"filter\", fo, N_(\"args\"), \\\n-\t N_(\"object filtering\"), \\\n-\t opt_parse_list_objects_filter)\n+\tOPT_PARSE_LIST_OBJECTS_FILTER_INIT((fo), NULL)\n \n /*\n * Translates abbreviated numbers in the filter's filter_spec into their\n-- \n2.35.1.1541.g9c2d54e20ab\n\n"},{"id":"452480","messageId":"4781bae6-4ac6-1217-b018-aef286cdca7c@github.com","threadId":"57600","inReplyTo":"patch-v2-1.1-9951d92176e-20220328T154049Z-avarab@gmail.com","subject":"Re: [PATCH v2] pack-objects: lazily set up \"struct rev_info\", don't leak","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-03-28T15:58:16Z","receivedAt":"2022-03-28T15:58:25Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 3/28/2022 11:43 AM, Ævar Arnfjörð Bjarmason wrote:\n> An earlier version of this commit[3] went behind\n> opt_parse_list_objects_filter()'s back by faking up a \"struct option\"\n> before calling it. Let's avoid that and instead create a blessed API\n> for this pattern.\n\n...\n\n> diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c\n> index f02d8df1422..4b25287886d 100644\n> --- a/list-objects-filter-options.c\n> +++ b/list-objects-filter-options.c\n> @@ -285,6 +285,10 @@ 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);\n> diff --git a/list-objects-filter-options.h b/list-objects-filter-options.h\n> index 90e4bc96252..ffc02d77e76 100644\n> --- a/list-objects-filter-options.h\n> +++ b/list-objects-filter-options.h\n> @@ -104,13 +104,31 @@ 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_CALLBACK(0, \"filter\", fo, N_(\"args\"), \\\n> -\t N_(\"object filtering\"), \\\n> -\t opt_parse_list_objects_filter)\n> +\tOPT_PARSE_LIST_OBJECTS_FILTER_INIT((fo), NULL)\n\nI like this way of combining the macros into one implementation\n(guarded with a NULL check).\n\nThis version looks good to me.\n\nThanks,\n-Stolee\n"},{"id":"452485","messageId":"xmqqmtha2eb9.fsf@gitster.g","threadId":"57600","inReplyTo":"patch-v2-1.1-9951d92176e-20220328T154049Z-avarab@gmail.com","subject":"Re: [PATCH v2] pack-objects: lazily set up \"struct rev_info\", don't leak","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-28T17:10:34Z","receivedAt":"2022-03-28T17:10:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> diff --git a/list-objects-filter-options.h b/list-objects-filter-options.h\n> index 90e4bc96252..ffc02d77e76 100644\n> --- a/list-objects-filter-options.h\n> +++ b/list-objects-filter-options.h\n> @@ -104,13 +104,31 @@ void parse_list_objects_filter(\n> ...\n> +typedef struct list_objects_filter_options *(*opt_lof_init)(void *);\n\nDo we want or need to expose this type to anybody outside the\nlist_objects_filter_options.c file? After all, the client has to\ndefine a function of that type like so:\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\nand not\n\n\tstatic opt_lof_init po_filter_revs_init\n\t{\n\t\tbody of the function\n\t}\n\nand the use of it, i.e. \n\n> @@ -3954,7 +3969,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(&revs.filter),\n> +\t\tOPT_PARSE_LIST_OBJECTS_FILTER_INIT(&pfd, po_filter_revs_init),\n\ncasts the type away to (intptr_t) so having a handy way to spell the\ntype out does not contribute to type safety, either.\n\nOther than that, looks good to me.\n\nThanks, will queue directly on top of Derrick's partial-bundle\nclean-up series.\n\n"}]}