{"thread":{"id":"56640","subject":"[PATCH 00/10] unpack-trees & dir APIs: fix memory leaks","startedAt":"2021-10-04T00:46:39Z","lastAt":"2021-10-04T16:29:19Z","messageCount":31,"participants":["Ævar Arnfjörð Bjarmason","Phillip Wood","Elijah Newren"],"isPatch":true,"patchVersion":1,"patchTotal":10},"messages":[{"id":"437817","messageId":"cover-00.10-00000000000-20211004T002226Z-avarab@gmail.com","threadId":"56640","inReplyTo":null,"subject":"[PATCH 00/10] unpack-trees & dir APIs: fix memory leaks","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-04T00:46:01Z","receivedAt":"2021-10-04T00:46:39Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"This series fixes memory leaks in the unpack-trees and dir APIs for\nall their callers. There's been a discussion between myself & Elijah\non his en/removing-untracked-fixes series[1] about the memory leak\nfixing aspect of his series.\n\nI've got locally queued patches that fix widespread memory leaks in\nthe test suite and make much of it pass under SANITIZE=leak, once the\ncommon leaks in revisions.c (git rev-list/show/log etc.), \"checkout\",\n\"dir\" and \"unpack-trees\" are fixed a lot of tests become entirely\nleak-free, as much code that needs to setup various basic things will\nrequire one of those commands.\n\nI think that the more narrow fixes[2] to the memory leaks around\nunpack-trees in Elijah's series[3] are better skipped and that series\nrebased on top of this one (I'll submit an RFC version of his that is\nrebased on this as a follow-up).\n\nI.e. his solves a very small amount of the memory leaks in this area,\nwhereas this is something I've got running as part of end-to-end\nSANITIZE=leak testing, so I think that the difference in approaches we\npicked when it comes to fixing them is likely because of that.\n\nE.g. continuing to allocate the \"struct dir_struct\" on the heap in his\nversion[4] in his is, I think, something that makes more sense for\nfixes that haven't pulled at the thread of how much merge-recursive.c\nis making that question of ownerhip confusing. There's also changen in\nhis that'll become simpler as the complexity of the underlying APIs\nhas gone away, e.g. [6].\n\n1. https://lore.kernel.org/git/87ilyjviiy.fsf@evledraar.gmail.com/\n2. https://lore.kernel.org/git/0c74285b25311c83bb158cf89a551160a0f3a5d3.1632760428.git.gitgitgadget@gmail.com/\n3. https://lore.kernel.org/git/pull.1036.v3.git.1632760428.gitgitgadget@gmail.com/\n4. https://lore.kernel.org/git/0d119142778dce8617dd9b2c102b5f5bfdc9dc0f.1632760428.git.gitgitgadget@gmail.com/\n6. https://lore.kernel.org/git/f1a0700e598e52d6cdb507fe8a09c4fa9291c982.1632760428.git.gitgitgadget@gmail.com/\n\nComments on individual patches below:\n\nÆvar Arnfjörð Bjarmason (10):\n  unpack-trees.[ch]: define and use a UNPACK_TREES_OPTIONS_INIT\n\nI had this at the end of the v3 of my designated initializer cleanup\nseries[7]\n\nI think Junio fairly commented that this in isolation looked like it\nwas going nowhere[8] since we didn' get past initializing \"struct\nunpack_trees_options\" as \"{ 0 }\", but that'll soon be the case in this\nseries...\n\n  merge-recursive.c: call a new unpack_trees_options_init() function\n\nDetails how merge-recursive.c calls unpack_trees() differently than\nevery other caller when it comes to \"struct unpack_trees_options\"\nsetup.\n\n  unpack-trees.[ch]: embed \"dir\" in \"struct unpack_trees_options\"\n\nElijah's series ends up with a \"dir\" still heap-allocated in \"struct\nunpack_trees_options\", just dynamically and \"privately\". Here it's\nallocated on the stack (or for merge-recursive.c, as defined in\nUNPACK_TREES_OPTIONS_INIT), because we could untangle the\nmerge-recursive.c edge-case earlier.\n\n  unpack-trees API: don't have clear_unpack_trees_porcelain() reset\n\nMove merge-recursive.c special-snowflake behavior into\nmerge-recursive.c.\n\n  dir.[ch]: make DIR_INIT mandatory\n  dir.c: get rid of lazy initialization\n\nThe last caller not using \"struct dir_struct\" via DIR_INIT goes away,\nallowing for untangling the mess I commented on at length in [9].\n\n  unpack-trees API: rename clear_unpack_trees_porcelain()\n\nJust a s/clear_unpack_trees_porcelain/unpack_trees_options_release/g,\nsince that's what it does now.\n\n  unpack-trees: don't leak memory in verify_clean_subdirectory()\n  merge.c: avoid duplicate unpack_trees_options_release() code\n  built-ins: plug memory leaks with unpack_trees_options_release()\n\nA lot of memory leak fixes both in unpack-trees.c and its users, only\na subset of this is in Elijah's series.\n\n7. https://lore.kernel.org/git/cover-v3-0.6-00000000000-20211001T102056Z-avarab@gmail.com/\n8. https://lore.kernel.org/git/xmqqk0iw4e7v.fsf@gitster.g/\n9. https://lore.kernel.org/git/87sfxhohsj.fsf@evledraar.gmail.com/\n\n add-interactive.c         |  2 +-\n archive.c                 | 12 ++++++++----\n builtin/am.c              | 23 ++++++++++++++---------\n builtin/checkout.c        | 22 ++++++++++++----------\n builtin/clone.c           |  4 ++--\n builtin/commit.c          |  9 ++++++---\n builtin/merge.c           |  9 +++++----\n builtin/read-tree.c       | 28 +++++++++++++++-------------\n builtin/reset.c           | 16 ++++++++++------\n builtin/sparse-checkout.c |  5 ++---\n builtin/stash.c           | 20 ++++++++++++--------\n diff-lib.c                |  8 +++++---\n dir.c                     |  8 --------\n dir.h                     |  6 ++++--\n merge-ort.c               | 12 ++++--------\n merge-recursive.c         |  6 ++++--\n merge.c                   | 23 +++++++++++------------\n reset.c                   |  4 ++--\n sequencer.c               |  3 +--\n unpack-trees.c            | 24 +++++++++++++-----------\n unpack-trees.h            | 17 +++++++++++++----\n 21 files changed, 144 insertions(+), 117 deletions(-)\n\n-- \n2.33.0.1404.g83021034c5d\n\n"},{"id":"437818","messageId":"patch-02.10-050399cbfbf-20211004T002226Z-avarab@gmail.com","threadId":"56640","inReplyTo":"cover-00.10-00000000000-20211004T002226Z-avarab@gmail.com","subject":"[PATCH 02/10] merge-recursive.c: call a new unpack_trees_options_init() function","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-04T00:46:03Z","receivedAt":"2021-10-04T00:46:39Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"In the preceding commit we introduced a new UNPACK_TREES_OPTIONS_INIT\nmacro, but \"merge-recursive.c\" could not be converted to it since\nit (re-)initializes a \"struct unpack_trees_options\" on the heap.\n\nIn order to convert use the macro as the source of truth for\ninitialization we need to not only convert the initialization in\nunpack_trees_start(), but also call the new\nunpack_trees_options_init() just after the CALLOC_ARRAY() in\nmerge_start().\n\nWhen we call merge_trees() we'll call merge_start() followed by\nmerge_trees_internal(), and it will call unpack_trees_start() before\nit does much of anything. So it's covered by an initialization in\nunpack_trees_start().\n\nBut when merge_recursive() is called it will call merge_start()\nfollowed by merge_recursive_internal(), which won't call\nunpack_trees_start() until its own call call to\nmerge_trees_internal(), but in the meantime it might end up using that\ncalloc'd \"struct unpack_trees_options\".\n\nThis was OK before, as setup_unpack_trees_porcelain() would call\nstrvec_init(), and our \"struct dir_struct\" in turn is OK with being\nNULL'd. Let's convert the former to macro initialization, we'll deal\nwith the latter in a subsequent commit.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n merge-recursive.c | 3 ++-\n unpack-trees.c    | 8 ++++++--\n unpack-trees.h    | 5 ++++-\n 3 files changed, 12 insertions(+), 4 deletions(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex e594d4c3fa1..d24a4903f1d 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -405,7 +405,7 @@ static int unpack_trees_start(struct merge_options *opt,\n \tstruct tree_desc t[3];\n \tstruct index_state tmp_index = { NULL };\n \n-\tmemset(&opt->priv->unpack_opts, 0, sizeof(opt->priv->unpack_opts));\n+\tunpack_trees_options_init(&opt->priv->unpack_opts);\n \tif (opt->priv->call_depth)\n \t\topt->priv->unpack_opts.index_only = 1;\n \telse\n@@ -3704,6 +3704,7 @@ static int merge_start(struct merge_options *opt, struct tree *head)\n \n \tCALLOC_ARRAY(opt->priv, 1);\n \tstring_list_init_dup(&opt->priv->df_conflict_file_set);\n+\tunpack_trees_options_init(&opt->priv->unpack_opts);\n \treturn 0;\n }\n \ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 8ea0a542da8..393c1f35a5d 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -108,8 +108,6 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,\n \tconst char **msgs = opts->msgs;\n \tconst char *msg;\n \n-\tstrvec_init(&opts->msgs_to_free);\n-\n \tif (!strcmp(cmd, \"checkout\"))\n \t\tmsg = advice_enabled(ADVICE_COMMIT_BEFORE_MERGE)\n \t\t      ? _(\"Your local changes to the following files would be overwritten by checkout:\\n%%s\"\n@@ -189,6 +187,12 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,\n \t\topts->unpack_rejects[i].strdup_strings = 1;\n }\n \n+void unpack_trees_options_init(struct unpack_trees_options *o)\n+{\n+\tstruct unpack_trees_options blank = UNPACK_TREES_OPTIONS_INIT;\n+\tmemcpy(o, &blank, sizeof(*o));\n+}\n+\n void clear_unpack_trees_porcelain(struct unpack_trees_options *opts)\n {\n \tstrvec_clear(&opts->msgs_to_free);\ndiff --git a/unpack-trees.h b/unpack-trees.h\nindex ecf256cbcea..892b65ea623 100644\n--- a/unpack-trees.h\n+++ b/unpack-trees.h\n@@ -91,7 +91,10 @@ struct unpack_trees_options {\n \tstruct checkout_metadata meta;\n };\n \n-#define UNPACK_TREES_OPTIONS_INIT { 0 }\n+#define UNPACK_TREES_OPTIONS_INIT { \\\n+\t.msgs_to_free = STRVEC_INIT, \\\n+}\n+void unpack_trees_options_init(struct unpack_trees_options *o);\n \n int unpack_trees(unsigned n, struct tree_desc *t,\n \t\t struct unpack_trees_options *options);\n-- \n2.33.0.1404.g83021034c5d\n\n"},{"id":"437819","messageId":"patch-04.10-9d62edd35bf-20211004T002226Z-avarab@gmail.com","threadId":"56640","inReplyTo":"cover-00.10-00000000000-20211004T002226Z-avarab@gmail.com","subject":"[PATCH 04/10] unpack-trees API: don't have clear_unpack_trees_porcelain() reset","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-04T00:46:05Z","receivedAt":"2021-10-04T00:46:39Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the clear_unpack_trees_porcelain() to be like a *_release()\nfunction, not a *_reset() (in strbuf.c terms). Let's move the only API\nuser that relied on the latter to doing its own\nunpack_trees_options_init(). See the commit that introduced\nunpack_trees_options_init() for details on the control flow involved\nhere.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n merge-recursive.c | 1 +\n unpack-trees.c    | 1 -\n 2 files changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex d24a4903f1d..a77f66b006c 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -442,6 +442,7 @@ static void unpack_trees_finish(struct merge_options *opt)\n {\n \tdiscard_index(&opt->priv->orig_index);\n \tclear_unpack_trees_porcelain(&opt->priv->unpack_opts);\n+\tunpack_trees_options_init(&opt->priv->unpack_opts);\n }\n \n static int save_files_dirs(const struct object_id *oid,\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 94767d3f96f..e7365322e82 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -197,7 +197,6 @@ void clear_unpack_trees_porcelain(struct unpack_trees_options *opts)\n {\n \tstrvec_clear(&opts->msgs_to_free);\n \tdir_clear(&opts->dir);\n-\tmemset(opts->msgs, 0, sizeof(opts->msgs));\n }\n \n static int do_add_entry(struct unpack_trees_options *o, struct cache_entry *ce,\n-- \n2.33.0.1404.g83021034c5d\n\n"},{"id":"437820","messageId":"patch-09.10-aa2bec76f6e-20211004T002226Z-avarab@gmail.com","threadId":"56640","inReplyTo":"cover-00.10-00000000000-20211004T002226Z-avarab@gmail.com","subject":"[PATCH 09/10] merge.c: avoid duplicate unpack_trees_options_release() code","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-04T00:46:10Z","receivedAt":"2021-10-04T00:46:39Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Refactor code added in 1c41d2805e4 (unpack_trees_options: free\nmessages when done, 2018-05-21) to use a ret/goto pattern, rather than\nduplicating the end cleanup in the function.\n\nThis control flow will be matched in subsequent commits by various\nother similar code, which often needs to do more than just call\nunpack_trees_options_release(). Let's change this to consistently use\nthe same pattern everywhere.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n merge.c | 13 ++++++++-----\n 1 file changed, 8 insertions(+), 5 deletions(-)\n\ndiff --git a/merge.c b/merge.c\nindex 2f618425aff..2e3714ccaa0 100644\n--- a/merge.c\n+++ b/merge.c\n@@ -54,6 +54,7 @@ int checkout_fast_forward(struct repository *r,\n \tstruct tree_desc t[MAX_UNPACK_TREES];\n \tint i, nr_trees = 0;\n \tstruct lock_file lock_file = LOCK_INIT;\n+\tint ret = 0;\n \n \trefresh_index(r->index, REFRESH_QUIET, NULL, NULL, NULL);\n \n@@ -95,12 +96,14 @@ int checkout_fast_forward(struct repository *r,\n \n \tif (unpack_trees(nr_trees, t, &opts)) {\n \t\trollback_lock_file(&lock_file);\n-\t\tunpack_trees_options_release(&opts);\n-\t\treturn -1;\n+\t\tret = -1;\n+\t\tgoto cleanup;\n \t}\n-\tunpack_trees_options_release(&opts);\n \n \tif (write_locked_index(r->index, &lock_file, COMMIT_LOCK))\n-\t\treturn error(_(\"unable to write new index file\"));\n-\treturn 0;\n+\t\tret = error(_(\"unable to write new index file\"));\n+\n+cleanup:\n+\tunpack_trees_options_release(&opts);\n+\treturn ret;\n }\n-- \n2.33.0.1404.g83021034c5d\n\n"},{"id":"437821","messageId":"patch-03.10-aaad6fce194-20211004T002226Z-avarab@gmail.com","threadId":"56640","inReplyTo":"cover-00.10-00000000000-20211004T002226Z-avarab@gmail.com","subject":"[PATCH 03/10] unpack-trees.[ch]: embed \"dir\" in \"struct unpack_trees_options\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-04T00:46:04Z","receivedAt":"2021-10-04T00:46:39Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the \"struct dir\" in \"struct unpack_trees_options\" away from a\npointer to being embedded in the struct itself, this mean that we can\ninitialize it with our new UNPACK_TREES_OPTIONS_INIT macro.\n\nAs it turned out nothing actually needed to provide its own \"struct\ndir\" to this API, there's another patch to \"hide\" the struct instead,\nsee the discussion at [1], but I think just allocating it on the stack\nalong with the rest makes more sense.\n\nThis fixes a memory leak in \"builtin/checkout.c\" that's been there\nsince the clear_unpack_trees_porcelain() function was added in\n1c41d2805e4 (unpack_trees_options: free messages when done,\n2018-05-21), i.e. we still had a memory leak from allocating the \"dir\"\nmember here. That \"dir\" member had in turn been with us ever since\n782c2d65c24 (Build in checkout, 2008-02-07).\n\nThis fixes that memory leak, and allows us to remove the boilerplate\ndir_clear() elsewhere in favor of just using\nclear_unpack_trees_porcelain().\n\n1. https://lore.kernel.org/git/87ilyjviiy.fsf@evledraar.gmail.com/\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n builtin/checkout.c  |  5 ++---\n builtin/read-tree.c | 11 ++++-------\n merge-ort.c         |  7 ++-----\n merge.c             |  7 ++-----\n unpack-trees.c      | 10 ++++------\n unpack-trees.h      |  4 +++-\n 6 files changed, 17 insertions(+), 27 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex fea4533dbec..d4b88affba7 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -745,9 +745,8 @@ static int merge_working_tree(const struct checkout_opts *opts,\n \t\t\t\t       &new_branch_info->commit->object.oid :\n \t\t\t\t       &new_branch_info->oid, NULL);\n \t\tif (opts->overwrite_ignore) {\n-\t\t\ttopts.dir = xcalloc(1, sizeof(*topts.dir));\n-\t\t\ttopts.dir->flags |= DIR_SHOW_IGNORED;\n-\t\t\tsetup_standard_excludes(topts.dir);\n+\t\t\ttopts.dir.flags |= DIR_SHOW_IGNORED;\n+\t\t\tsetup_standard_excludes(&topts.dir);\n \t\t}\n \t\ttree = parse_tree_indirect(old_branch_info->commit ?\n \t\t\t\t\t   &old_branch_info->commit->object.oid :\ndiff --git a/builtin/read-tree.c b/builtin/read-tree.c\nindex 847182fdad6..06f3b97ac05 100644\n--- a/builtin/read-tree.c\n+++ b/builtin/read-tree.c\n@@ -53,20 +53,17 @@ static int index_output_cb(const struct option *opt, const char *arg,\n static int exclude_per_directory_cb(const struct option *opt, const char *arg,\n \t\t\t\t    int unset)\n {\n-\tstruct dir_struct *dir;\n \tstruct unpack_trees_options *opts;\n \n \tBUG_ON_OPT_NEG(unset);\n \n \topts = (struct unpack_trees_options *)opt->value;\n \n-\tif (opts->dir)\n+\tif (opts->dir.exclude_per_dir)\n \t\tdie(\"more than one --exclude-per-directory given.\");\n \n-\tdir = xcalloc(1, sizeof(*opts->dir));\n-\tdir->flags |= DIR_SHOW_IGNORED;\n-\tdir->exclude_per_dir = arg;\n-\topts->dir = dir;\n+\topts->dir.flags |= DIR_SHOW_IGNORED;\n+\topts->dir.exclude_per_dir = arg;\n \t/* We do not need to nor want to do read-directory\n \t * here; we are merely interested in reusing the\n \t * per directory ignore stack mechanism.\n@@ -208,7 +205,7 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)\n \tif ((opts.update || opts.index_only) && !opts.merge)\n \t\tdie(\"%s is meaningless without -m, --reset, or --prefix\",\n \t\t    opts.update ? \"-u\" : \"-i\");\n-\tif ((opts.dir && !opts.update))\n+\tif ((opts.dir.exclude_per_dir && !opts.update))\n \t\tdie(\"--exclude-per-directory is meaningless unless -u\");\n \tif (opts.merge && !opts.index_only)\n \t\tsetup_work_tree();\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 75d2b8e4b99..e526b78b88d 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -4045,9 +4045,8 @@ static int checkout(struct merge_options *opt,\n \tunpack_opts.verbose_update = (opt->verbosity > 2);\n \tunpack_opts.fn = twoway_merge;\n \tif (1/* FIXME: opts->overwrite_ignore*/) {\n-\t\tCALLOC_ARRAY(unpack_opts.dir, 1);\n-\t\tunpack_opts.dir->flags |= DIR_SHOW_IGNORED;\n-\t\tsetup_standard_excludes(unpack_opts.dir);\n+\t\tunpack_opts.dir.flags |= DIR_SHOW_IGNORED;\n+\t\tsetup_standard_excludes(&unpack_opts.dir);\n \t}\n \tparse_tree(prev);\n \tinit_tree_desc(&trees[0], prev->buffer, prev->size);\n@@ -4056,8 +4055,6 @@ static int checkout(struct merge_options *opt,\n \n \tret = unpack_trees(2, trees, &unpack_opts);\n \tclear_unpack_trees_porcelain(&unpack_opts);\n-\tdir_clear(unpack_opts.dir);\n-\tFREE_AND_NULL(unpack_opts.dir);\n \treturn ret;\n }\n \ndiff --git a/merge.c b/merge.c\nindex 086f04b0f97..9cb32990dd9 100644\n--- a/merge.c\n+++ b/merge.c\n@@ -53,7 +53,6 @@ int checkout_fast_forward(struct repository *r,\n \tstruct unpack_trees_options opts = UNPACK_TREES_OPTIONS_INIT;\n \tstruct tree_desc t[MAX_UNPACK_TREES];\n \tint i, nr_trees = 0;\n-\tstruct dir_struct dir = DIR_INIT;\n \tstruct lock_file lock_file = LOCK_INIT;\n \n \trefresh_index(r->index, REFRESH_QUIET, NULL, NULL, NULL);\n@@ -80,9 +79,8 @@ int checkout_fast_forward(struct repository *r,\n \t}\n \n \tif (overwrite_ignore) {\n-\t\tdir.flags |= DIR_SHOW_IGNORED;\n-\t\tsetup_standard_excludes(&dir);\n-\t\topts.dir = &dir;\n+\t\topts.dir.flags |= DIR_SHOW_IGNORED;\n+\t\tsetup_standard_excludes(&opts.dir);\n \t}\n \n \topts.head_idx = 1;\n@@ -100,7 +98,6 @@ int checkout_fast_forward(struct repository *r,\n \t\tclear_unpack_trees_porcelain(&opts);\n \t\treturn -1;\n \t}\n-\tdir_clear(&dir);\n \tclear_unpack_trees_porcelain(&opts);\n \n \tif (write_locked_index(r->index, &lock_file, COMMIT_LOCK))\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 393c1f35a5d..94767d3f96f 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -196,6 +196,7 @@ void unpack_trees_options_init(struct unpack_trees_options *o)\n void clear_unpack_trees_porcelain(struct unpack_trees_options *opts)\n {\n \tstrvec_clear(&opts->msgs_to_free);\n+\tdir_clear(&opts->dir);\n \tmemset(opts->msgs, 0, sizeof(opts->msgs));\n }\n \n@@ -2085,7 +2086,7 @@ static int verify_clean_subdirectory(const struct cache_entry *ce,\n \t */\n \tint namelen;\n \tint i;\n-\tstruct dir_struct d;\n+\tstruct dir_struct d = DIR_INIT;\n \tchar *pathbuf;\n \tint cnt = 0;\n \n@@ -2136,9 +2137,7 @@ static int verify_clean_subdirectory(const struct cache_entry *ce,\n \t */\n \tpathbuf = xstrfmt(\"%.*s/\", namelen, ce->name);\n \n-\tmemset(&d, 0, sizeof(d));\n-\tif (o->dir)\n-\t\td.exclude_per_dir = o->dir->exclude_per_dir;\n+\td.exclude_per_dir = o->dir.exclude_per_dir;\n \ti = read_directory(&d, o->src_index, pathbuf, namelen+1, NULL);\n \tif (i)\n \t\treturn add_rejected_path(o, ERROR_NOT_UPTODATE_DIR, ce->name);\n@@ -2179,8 +2178,7 @@ static int check_ok_to_remove(const char *name, int len, int dtype,\n \tif (ignore_case && icase_exists(o, name, len, st))\n \t\treturn 0;\n \n-\tif (o->dir &&\n-\t    is_excluded(o->dir, o->src_index, name, &dtype))\n+\tif (is_excluded(&o->dir, o->src_index, name, &dtype))\n \t\t/*\n \t\t * ce->name is explicitly excluded, so it is Ok to\n \t\t * overwrite it.\ndiff --git a/unpack-trees.h b/unpack-trees.h\nindex 892b65ea623..40c4841748d 100644\n--- a/unpack-trees.h\n+++ b/unpack-trees.h\n@@ -5,6 +5,7 @@\n #include \"strvec.h\"\n #include \"string-list.h\"\n #include \"tree-walk.h\"\n+#include \"dir.h\"\n \n #define MAX_UNPACK_TREES MAX_TRAVERSE_TREES\n \n@@ -66,7 +67,7 @@ struct unpack_trees_options {\n \t\t     dry_run;\n \tconst char *prefix;\n \tint cache_bottom;\n-\tstruct dir_struct *dir;\n+\tstruct dir_struct dir;\n \tstruct pathspec *pathspec;\n \tmerge_fn_t fn;\n \tconst char *msgs[NB_UNPACK_TREES_WARNING_TYPES];\n@@ -93,6 +94,7 @@ struct unpack_trees_options {\n \n #define UNPACK_TREES_OPTIONS_INIT { \\\n \t.msgs_to_free = STRVEC_INIT, \\\n+\t.dir = DIR_INIT, \\\n }\n void unpack_trees_options_init(struct unpack_trees_options *o);\n \n-- \n2.33.0.1404.g83021034c5d\n\n"},{"id":"437822","messageId":"patch-01.10-b550c290c11-20211004T002226Z-avarab@gmail.com","threadId":"56640","inReplyTo":"cover-00.10-00000000000-20211004T002226Z-avarab@gmail.com","subject":"[PATCH 01/10] unpack-trees.[ch]: define and use a UNPACK_TREES_OPTIONS_INIT","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-04T00:46:02Z","receivedAt":"2021-10-04T00:46:39Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Almost all users of \"struct unpack_trees_options\" allocate it on the\nstack already, let's provide an *_INIT macro for them to use. This\nwill make later changes that would like to initialize other members of\nthe struct easier[1][2], but for now we're keeping it compatible with\na memset() to \"0\" with an \"{ 0 }\" initialization.\n\nThis covers all callers except a special-case in \"merge-recursive.c\",\nwhich will be dealt with in a subsequent commit.\n\n1. https://lore.kernel.org/git/87k0ixrv23.fsf@evledraar.gmail.com/\n2. https://lore.kernel.org/git/87fstlrumj.fsf@evledraar.gmail.com/\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n archive.c                 | 3 +--\n builtin/am.c              | 6 ++----\n builtin/checkout.c        | 6 ++----\n builtin/clone.c           | 3 +--\n builtin/commit.c          | 3 +--\n builtin/merge.c           | 3 +--\n builtin/read-tree.c       | 3 +--\n builtin/reset.c           | 3 +--\n builtin/sparse-checkout.c | 3 +--\n builtin/stash.c           | 6 ++----\n diff-lib.c                | 3 +--\n merge-ort.c               | 3 +--\n merge.c                   | 3 +--\n reset.c                   | 2 +-\n sequencer.c               | 3 +--\n unpack-trees.h            | 2 ++\n 16 files changed, 20 insertions(+), 35 deletions(-)\n\ndiff --git a/archive.c b/archive.c\nindex a3bbb091256..210d7235c5a 100644\n--- a/archive.c\n+++ b/archive.c\n@@ -269,7 +269,7 @@ int write_archive_entries(struct archiver_args *args,\n \t\twrite_archive_entry_fn_t write_entry)\n {\n \tstruct archiver_context context;\n-\tstruct unpack_trees_options opts;\n+\tstruct unpack_trees_options opts = UNPACK_TREES_OPTIONS_INIT;\n \tstruct tree_desc t;\n \tint err;\n \tstruct strbuf path_in_archive = STRBUF_INIT;\n@@ -300,7 +300,6 @@ int write_archive_entries(struct archiver_args *args,\n \t * Setup index and instruct attr to read index only\n \t */\n \tif (!args->worktree_attributes) {\n-\t\tmemset(&opts, 0, sizeof(opts));\n \t\topts.index_only = 1;\n \t\topts.head_idx = -1;\n \t\topts.src_index = args->repo->index;\ndiff --git a/builtin/am.c b/builtin/am.c\nindex e4a0ff9cd7c..82641ce68ec 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -1901,7 +1901,7 @@ static void am_resolve(struct am_state *state)\n static int fast_forward_to(struct tree *head, struct tree *remote, int reset)\n {\n \tstruct lock_file lock_file = LOCK_INIT;\n-\tstruct unpack_trees_options opts;\n+\tstruct unpack_trees_options opts = UNPACK_TREES_OPTIONS_INIT;\n \tstruct tree_desc t[2];\n \n \tif (parse_tree(head) || parse_tree(remote))\n@@ -1911,7 +1911,6 @@ static int fast_forward_to(struct tree *head, struct tree *remote, int reset)\n \n \trefresh_cache(REFRESH_QUIET);\n \n-\tmemset(&opts, 0, sizeof(opts));\n \topts.head_idx = 1;\n \topts.src_index = &the_index;\n \topts.dst_index = &the_index;\n@@ -1940,7 +1939,7 @@ static int fast_forward_to(struct tree *head, struct tree *remote, int reset)\n static int merge_tree(struct tree *tree)\n {\n \tstruct lock_file lock_file = LOCK_INIT;\n-\tstruct unpack_trees_options opts;\n+\tstruct unpack_trees_options opts = UNPACK_TREES_OPTIONS_INIT;\n \tstruct tree_desc t[1];\n \n \tif (parse_tree(tree))\n@@ -1948,7 +1947,6 @@ static int merge_tree(struct tree *tree)\n \n \thold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);\n \n-\tmemset(&opts, 0, sizeof(opts));\n \topts.head_idx = 1;\n \topts.src_index = &the_index;\n \topts.dst_index = &the_index;\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 8c69dcdf72a..fea4533dbec 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -639,10 +639,9 @@ static int reset_tree(struct tree *tree, const struct checkout_opts *o,\n \t\t      int worktree, int *writeout_error,\n \t\t      struct branch_info *info)\n {\n-\tstruct unpack_trees_options opts;\n+\tstruct unpack_trees_options opts = UNPACK_TREES_OPTIONS_INIT;\n \tstruct tree_desc tree_desc;\n \n-\tmemset(&opts, 0, sizeof(opts));\n \topts.head_idx = -1;\n \topts.update = worktree;\n \topts.skip_unmerged = !worktree;\n@@ -719,9 +718,8 @@ static int merge_working_tree(const struct checkout_opts *opts,\n \t} else {\n \t\tstruct tree_desc trees[2];\n \t\tstruct tree *tree;\n-\t\tstruct unpack_trees_options topts;\n+\t\tstruct unpack_trees_options topts = UNPACK_TREES_OPTIONS_INIT;\n \n-\t\tmemset(&topts, 0, sizeof(topts));\n \t\ttopts.head_idx = -1;\n \t\ttopts.src_index = &the_index;\n \t\ttopts.dst_index = &the_index;\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex ff1d3d447a3..0df820c5970 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -655,7 +655,7 @@ static int checkout(int submodule_progress)\n \tstruct object_id oid;\n \tchar *head;\n \tstruct lock_file lock_file = LOCK_INIT;\n-\tstruct unpack_trees_options opts;\n+\tstruct unpack_trees_options opts = UNPACK_TREES_OPTIONS_INIT;\n \tstruct tree *tree;\n \tstruct tree_desc t;\n \tint err = 0;\n@@ -683,7 +683,6 @@ static int checkout(int submodule_progress)\n \n \thold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);\n \n-\tmemset(&opts, 0, sizeof opts);\n \topts.update = 1;\n \topts.merge = 1;\n \topts.clone = 1;\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex e7320f66f95..6cc7313bad8 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -303,7 +303,7 @@ static void add_remove_files(struct string_list *list)\n static void create_base_index(const struct commit *current_head)\n {\n \tstruct tree *tree;\n-\tstruct unpack_trees_options opts;\n+\tstruct unpack_trees_options opts = UNPACK_TREES_OPTIONS_INIT;\n \tstruct tree_desc t;\n \n \tif (!current_head) {\n@@ -311,7 +311,6 @@ static void create_base_index(const struct commit *current_head)\n \t\treturn;\n \t}\n \n-\tmemset(&opts, 0, sizeof(opts));\n \topts.head_idx = 1;\n \topts.index_only = 1;\n \topts.merge = 1;\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 3fbdacc7db4..73290a07fcc 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -670,9 +670,8 @@ static int read_tree_trivial(struct object_id *common, struct object_id *head,\n \tint i, nr_trees = 0;\n \tstruct tree *trees[MAX_UNPACK_TREES];\n \tstruct tree_desc t[MAX_UNPACK_TREES];\n-\tstruct unpack_trees_options opts;\n+\tstruct unpack_trees_options opts = UNPACK_TREES_OPTIONS_INIT;\n \n-\tmemset(&opts, 0, sizeof(opts));\n \topts.head_idx = 2;\n \topts.src_index = &the_index;\n \topts.dst_index = &the_index;\ndiff --git a/builtin/read-tree.c b/builtin/read-tree.c\nindex 485e7b04794..847182fdad6 100644\n--- a/builtin/read-tree.c\n+++ b/builtin/read-tree.c\n@@ -116,7 +116,7 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)\n \tint i, stage = 0;\n \tstruct object_id oid;\n \tstruct tree_desc t[MAX_UNPACK_TREES];\n-\tstruct unpack_trees_options opts;\n+\tstruct unpack_trees_options opts = UNPACK_TREES_OPTIONS_INIT;\n \tint prefix_set = 0;\n \tstruct lock_file lock_file = LOCK_INIT;\n \tconst struct option read_tree_options[] = {\n@@ -158,7 +158,6 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)\n \t\tOPT_END()\n \t};\n \n-\tmemset(&opts, 0, sizeof(opts));\n \topts.head_idx = -1;\n \topts.src_index = &the_index;\n \topts.dst_index = &the_index;\ndiff --git a/builtin/reset.c b/builtin/reset.c\nindex 51c9e2f43ff..86c604b21e9 100644\n--- a/builtin/reset.c\n+++ b/builtin/reset.c\n@@ -51,10 +51,9 @@ static int reset_index(const char *ref, const struct object_id *oid, int reset_t\n \tint i, nr = 0;\n \tstruct tree_desc desc[2];\n \tstruct tree *tree;\n-\tstruct unpack_trees_options opts;\n+\tstruct unpack_trees_options opts = UNPACK_TREES_OPTIONS_INIT;\n \tint ret = -1;\n \n-\tmemset(&opts, 0, sizeof(opts));\n \topts.head_idx = 1;\n \topts.src_index = &the_index;\n \topts.dst_index = &the_index;\ndiff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c\nindex d0f5c4702be..4c3c29fb580 100644\n--- a/builtin/sparse-checkout.c\n+++ b/builtin/sparse-checkout.c\n@@ -195,7 +195,7 @@ static void clean_tracked_sparse_directories(struct repository *r)\n static int update_working_directory(struct pattern_list *pl)\n {\n \tenum update_sparsity_result result;\n-\tstruct unpack_trees_options o;\n+\tstruct unpack_trees_options o = UNPACK_TREES_OPTIONS_INIT;\n \tstruct lock_file lock_file = LOCK_INIT;\n \tstruct repository *r = the_repository;\n \n@@ -205,7 +205,6 @@ static int update_working_directory(struct pattern_list *pl)\n \n \tr->index->sparse_checkout_patterns = pl;\n \n-\tmemset(&o, 0, sizeof(o));\n \to.verbose_update = isatty(2);\n \to.update = 1;\n \to.head_idx = -1;\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex 8f42360ca91..1137e5fcbe8 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -233,7 +233,7 @@ static int clear_stash(int argc, const char **argv, const char *prefix)\n static int reset_tree(struct object_id *i_tree, int update, int reset)\n {\n \tint nr_trees = 1;\n-\tstruct unpack_trees_options opts;\n+\tstruct unpack_trees_options opts = UNPACK_TREES_OPTIONS_INIT;\n \tstruct tree_desc t[MAX_UNPACK_TREES];\n \tstruct tree *tree;\n \tstruct lock_file lock_file = LOCK_INIT;\n@@ -244,8 +244,6 @@ static int reset_tree(struct object_id *i_tree, int update, int reset)\n \n \thold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);\n \n-\tmemset(&opts, 0, sizeof(opts));\n-\n \ttree = parse_tree_indirect(i_tree);\n \tif (parse_tree(tree))\n \t\treturn -1;\n@@ -799,7 +797,7 @@ static void diff_include_untracked(const struct stash_info *info, struct diff_op\n \tconst struct object_id *oid[] = { &info->w_commit, &info->u_tree };\n \tstruct tree *tree[ARRAY_SIZE(oid)];\n \tstruct tree_desc tree_desc[ARRAY_SIZE(oid)];\n-\tstruct unpack_trees_options unpack_tree_opt = { 0 };\n+\tstruct unpack_trees_options unpack_tree_opt = UNPACK_TREES_OPTIONS_INIT;\n \tint i;\n \n \tfor (i = 0; i < ARRAY_SIZE(oid); i++) {\ndiff --git a/diff-lib.c b/diff-lib.c\nindex ca085a03efc..8a08d9af4eb 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -526,13 +526,12 @@ static int diff_cache(struct rev_info *revs,\n {\n \tstruct tree *tree;\n \tstruct tree_desc t;\n-\tstruct unpack_trees_options opts;\n+\tstruct unpack_trees_options opts = UNPACK_TREES_OPTIONS_INIT;\n \n \ttree = parse_tree_indirect(tree_oid);\n \tif (!tree)\n \t\treturn error(\"bad tree object %s\",\n \t\t\t     tree_name ? tree_name : oid_to_hex(tree_oid));\n-\tmemset(&opts, 0, sizeof(opts));\n \topts.head_idx = 1;\n \topts.index_only = cached;\n \topts.diff_index_cached = (cached &&\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 35aa979c3a4..75d2b8e4b99 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -4021,9 +4021,8 @@ static int checkout(struct merge_options *opt,\n \t/* Switch the index/working copy from old to new */\n \tint ret;\n \tstruct tree_desc trees[2];\n-\tstruct unpack_trees_options unpack_opts;\n+\tstruct unpack_trees_options unpack_opts = UNPACK_TREES_OPTIONS_INIT;\n \n-\tmemset(&unpack_opts, 0, sizeof(unpack_opts));\n \tunpack_opts.head_idx = -1;\n \tunpack_opts.src_index = opt->repo->index;\n \tunpack_opts.dst_index = opt->repo->index;\ndiff --git a/merge.c b/merge.c\nindex 6e736881d90..086f04b0f97 100644\n--- a/merge.c\n+++ b/merge.c\n@@ -50,7 +50,7 @@ int checkout_fast_forward(struct repository *r,\n \t\t\t  int overwrite_ignore)\n {\n \tstruct tree *trees[MAX_UNPACK_TREES];\n-\tstruct unpack_trees_options opts;\n+\tstruct unpack_trees_options opts = UNPACK_TREES_OPTIONS_INIT;\n \tstruct tree_desc t[MAX_UNPACK_TREES];\n \tint i, nr_trees = 0;\n \tstruct dir_struct dir = DIR_INIT;\n@@ -79,7 +79,6 @@ int checkout_fast_forward(struct repository *r,\n \t\tinit_tree_desc(t+i, trees[i]->buffer, trees[i]->size);\n \t}\n \n-\tmemset(&opts, 0, sizeof(opts));\n \tif (overwrite_ignore) {\n \t\tdir.flags |= DIR_SHOW_IGNORED;\n \t\tsetup_standard_excludes(&dir);\ndiff --git a/reset.c b/reset.c\nindex 79310ae071b..d13984ab781 100644\n--- a/reset.c\n+++ b/reset.c\n@@ -21,7 +21,7 @@ int reset_head(struct repository *r, struct object_id *oid, const char *action,\n \tstruct object_id head_oid;\n \tstruct tree_desc desc[2] = { { NULL }, { NULL } };\n \tstruct lock_file lock = LOCK_INIT;\n-\tstruct unpack_trees_options unpack_tree_opts = { 0 };\n+\tstruct unpack_trees_options unpack_tree_opts = UNPACK_TREES_OPTIONS_INIT;\n \tstruct tree *tree;\n \tconst char *reflog_action;\n \tstruct strbuf msg = STRBUF_INIT;\ndiff --git a/sequencer.c b/sequencer.c\nindex 614d56f5e21..abd85b6c562 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3652,7 +3652,7 @@ static int do_reset(struct repository *r,\n \tstruct lock_file lock = LOCK_INIT;\n \tstruct tree_desc desc;\n \tstruct tree *tree;\n-\tstruct unpack_trees_options unpack_tree_opts;\n+\tstruct unpack_trees_options unpack_tree_opts = UNPACK_TREES_OPTIONS_INIT;\n \tint ret = 0;\n \n \tif (repo_hold_locked_index(r, &lock, LOCK_REPORT_ON_ERROR) < 0)\n@@ -3691,7 +3691,6 @@ static int do_reset(struct repository *r,\n \t\t}\n \t}\n \n-\tmemset(&unpack_tree_opts, 0, sizeof(unpack_tree_opts));\n \tsetup_unpack_trees_porcelain(&unpack_tree_opts, \"reset\");\n \tunpack_tree_opts.head_idx = 1;\n \tunpack_tree_opts.src_index = r->index;\ndiff --git a/unpack-trees.h b/unpack-trees.h\nindex 2d88b19dca7..ecf256cbcea 100644\n--- a/unpack-trees.h\n+++ b/unpack-trees.h\n@@ -91,6 +91,8 @@ struct unpack_trees_options {\n \tstruct checkout_metadata meta;\n };\n \n+#define UNPACK_TREES_OPTIONS_INIT { 0 }\n+\n int unpack_trees(unsigned n, struct tree_desc *t,\n \t\t struct unpack_trees_options *options);\n \n-- \n2.33.0.1404.g83021034c5d\n\n"},{"id":"437823","messageId":"patch-05.10-042080b8d6d-20211004T002226Z-avarab@gmail.com","threadId":"56640","inReplyTo":"cover-00.10-00000000000-20211004T002226Z-avarab@gmail.com","subject":"[PATCH 05/10] dir.[ch]: make DIR_INIT mandatory","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-04T00:46:06Z","receivedAt":"2021-10-04T00:46:39Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"The dir_init() initializer has been documented as being mandatory\nsince eceba532141 (dir: fix problematic API to avoid memory leaks,\n2020-08-18), but both it and my ce93a4c6127 (dir.[ch]: replace\ndir_init() with DIR_INIT, 2021-07-01) managed to miss this callsite in\n\"add-interactive.c\" added before those two commits in\nab1e1cccaf6 (built-in add -i: re-implement `add-untracked` in C,\n2019-11-29).\n\nIn addition my change to remove dir_init() neglected to update this\ndocumentation. Let's use \"must be initialized with\" in reference to\n\"DIR_INIT\". We have one lazy initialization which pre-dates\neceba532141 in dir.c. Adjusting this callsite is a prerequisite for\nremoving it in favor of trusting the macro to initialize the \"struct\ndir_struct\" correctly.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n add-interactive.c | 2 +-\n dir.h             | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/add-interactive.c b/add-interactive.c\nindex 6498ae196f1..27daea8d1b3 100644\n--- a/add-interactive.c\n+++ b/add-interactive.c\n@@ -826,7 +826,7 @@ static int get_untracked_files(struct repository *r,\n \t\t\t       struct prefix_item_list *files,\n \t\t\t       const struct pathspec *ps)\n {\n-\tstruct dir_struct dir = { 0 };\n+\tstruct dir_struct dir = DIR_INIT;\n \tsize_t i;\n \tstruct strbuf buf = STRBUF_INIT;\n \ndiff --git a/dir.h b/dir.h\nindex 83f46c0fb4c..ff3b4a7f602 100644\n--- a/dir.h\n+++ b/dir.h\n@@ -19,7 +19,7 @@\n  * CE_SKIP_WORKTREE marked. If you want to exclude files, make sure you have\n  * loaded the index first.\n  *\n- * - Prepare `struct dir_struct dir` using `dir_init()` function.\n+ * - The `struct dir_struct dir` must be initialized with `DIR_INIT`.\n  *\n  * - To add single exclude pattern, call `add_pattern_list()` and then\n  *   `add_pattern()`.\n-- \n2.33.0.1404.g83021034c5d\n\n"},{"id":"437824","messageId":"patch-06.10-2b243d91696-20211004T002226Z-avarab@gmail.com","threadId":"56640","inReplyTo":"cover-00.10-00000000000-20211004T002226Z-avarab@gmail.com","subject":"[PATCH 06/10] dir.c: get rid of lazy initialization","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-04T00:46:07Z","receivedAt":"2021-10-04T00:46:39Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Remove the \"Lazy initialization\" in prep_exclude() left behind by\naceb9429b37 (prep_exclude: remove the artificial PATH_MAX limit,\n2014-07-14).\n\nNow that every caller who sets up a \"struct dir_struct\" is using the\nDIR_INIT macro we can rely on it to have done the initialization. As\nnoted in an analysis of the previous control flow[1] an earlier\npassing of of \"dir->basebuf.buf\" to strncmp() wasn't buggy, as we'd\nonly reach that code on subsequent invocations of prep_exclude(),\ni.e. after this strbuf_init() had been run. But keeping track of that\nmakes for hard-to-read code. Let's just rely on the initialization\ninstead.\n\nThis does change the behavior of this code in that it won't be\npre-growing the strbuf to a size of PATH_MAX. I think that's OK.\n\nThat we were using PATH_MAX at all is just a relic from this being a\nfixed buffer from way back in f87f9497486 (git-ls-files: --exclude\nmechanism updates., 2005-07-24).\n\nPre-allocating PATH_MAX was the opposite of an optimization in this\ncase. I logged all \"basebuf.buf\" values when running the test suite,\nand by far the most common one (around 80%) is \"\", which we now won't\nallocate at all for, and just use the \"strbuf_slopbuf\".\n\nThe second most common one was \"a/\", followed by other common cases of\nshort relative paths. So using the default \"struct strbuf\" growth\npattern is a much better allocation optimization in this case.\n\n1. https://lore.kernel.org/git/87sfxhohsj.fsf@evledraar.gmail.com/\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n dir.c | 8 --------\n dir.h | 4 +++-\n 2 files changed, 3 insertions(+), 9 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex 39fce3bcba7..efc87c2e405 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -1550,14 +1550,6 @@ static void prep_exclude(struct dir_struct *dir,\n \tif (dir->pattern)\n \t\treturn;\n \n-\t/*\n-\t * Lazy initialization. All call sites currently just\n-\t * memset(dir, 0, sizeof(*dir)) before use. Changing all of\n-\t * them seems lots of work for little benefit.\n-\t */\n-\tif (!dir->basebuf.buf)\n-\t\tstrbuf_init(&dir->basebuf, PATH_MAX);\n-\n \t/* Read from the parent directories and push them down. */\n \tcurrent = stk ? stk->baselen : -1;\n \tstrbuf_setlen(&dir->basebuf, current < 0 ? 0 : current);\ndiff --git a/dir.h b/dir.h\nindex ff3b4a7f602..e3757c6099e 100644\n--- a/dir.h\n+++ b/dir.h\n@@ -342,7 +342,9 @@ struct dir_struct {\n \tunsigned visited_directories;\n };\n \n-#define DIR_INIT { 0 }\n+#define DIR_INIT { \\\n+\t.basebuf = STRBUF_INIT, \\\n+}\n \n struct dirent *readdir_skip_dot_and_dotdot(DIR *dirp);\n \n-- \n2.33.0.1404.g83021034c5d\n\n"},{"id":"437825","messageId":"patch-07.10-646e610d8dd-20211004T002226Z-avarab@gmail.com","threadId":"56640","inReplyTo":"cover-00.10-00000000000-20211004T002226Z-avarab@gmail.com","subject":"[PATCH 07/10] unpack-trees API: rename clear_unpack_trees_porcelain()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-04T00:46:08Z","receivedAt":"2021-10-04T00:46:39Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Since a preceding commit we've been using\nclear_unpack_trees_porcelain() to call dir_clear(). So it's no longer\na function that corresponds to setup_unpack_trees_porcelain(), as it\nwas when it was added in 1c41d2805e4 (unpack_trees_options: free\nmessages when done, 2018-05-21).\n\nInstead it's similar to strbuf_release() and other similar generic\n\"free\" functions. Let's rename it to avoid any future confusion on the\ntopic.\n\nLet's also update the API documentation for it to note this, and to\ncover e.g. the code added around update_sparsity() in\n4ee5d50fc39 (sparse-checkout: use improved unpack_trees porcelain\nmessages, 2020-03-27).\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n builtin/checkout.c        | 2 +-\n builtin/sparse-checkout.c | 2 +-\n merge-ort.c               | 2 +-\n merge-recursive.c         | 2 +-\n merge.c                   | 4 ++--\n reset.c                   | 2 +-\n unpack-trees.c            | 2 +-\n unpack-trees.h            | 8 +++++---\n 8 files changed, 13 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex d4b88affba7..482d17676a0 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -757,7 +757,7 @@ static int merge_working_tree(const struct checkout_opts *opts,\n \t\tinit_tree_desc(&trees[1], tree->buffer, tree->size);\n \n \t\tret = unpack_trees(2, trees, &topts);\n-\t\tclear_unpack_trees_porcelain(&topts);\n+\t\tunpack_trees_options_release(&topts);\n \t\tif (ret == -1) {\n \t\t\t/*\n \t\t\t * Unpack couldn't do a trivial merge; either\ndiff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c\nindex 4c3c29fb580..b1221fd01d3 100644\n--- a/builtin/sparse-checkout.c\n+++ b/builtin/sparse-checkout.c\n@@ -219,7 +219,7 @@ static int update_working_directory(struct pattern_list *pl)\n \n \tsetup_unpack_trees_porcelain(&o, \"sparse-checkout\");\n \tresult = update_sparsity(&o);\n-\tclear_unpack_trees_porcelain(&o);\n+\tunpack_trees_options_release(&o);\n \n \tif (result == UPDATE_SPARSITY_WARNINGS)\n \t\t/*\ndiff --git a/merge-ort.c b/merge-ort.c\nindex e526b78b88d..0a5937364c9 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -4054,7 +4054,7 @@ static int checkout(struct merge_options *opt,\n \tinit_tree_desc(&trees[1], next->buffer, next->size);\n \n \tret = unpack_trees(2, trees, &unpack_opts);\n-\tclear_unpack_trees_porcelain(&unpack_opts);\n+\tunpack_trees_options_release(&unpack_opts);\n \treturn ret;\n }\n \ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex a77f66b006c..316cb2ca907 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -441,7 +441,7 @@ static int unpack_trees_start(struct merge_options *opt,\n static void unpack_trees_finish(struct merge_options *opt)\n {\n \tdiscard_index(&opt->priv->orig_index);\n-\tclear_unpack_trees_porcelain(&opt->priv->unpack_opts);\n+\tunpack_trees_options_release(&opt->priv->unpack_opts);\n \tunpack_trees_options_init(&opt->priv->unpack_opts);\n }\n \ndiff --git a/merge.c b/merge.c\nindex 9cb32990dd9..2f618425aff 100644\n--- a/merge.c\n+++ b/merge.c\n@@ -95,10 +95,10 @@ int checkout_fast_forward(struct repository *r,\n \n \tif (unpack_trees(nr_trees, t, &opts)) {\n \t\trollback_lock_file(&lock_file);\n-\t\tclear_unpack_trees_porcelain(&opts);\n+\t\tunpack_trees_options_release(&opts);\n \t\treturn -1;\n \t}\n-\tclear_unpack_trees_porcelain(&opts);\n+\tunpack_trees_options_release(&opts);\n \n \tif (write_locked_index(r->index, &lock_file, COMMIT_LOCK))\n \t\treturn error(_(\"unable to write new index file\"));\ndiff --git a/reset.c b/reset.c\nindex d13984ab781..f4bf3fbfac0 100644\n--- a/reset.c\n+++ b/reset.c\n@@ -133,7 +133,7 @@ int reset_head(struct repository *r, struct object_id *oid, const char *action,\n leave_reset_head:\n \tstrbuf_release(&msg);\n \trollback_lock_file(&lock);\n-\tclear_unpack_trees_porcelain(&unpack_tree_opts);\n+\tunpack_trees_options_release(&unpack_tree_opts);\n \twhile (nr)\n \t\tfree((void *)desc[--nr].buffer);\n \treturn ret;\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex e7365322e82..bea598c9ece 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -193,7 +193,7 @@ void unpack_trees_options_init(struct unpack_trees_options *o)\n \tmemcpy(o, &blank, sizeof(*o));\n }\n \n-void clear_unpack_trees_porcelain(struct unpack_trees_options *opts)\n+void unpack_trees_options_release(struct unpack_trees_options *opts)\n {\n \tstrvec_clear(&opts->msgs_to_free);\n \tdir_clear(&opts->dir);\ndiff --git a/unpack-trees.h b/unpack-trees.h\nindex 40c4841748d..a8d1f083b33 100644\n--- a/unpack-trees.h\n+++ b/unpack-trees.h\n@@ -41,10 +41,12 @@ enum unpack_trees_error_types {\n void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,\n \t\t\t\t  const char *cmd);\n \n-/*\n- * Frees resources allocated by setup_unpack_trees_porcelain().\n+/**\n+ * Frees resources allocated by function that take the \"struct\n+ * unpack_trees_options\". Always call this after using unpack_trees(),\n+ * update_sparsity() etc.\n  */\n-void clear_unpack_trees_porcelain(struct unpack_trees_options *opts);\n+void unpack_trees_options_release(struct unpack_trees_options *opts);\n \n struct unpack_trees_options {\n \tunsigned int reset,\n-- \n2.33.0.1404.g83021034c5d\n\n"},{"id":"437826","messageId":"patch-08.10-32acc5e5c9e-20211004T002226Z-avarab@gmail.com","threadId":"56640","inReplyTo":"cover-00.10-00000000000-20211004T002226Z-avarab@gmail.com","subject":"[PATCH 08/10] unpack-trees: don't leak memory in verify_clean_subdirectory()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-04T00:46:09Z","receivedAt":"2021-10-04T00:47:22Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Fix two different but related memory leaks in\nverify_clean_subdirectory(). We leaked both the \"pathbuf\" if\nread_directory() returned non-zero, and we never cleaned up our own\n\"struct dir_struct\" either.\n\n * \"pathbuf\": When the read_directory() call followed by the\n   free(pathbuf) was added in c81935348be (Fix switching to a branch\n   with D/F when current branch has file D., 2007-03-15) we didn't\n   bother to free() before we called die().\n\n   But when this code was later libified in 203a2fe1170 (Allow callers\n   of unpack_trees() to handle failure, 2008-02-07) we started to leak\n   as we returned data to the caller. This fixes that memory leak,\n   which can be observed under SANITIZE=leak with e.g. the\n   \"t1001-read-tree-m-2way.sh\" test.\n\n * \"struct dir_struct\": We've leaked the dir_struct ever since this\n   code was added back in c81935348be.\n\n   When that commit was written there wasn't an equivalent of\n   dir_clear(). Since it was added in 270be816049 (dir.c: provide\n   clear_directory() for reclaiming dir_struct memory, 2013-01-06)\n   we've omitted freeing the memory allocated here.\n\n   This memory leak could also be observed under SANITIZE=leak and the\n   \"t1001-read-tree-m-2way.sh\" test.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n unpack-trees.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex bea598c9ece..260e7ec5bb4 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -2138,9 +2138,10 @@ static int verify_clean_subdirectory(const struct cache_entry *ce,\n \n \td.exclude_per_dir = o->dir.exclude_per_dir;\n \ti = read_directory(&d, o->src_index, pathbuf, namelen+1, NULL);\n+\tdir_clear(&d);\n+\tfree(pathbuf);\n \tif (i)\n \t\treturn add_rejected_path(o, ERROR_NOT_UPTODATE_DIR, ce->name);\n-\tfree(pathbuf);\n \treturn cnt;\n }\n \n-- \n2.33.0.1404.g83021034c5d\n\n"},{"id":"437827","messageId":"patch-10.10-06ad6baf566-20211004T002226Z-avarab@gmail.com","threadId":"56640","inReplyTo":"cover-00.10-00000000000-20211004T002226Z-avarab@gmail.com","subject":"[PATCH 10/10] built-ins: plug memory leaks with unpack_trees_options_release()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-04T00:46:11Z","receivedAt":"2021-10-04T00:47:22Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Plug memory leaks in various built-ins that were missing\nunpack_trees_options_release() calls. In an earlier commit these\nfunctions were all made to use the \"UNPACK_TREES_OPTIONS_INIT\" macro,\nnow let's have the ones that didn't clean up their memory do so.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n archive.c           |  9 +++++++--\n builtin/am.c        | 17 ++++++++++++-----\n builtin/checkout.c  |  9 +++++++--\n builtin/clone.c     |  1 +\n builtin/commit.c    |  6 +++++-\n builtin/merge.c     |  6 ++++--\n builtin/read-tree.c | 14 ++++++++++----\n builtin/reset.c     | 13 +++++++++----\n builtin/stash.c     | 14 ++++++++++----\n diff-lib.c          |  5 ++++-\n 10 files changed, 69 insertions(+), 25 deletions(-)\n\ndiff --git a/archive.c b/archive.c\nindex 210d7235c5a..003db7d355d 100644\n--- a/archive.c\n+++ b/archive.c\n@@ -306,8 +306,10 @@ int write_archive_entries(struct archiver_args *args,\n \t\topts.dst_index = args->repo->index;\n \t\topts.fn = oneway_merge;\n \t\tinit_tree_desc(&t, args->tree->buffer, args->tree->size);\n-\t\tif (unpack_trees(1, &t, &opts))\n-\t\t\treturn -1;\n+\t\tif (unpack_trees(1, &t, &opts)) {\n+\t\t\terr = -1;\n+\t\t\tgoto cleanup;\n+\t\t}\n \t\tgit_attr_set_direction(GIT_ATTR_INDEX);\n \t}\n \n@@ -346,8 +348,11 @@ int write_archive_entries(struct archiver_args *args,\n \t\tif (err)\n \t\t\tbreak;\n \t}\n+\n+cleanup:\n \tstrbuf_release(&path_in_archive);\n \tstrbuf_release(&content);\n+\tunpack_trees_options_release(&opts);\n \n \treturn err;\n }\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 82641ce68ec..4d4bb473c0f 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -1903,6 +1903,7 @@ static int fast_forward_to(struct tree *head, struct tree *remote, int reset)\n \tstruct lock_file lock_file = LOCK_INIT;\n \tstruct unpack_trees_options opts = UNPACK_TREES_OPTIONS_INIT;\n \tstruct tree_desc t[2];\n+\tint ret = 0;\n \n \tif (parse_tree(head) || parse_tree(remote))\n \t\treturn -1;\n@@ -1923,13 +1924,15 @@ static int fast_forward_to(struct tree *head, struct tree *remote, int reset)\n \n \tif (unpack_trees(2, t, &opts)) {\n \t\trollback_lock_file(&lock_file);\n-\t\treturn -1;\n+\t\tret = -1;\n+\t\tgoto cleanup;\n \t}\n \n \tif (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n \t\tdie(_(\"unable to write new index file\"));\n-\n-\treturn 0;\n+cleanup:\n+\tunpack_trees_options_release(&opts);\n+\treturn ret;\n }\n \n /**\n@@ -1941,6 +1944,7 @@ static int merge_tree(struct tree *tree)\n \tstruct lock_file lock_file = LOCK_INIT;\n \tstruct unpack_trees_options opts = UNPACK_TREES_OPTIONS_INIT;\n \tstruct tree_desc t[1];\n+\tint ret = 0;\n \n \tif (parse_tree(tree))\n \t\treturn -1;\n@@ -1956,13 +1960,16 @@ static int merge_tree(struct tree *tree)\n \n \tif (unpack_trees(1, t, &opts)) {\n \t\trollback_lock_file(&lock_file);\n-\t\treturn -1;\n+\t\tret = -1;\n+\t\tgoto cleanup;\n \t}\n \n \tif (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n \t\tdie(_(\"unable to write new index file\"));\n \n-\treturn 0;\n+cleanup:\n+\tunpack_trees_options_release(&opts);\n+\treturn ret;\n }\n \n /**\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 482d17676a0..fd76b504861 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -641,6 +641,7 @@ static int reset_tree(struct tree *tree, const struct checkout_opts *o,\n {\n \tstruct unpack_trees_options opts = UNPACK_TREES_OPTIONS_INIT;\n \tstruct tree_desc tree_desc;\n+\tint ret;\n \n \topts.head_idx = -1;\n \topts.update = worktree;\n@@ -667,10 +668,14 @@ static int reset_tree(struct tree *tree, const struct checkout_opts *o,\n \t\t */\n \t\t/* fallthrough */\n \tcase 0:\n-\t\treturn 0;\n+\t\tret = 0;\n+\t\tbreak;\n \tdefault:\n-\t\treturn 128;\n+\t\tret = 128;\n \t}\n+\n+\tunpack_trees_options_release(&opts);\n+\treturn ret;\n }\n \n static void setup_branch_path(struct branch_info *branch)\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 0df820c5970..df3bb9a7884 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -697,6 +697,7 @@ static int checkout(int submodule_progress)\n \tinit_tree_desc(&t, tree->buffer, tree->size);\n \tif (unpack_trees(1, &t, &opts) < 0)\n \t\tdie(_(\"unable to checkout working tree\"));\n+\tunpack_trees_options_release(&opts);\n \n \tfree(head);\n \ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 6cc7313bad8..84c79ecb5a5 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -305,6 +305,7 @@ static void create_base_index(const struct commit *current_head)\n \tstruct tree *tree;\n \tstruct unpack_trees_options opts = UNPACK_TREES_OPTIONS_INIT;\n \tstruct tree_desc t;\n+\tint exit_early = 0;\n \n \tif (!current_head) {\n \t\tdiscard_cache();\n@@ -324,7 +325,10 @@ static void create_base_index(const struct commit *current_head)\n \tparse_tree(tree);\n \tinit_tree_desc(&t, tree->buffer, tree->size);\n \tif (unpack_trees(1, &t, &opts))\n-\t\texit(128); /* We've already reported the error, finish dying */\n+\t\texit_early = 1; /* We've already reported the error, finish dying */\n+\tunpack_trees_options_release(&opts);\n+\tif (exit_early)\n+\t\texit(128);\n }\n \n static void refresh_cache_or_die(int refresh_flags)\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 73290a07fcc..28089e2c5ed 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -671,6 +671,7 @@ static int read_tree_trivial(struct object_id *common, struct object_id *head,\n \tstruct tree *trees[MAX_UNPACK_TREES];\n \tstruct tree_desc t[MAX_UNPACK_TREES];\n \tstruct unpack_trees_options opts = UNPACK_TREES_OPTIONS_INIT;\n+\tint ret = 0;\n \n \topts.head_idx = 2;\n \topts.src_index = &the_index;\n@@ -695,8 +696,9 @@ static int read_tree_trivial(struct object_id *common, struct object_id *head,\n \t\tinit_tree_desc(t+i, trees[i]->buffer, trees[i]->size);\n \t}\n \tif (unpack_trees(nr_trees, t, &opts))\n-\t\treturn -1;\n-\treturn 0;\n+\t\tret = -1;\n+\tunpack_trees_options_release(&opts);\n+\treturn ret;\n }\n \n static void write_tree_trivial(struct object_id *oid)\ndiff --git a/builtin/read-tree.c b/builtin/read-tree.c\nindex 06f3b97ac05..8f1b8a7e74c 100644\n--- a/builtin/read-tree.c\n+++ b/builtin/read-tree.c\n@@ -154,6 +154,7 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)\n \t\tOPT__QUIET(&opts.quiet, N_(\"suppress feedback messages\")),\n \t\tOPT_END()\n \t};\n+\tint ret = 0;\n \n \topts.head_idx = -1;\n \topts.src_index = &the_index;\n@@ -243,11 +244,13 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)\n \t\tparse_tree(tree);\n \t\tinit_tree_desc(t+i, tree->buffer, tree->size);\n \t}\n-\tif (unpack_trees(nr_trees, t, &opts))\n-\t\treturn 128;\n+\tif (unpack_trees(nr_trees, t, &opts)) {\n+\t\tret = 128;\n+\t\tgoto cleanup;\n+\t}\n \n \tif (opts.debug_unpack || opts.dry_run)\n-\t\treturn 0; /* do not write the index out */\n+\t\tgoto cleanup; /* do not write the index out */\n \n \t/*\n \t * When reading only one tree (either the most basic form,\n@@ -262,5 +265,8 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)\n \n \tif (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n \t\tdie(\"unable to write new index file\");\n-\treturn 0;\n+\n+cleanup:\n+\tunpack_trees_options_release(&opts);\n+\treturn ret;\n }\ndiff --git a/builtin/reset.c b/builtin/reset.c\nindex 86c604b21e9..713d084c3eb 100644\n--- a/builtin/reset.c\n+++ b/builtin/reset.c\n@@ -78,10 +78,14 @@ static int reset_index(const char *ref, const struct object_id *oid, int reset_t\n \n \tif (reset_type == KEEP) {\n \t\tstruct object_id head_oid;\n-\t\tif (get_oid(\"HEAD\", &head_oid))\n-\t\t\treturn error(_(\"You do not have a valid HEAD.\"));\n-\t\tif (!fill_tree_descriptor(the_repository, desc + nr, &head_oid))\n-\t\t\treturn error(_(\"Failed to find tree of HEAD.\"));\n+\t\tif (get_oid(\"HEAD\", &head_oid)) {\n+\t\t\terror(_(\"You do not have a valid HEAD.\"));\n+\t\t\tgoto out;\n+\t\t}\n+\t\tif (!fill_tree_descriptor(the_repository, desc + nr, &head_oid)) {\n+\t\t\terror(_(\"Failed to find tree of HEAD.\"));\n+\t\t\tgoto out;\n+\t\t}\n \t\tnr++;\n \t\topts.fn = twoway_merge;\n \t}\n@@ -103,6 +107,7 @@ static int reset_index(const char *ref, const struct object_id *oid, int reset_t\n \tret = 0;\n \n out:\n+\tunpack_trees_options_release(&opts);\n \tfor (i = 0; i < nr; i++)\n \t\tfree((void *)desc[i].buffer);\n \treturn ret;\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex 1137e5fcbe8..be6ecb1ae11 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -237,6 +237,7 @@ static int reset_tree(struct object_id *i_tree, int update, int reset)\n \tstruct tree_desc t[MAX_UNPACK_TREES];\n \tstruct tree *tree;\n \tstruct lock_file lock_file = LOCK_INIT;\n+\tint ret = 0;\n \n \tread_cache_preload(NULL);\n \tif (refresh_cache(REFRESH_QUIET))\n@@ -258,13 +259,17 @@ static int reset_tree(struct object_id *i_tree, int update, int reset)\n \topts.update = update;\n \topts.fn = oneway_merge;\n \n-\tif (unpack_trees(nr_trees, t, &opts))\n-\t\treturn -1;\n+\tif (unpack_trees(nr_trees, t, &opts)) {\n+\t\tret = -1;\n+\t\tgoto cleanup;\n+\t}\n \n \tif (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n-\t\treturn error(_(\"unable to write new index file\"));\n+\t\tret = error(_(\"unable to write new index file\"));\n \n-\treturn 0;\n+cleanup:\n+\tunpack_trees_options_release(&opts);\n+\treturn ret;\n }\n \n static int diff_tree_binary(struct strbuf *out, struct object_id *w_commit)\n@@ -815,6 +820,7 @@ static void diff_include_untracked(const struct stash_info *info, struct diff_op\n \n \tif (unpack_trees(ARRAY_SIZE(tree_desc), tree_desc, &unpack_tree_opt))\n \t\tdie(_(\"failed to unpack trees\"));\n+\tunpack_trees_options_release(&unpack_tree_opt);\n \n \tdo_diff_cache(&info->b_commit, diff_opt);\n }\ndiff --git a/diff-lib.c b/diff-lib.c\nindex 8a08d9af4eb..2d8a90a51b2 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -527,6 +527,7 @@ static int diff_cache(struct rev_info *revs,\n \tstruct tree *tree;\n \tstruct tree_desc t;\n \tstruct unpack_trees_options opts = UNPACK_TREES_OPTIONS_INIT;\n+\tint ret;\n \n \ttree = parse_tree_indirect(tree_oid);\n \tif (!tree)\n@@ -545,7 +546,9 @@ static int diff_cache(struct rev_info *revs,\n \topts.pathspec->recursive = 1;\n \n \tinit_tree_desc(&t, tree->buffer, tree->size);\n-\treturn unpack_trees(1, &t, &opts);\n+\tret = unpack_trees(1, &t, &opts);\n+\tunpack_trees_options_release(&opts);\n+\treturn ret;\n }\n \n void diff_get_merge_base(const struct rev_info *revs, struct object_id *mb)\n-- \n2.33.0.1404.g83021034c5d\n\n"},{"id":"437865","messageId":"f24733e9-8742-33b6-5fae-93d89698cc00@gmail.com","threadId":"56640","inReplyTo":"patch-04.10-9d62edd35bf-20211004T002226Z-avarab@gmail.com","subject":"Re: [PATCH 04/10] unpack-trees API: don't have clear_unpack_trees_porcelain() reset","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2021-10-04T09:31:48Z","receivedAt":"2021-10-04T09:31:54Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Ævar\n\nOn 04/10/2021 01:46, Ævar Arnfjörð Bjarmason wrote:\n> Change the clear_unpack_trees_porcelain() to be like a *_release()\n> function, not a *_reset() (in strbuf.c terms). Let's move the only API\n> user that relied on the latter to doing its own\n> unpack_trees_options_init(). See the commit that introduced\n> unpack_trees_options_init() for details on the control flow involved\n> here.\n\nBefore this change if there was a call to unpack_trees() after \nclear_unpack_trees_porcelain() then that caller would get the default \nerror messages. After this change we end up with a use-after-free error \nin that situation. I found the subject line of this patch hard to \nunderstand, the commit message explains what it is doing but I'm still \nnot sure what the motivation for this change is.\n\nBest Wishes\n\nPhillip\n\n> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n> ---\n>   merge-recursive.c | 1 +\n>   unpack-trees.c    | 1 -\n>   2 files changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/merge-recursive.c b/merge-recursive.c\n> index d24a4903f1d..a77f66b006c 100644\n> --- a/merge-recursive.c\n> +++ b/merge-recursive.c\n> @@ -442,6 +442,7 @@ static void unpack_trees_finish(struct merge_options *opt)\n>   {\n>   \tdiscard_index(&opt->priv->orig_index);\n>   \tclear_unpack_trees_porcelain(&opt->priv->unpack_opts);\n> +\tunpack_trees_options_init(&opt->priv->unpack_opts);\n>   }\n>   \n>   static int save_files_dirs(const struct object_id *oid,\n> diff --git a/unpack-trees.c b/unpack-trees.c\n> index 94767d3f96f..e7365322e82 100644\n> --- a/unpack-trees.c\n> +++ b/unpack-trees.c\n> @@ -197,7 +197,6 @@ void clear_unpack_trees_porcelain(struct unpack_trees_options *opts)\n>   {\n>   \tstrvec_clear(&opts->msgs_to_free);\n>   \tdir_clear(&opts->dir);\n> -\tmemset(opts->msgs, 0, sizeof(opts->msgs));\n>   }\n>   \n>   static int do_add_entry(struct unpack_trees_options *o, struct cache_entry *ce,\n> \n"},{"id":"437866","messageId":"ebc72146-4431-c29c-ebab-37cf886cf28f@gmail.com","threadId":"56640","inReplyTo":"patch-07.10-646e610d8dd-20211004T002226Z-avarab@gmail.com","subject":"Re: [PATCH 07/10] unpack-trees API: rename clear_unpack_trees_porcelain()","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2021-10-04T09:38:40Z","receivedAt":"2021-10-04T09:38:46Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Ævar\n\nOn 04/10/2021 01:46, Ævar Arnfjörð Bjarmason wrote:\n> Since a preceding commit we've been using\n> clear_unpack_trees_porcelain() to call dir_clear(). So it's no longer\n> a function that corresponds to setup_unpack_trees_porcelain(), as it\n> was when it was added in 1c41d2805e4 (unpack_trees_options: free\n> messages when done, 2018-05-21).\n> \n> Instead it's similar to strbuf_release() and other similar generic\n> \"free\" functions. Let's rename it to avoid any future confusion on the\n> topic.\n> \n> Let's also update the API documentation for it to note this, and to\n> cover e.g. the code added around update_sparsity() in\n> 4ee5d50fc39 (sparse-checkout: use improved unpack_trees porcelain\n> messages, 2020-03-27).\n> \n> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n> ---\n>   builtin/checkout.c        | 2 +-\n>   builtin/sparse-checkout.c | 2 +-\n>   merge-ort.c               | 2 +-\n>   merge-recursive.c         | 2 +-\n>   merge.c                   | 4 ++--\n>   reset.c                   | 2 +-\n\nI was expecting to see sequencer.c here, but it appears it does not call \nclear_unpack_trees_porcelain() to free the memory allocated with \nsetup_unpack_trees_porcelain(). If you're interested in fixing memory \nleaks from unpack trees it might be worth checking that all the calls to \nsetup_unpack_trees_porcelain() have a matching call to \nclear_unpack_trees_porcelain().\n\nBest Wishes\n\nPhillip\n\n>   unpack-trees.c            | 2 +-\n>   unpack-trees.h            | 8 +++++---\n>   8 files changed, 13 insertions(+), 11 deletions(-)\n> \n> diff --git a/builtin/checkout.c b/builtin/checkout.c\n> index d4b88affba7..482d17676a0 100644\n> --- a/builtin/checkout.c\n> +++ b/builtin/checkout.c\n> @@ -757,7 +757,7 @@ static int merge_working_tree(const struct checkout_opts *opts,\n>   \t\tinit_tree_desc(&trees[1], tree->buffer, tree->size);\n>   \n>   \t\tret = unpack_trees(2, trees, &topts);\n> -\t\tclear_unpack_trees_porcelain(&topts);\n> +\t\tunpack_trees_options_release(&topts);\n>   \t\tif (ret == -1) {\n>   \t\t\t/*\n>   \t\t\t * Unpack couldn't do a trivial merge; either\n> diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c\n> index 4c3c29fb580..b1221fd01d3 100644\n> --- a/builtin/sparse-checkout.c\n> +++ b/builtin/sparse-checkout.c\n> @@ -219,7 +219,7 @@ static int update_working_directory(struct pattern_list *pl)\n>   \n>   \tsetup_unpack_trees_porcelain(&o, \"sparse-checkout\");\n>   \tresult = update_sparsity(&o);\n> -\tclear_unpack_trees_porcelain(&o);\n> +\tunpack_trees_options_release(&o);\n>   \n>   \tif (result == UPDATE_SPARSITY_WARNINGS)\n>   \t\t/*\n> diff --git a/merge-ort.c b/merge-ort.c\n> index e526b78b88d..0a5937364c9 100644\n> --- a/merge-ort.c\n> +++ b/merge-ort.c\n> @@ -4054,7 +4054,7 @@ static int checkout(struct merge_options *opt,\n>   \tinit_tree_desc(&trees[1], next->buffer, next->size);\n>   \n>   \tret = unpack_trees(2, trees, &unpack_opts);\n> -\tclear_unpack_trees_porcelain(&unpack_opts);\n> +\tunpack_trees_options_release(&unpack_opts);\n>   \treturn ret;\n>   }\n>   \n> diff --git a/merge-recursive.c b/merge-recursive.c\n> index a77f66b006c..316cb2ca907 100644\n> --- a/merge-recursive.c\n> +++ b/merge-recursive.c\n> @@ -441,7 +441,7 @@ static int unpack_trees_start(struct merge_options *opt,\n>   static void unpack_trees_finish(struct merge_options *opt)\n>   {\n>   \tdiscard_index(&opt->priv->orig_index);\n> -\tclear_unpack_trees_porcelain(&opt->priv->unpack_opts);\n> +\tunpack_trees_options_release(&opt->priv->unpack_opts);\n>   \tunpack_trees_options_init(&opt->priv->unpack_opts);\n>   }\n>   \n> diff --git a/merge.c b/merge.c\n> index 9cb32990dd9..2f618425aff 100644\n> --- a/merge.c\n> +++ b/merge.c\n> @@ -95,10 +95,10 @@ int checkout_fast_forward(struct repository *r,\n>   \n>   \tif (unpack_trees(nr_trees, t, &opts)) {\n>   \t\trollback_lock_file(&lock_file);\n> -\t\tclear_unpack_trees_porcelain(&opts);\n> +\t\tunpack_trees_options_release(&opts);\n>   \t\treturn -1;\n>   \t}\n> -\tclear_unpack_trees_porcelain(&opts);\n> +\tunpack_trees_options_release(&opts);\n>   \n>   \tif (write_locked_index(r->index, &lock_file, COMMIT_LOCK))\n>   \t\treturn error(_(\"unable to write new index file\"));\n> diff --git a/reset.c b/reset.c\n> index d13984ab781..f4bf3fbfac0 100644\n> --- a/reset.c\n> +++ b/reset.c\n> @@ -133,7 +133,7 @@ int reset_head(struct repository *r, struct object_id *oid, const char *action,\n>   leave_reset_head:\n>   \tstrbuf_release(&msg);\n>   \trollback_lock_file(&lock);\n> -\tclear_unpack_trees_porcelain(&unpack_tree_opts);\n> +\tunpack_trees_options_release(&unpack_tree_opts);\n>   \twhile (nr)\n>   \t\tfree((void *)desc[--nr].buffer);\n>   \treturn ret;\n> diff --git a/unpack-trees.c b/unpack-trees.c\n> index e7365322e82..bea598c9ece 100644\n> --- a/unpack-trees.c\n> +++ b/unpack-trees.c\n> @@ -193,7 +193,7 @@ void unpack_trees_options_init(struct unpack_trees_options *o)\n>   \tmemcpy(o, &blank, sizeof(*o));\n>   }\n>   \n> -void clear_unpack_trees_porcelain(struct unpack_trees_options *opts)\n> +void unpack_trees_options_release(struct unpack_trees_options *opts)\n>   {\n>   \tstrvec_clear(&opts->msgs_to_free);\n>   \tdir_clear(&opts->dir);\n> diff --git a/unpack-trees.h b/unpack-trees.h\n> index 40c4841748d..a8d1f083b33 100644\n> --- a/unpack-trees.h\n> +++ b/unpack-trees.h\n> @@ -41,10 +41,12 @@ enum unpack_trees_error_types {\n>   void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,\n>   \t\t\t\t  const char *cmd);\n>   \n> -/*\n> - * Frees resources allocated by setup_unpack_trees_porcelain().\n> +/**\n> + * Frees resources allocated by function that take the \"struct\n> + * unpack_trees_options\". Always call this after using unpack_trees(),\n> + * update_sparsity() etc.\n>    */\n> -void clear_unpack_trees_porcelain(struct unpack_trees_options *opts);\n> +void unpack_trees_options_release(struct unpack_trees_options *opts);\n>   \n>   struct unpack_trees_options {\n>   \tunsigned int reset,\n> \n"},{"id":"437874","messageId":"87fsthnix1.fsf@evledraar.gmail.com","threadId":"56640","inReplyTo":"ebc72146-4431-c29c-ebab-37cf886cf28f@gmail.com","subject":"Re: [PATCH 07/10] unpack-trees API: rename clear_unpack_trees_porcelain()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-04T11:10:37Z","receivedAt":"2021-10-04T11:11:58Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Oct 04 2021, Phillip Wood wrote:\n\n> Hi Ævar\n>\n> On 04/10/2021 01:46, Ævar Arnfjörð Bjarmason wrote:\n>> Since a preceding commit we've been using\n>> clear_unpack_trees_porcelain() to call dir_clear(). So it's no longer\n>> a function that corresponds to setup_unpack_trees_porcelain(), as it\n>> was when it was added in 1c41d2805e4 (unpack_trees_options: free\n>> messages when done, 2018-05-21).\n>> Instead it's similar to strbuf_release() and other similar generic\n>> \"free\" functions. Let's rename it to avoid any future confusion on the\n>> topic.\n>> Let's also update the API documentation for it to note this, and to\n>> cover e.g. the code added around update_sparsity() in\n>> 4ee5d50fc39 (sparse-checkout: use improved unpack_trees porcelain\n>> messages, 2020-03-27).\n>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n>> ---\n>>   builtin/checkout.c        | 2 +-\n>>   builtin/sparse-checkout.c | 2 +-\n>>   merge-ort.c               | 2 +-\n>>   merge-recursive.c         | 2 +-\n>>   merge.c                   | 4 ++--\n>>   reset.c                   | 2 +-\n>\n> I was expecting to see sequencer.c here, but it appears it does not\n> call clear_unpack_trees_porcelain() to free the memory allocated with \n> setup_unpack_trees_porcelain(). If you're interested in fixing memory\n> leaks from unpack trees it might be worth checking that all the calls\n> to setup_unpack_trees_porcelain() have a matching call to \n> clear_unpack_trees_porcelain().\n\nWell spotted, I thought I was doing that, but see that I missed this one\nspecial-case, will fix.\n\nWith this series grepping for \"UNPACK_TREES_OPTIONS_INIT\" is better (and\nknowing about the one special-case of merge-recursive.c).\n"},{"id":"437875","messageId":"87bl45niqs.fsf@evledraar.gmail.com","threadId":"56640","inReplyTo":"f24733e9-8742-33b6-5fae-93d89698cc00@gmail.com","subject":"Re: [PATCH 04/10] unpack-trees API: don't have clear_unpack_trees_porcelain() reset","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-04T11:12:16Z","receivedAt":"2021-10-04T11:15:44Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Oct 04 2021, Phillip Wood wrote:\n\n> Hi Ævar\n>\n> On 04/10/2021 01:46, Ævar Arnfjörð Bjarmason wrote:\n>> Change the clear_unpack_trees_porcelain() to be like a *_release()\n>> function, not a *_reset() (in strbuf.c terms). Let's move the only API\n>> user that relied on the latter to doing its own\n>> unpack_trees_options_init(). See the commit that introduced\n>> unpack_trees_options_init() for details on the control flow involved\n>> here.\n>\n> Before this change if there was a call to unpack_trees() after\n> clear_unpack_trees_porcelain() then that caller would get the default \n> error messages. After this change we end up with a use-after-free\n> error in that situation. I found the subject line of this patch hard\n> to understand, the commit message explains what it is doing but I'm\n> still not sure what the motivation for this change is.\n\nI'll work on the commit message part.\n\nWith this series such a caller is purely hypothetical, isn't it?\nI.e. the journey in 02/10 & 04/10, and later in the 07/10 you commented\non is to make the API behave similarly to e.g. strbuf, where there's a\nrelease() that leaves it in such a state, different from a \"reset\".\n\nPerhaps this step in isolation is confusing at it leaves the function\nnamed as clear_unpack_trees_porcelain(). I had this all in one change\ninitially, but figured having such a large rename diff & one behavior\nchange was worse.\n\nWe could just leave the \"reset\" semantics in place for everyone, but\njust like \"strbuf_release()\" et al I think it's good for\nself-documentation purposes to explicitly make clear if you're re-using\nthe struct, or just freeing it at the end.\n\nFor this API only one user of the API cares about the re-use case,\nmerge-recursive.c.\n\n>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n>> ---\n>>   merge-recursive.c | 1 +\n>>   unpack-trees.c    | 1 -\n>>   2 files changed, 1 insertion(+), 1 deletion(-)\n>> diff --git a/merge-recursive.c b/merge-recursive.c\n>> index d24a4903f1d..a77f66b006c 100644\n>> --- a/merge-recursive.c\n>> +++ b/merge-recursive.c\n>> @@ -442,6 +442,7 @@ static void unpack_trees_finish(struct merge_options *opt)\n>>   {\n>>   \tdiscard_index(&opt->priv->orig_index);\n>>   \tclear_unpack_trees_porcelain(&opt->priv->unpack_opts);\n>> +\tunpack_trees_options_init(&opt->priv->unpack_opts);\n>>   }\n>>     static int save_files_dirs(const struct object_id *oid,\n>> diff --git a/unpack-trees.c b/unpack-trees.c\n>> index 94767d3f96f..e7365322e82 100644\n>> --- a/unpack-trees.c\n>> +++ b/unpack-trees.c\n>> @@ -197,7 +197,6 @@ void clear_unpack_trees_porcelain(struct unpack_trees_options *opts)\n>>   {\n>>   \tstrvec_clear(&opts->msgs_to_free);\n>>   \tdir_clear(&opts->dir);\n>> -\tmemset(opts->msgs, 0, sizeof(opts->msgs));\n>>   }\n>>     static int do_add_entry(struct unpack_trees_options *o, struct\n>> cache_entry *ce,\n>> \n\n"},{"id":"437880","messageId":"CABPp-BE=gnjQ1=Z1KwM5+QqMDMTgJ7aPd=cv1VTy3GkpOL+vPQ@mail.gmail.com","threadId":"56640","inReplyTo":"patch-08.10-32acc5e5c9e-20211004T002226Z-avarab@gmail.com","subject":"Re: [PATCH 08/10] unpack-trees: don't leak memory in verify_clean_subdirectory()","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-10-04T13:45:00Z","receivedAt":"2021-10-04T13:54:46Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Sun, Oct 3, 2021 at 5:46 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:\n>\n> Fix two different but related memory leaks in\n> verify_clean_subdirectory(). We leaked both the \"pathbuf\" if\n> read_directory() returned non-zero, and we never cleaned up our own\n> \"struct dir_struct\" either.\n>\n>  * \"pathbuf\": When the read_directory() call followed by the\n>    free(pathbuf) was added in c81935348be (Fix switching to a branch\n>    with D/F when current branch has file D., 2007-03-15) we didn't\n>    bother to free() before we called die().\n>\n>    But when this code was later libified in 203a2fe1170 (Allow callers\n>    of unpack_trees() to handle failure, 2008-02-07) we started to leak\n>    as we returned data to the caller. This fixes that memory leak,\n>    which can be observed under SANITIZE=leak with e.g. the\n>    \"t1001-read-tree-m-2way.sh\" test.\n>\n>  * \"struct dir_struct\": We've leaked the dir_struct ever since this\n>    code was added back in c81935348be.\n>\n>    When that commit was written there wasn't an equivalent of\n>    dir_clear(). Since it was added in 270be816049 (dir.c: provide\n>    clear_directory() for reclaiming dir_struct memory, 2013-01-06)\n>    we've omitted freeing the memory allocated here.\n>\n>    This memory leak could also be observed under SANITIZE=leak and the\n>    \"t1001-read-tree-m-2way.sh\" test.\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n> ---\n>  unpack-trees.c | 3 ++-\n>  1 file changed, 2 insertions(+), 1 deletion(-)\n>\n> diff --git a/unpack-trees.c b/unpack-trees.c\n> index bea598c9ece..260e7ec5bb4 100644\n> --- a/unpack-trees.c\n> +++ b/unpack-trees.c\n> @@ -2138,9 +2138,10 @@ static int verify_clean_subdirectory(const struct cache_entry *ce,\n>\n>         d.exclude_per_dir = o->dir.exclude_per_dir;\n>         i = read_directory(&d, o->src_index, pathbuf, namelen+1, NULL);\n> +       dir_clear(&d);\n> +       free(pathbuf);\n>         if (i)\n>                 return add_rejected_path(o, ERROR_NOT_UPTODATE_DIR, ce->name);\n> -       free(pathbuf);\n>         return cnt;\n>  }\n>\n> --\n> 2.33.0.1404.g83021034c5d\n\nGood catches, and nicely explained.\n"},{"id":"437881","messageId":"CABPp-BFr7R_swdxswmoGRF3=uu0s8HgppfrnFEQ+NvS1oh+_og@mail.gmail.com","threadId":"56640","inReplyTo":"patch-10.10-06ad6baf566-20211004T002226Z-avarab@gmail.com","subject":"Re: [PATCH 10/10] built-ins: plug memory leaks with unpack_trees_options_release()","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-10-04T13:45:00Z","receivedAt":"2021-10-04T13:54:48Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Sun, Oct 3, 2021 at 5:46 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:\n>\n> Plug memory leaks in various built-ins that were missing\n> unpack_trees_options_release() calls. In an earlier commit these\n> functions were all made to use the \"UNPACK_TREES_OPTIONS_INIT\" macro,\n> now let's have the ones that didn't clean up their memory do so.\n\nThis commit message doesn't say anything about what was leaking.  You\nshould really bring up that it was the dir_clear() you added to\nunpack_trees_options_release() earlier in the series that is important\nhere (or at least that's what I presume is the important fix).\n\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n> ---\n>  archive.c           |  9 +++++++--\n>  builtin/am.c        | 17 ++++++++++++-----\n>  builtin/checkout.c  |  9 +++++++--\n>  builtin/clone.c     |  1 +\n>  builtin/commit.c    |  6 +++++-\n>  builtin/merge.c     |  6 ++++--\n>  builtin/read-tree.c | 14 ++++++++++----\n>  builtin/reset.c     | 13 +++++++++----\n>  builtin/stash.c     | 14 ++++++++++----\n>  diff-lib.c          |  5 ++++-\n>  10 files changed, 69 insertions(+), 25 deletions(-)\n>\n> diff --git a/archive.c b/archive.c\n> index 210d7235c5a..003db7d355d 100644\n> --- a/archive.c\n> +++ b/archive.c\n> @@ -306,8 +306,10 @@ int write_archive_entries(struct archiver_args *args,\n>                 opts.dst_index = args->repo->index;\n>                 opts.fn = oneway_merge;\n>                 init_tree_desc(&t, args->tree->buffer, args->tree->size);\n> -               if (unpack_trees(1, &t, &opts))\n> -                       return -1;\n> +               if (unpack_trees(1, &t, &opts)) {\n> +                       err = -1;\n> +                       goto cleanup;\n> +               }\n>                 git_attr_set_direction(GIT_ATTR_INDEX);\n>         }\n>\n> @@ -346,8 +348,11 @@ int write_archive_entries(struct archiver_args *args,\n>                 if (err)\n>                         break;\n>         }\n> +\n> +cleanup:\n>         strbuf_release(&path_in_archive);\n>         strbuf_release(&content);\n> +       unpack_trees_options_release(&opts);\n>\n>         return err;\n>  }\n> diff --git a/builtin/am.c b/builtin/am.c\n> index 82641ce68ec..4d4bb473c0f 100644\n> --- a/builtin/am.c\n> +++ b/builtin/am.c\n> @@ -1903,6 +1903,7 @@ static int fast_forward_to(struct tree *head, struct tree *remote, int reset)\n>         struct lock_file lock_file = LOCK_INIT;\n>         struct unpack_trees_options opts = UNPACK_TREES_OPTIONS_INIT;\n>         struct tree_desc t[2];\n> +       int ret = 0;\n>\n>         if (parse_tree(head) || parse_tree(remote))\n>                 return -1;\n> @@ -1923,13 +1924,15 @@ static int fast_forward_to(struct tree *head, struct tree *remote, int reset)\n>\n>         if (unpack_trees(2, t, &opts)) {\n>                 rollback_lock_file(&lock_file);\n> -               return -1;\n> +               ret = -1;\n> +               goto cleanup;\n>         }\n>\n>         if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n>                 die(_(\"unable to write new index file\"));\n> -\n> -       return 0;\n> +cleanup:\n> +       unpack_trees_options_release(&opts);\n> +       return ret;\n>  }\n>\n>  /**\n> @@ -1941,6 +1944,7 @@ static int merge_tree(struct tree *tree)\n>         struct lock_file lock_file = LOCK_INIT;\n>         struct unpack_trees_options opts = UNPACK_TREES_OPTIONS_INIT;\n>         struct tree_desc t[1];\n> +       int ret = 0;\n>\n>         if (parse_tree(tree))\n>                 return -1;\n> @@ -1956,13 +1960,16 @@ static int merge_tree(struct tree *tree)\n>\n>         if (unpack_trees(1, t, &opts)) {\n>                 rollback_lock_file(&lock_file);\n> -               return -1;\n> +               ret = -1;\n> +               goto cleanup;\n>         }\n>\n>         if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n>                 die(_(\"unable to write new index file\"));\n>\n> -       return 0;\n> +cleanup:\n> +       unpack_trees_options_release(&opts);\n> +       return ret;\n>  }\n>\n>  /**\n> diff --git a/builtin/checkout.c b/builtin/checkout.c\n> index 482d17676a0..fd76b504861 100644\n> --- a/builtin/checkout.c\n> +++ b/builtin/checkout.c\n> @@ -641,6 +641,7 @@ static int reset_tree(struct tree *tree, const struct checkout_opts *o,\n>  {\n>         struct unpack_trees_options opts = UNPACK_TREES_OPTIONS_INIT;\n>         struct tree_desc tree_desc;\n> +       int ret;\n>\n>         opts.head_idx = -1;\n>         opts.update = worktree;\n> @@ -667,10 +668,14 @@ static int reset_tree(struct tree *tree, const struct checkout_opts *o,\n>                  */\n>                 /* fallthrough */\n>         case 0:\n> -               return 0;\n> +               ret = 0;\n> +               break;\n>         default:\n> -               return 128;\n> +               ret = 128;\n>         }\n> +\n> +       unpack_trees_options_release(&opts);\n> +       return ret;\n>  }\n>\n>  static void setup_branch_path(struct branch_info *branch)\n> diff --git a/builtin/clone.c b/builtin/clone.c\n> index 0df820c5970..df3bb9a7884 100644\n> --- a/builtin/clone.c\n> +++ b/builtin/clone.c\n> @@ -697,6 +697,7 @@ static int checkout(int submodule_progress)\n>         init_tree_desc(&t, tree->buffer, tree->size);\n>         if (unpack_trees(1, &t, &opts) < 0)\n>                 die(_(\"unable to checkout working tree\"));\n> +       unpack_trees_options_release(&opts);\n>\n>         free(head);\n>\n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index 6cc7313bad8..84c79ecb5a5 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -305,6 +305,7 @@ static void create_base_index(const struct commit *current_head)\n>         struct tree *tree;\n>         struct unpack_trees_options opts = UNPACK_TREES_OPTIONS_INIT;\n>         struct tree_desc t;\n> +       int exit_early = 0;\n>\n>         if (!current_head) {\n>                 discard_cache();\n> @@ -324,7 +325,10 @@ static void create_base_index(const struct commit *current_head)\n>         parse_tree(tree);\n>         init_tree_desc(&t, tree->buffer, tree->size);\n>         if (unpack_trees(1, &t, &opts))\n> -               exit(128); /* We've already reported the error, finish dying */\n> +               exit_early = 1; /* We've already reported the error, finish dying */\n> +       unpack_trees_options_release(&opts);\n> +       if (exit_early)\n> +               exit(128);\n>  }\n>\n>  static void refresh_cache_or_die(int refresh_flags)\n> diff --git a/builtin/merge.c b/builtin/merge.c\n> index 73290a07fcc..28089e2c5ed 100644\n> --- a/builtin/merge.c\n> +++ b/builtin/merge.c\n> @@ -671,6 +671,7 @@ static int read_tree_trivial(struct object_id *common, struct object_id *head,\n>         struct tree *trees[MAX_UNPACK_TREES];\n>         struct tree_desc t[MAX_UNPACK_TREES];\n>         struct unpack_trees_options opts = UNPACK_TREES_OPTIONS_INIT;\n> +       int ret = 0;\n>\n>         opts.head_idx = 2;\n>         opts.src_index = &the_index;\n> @@ -695,8 +696,9 @@ static int read_tree_trivial(struct object_id *common, struct object_id *head,\n>                 init_tree_desc(t+i, trees[i]->buffer, trees[i]->size);\n>         }\n>         if (unpack_trees(nr_trees, t, &opts))\n> -               return -1;\n> -       return 0;\n> +               ret = -1;\n> +       unpack_trees_options_release(&opts);\n> +       return ret;\n>  }\n>\n>  static void write_tree_trivial(struct object_id *oid)\n> diff --git a/builtin/read-tree.c b/builtin/read-tree.c\n> index 06f3b97ac05..8f1b8a7e74c 100644\n> --- a/builtin/read-tree.c\n> +++ b/builtin/read-tree.c\n> @@ -154,6 +154,7 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)\n>                 OPT__QUIET(&opts.quiet, N_(\"suppress feedback messages\")),\n>                 OPT_END()\n>         };\n> +       int ret = 0;\n>\n>         opts.head_idx = -1;\n>         opts.src_index = &the_index;\n> @@ -243,11 +244,13 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)\n>                 parse_tree(tree);\n>                 init_tree_desc(t+i, tree->buffer, tree->size);\n>         }\n> -       if (unpack_trees(nr_trees, t, &opts))\n> -               return 128;\n> +       if (unpack_trees(nr_trees, t, &opts)) {\n> +               ret = 128;\n> +               goto cleanup;\n> +       }\n>\n>         if (opts.debug_unpack || opts.dry_run)\n> -               return 0; /* do not write the index out */\n> +               goto cleanup; /* do not write the index out */\n>\n>         /*\n>          * When reading only one tree (either the most basic form,\n> @@ -262,5 +265,8 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)\n>\n>         if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n>                 die(\"unable to write new index file\");\n> -       return 0;\n> +\n> +cleanup:\n> +       unpack_trees_options_release(&opts);\n> +       return ret;\n>  }\n> diff --git a/builtin/reset.c b/builtin/reset.c\n> index 86c604b21e9..713d084c3eb 100644\n> --- a/builtin/reset.c\n> +++ b/builtin/reset.c\n> @@ -78,10 +78,14 @@ static int reset_index(const char *ref, const struct object_id *oid, int reset_t\n>\n>         if (reset_type == KEEP) {\n>                 struct object_id head_oid;\n> -               if (get_oid(\"HEAD\", &head_oid))\n> -                       return error(_(\"You do not have a valid HEAD.\"));\n> -               if (!fill_tree_descriptor(the_repository, desc + nr, &head_oid))\n> -                       return error(_(\"Failed to find tree of HEAD.\"));\n> +               if (get_oid(\"HEAD\", &head_oid)) {\n> +                       error(_(\"You do not have a valid HEAD.\"));\n> +                       goto out;\n> +               }\n> +               if (!fill_tree_descriptor(the_repository, desc + nr, &head_oid)) {\n> +                       error(_(\"Failed to find tree of HEAD.\"));\n> +                       goto out;\n> +               }\n>                 nr++;\n>                 opts.fn = twoway_merge;\n>         }\n> @@ -103,6 +107,7 @@ static int reset_index(const char *ref, const struct object_id *oid, int reset_t\n>         ret = 0;\n>\n>  out:\n> +       unpack_trees_options_release(&opts);\n>         for (i = 0; i < nr; i++)\n>                 free((void *)desc[i].buffer);\n>         return ret;\n> diff --git a/builtin/stash.c b/builtin/stash.c\n> index 1137e5fcbe8..be6ecb1ae11 100644\n> --- a/builtin/stash.c\n> +++ b/builtin/stash.c\n> @@ -237,6 +237,7 @@ static int reset_tree(struct object_id *i_tree, int update, int reset)\n>         struct tree_desc t[MAX_UNPACK_TREES];\n>         struct tree *tree;\n>         struct lock_file lock_file = LOCK_INIT;\n> +       int ret = 0;\n>\n>         read_cache_preload(NULL);\n>         if (refresh_cache(REFRESH_QUIET))\n> @@ -258,13 +259,17 @@ static int reset_tree(struct object_id *i_tree, int update, int reset)\n>         opts.update = update;\n>         opts.fn = oneway_merge;\n>\n> -       if (unpack_trees(nr_trees, t, &opts))\n> -               return -1;\n> +       if (unpack_trees(nr_trees, t, &opts)) {\n> +               ret = -1;\n> +               goto cleanup;\n> +       }\n>\n>         if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n> -               return error(_(\"unable to write new index file\"));\n> +               ret = error(_(\"unable to write new index file\"));\n>\n> -       return 0;\n> +cleanup:\n> +       unpack_trees_options_release(&opts);\n> +       return ret;\n>  }\n>\n>  static int diff_tree_binary(struct strbuf *out, struct object_id *w_commit)\n> @@ -815,6 +820,7 @@ static void diff_include_untracked(const struct stash_info *info, struct diff_op\n>\n>         if (unpack_trees(ARRAY_SIZE(tree_desc), tree_desc, &unpack_tree_opt))\n>                 die(_(\"failed to unpack trees\"));\n> +       unpack_trees_options_release(&unpack_tree_opt);\n>\n>         do_diff_cache(&info->b_commit, diff_opt);\n>  }\n> diff --git a/diff-lib.c b/diff-lib.c\n> index 8a08d9af4eb..2d8a90a51b2 100644\n> --- a/diff-lib.c\n> +++ b/diff-lib.c\n> @@ -527,6 +527,7 @@ static int diff_cache(struct rev_info *revs,\n>         struct tree *tree;\n>         struct tree_desc t;\n>         struct unpack_trees_options opts = UNPACK_TREES_OPTIONS_INIT;\n> +       int ret;\n>\n>         tree = parse_tree_indirect(tree_oid);\n>         if (!tree)\n> @@ -545,7 +546,9 @@ static int diff_cache(struct rev_info *revs,\n>         opts.pathspec->recursive = 1;\n>\n>         init_tree_desc(&t, tree->buffer, tree->size);\n> -       return unpack_trees(1, &t, &opts);\n> +       ret = unpack_trees(1, &t, &opts);\n> +       unpack_trees_options_release(&opts);\n> +       return ret;\n>  }\n>\n>  void diff_get_merge_base(const struct rev_info *revs, struct object_id *mb)\n> --\n> 2.33.0.1404.g83021034c5d\n\nLooks good to me; thanks for catching and fixing these leaks.\n"},{"id":"437883","messageId":"CABPp-BHZZ4ETPAwf_jBZDZfHCvOUsL5Xnz8Ai0dHWAzazz7nnA@mail.gmail.com","threadId":"56640","inReplyTo":"patch-05.10-042080b8d6d-20211004T002226Z-avarab@gmail.com","subject":"Re: [PATCH 05/10] dir.[ch]: make DIR_INIT mandatory","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-10-04T13:45:00Z","receivedAt":"2021-10-04T13:54:59Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Sun, Oct 3, 2021 at 5:46 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:\n\nThe subject seems misleading; this patch doesn't make DIR_INIT\nmandatory, it is just updating the docs to reflect that and updating\nan out-of-date callsite.\n\n>\n> The dir_init() initializer has been documented as being mandatory\n> since eceba532141 (dir: fix problematic API to avoid memory leaks,\n> 2020-08-18), but both it and my ce93a4c6127 (dir.[ch]: replace\n> dir_init() with DIR_INIT, 2021-07-01) managed to miss this callsite in\n> \"add-interactive.c\" added before those two commits in\n> ab1e1cccaf6 (built-in add -i: re-implement `add-untracked` in C,\n> 2019-11-29).\n>\n> In addition my change to remove dir_init() neglected to update this\n> documentation. Let's use \"must be initialized with\" in reference to\n> \"DIR_INIT\". We have one lazy initialization which pre-dates\n> eceba532141 in dir.c. Adjusting this callsite is a prerequisite for\n> removing it in favor of trusting the macro to initialize the \"struct\n> dir_struct\" correctly.\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n> ---\n>  add-interactive.c | 2 +-\n>  dir.h             | 2 +-\n>  2 files changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/add-interactive.c b/add-interactive.c\n> index 6498ae196f1..27daea8d1b3 100644\n> --- a/add-interactive.c\n> +++ b/add-interactive.c\n> @@ -826,7 +826,7 @@ static int get_untracked_files(struct repository *r,\n>                                struct prefix_item_list *files,\n>                                const struct pathspec *ps)\n>  {\n> -       struct dir_struct dir = { 0 };\n> +       struct dir_struct dir = DIR_INIT;\n>         size_t i;\n>         struct strbuf buf = STRBUF_INIT;\n>\n> diff --git a/dir.h b/dir.h\n> index 83f46c0fb4c..ff3b4a7f602 100644\n> --- a/dir.h\n> +++ b/dir.h\n> @@ -19,7 +19,7 @@\n>   * CE_SKIP_WORKTREE marked. If you want to exclude files, make sure you have\n>   * loaded the index first.\n>   *\n> - * - Prepare `struct dir_struct dir` using `dir_init()` function.\n> + * - The `struct dir_struct dir` must be initialized with `DIR_INIT`.\n>   *\n>   * - To add single exclude pattern, call `add_pattern_list()` and then\n>   *   `add_pattern()`.\n> --\n> 2.33.0.1404.g83021034c5d\n\n...other than the subject, the patch looks good\n"},{"id":"437884","messageId":"CABPp-BH4ubjJ98Nvgp2iyKxmU9X+ypw4m1o=iL9Z4vSNZ-QTDw@mail.gmail.com","threadId":"56640","inReplyTo":"patch-04.10-9d62edd35bf-20211004T002226Z-avarab@gmail.com","subject":"Re: [PATCH 04/10] unpack-trees API: don't have clear_unpack_trees_porcelain() reset","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-10-04T13:45:00Z","receivedAt":"2021-10-04T13:55:01Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Sun, Oct 3, 2021 at 5:46 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:\n>\n> Change the clear_unpack_trees_porcelain() to be like a *_release()\n> function, not a *_reset() (in strbuf.c terms). Let's move the only API\n> user that relied on the latter to doing its own\n> unpack_trees_options_init(). See the commit that introduced\n> unpack_trees_options_init() for details on the control flow involved\n> here.\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n> ---\n>  merge-recursive.c | 1 +\n>  unpack-trees.c    | 1 -\n>  2 files changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/merge-recursive.c b/merge-recursive.c\n> index d24a4903f1d..a77f66b006c 100644\n> --- a/merge-recursive.c\n> +++ b/merge-recursive.c\n> @@ -442,6 +442,7 @@ static void unpack_trees_finish(struct merge_options *opt)\n>  {\n>         discard_index(&opt->priv->orig_index);\n>         clear_unpack_trees_porcelain(&opt->priv->unpack_opts);\n> +       unpack_trees_options_init(&opt->priv->unpack_opts);\n\nThis is wrong.  It suggests that unpack_opts is used after\nunpack_trees_finish() (other than an outer merge first calling\nunpack_trees_start() again), which can only serve to greatly confuse\nfuture readers.  Drop this hunk.\n\n>  }\n>\n>  static int save_files_dirs(const struct object_id *oid,\n> diff --git a/unpack-trees.c b/unpack-trees.c\n> index 94767d3f96f..e7365322e82 100644\n> --- a/unpack-trees.c\n> +++ b/unpack-trees.c\n> @@ -197,7 +197,6 @@ void clear_unpack_trees_porcelain(struct unpack_trees_options *opts)\n>  {\n>         strvec_clear(&opts->msgs_to_free);\n>         dir_clear(&opts->dir);\n> -       memset(opts->msgs, 0, sizeof(opts->msgs));\n\nThis seems like a very dangerous change.  You want to leave opts->msgs\npointing at freed memory?\n"},{"id":"437885","messageId":"CABPp-BFpyyJ-e8p5fbmCvyaEsfUow=RP45Nw0ckiwNEvVC4zrg@mail.gmail.com","threadId":"56640","inReplyTo":"patch-06.10-2b243d91696-20211004T002226Z-avarab@gmail.com","subject":"Re: [PATCH 06/10] dir.c: get rid of lazy initialization","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-10-04T13:45:00Z","receivedAt":"2021-10-04T13:55:04Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Sun, Oct 3, 2021 at 5:46 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:\n>\n> Remove the \"Lazy initialization\" in prep_exclude() left behind by\n> aceb9429b37 (prep_exclude: remove the artificial PATH_MAX limit,\n> 2014-07-14).\n>\n> Now that every caller who sets up a \"struct dir_struct\" is using the\n> DIR_INIT macro we can rely on it to have done the initialization. As\n> noted in an analysis of the previous control flow[1] an earlier\n> passing of of \"dir->basebuf.buf\" to strncmp() wasn't buggy, as we'd\n> only reach that code on subsequent invocations of prep_exclude(),\n> i.e. after this strbuf_init() had been run. But keeping track of that\n> makes for hard-to-read code. Let's just rely on the initialization\n> instead.\n\nHaving read through the link previously, this all makes sense to me,\nbut I'm not sure if this paragraph motivates the change without that\ncontext.  Maybe another reader can comment.\n\n> This does change the behavior of this code in that it won't be\n> pre-growing the strbuf to a size of PATH_MAX. I think that's OK.\n>\n> That we were using PATH_MAX at all is just a relic from this being a\n> fixed buffer from way back in f87f9497486 (git-ls-files: --exclude\n> mechanism updates., 2005-07-24).\n>\n> Pre-allocating PATH_MAX was the opposite of an optimization in this\n> case. I logged all \"basebuf.buf\" values when running the test suite,\n> and by far the most common one (around 80%) is \"\", which we now won't\n> allocate at all for, and just use the \"strbuf_slopbuf\".\n>\n> The second most common one was \"a/\", followed by other common cases of\n> short relative paths. So using the default \"struct strbuf\" growth\n> pattern is a much better allocation optimization in this case.\n>\n> 1. https://lore.kernel.org/git/87sfxhohsj.fsf@evledraar.gmail.com/\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n> ---\n>  dir.c | 8 --------\n>  dir.h | 4 +++-\n>  2 files changed, 3 insertions(+), 9 deletions(-)\n>\n> diff --git a/dir.c b/dir.c\n> index 39fce3bcba7..efc87c2e405 100644\n> --- a/dir.c\n> +++ b/dir.c\n> @@ -1550,14 +1550,6 @@ static void prep_exclude(struct dir_struct *dir,\n>         if (dir->pattern)\n>                 return;\n>\n> -       /*\n> -        * Lazy initialization. All call sites currently just\n> -        * memset(dir, 0, sizeof(*dir)) before use. Changing all of\n> -        * them seems lots of work for little benefit.\n> -        */\n> -       if (!dir->basebuf.buf)\n> -               strbuf_init(&dir->basebuf, PATH_MAX);\n> -\n>         /* Read from the parent directories and push them down. */\n>         current = stk ? stk->baselen : -1;\n>         strbuf_setlen(&dir->basebuf, current < 0 ? 0 : current);\n> diff --git a/dir.h b/dir.h\n> index ff3b4a7f602..e3757c6099e 100644\n> --- a/dir.h\n> +++ b/dir.h\n> @@ -342,7 +342,9 @@ struct dir_struct {\n>         unsigned visited_directories;\n>  };\n>\n> -#define DIR_INIT { 0 }\n> +#define DIR_INIT { \\\n> +       .basebuf = STRBUF_INIT, \\\n> +}\n>\n>  struct dirent *readdir_skip_dot_and_dotdot(DIR *dirp);\n>\n> --\n> 2.33.0.1404.g83021034c5d\n\nWahoo!  Nice code cleanup.\n"},{"id":"437886","messageId":"CABPp-BH053q8AkM+L0GNgeBq0MvN8vTpjnL9GM5HyYx+xuMitw@mail.gmail.com","threadId":"56640","inReplyTo":"patch-03.10-aaad6fce194-20211004T002226Z-avarab@gmail.com","subject":"Re: [PATCH 03/10] unpack-trees.[ch]: embed \"dir\" in \"struct unpack_trees_options\"","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-10-04T13:45:00Z","receivedAt":"2021-10-04T13:55:06Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Sun, Oct 3, 2021 at 5:46 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:\n>\n> Change the \"struct dir\" in \"struct unpack_trees_options\" away from a\n> pointer to being embedded in the struct itself, this mean that we can\n> initialize it with our new UNPACK_TREES_OPTIONS_INIT macro.\n>\n> As it turned out nothing actually needed to provide its own \"struct\n> dir\" to this API, there's another patch to \"hide\" the struct instead,\n> see the discussion at [1], but I think just allocating it on the stack\n> along with the rest makes more sense.\n>\n> This fixes a memory leak in \"builtin/checkout.c\" that's been there\n> since the clear_unpack_trees_porcelain() function was added in\n> 1c41d2805e4 (unpack_trees_options: free messages when done,\n> 2018-05-21), i.e. we still had a memory leak from allocating the \"dir\"\n> member here. That \"dir\" member had in turn been with us ever since\n> 782c2d65c24 (Build in checkout, 2008-02-07).\n>\n> This fixes that memory leak, and allows us to remove the boilerplate\n> dir_clear() elsewhere in favor of just using\n> clear_unpack_trees_porcelain().\n>\n> 1. https://lore.kernel.org/git/87ilyjviiy.fsf@evledraar.gmail.com/\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n> ---\n>  builtin/checkout.c  |  5 ++---\n>  builtin/read-tree.c | 11 ++++-------\n>  merge-ort.c         |  7 ++-----\n>  merge.c             |  7 ++-----\n>  unpack-trees.c      | 10 ++++------\n>  unpack-trees.h      |  4 +++-\n>  6 files changed, 17 insertions(+), 27 deletions(-)\n>\n> diff --git a/builtin/checkout.c b/builtin/checkout.c\n> index fea4533dbec..d4b88affba7 100644\n> --- a/builtin/checkout.c\n> +++ b/builtin/checkout.c\n> @@ -745,9 +745,8 @@ static int merge_working_tree(const struct checkout_opts *opts,\n>                                        &new_branch_info->commit->object.oid :\n>                                        &new_branch_info->oid, NULL);\n>                 if (opts->overwrite_ignore) {\n> -                       topts.dir = xcalloc(1, sizeof(*topts.dir));\n> -                       topts.dir->flags |= DIR_SHOW_IGNORED;\n> -                       setup_standard_excludes(topts.dir);\n> +                       topts.dir.flags |= DIR_SHOW_IGNORED;\n> +                       setup_standard_excludes(&topts.dir);\n>                 }\n>                 tree = parse_tree_indirect(old_branch_info->commit ?\n>                                            &old_branch_info->commit->object.oid :\n> diff --git a/builtin/read-tree.c b/builtin/read-tree.c\n> index 847182fdad6..06f3b97ac05 100644\n> --- a/builtin/read-tree.c\n> +++ b/builtin/read-tree.c\n> @@ -53,20 +53,17 @@ static int index_output_cb(const struct option *opt, const char *arg,\n>  static int exclude_per_directory_cb(const struct option *opt, const char *arg,\n>                                     int unset)\n>  {\n> -       struct dir_struct *dir;\n>         struct unpack_trees_options *opts;\n>\n>         BUG_ON_OPT_NEG(unset);\n>\n>         opts = (struct unpack_trees_options *)opt->value;\n>\n> -       if (opts->dir)\n> +       if (opts->dir.exclude_per_dir)\n>                 die(\"more than one --exclude-per-directory given.\");\n>\n> -       dir = xcalloc(1, sizeof(*opts->dir));\n> -       dir->flags |= DIR_SHOW_IGNORED;\n> -       dir->exclude_per_dir = arg;\n> -       opts->dir = dir;\n> +       opts->dir.flags |= DIR_SHOW_IGNORED;\n> +       opts->dir.exclude_per_dir = arg;\n>         /* We do not need to nor want to do read-directory\n>          * here; we are merely interested in reusing the\n>          * per directory ignore stack mechanism.\n> @@ -208,7 +205,7 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)\n>         if ((opts.update || opts.index_only) && !opts.merge)\n>                 die(\"%s is meaningless without -m, --reset, or --prefix\",\n>                     opts.update ? \"-u\" : \"-i\");\n> -       if ((opts.dir && !opts.update))\n> +       if ((opts.dir.exclude_per_dir && !opts.update))\n>                 die(\"--exclude-per-directory is meaningless unless -u\");\n>         if (opts.merge && !opts.index_only)\n>                 setup_work_tree();\n> diff --git a/merge-ort.c b/merge-ort.c\n> index 75d2b8e4b99..e526b78b88d 100644\n> --- a/merge-ort.c\n> +++ b/merge-ort.c\n> @@ -4045,9 +4045,8 @@ static int checkout(struct merge_options *opt,\n>         unpack_opts.verbose_update = (opt->verbosity > 2);\n>         unpack_opts.fn = twoway_merge;\n>         if (1/* FIXME: opts->overwrite_ignore*/) {\n> -               CALLOC_ARRAY(unpack_opts.dir, 1);\n> -               unpack_opts.dir->flags |= DIR_SHOW_IGNORED;\n> -               setup_standard_excludes(unpack_opts.dir);\n> +               unpack_opts.dir.flags |= DIR_SHOW_IGNORED;\n> +               setup_standard_excludes(&unpack_opts.dir);\n>         }\n>         parse_tree(prev);\n>         init_tree_desc(&trees[0], prev->buffer, prev->size);\n> @@ -4056,8 +4055,6 @@ static int checkout(struct merge_options *opt,\n>\n>         ret = unpack_trees(2, trees, &unpack_opts);\n>         clear_unpack_trees_porcelain(&unpack_opts);\n> -       dir_clear(unpack_opts.dir);\n> -       FREE_AND_NULL(unpack_opts.dir);\n>         return ret;\n>  }\n>\n> diff --git a/merge.c b/merge.c\n> index 086f04b0f97..9cb32990dd9 100644\n> --- a/merge.c\n> +++ b/merge.c\n> @@ -53,7 +53,6 @@ int checkout_fast_forward(struct repository *r,\n>         struct unpack_trees_options opts = UNPACK_TREES_OPTIONS_INIT;\n>         struct tree_desc t[MAX_UNPACK_TREES];\n>         int i, nr_trees = 0;\n> -       struct dir_struct dir = DIR_INIT;\n>         struct lock_file lock_file = LOCK_INIT;\n>\n>         refresh_index(r->index, REFRESH_QUIET, NULL, NULL, NULL);\n> @@ -80,9 +79,8 @@ int checkout_fast_forward(struct repository *r,\n>         }\n>\n>         if (overwrite_ignore) {\n> -               dir.flags |= DIR_SHOW_IGNORED;\n> -               setup_standard_excludes(&dir);\n> -               opts.dir = &dir;\n> +               opts.dir.flags |= DIR_SHOW_IGNORED;\n> +               setup_standard_excludes(&opts.dir);\n>         }\n>\n>         opts.head_idx = 1;\n> @@ -100,7 +98,6 @@ int checkout_fast_forward(struct repository *r,\n>                 clear_unpack_trees_porcelain(&opts);\n>                 return -1;\n>         }\n> -       dir_clear(&dir);\n>         clear_unpack_trees_porcelain(&opts);\n>\n>         if (write_locked_index(r->index, &lock_file, COMMIT_LOCK))\n> diff --git a/unpack-trees.c b/unpack-trees.c\n> index 393c1f35a5d..94767d3f96f 100644\n> --- a/unpack-trees.c\n> +++ b/unpack-trees.c\n> @@ -196,6 +196,7 @@ void unpack_trees_options_init(struct unpack_trees_options *o)\n>  void clear_unpack_trees_porcelain(struct unpack_trees_options *opts)\n>  {\n>         strvec_clear(&opts->msgs_to_free);\n> +       dir_clear(&opts->dir);\n>         memset(opts->msgs, 0, sizeof(opts->msgs));\n>  }\n>\n> @@ -2085,7 +2086,7 @@ static int verify_clean_subdirectory(const struct cache_entry *ce,\n>          */\n>         int namelen;\n>         int i;\n> -       struct dir_struct d;\n> +       struct dir_struct d = DIR_INIT;\n>         char *pathbuf;\n>         int cnt = 0;\n>\n> @@ -2136,9 +2137,7 @@ static int verify_clean_subdirectory(const struct cache_entry *ce,\n>          */\n>         pathbuf = xstrfmt(\"%.*s/\", namelen, ce->name);\n>\n> -       memset(&d, 0, sizeof(d));\n> -       if (o->dir)\n> -               d.exclude_per_dir = o->dir->exclude_per_dir;\n> +       d.exclude_per_dir = o->dir.exclude_per_dir;\n>         i = read_directory(&d, o->src_index, pathbuf, namelen+1, NULL);\n>         if (i)\n>                 return add_rejected_path(o, ERROR_NOT_UPTODATE_DIR, ce->name);\n> @@ -2179,8 +2178,7 @@ static int check_ok_to_remove(const char *name, int len, int dtype,\n>         if (ignore_case && icase_exists(o, name, len, st))\n>                 return 0;\n>\n> -       if (o->dir &&\n> -           is_excluded(o->dir, o->src_index, name, &dtype))\n> +       if (is_excluded(&o->dir, o->src_index, name, &dtype))\n>                 /*\n>                  * ce->name is explicitly excluded, so it is Ok to\n>                  * overwrite it.\n> diff --git a/unpack-trees.h b/unpack-trees.h\n> index 892b65ea623..40c4841748d 100644\n> --- a/unpack-trees.h\n> +++ b/unpack-trees.h\n> @@ -5,6 +5,7 @@\n>  #include \"strvec.h\"\n>  #include \"string-list.h\"\n>  #include \"tree-walk.h\"\n> +#include \"dir.h\"\n>\n>  #define MAX_UNPACK_TREES MAX_TRAVERSE_TREES\n>\n> @@ -66,7 +67,7 @@ struct unpack_trees_options {\n>                      dry_run;\n>         const char *prefix;\n>         int cache_bottom;\n> -       struct dir_struct *dir;\n> +       struct dir_struct dir;\n>         struct pathspec *pathspec;\n>         merge_fn_t fn;\n>         const char *msgs[NB_UNPACK_TREES_WARNING_TYPES];\n> @@ -93,6 +94,7 @@ struct unpack_trees_options {\n>\n>  #define UNPACK_TREES_OPTIONS_INIT { \\\n>         .msgs_to_free = STRVEC_INIT, \\\n> +       .dir = DIR_INIT, \\\n>  }\n>  void unpack_trees_options_init(struct unpack_trees_options *o);\n>\n> --\n> 2.33.0.1404.g83021034c5d\n\nThis patch is going to conflict HEAVILY with\nen/removing-untracked-fixes, which Junio already said he'd merge to\nnext.\n\nYou should rebase your patches on that series.\n"},{"id":"437887","messageId":"CABPp-BHqzRXxatAuMn8rNDPSuN5nAWJFVNziLL3+LS+YJ98V-w@mail.gmail.com","threadId":"56640","inReplyTo":"patch-09.10-aa2bec76f6e-20211004T002226Z-avarab@gmail.com","subject":"Re: [PATCH 09/10] merge.c: avoid duplicate unpack_trees_options_release() code","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-10-04T13:45:00Z","receivedAt":"2021-10-04T13:55:07Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Sun, Oct 3, 2021 at 5:46 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:\n>\n> Refactor code added in 1c41d2805e4 (unpack_trees_options: free\n> messages when done, 2018-05-21) to use a ret/goto pattern, rather than\n> duplicating the end cleanup in the function.\n>\n> This control flow will be matched in subsequent commits by various\n> other similar code, which often needs to do more than just call\n> unpack_trees_options_release(). Let's change this to consistently use\n> the same pattern everywhere.\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n> ---\n>  merge.c | 13 ++++++++-----\n>  1 file changed, 8 insertions(+), 5 deletions(-)\n>\n> diff --git a/merge.c b/merge.c\n> index 2f618425aff..2e3714ccaa0 100644\n> --- a/merge.c\n> +++ b/merge.c\n> @@ -54,6 +54,7 @@ int checkout_fast_forward(struct repository *r,\n>         struct tree_desc t[MAX_UNPACK_TREES];\n>         int i, nr_trees = 0;\n>         struct lock_file lock_file = LOCK_INIT;\n> +       int ret = 0;\n>\n>         refresh_index(r->index, REFRESH_QUIET, NULL, NULL, NULL);\n>\n> @@ -95,12 +96,14 @@ int checkout_fast_forward(struct repository *r,\n>\n>         if (unpack_trees(nr_trees, t, &opts)) {\n>                 rollback_lock_file(&lock_file);\n> -               unpack_trees_options_release(&opts);\n> -               return -1;\n> +               ret = -1;\n> +               goto cleanup;\n>         }\n> -       unpack_trees_options_release(&opts);\n>\n>         if (write_locked_index(r->index, &lock_file, COMMIT_LOCK))\n> -               return error(_(\"unable to write new index file\"));\n> -       return 0;\n> +               ret = error(_(\"unable to write new index file\"));\n> +\n> +cleanup:\n> +       unpack_trees_options_release(&opts);\n> +       return ret;\n>  }\n> --\n> 2.33.0.1404.g83021034c5d\n\nI would say 'meh'...but the commit message is downright confusing.  It\nsuggests that subsequent commits, plural, will be modifying this code\nfurther.  There is only one more commit in this series, and looking\nahead, it doesn't even touch this file.  So, there actually isn't any\nreason for these changes beyond what we see in this file, at least not\nfor this series.  And as far as this series is concerned, I think it's\nactually less readable.  If there's a subsequent series that will use\nthis and make further changes I could be convinced, but then I'd say\nthis commit belongs as part of the later series, not this one.\n"},{"id":"437889","messageId":"CABPp-BFYxWXZQXvDSrM1Y1ZaQ1d2TENQDvx1cyawvrDO1OvExA@mail.gmail.com","threadId":"56640","inReplyTo":"patch-02.10-050399cbfbf-20211004T002226Z-avarab@gmail.com","subject":"Re: [PATCH 02/10] merge-recursive.c: call a new unpack_trees_options_init() function","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-10-04T13:45:00Z","receivedAt":"2021-10-04T13:55:10Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Sun, Oct 3, 2021 at 5:46 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:\n>\n> In the preceding commit we introduced a new UNPACK_TREES_OPTIONS_INIT\n> macro, but \"merge-recursive.c\" could not be converted to it since\n> it (re-)initializes a \"struct unpack_trees_options\" on the heap.\n>\n> In order to convert use the macro as the source of truth for\n> initialization we need to not only convert the initialization in\n> unpack_trees_start(), but also call the new\n> unpack_trees_options_init() just after the CALLOC_ARRAY() in\n> merge_start().\n\nUm...why?\n\n> When we call merge_trees() we'll call merge_start() followed by\n> merge_trees_internal(), and it will call unpack_trees_start() before\n> it does much of anything. So it's covered by an initialization in\n> unpack_trees_start().\n>\n> But when merge_recursive() is called it will call merge_start()\n> followed by merge_recursive_internal(), which won't call\n> unpack_trees_start() until its own call call to\n> merge_trees_internal(), but in the meantime it might end up using that\n> calloc'd \"struct unpack_trees_options\".\n\nNothing in merge-recursive.c usings unpack_opts before\nunpack_trees_start() or after unpack_trees_finish().  If anyone\nattempts to use it elsewhere, that would itself be a bug.  So, I'd\nreplace the above three paragraphs with:\n\n\"Change the initialization of opt->priv_unpack_opts from using memset\nto 0, with unpack_trees_options_init().\"\n\nor something like that, and then drop your change to merge_start().\n\n> This was OK before, as setup_unpack_trees_porcelain() would call\n> strvec_init(), and our \"struct dir_struct\" in turn is OK with being\n> NULL'd. Let's convert the former to macro initialization, we'll deal\n> with the latter in a subsequent commit.\n\nThis is quite confusing; it's really hard to understand how this\nrelates to the rest of the commit message.  I have to read the code to\nsee what you're doing, and then write my own commit message in my head\nbecause the wording in this paragraph still doesn't parse.\n\nI'd make the change strvec_init/STRVEC_INIT changes be a separate\npatch.  I suspect it'll be easier to write the commit message for it\nas a standalone commit as well.\n\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n> ---\n>  merge-recursive.c | 3 ++-\n>  unpack-trees.c    | 8 ++++++--\n>  unpack-trees.h    | 5 ++++-\n>  3 files changed, 12 insertions(+), 4 deletions(-)\n>\n> diff --git a/merge-recursive.c b/merge-recursive.c\n> index e594d4c3fa1..d24a4903f1d 100644\n> --- a/merge-recursive.c\n> +++ b/merge-recursive.c\n> @@ -405,7 +405,7 @@ static int unpack_trees_start(struct merge_options *opt,\n>         struct tree_desc t[3];\n>         struct index_state tmp_index = { NULL };\n>\n> -       memset(&opt->priv->unpack_opts, 0, sizeof(opt->priv->unpack_opts));\n> +       unpack_trees_options_init(&opt->priv->unpack_opts);\n>         if (opt->priv->call_depth)\n>                 opt->priv->unpack_opts.index_only = 1;\n>         else\n> @@ -3704,6 +3704,7 @@ static int merge_start(struct merge_options *opt, struct tree *head)\n>\n>         CALLOC_ARRAY(opt->priv, 1);\n>         string_list_init_dup(&opt->priv->df_conflict_file_set);\n> +       unpack_trees_options_init(&opt->priv->unpack_opts);\n\nDrop this hunk.\n\n\n>         return 0;\n>  }\n>\n> diff --git a/unpack-trees.c b/unpack-trees.c\n> index 8ea0a542da8..393c1f35a5d 100644\n> --- a/unpack-trees.c\n> +++ b/unpack-trees.c\n> @@ -108,8 +108,6 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,\n>         const char **msgs = opts->msgs;\n>         const char *msg;\n>\n> -       strvec_init(&opts->msgs_to_free);\n> -\n>         if (!strcmp(cmd, \"checkout\"))\n>                 msg = advice_enabled(ADVICE_COMMIT_BEFORE_MERGE)\n>                       ? _(\"Your local changes to the following files would be overwritten by checkout:\\n%%s\"\n> @@ -189,6 +187,12 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,\n>                 opts->unpack_rejects[i].strdup_strings = 1;\n>  }\n>\n> +void unpack_trees_options_init(struct unpack_trees_options *o)\n> +{\n> +       struct unpack_trees_options blank = UNPACK_TREES_OPTIONS_INIT;\n> +       memcpy(o, &blank, sizeof(*o));\n> +}\n> +\n>  void clear_unpack_trees_porcelain(struct unpack_trees_options *opts)\n>  {\n>         strvec_clear(&opts->msgs_to_free);\n> diff --git a/unpack-trees.h b/unpack-trees.h\n> index ecf256cbcea..892b65ea623 100644\n> --- a/unpack-trees.h\n> +++ b/unpack-trees.h\n> @@ -91,7 +91,10 @@ struct unpack_trees_options {\n>         struct checkout_metadata meta;\n>  };\n>\n> -#define UNPACK_TREES_OPTIONS_INIT { 0 }\n> +#define UNPACK_TREES_OPTIONS_INIT { \\\n> +       .msgs_to_free = STRVEC_INIT, \\\n> +}\n> +void unpack_trees_options_init(struct unpack_trees_options *o);\n>\n>  int unpack_trees(unsigned n, struct tree_desc *t,\n>                  struct unpack_trees_options *options);\n> --\n> 2.33.0.1404.g83021034c5d\n>\n"},{"id":"437890","messageId":"CABPp-BEr28xzbpEZc5dq-RVDupXy+h-+PH6CoANF4e0kmxqf0Q@mail.gmail.com","threadId":"56640","inReplyTo":"cover-00.10-00000000000-20211004T002226Z-avarab@gmail.com","subject":"Re: [PATCH 00/10] unpack-trees & dir APIs: fix memory leaks","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-10-04T13:45:00Z","receivedAt":"2021-10-04T13:55:19Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Sun, Oct 3, 2021 at 5:46 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:\n>\n> This series fixes memory leaks in the unpack-trees and dir APIs for\n> all their callers.\n\nThere are several good fixes in this series.  Thanks for working on them!\n\n> There's been a discussion between myself & Elijah\n> on his en/removing-untracked-fixes series[1] about the memory leak\n> fixing aspect of his series.\n\nNot really, the memory leak fixing aspect of my series was patch 2;\nmost of our discussion was on patch 4, including your footnote link.\nPatch 4 did not in any way involve fixing a memory leak, which you\nyourself later acknowledged.  So most of our discussion was mostly\nabout aspects _other_ than leak fixing.\n\n> I've got locally queued patches that fix widespread memory leaks in\n> the test suite and make much of it pass under SANITIZE=leak, once the\n> common leaks in revisions.c (git rev-list/show/log etc.), \"checkout\",\n> \"dir\" and \"unpack-trees\" are fixed a lot of tests become entirely\n> leak-free, as much code that needs to setup various basic things will\n> require one of those commands.\n\nYaay!  This is great stuff!\n\n> I think that the more narrow fixes[2] to the memory leaks around\n> unpack-trees in Elijah's series[3] are better skipped and that series\n> rebased on top of this one (I'll submit an RFC version of his that is\n> rebased on this as a follow-up).\n\nI *strongly* disagree.\n\n> I.e. his solves a very small amount of the memory leaks in this area,\n> whereas this is something I've got running as part of end-to-end\n> SANITIZE=leak testing, so I think that the difference in approaches we\n> picked when it comes to fixing them is likely because of that.\n>\n> E.g. continuing to allocate the \"struct dir_struct\" on the heap in his\n> version[4] in his is, I think, something that makes more sense for\n> fixes that haven't pulled at the thread of how much merge-recursive.c\n> is making that question of ownerhip confusing. There's also changen in\n> his that'll become simpler as the complexity of the underlying APIs\n> has gone away, e.g. [6].\n\n*Sigh*.  unpack_trees_options->dir is not allocated on the heap at the\nend of my series.  I could understand missing that in the patches, but\nI've also pointed it out to you two additional times in discussions on\nthe patches so far.  And you supposedly looked at all the patches\nagain while rebasing and adding your signed-off-by.\n\nYou also continue to refer to our discussion as though it was about\nleakfixes, even though the patch we discussed in my series did not\ninvolve any leak fixing.  I pointed that out and you said you stood\ncorrected (last comment at\nhttps://lore.kernel.org/git/87k0ivpzfx.fsf@evledraar.gmail.com/), but\nnow you're referring to it that way again?  Even after rebasing my\nseries and adding your Signed-off-by, suggesting you should understand\nit?  The leakfix was a different patch of the series, namely patch #2.\n\nI agree that merge-recursive.c has confusing points.  I totally agree.\nUnfortunately, both your patches that touch merge-recursive.c make it\nworse; more so than the problems you were trying to fix in that file.\n\n> 1. https://lore.kernel.org/git/87ilyjviiy.fsf@evledraar.gmail.com/\n> 2. https://lore.kernel.org/git/0c74285b25311c83bb158cf89a551160a0f3a5d3.1632760428.git.gitgitgadget@gmail.com/\n> 3. https://lore.kernel.org/git/pull.1036.v3.git.1632760428.gitgitgadget@gmail.com/\n> 4. https://lore.kernel.org/git/0d119142778dce8617dd9b2c102b5f5bfdc9dc0f.1632760428.git.gitgitgadget@gmail.com/\n> 6. https://lore.kernel.org/git/f1a0700e598e52d6cdb507fe8a09c4fa9291c982.1632760428.git.gitgitgadget@gmail.com/\n>\n...\n>   merge-recursive.c: call a new unpack_trees_options_init() function\n>\n> Details how merge-recursive.c calls unpack_trees() differently than\n> every other caller when it comes to \"struct unpack_trees_options\"\n> setup.\n\nSaying:\n\n\"merge-recursive.c has a heap-allocated unpack_trees_options, and thus\ncan't naturally use UNPACK_TREES_OPTIONS_INIT\"\n\nwould have been shorter, and also explained things in full detail.\nYour version makes it sound like it's doing something really weird and\nneeds a much more expansive explanation.\n\n>   unpack-trees.[ch]: embed \"dir\" in \"struct unpack_trees_options\"\n>\n> Elijah's series ends up with a \"dir\" still heap-allocated in \"struct\n> unpack_trees_options\", just dynamically and \"privately\".\n\nAs noted above, this is not true.  I'm confused why you try to claim\notherwise.  (I mean, it's really not all that important, I'm just\nconfused why you find it important to call out, especially when the\nstack-based point was highlighted multiple times before but you still\ninsist on referring to it as heap-allocated.)\n\n...\n>   unpack-trees: don't leak memory in verify_clean_subdirectory()\n>   merge.c: avoid duplicate unpack_trees_options_release() code\n>   built-ins: plug memory leaks with unpack_trees_options_release()\n>\n> A lot of memory leak fixes both in unpack-trees.c and its users, only\n> a subset of this is in Elijah's series.\n\nNot sure why you feel the need to include the final phrase there; it\nalmost feels like you're trying to portray my series as a leakfix,\nwhich feels misleading.  My series wasn't even about fixing leaks.  In\nmy first round, I knew of leaks, and intentionally attempted to avoid\nfixing them because it was orthogonal to the point of my series (I\nfigured I could come back in a follow-on series and deal with it).  In\na subsequent round, I fixed one leak incidentally, in part because you\ncalled it out, but more so because otherwise when I attempted to\nconsolidate code into one place it would appear to reviewers that the\nconsolidated code didn't match some of the callers.  In particular,\nsome of the sites had a leak and others didn't.  Adding a preparatory\nleakfix (again, patch #2, NOT patch #4) made clear that the later\nconsolidation (in patch #4) really was just that -- moving several\nidentical code chunks into a single place.\n\n\n....anyway, all that said, you've got some good fixes in this series.\nYou've also got three very problematic bits that need to be ripped\nout.  And you should rebase this series on top of v3 of\nen/removing-untracked-fixes.\n"},{"id":"437891","messageId":"CABPp-BHQVimcO_E=OPjojJCQ_wfV6+Wy+-2QMTAqnYDk1z3PYQ@mail.gmail.com","threadId":"56640","inReplyTo":"patch-07.10-646e610d8dd-20211004T002226Z-avarab@gmail.com","subject":"Re: [PATCH 07/10] unpack-trees API: rename clear_unpack_trees_porcelain()","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-10-04T13:45:00Z","receivedAt":"2021-10-04T13:55:20Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Sun, Oct 3, 2021 at 5:46 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:\n>\n> Since a preceding commit we've been using\n> clear_unpack_trees_porcelain() to call dir_clear(). So it's no longer\n> a function that corresponds to setup_unpack_trees_porcelain(), as it\n> was when it was added in 1c41d2805e4 (unpack_trees_options: free\n> messages when done, 2018-05-21).\n>\n> Instead it's similar to strbuf_release() and other similar generic\n> \"free\" functions. Let's rename it to avoid any future confusion on the\n> topic.\n>\n> Let's also update the API documentation for it to note this, and to\n> cover e.g. the code added around update_sparsity() in\n> 4ee5d50fc39 (sparse-checkout: use improved unpack_trees porcelain\n> messages, 2020-03-27).\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n> ---\n>  builtin/checkout.c        | 2 +-\n>  builtin/sparse-checkout.c | 2 +-\n>  merge-ort.c               | 2 +-\n>  merge-recursive.c         | 2 +-\n>  merge.c                   | 4 ++--\n>  reset.c                   | 2 +-\n>  unpack-trees.c            | 2 +-\n>  unpack-trees.h            | 8 +++++---\n>  8 files changed, 13 insertions(+), 11 deletions(-)\n>\n> diff --git a/builtin/checkout.c b/builtin/checkout.c\n> index d4b88affba7..482d17676a0 100644\n> --- a/builtin/checkout.c\n> +++ b/builtin/checkout.c\n> @@ -757,7 +757,7 @@ static int merge_working_tree(const struct checkout_opts *opts,\n>                 init_tree_desc(&trees[1], tree->buffer, tree->size);\n>\n>                 ret = unpack_trees(2, trees, &topts);\n> -               clear_unpack_trees_porcelain(&topts);\n> +               unpack_trees_options_release(&topts);\n>                 if (ret == -1) {\n>                         /*\n>                          * Unpack couldn't do a trivial merge; either\n> diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c\n> index 4c3c29fb580..b1221fd01d3 100644\n> --- a/builtin/sparse-checkout.c\n> +++ b/builtin/sparse-checkout.c\n> @@ -219,7 +219,7 @@ static int update_working_directory(struct pattern_list *pl)\n>\n>         setup_unpack_trees_porcelain(&o, \"sparse-checkout\");\n>         result = update_sparsity(&o);\n> -       clear_unpack_trees_porcelain(&o);\n> +       unpack_trees_options_release(&o);\n>\n>         if (result == UPDATE_SPARSITY_WARNINGS)\n>                 /*\n> diff --git a/merge-ort.c b/merge-ort.c\n> index e526b78b88d..0a5937364c9 100644\n> --- a/merge-ort.c\n> +++ b/merge-ort.c\n> @@ -4054,7 +4054,7 @@ static int checkout(struct merge_options *opt,\n>         init_tree_desc(&trees[1], next->buffer, next->size);\n>\n>         ret = unpack_trees(2, trees, &unpack_opts);\n> -       clear_unpack_trees_porcelain(&unpack_opts);\n> +       unpack_trees_options_release(&unpack_opts);\n>         return ret;\n>  }\n>\n> diff --git a/merge-recursive.c b/merge-recursive.c\n> index a77f66b006c..316cb2ca907 100644\n> --- a/merge-recursive.c\n> +++ b/merge-recursive.c\n> @@ -441,7 +441,7 @@ static int unpack_trees_start(struct merge_options *opt,\n>  static void unpack_trees_finish(struct merge_options *opt)\n>  {\n>         discard_index(&opt->priv->orig_index);\n> -       clear_unpack_trees_porcelain(&opt->priv->unpack_opts);\n> +       unpack_trees_options_release(&opt->priv->unpack_opts);\n>         unpack_trees_options_init(&opt->priv->unpack_opts);\n>  }\n>\n> diff --git a/merge.c b/merge.c\n> index 9cb32990dd9..2f618425aff 100644\n> --- a/merge.c\n> +++ b/merge.c\n> @@ -95,10 +95,10 @@ int checkout_fast_forward(struct repository *r,\n>\n>         if (unpack_trees(nr_trees, t, &opts)) {\n>                 rollback_lock_file(&lock_file);\n> -               clear_unpack_trees_porcelain(&opts);\n> +               unpack_trees_options_release(&opts);\n>                 return -1;\n>         }\n> -       clear_unpack_trees_porcelain(&opts);\n> +       unpack_trees_options_release(&opts);\n>\n>         if (write_locked_index(r->index, &lock_file, COMMIT_LOCK))\n>                 return error(_(\"unable to write new index file\"));\n> diff --git a/reset.c b/reset.c\n> index d13984ab781..f4bf3fbfac0 100644\n> --- a/reset.c\n> +++ b/reset.c\n> @@ -133,7 +133,7 @@ int reset_head(struct repository *r, struct object_id *oid, const char *action,\n>  leave_reset_head:\n>         strbuf_release(&msg);\n>         rollback_lock_file(&lock);\n> -       clear_unpack_trees_porcelain(&unpack_tree_opts);\n> +       unpack_trees_options_release(&unpack_tree_opts);\n>         while (nr)\n>                 free((void *)desc[--nr].buffer);\n>         return ret;\n> diff --git a/unpack-trees.c b/unpack-trees.c\n> index e7365322e82..bea598c9ece 100644\n> --- a/unpack-trees.c\n> +++ b/unpack-trees.c\n> @@ -193,7 +193,7 @@ void unpack_trees_options_init(struct unpack_trees_options *o)\n>         memcpy(o, &blank, sizeof(*o));\n>  }\n>\n> -void clear_unpack_trees_porcelain(struct unpack_trees_options *opts)\n> +void unpack_trees_options_release(struct unpack_trees_options *opts)\n>  {\n>         strvec_clear(&opts->msgs_to_free);\n>         dir_clear(&opts->dir);\n> diff --git a/unpack-trees.h b/unpack-trees.h\n> index 40c4841748d..a8d1f083b33 100644\n> --- a/unpack-trees.h\n> +++ b/unpack-trees.h\n> @@ -41,10 +41,12 @@ enum unpack_trees_error_types {\n>  void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,\n>                                   const char *cmd);\n>\n> -/*\n> - * Frees resources allocated by setup_unpack_trees_porcelain().\n> +/**\n> + * Frees resources allocated by function that take the \"struct\n> + * unpack_trees_options\". Always call this after using unpack_trees(),\n> + * update_sparsity() etc.\n>   */\n> -void clear_unpack_trees_porcelain(struct unpack_trees_options *opts);\n> +void unpack_trees_options_release(struct unpack_trees_options *opts);\n>\n>  struct unpack_trees_options {\n>         unsigned int reset,\n> --\n> 2.33.0.1404.g83021034c5d\n\nMakes sense.\n"},{"id":"437900","messageId":"87y278n8tt.fsf@evledraar.gmail.com","threadId":"56640","inReplyTo":"CABPp-BFYxWXZQXvDSrM1Y1ZaQ1d2TENQDvx1cyawvrDO1OvExA@mail.gmail.com","subject":"Re: [PATCH 02/10] merge-recursive.c: call a new unpack_trees_options_init() function","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-04T14:41:00Z","receivedAt":"2021-10-04T14:52:00Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Oct 04 2021, Elijah Newren wrote:\n\n> On Sun, Oct 3, 2021 at 5:46 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:\n>>\n>> In the preceding commit we introduced a new UNPACK_TREES_OPTIONS_INIT\n>> macro, but \"merge-recursive.c\" could not be converted to it since\n>> it (re-)initializes a \"struct unpack_trees_options\" on the heap.\n>>\n>> In order to convert use the macro as the source of truth for\n>> initialization we need to not only convert the initialization in\n>> unpack_trees_start(), but also call the new\n>> unpack_trees_options_init() just after the CALLOC_ARRAY() in\n>> merge_start().\n>\n> Um...why?\n\nReplied below.\n\n>> When we call merge_trees() we'll call merge_start() followed by\n>> merge_trees_internal(), and it will call unpack_trees_start() before\n>> it does much of anything. So it's covered by an initialization in\n>> unpack_trees_start().\n>>\n>> But when merge_recursive() is called it will call merge_start()\n>> followed by merge_recursive_internal(), which won't call\n>> unpack_trees_start() until its own call call to\n>> merge_trees_internal(), but in the meantime it might end up using that\n>> calloc'd \"struct unpack_trees_options\".\n>\n> Nothing in merge-recursive.c usings unpack_opts before\n> unpack_trees_start() or after unpack_trees_finish().  If anyone\n> attempts to use it elsewhere, that would itself be a bug.  So, I'd\n> replace the above three paragraphs with:\n>\n> \"Change the initialization of opt->priv_unpack_opts from using memset\n> to 0, with unpack_trees_options_init().\"\n>\n> or something like that, and then drop your change to merge_start().\n\nSure, I'll defer to you about being confident about that. I didn't want\nto leave a copy if it hanging without the proper initialization in case\nthere were new callers.\n\n>> This was OK before, as setup_unpack_trees_porcelain() would call\n>> strvec_init(), and our \"struct dir_struct\" in turn is OK with being\n>> NULL'd. Let's convert the former to macro initialization, we'll deal\n>> with the latter in a subsequent commit.\n>\n> This is quite confusing; it's really hard to understand how this\n> relates to the rest of the commit message.  I have to read the code to\n> see what you're doing, and then write my own commit message in my head\n> because the wording in this paragraph still doesn't parse.\n>\n> I'd make the change strvec_init/STRVEC_INIT changes be a separate\n> patch.  I suspect it'll be easier to write the commit message for it\n> as a standalone commit as well.\n\nSure, FWIW I had it split up locally, and figured it would be easier to\nexplain if the API usage change came with the initialization change that\nmade it possible.\n\n>>\n>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n>> ---\n>>  merge-recursive.c | 3 ++-\n>>  unpack-trees.c    | 8 ++++++--\n>>  unpack-trees.h    | 5 ++++-\n>>  3 files changed, 12 insertions(+), 4 deletions(-)\n>>\n>> diff --git a/merge-recursive.c b/merge-recursive.c\n>> index e594d4c3fa1..d24a4903f1d 100644\n>> --- a/merge-recursive.c\n>> +++ b/merge-recursive.c\n>> @@ -405,7 +405,7 @@ static int unpack_trees_start(struct merge_options *opt,\n>>         struct tree_desc t[3];\n>>         struct index_state tmp_index = { NULL };\n>>\n>> -       memset(&opt->priv->unpack_opts, 0, sizeof(opt->priv->unpack_opts));\n>> +       unpack_trees_options_init(&opt->priv->unpack_opts);\n>>         if (opt->priv->call_depth)\n>>                 opt->priv->unpack_opts.index_only = 1;\n>>         else\n>> @@ -3704,6 +3704,7 @@ static int merge_start(struct merge_options *opt, struct tree *head)\n>>\n>>         CALLOC_ARRAY(opt->priv, 1);\n>>         string_list_init_dup(&opt->priv->df_conflict_file_set);\n>> +       unpack_trees_options_init(&opt->priv->unpack_opts);\n>\n> Drop this hunk.\n\nSpeaking of splitting things up in more understandable ways: If we're\ngoing to hard rely on the interaction between merge_start() and\nunpack_trees_start() wouldn't it make sense to lead with a change that\ndropped that memset, which if this invariant holds is also redundant to\nthe CALLOC() of opt->priv in merge_start() in the pre-image.\n"},{"id":"437901","messageId":"87tuhwn8s7.fsf@evledraar.gmail.com","threadId":"56640","inReplyTo":"CABPp-BHqzRXxatAuMn8rNDPSuN5nAWJFVNziLL3+LS+YJ98V-w@mail.gmail.com","subject":"Re: [PATCH 09/10] merge.c: avoid duplicate unpack_trees_options_release() code","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-04T14:50:23Z","receivedAt":"2021-10-04T14:52:47Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Oct 04 2021, Elijah Newren wrote:\n\n> On Sun, Oct 3, 2021 at 5:46 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:\n>>\n>> Refactor code added in 1c41d2805e4 (unpack_trees_options: free\n>> messages when done, 2018-05-21) to use a ret/goto pattern, rather than\n>> duplicating the end cleanup in the function.\n>>\n>> This control flow will be matched in subsequent commits by various\n>> other similar code, which often needs to do more than just call\n>> unpack_trees_options_release(). Let's change this to consistently use\n>> the same pattern everywhere.\n>>\n>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n>> ---\n>>  merge.c | 13 ++++++++-----\n>>  1 file changed, 8 insertions(+), 5 deletions(-)\n>>\n>> diff --git a/merge.c b/merge.c\n>> index 2f618425aff..2e3714ccaa0 100644\n>> --- a/merge.c\n>> +++ b/merge.c\n>> @@ -54,6 +54,7 @@ int checkout_fast_forward(struct repository *r,\n>>         struct tree_desc t[MAX_UNPACK_TREES];\n>>         int i, nr_trees = 0;\n>>         struct lock_file lock_file = LOCK_INIT;\n>> +       int ret = 0;\n>>\n>>         refresh_index(r->index, REFRESH_QUIET, NULL, NULL, NULL);\n>>\n>> @@ -95,12 +96,14 @@ int checkout_fast_forward(struct repository *r,\n>>\n>>         if (unpack_trees(nr_trees, t, &opts)) {\n>>                 rollback_lock_file(&lock_file);\n>> -               unpack_trees_options_release(&opts);\n>> -               return -1;\n>> +               ret = -1;\n>> +               goto cleanup;\n>>         }\n>> -       unpack_trees_options_release(&opts);\n>>\n>>         if (write_locked_index(r->index, &lock_file, COMMIT_LOCK))\n>> -               return error(_(\"unable to write new index file\"));\n>> -       return 0;\n>> +               ret = error(_(\"unable to write new index file\"));\n>> +\n>> +cleanup:\n>> +       unpack_trees_options_release(&opts);\n>> +       return ret;\n>>  }\n>> --\n>> 2.33.0.1404.g83021034c5d\n>\n> I would say 'meh'...but the commit message is downright confusing.  It\n> suggests that subsequent commits, plural, will be modifying this code\n> further.  There is only one more commit in this series, and looking\n> ahead, it doesn't even touch this file.  So, there actually isn't any\n> reason for these changes beyond what we see in this file, at least not\n> for this series.  And as far as this series is concerned, I think it's\n> actually less readable.  If there's a subsequent series that will use\n> this and make further changes I could be convinced, but then I'd say\n> this commit belongs as part of the later series, not this one.\n\nWill fix, initially had these built-in fixes split into one commit per\nbuilt-in, but decided it was too verbose and squashed them all, and\ndidn't copyedit the commit message properly.\n"},{"id":"437902","messageId":"87pmskn8mc.fsf@evledraar.gmail.com","threadId":"56640","inReplyTo":"CABPp-BFr7R_swdxswmoGRF3=uu0s8HgppfrnFEQ+NvS1oh+_og@mail.gmail.com","subject":"Re: [PATCH 10/10] built-ins: plug memory leaks with unpack_trees_options_release()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-04T14:54:08Z","receivedAt":"2021-10-04T14:56:06Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Oct 04 2021, Elijah Newren wrote:\n\n> On Sun, Oct 3, 2021 at 5:46 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:\n>>\n>> Plug memory leaks in various built-ins that were missing\n>> unpack_trees_options_release() calls. In an earlier commit these\n>> functions were all made to use the \"UNPACK_TREES_OPTIONS_INIT\" macro,\n>> now let's have the ones that didn't clean up their memory do so.\n>\n> This commit message doesn't say anything about what was leaking.  You\n> should really bring up that it was the dir_clear() you added to\n> unpack_trees_options_release() earlier in the series that is important\n> here (or at least that's what I presume is the important fix).\n\n*nod*, willdo.\n\n> Looks good to me; thanks for catching and fixing these leaks.\n\n"},{"id":"437904","messageId":"CABPp-BEA2myh2Np_YpFWnE+jqmT5vz7ohigZ0=2tL-wizgYQmg@mail.gmail.com","threadId":"56640","inReplyTo":"87y278n8tt.fsf@evledraar.gmail.com","subject":"Re: [PATCH 02/10] merge-recursive.c: call a new unpack_trees_options_init() function","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-10-04T15:04:00Z","receivedAt":"2021-10-04T15:06:34Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, Oct 4, 2021 at 7:49 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:\n>\n> On Mon, Oct 04 2021, Elijah Newren wrote:\n>\n> > On Sun, Oct 3, 2021 at 5:46 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:\n> >>\n> >> In the preceding commit we introduced a new UNPACK_TREES_OPTIONS_INIT\n> >> macro, but \"merge-recursive.c\" could not be converted to it since\n> >> it (re-)initializes a \"struct unpack_trees_options\" on the heap.\n> >>\n> >> In order to convert use the macro as the source of truth for\n> >> initialization we need to not only convert the initialization in\n> >> unpack_trees_start(), but also call the new\n> >> unpack_trees_options_init() just after the CALLOC_ARRAY() in\n> >> merge_start().\n> >\n> > Um...why?\n>\n> Replied below.\n>\n> >> When we call merge_trees() we'll call merge_start() followed by\n> >> merge_trees_internal(), and it will call unpack_trees_start() before\n> >> it does much of anything. So it's covered by an initialization in\n> >> unpack_trees_start().\n> >>\n> >> But when merge_recursive() is called it will call merge_start()\n> >> followed by merge_recursive_internal(), which won't call\n> >> unpack_trees_start() until its own call call to\n> >> merge_trees_internal(), but in the meantime it might end up using that\n> >> calloc'd \"struct unpack_trees_options\".\n> >\n> > Nothing in merge-recursive.c usings unpack_opts before\n> > unpack_trees_start() or after unpack_trees_finish().  If anyone\n> > attempts to use it elsewhere, that would itself be a bug.  So, I'd\n> > replace the above three paragraphs with:\n> >\n> > \"Change the initialization of opt->priv_unpack_opts from using memset\n> > to 0, with unpack_trees_options_init().\"\n> >\n> > or something like that, and then drop your change to merge_start().\n>\n> Sure, I'll defer to you about being confident about that. I didn't want\n> to leave a copy if it hanging without the proper initialization in case\n> there were new callers.\n>\n> >> This was OK before, as setup_unpack_trees_porcelain() would call\n> >> strvec_init(), and our \"struct dir_struct\" in turn is OK with being\n> >> NULL'd. Let's convert the former to macro initialization, we'll deal\n> >> with the latter in a subsequent commit.\n> >\n> > This is quite confusing; it's really hard to understand how this\n> > relates to the rest of the commit message.  I have to read the code to\n> > see what you're doing, and then write my own commit message in my head\n> > because the wording in this paragraph still doesn't parse.\n> >\n> > I'd make the change strvec_init/STRVEC_INIT changes be a separate\n> > patch.  I suspect it'll be easier to write the commit message for it\n> > as a standalone commit as well.\n>\n> Sure, FWIW I had it split up locally, and figured it would be easier to\n> explain if the API usage change came with the initialization change that\n> made it possible.\n>\n> >>\n> >> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n> >> ---\n> >>  merge-recursive.c | 3 ++-\n> >>  unpack-trees.c    | 8 ++++++--\n> >>  unpack-trees.h    | 5 ++++-\n> >>  3 files changed, 12 insertions(+), 4 deletions(-)\n> >>\n> >> diff --git a/merge-recursive.c b/merge-recursive.c\n> >> index e594d4c3fa1..d24a4903f1d 100644\n> >> --- a/merge-recursive.c\n> >> +++ b/merge-recursive.c\n> >> @@ -405,7 +405,7 @@ static int unpack_trees_start(struct merge_options *opt,\n> >>         struct tree_desc t[3];\n> >>         struct index_state tmp_index = { NULL };\n> >>\n> >> -       memset(&opt->priv->unpack_opts, 0, sizeof(opt->priv->unpack_opts));\n> >> +       unpack_trees_options_init(&opt->priv->unpack_opts);\n> >>         if (opt->priv->call_depth)\n> >>                 opt->priv->unpack_opts.index_only = 1;\n> >>         else\n> >> @@ -3704,6 +3704,7 @@ static int merge_start(struct merge_options *opt, struct tree *head)\n> >>\n> >>         CALLOC_ARRAY(opt->priv, 1);\n> >>         string_list_init_dup(&opt->priv->df_conflict_file_set);\n> >> +       unpack_trees_options_init(&opt->priv->unpack_opts);\n> >\n> > Drop this hunk.\n>\n> Speaking of splitting things up in more understandable ways: If we're\n> going to hard rely on the interaction between merge_start() and\n> unpack_trees_start() wouldn't it make sense to lead with a change that\n> dropped that memset, which if this invariant holds is also redundant to\n> the CALLOC() of opt->priv in merge_start() in the pre-image.\n\nIt is not redundant, and that would be a very confusing change.\nmerge-recursive is so named because its main driving function contains\nrecursive calls to itself.  merge_start() is not involved in that\nrecursion.  For readability, we should initialize unpack_opts before\neach use.\n"},{"id":"437906","messageId":"87lf38n6e4.fsf@evledraar.gmail.com","threadId":"56640","inReplyTo":"CABPp-BH4ubjJ98Nvgp2iyKxmU9X+ypw4m1o=iL9Z4vSNZ-QTDw@mail.gmail.com","subject":"Re: [PATCH 04/10] unpack-trees API: don't have clear_unpack_trees_porcelain() reset","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-04T15:20:13Z","receivedAt":"2021-10-04T15:43:52Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Oct 04 2021, Elijah Newren wrote:\n\n> On Sun, Oct 3, 2021 at 5:46 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:\n>>\n>> Change the clear_unpack_trees_porcelain() to be like a *_release()\n>> function, not a *_reset() (in strbuf.c terms). Let's move the only API\n>> user that relied on the latter to doing its own\n>> unpack_trees_options_init(). See the commit that introduced\n>> unpack_trees_options_init() for details on the control flow involved\n>> here.\n>>\n>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n>> ---\n>>  merge-recursive.c | 1 +\n>>  unpack-trees.c    | 1 -\n>>  2 files changed, 1 insertion(+), 1 deletion(-)\n>>\n>> diff --git a/merge-recursive.c b/merge-recursive.c\n>> index d24a4903f1d..a77f66b006c 100644\n>> --- a/merge-recursive.c\n>> +++ b/merge-recursive.c\n>> @@ -442,6 +442,7 @@ static void unpack_trees_finish(struct merge_options *opt)\n>>  {\n>>         discard_index(&opt->priv->orig_index);\n>>         clear_unpack_trees_porcelain(&opt->priv->unpack_opts);\n>> +       unpack_trees_options_init(&opt->priv->unpack_opts);\n>\n> This is wrong.  It suggests that unpack_opts is used after\n> unpack_trees_finish() (other than an outer merge first calling\n> unpack_trees_start() again), which can only serve to greatly confuse\n> future readers.  Drop this hunk.\n\nSure, but (and also re:\nhttps://lore.kernel.org/git/CABPp-BEA2myh2Np_YpFWnE+jqmT5vz7ohigZ0=2tL-wizgYQmg@mail.gmail.com/)\nif you'd like not initialize things in merge_start() just for good\nmeasure wouldn't the diff-at-the-end on top of your 5bf7e5779ec\n(merge-recursive: split internal fields into a separate struct,\n2019-08-17) also make sense?\n\nI.e. the reason I entered this particular rabbit hole was in looking at\nexisting members of \"struct merge_options_internal\" & past commits and\nseeing how we did its initialization. That canary on top passes all our\ntests, and per my reading we also don't use \"df_conflict_file_set\" until\nas late as the things we setup in unpack_trees_start(). Should those be\nmoved to do the post-merge_start() setup at the same time?\n\n>>  }\n>>\n>>  static int save_files_dirs(const struct object_id *oid,\n>> diff --git a/unpack-trees.c b/unpack-trees.c\n>> index 94767d3f96f..e7365322e82 100644\n>> --- a/unpack-trees.c\n>> +++ b/unpack-trees.c\n>> @@ -197,7 +197,6 @@ void clear_unpack_trees_porcelain(struct unpack_trees_options *opts)\n>>  {\n>>         strvec_clear(&opts->msgs_to_free);\n>>         dir_clear(&opts->dir);\n>> -       memset(opts->msgs, 0, sizeof(opts->msgs));\n>\n> This seems like a very dangerous change.  You want to leave opts->msgs\n> pointing at freed memory?\n\nYes, as argued in\nhttp://lore.kernel.org/git/87bl45niqs.fsf@evledraar.gmail.com; In this\nseries we can see that nothing re-uses it, so it's as safe as our\nstrbuf_release(), or a plain free().\n\nMaybe I'm misunderstanding what you're getting at, and I could\nunderstand a \"let's just reset it for good measure\" POV. But I can't\nsquare your view that we shouldn't do setup in merge_start() for good\nmeasure in case some new future code accidentally uses the data earlier\n(which I'm fine with), but then also not finding it OK to skip the\nmemset() here ...\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex e594d4c3fa1..6d2b8e78896 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -601,6 +601,7 @@ static void record_df_conflict_files(struct merge_options *opt,\n \tdf_sorted_entries.cmp = string_list_df_name_compare;\n \tstring_list_sort(&df_sorted_entries);\n \n+\tassert(opt->priv->df_conflict_file_set.nr != 123456);\n \tstring_list_clear(&opt->priv->df_conflict_file_set, 1);\n \tfor (i = 0; i < df_sorted_entries.nr; i++) {\n \t\tconst char *path = df_sorted_entries.items[i].string;\n@@ -869,6 +870,7 @@ static int make_room_for_path(struct merge_options *opt, const char *path)\n \tconst char *msg = _(\"failed to create path '%s'%s\");\n \n \t/* Unlink any D/F conflict files that are in the way */\n+\tassert(opt->priv->df_conflict_file_set.nr != 123456);\n \tfor (i = 0; i < opt->priv->df_conflict_file_set.nr; i++) {\n \t\tconst char *df_path = opt->priv->df_conflict_file_set.items[i].string;\n \t\tsize_t pathlen = strlen(path);\n@@ -3467,6 +3469,7 @@ static int merge_trees_internal(struct merge_options *opt,\n \t\treturn 1;\n \t}\n \n+\tstring_list_init_dup(&opt->priv->df_conflict_file_set);\n \tcode = unpack_trees_start(opt, merge_base, head, merge);\n \n \tif (code != 0) {\n@@ -3703,7 +3706,7 @@ static int merge_start(struct merge_options *opt, struct tree *head)\n \t}\n \n \tCALLOC_ARRAY(opt->priv, 1);\n-\tstring_list_init_dup(&opt->priv->df_conflict_file_set);\n+\topt->priv->df_conflict_file_set.nr = 123456;\n \treturn 0;\n }\n \n"},{"id":"437909","messageId":"CABPp-BGN_cyLVRRcz_BfriK5Gw=3mdUoSmePT4qFSmV6uYgJ3Q@mail.gmail.com","threadId":"56640","inReplyTo":"87lf38n6e4.fsf@evledraar.gmail.com","subject":"Re: [PATCH 04/10] unpack-trees API: don't have clear_unpack_trees_porcelain() reset","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-10-04T16:28:18Z","receivedAt":"2021-10-04T16:29:19Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, Oct 4, 2021 at 8:42 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:\n>\n> On Mon, Oct 04 2021, Elijah Newren wrote:\n>\n> > On Sun, Oct 3, 2021 at 5:46 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:\n> >>\n> >> Change the clear_unpack_trees_porcelain() to be like a *_release()\n> >> function, not a *_reset() (in strbuf.c terms). Let's move the only API\n> >> user that relied on the latter to doing its own\n> >> unpack_trees_options_init(). See the commit that introduced\n> >> unpack_trees_options_init() for details on the control flow involved\n> >> here.\n> >>\n> >> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n> >> ---\n> >>  merge-recursive.c | 1 +\n> >>  unpack-trees.c    | 1 -\n> >>  2 files changed, 1 insertion(+), 1 deletion(-)\n> >>\n> >> diff --git a/merge-recursive.c b/merge-recursive.c\n> >> index d24a4903f1d..a77f66b006c 100644\n> >> --- a/merge-recursive.c\n> >> +++ b/merge-recursive.c\n> >> @@ -442,6 +442,7 @@ static void unpack_trees_finish(struct merge_options *opt)\n> >>  {\n> >>         discard_index(&opt->priv->orig_index);\n> >>         clear_unpack_trees_porcelain(&opt->priv->unpack_opts);\n> >> +       unpack_trees_options_init(&opt->priv->unpack_opts);\n> >\n> > This is wrong.  It suggests that unpack_opts is used after\n> > unpack_trees_finish() (other than an outer merge first calling\n> > unpack_trees_start() again), which can only serve to greatly confuse\n> > future readers.  Drop this hunk.\n>\n> Sure, but (and also re:\n> https://lore.kernel.org/git/CABPp-BEA2myh2Np_YpFWnE+jqmT5vz7ohigZ0=2tL-wizgYQmg@mail.gmail.com/)\n> if you'd like not initialize things in merge_start() just for good\n> measure wouldn't the diff-at-the-end on top of your 5bf7e5779ec\n> (merge-recursive: split internal fields into a separate struct,\n> 2019-08-17) also make sense?\n\nSorry, I can't parse this sentence.  Could you retry?\n\n> I.e. the reason I entered this particular rabbit hole was in looking at\n> existing members of \"struct merge_options_internal\" & past commits and\n> seeing how we did its initialization. That canary on top passes all our\n> tests, and per my reading we also don't use \"df_conflict_file_set\" until\n> as late as the things we setup in unpack_trees_start(). Should those be\n> moved to do the post-merge_start() setup at the same time?\n\nIt appears df_conflict_file_set has some theoretical memory leaks\n(though in practice unlikely and quite small in the few cases that\ncould be constructed to trigger it).  Initializing it nearer to use\nand free'ing when done (in merge_trees_internal()) would  make more\nsense, yes.\n\nBut, merge-recursive.c right now is supposed to be the stable fallback\nin case someone runs into an issue with merge-ort.  I'd rather keep it\nstable in preparation for deleting it, not churning its code\nunnecessarily.\n\n> >>  }\n> >>\n> >>  static int save_files_dirs(const struct object_id *oid,\n> >> diff --git a/unpack-trees.c b/unpack-trees.c\n> >> index 94767d3f96f..e7365322e82 100644\n> >> --- a/unpack-trees.c\n> >> +++ b/unpack-trees.c\n> >> @@ -197,7 +197,6 @@ void clear_unpack_trees_porcelain(struct unpack_trees_options *opts)\n> >>  {\n> >>         strvec_clear(&opts->msgs_to_free);\n> >>         dir_clear(&opts->dir);\n> >> -       memset(opts->msgs, 0, sizeof(opts->msgs));\n> >\n> > This seems like a very dangerous change.  You want to leave opts->msgs\n> > pointing at freed memory?\n>\n> Yes, as argued in\n> http://lore.kernel.org/git/87bl45niqs.fsf@evledraar.gmail.com; In this\n> series we can see that nothing re-uses it, so it's as safe as our\n> strbuf_release(), or a plain free().\n\nstrbuf_release() sets sb->buf to strbuf_slopbuf, and sets sb->len =\nsb->alloc = 0.  The strbuf can thus be reused after calling\nstrbuf_release().\n\nstrvec_clear() also calls strvec_init() afterwards to set the vector\nto be usable though 0-sized.\n\nhashmap_clear() also clears out existing data, but makes it ready for\nreuse (as per 6da1a25814)\n\nstrmap_clear(), strintmap_clear(), and strset_clear() also set up the\ndata structure for reuse.\n\nThere's a longstanding presumption that something named `*_clear()`\nwill make it still usable afterwards.  Rename it to end with `_free`\nif you want it to be an analogy to free() where usage afterwards would\ncause use-after-free errors.\n\n> Maybe I'm misunderstanding what you're getting at, and I could\n> understand a \"let's just reset it for good measure\" POV. But I can't\n> square your view that we shouldn't do setup in merge_start() for good\n> measure in case some new future code accidentally uses the data earlier\n> (which I'm fine with), but then also not finding it OK to skip the\n> memset() here ...\n\nNo existing caller needs to make use of the fact that it's a `_clear`\nfunction rather than a `_free` function, but if you want to take\nadvantage of that to do less work, you should both call it out in your\ncommit message and rename the function.  You didn't do either.  In\nfact, your existing commit message mentions strbuf_release(), which\nreinforces the `_clear` presumption of reusability and thus makes me\nflag the change as dangerous.\n"}]}