This series fixes memory leaks in the unpack-trees and dir APIs for
all their callers. There's been a discussion between myself & Elijah
on his en/removing-untracked-fixes series[1] about the memory leak
fixing aspect of his series.
I've got locally queued patches that fix widespread memory leaks in
the test suite and make much of it pass under SANITIZE=leak, once the
common leaks in revisions.c (git rev-list/show/log etc.), "checkout",
"dir" and "unpack-trees" are fixed a lot of tests become entirely
leak-free, as much code that needs to setup various basic things will
require one of those commands.
I think that the more narrow fixes[2] to the memory leaks around
unpack-trees in Elijah's series[3] are better skipped and that series
rebased on top of this one (I'll submit an RFC version of his that is
rebased on this as a follow-up).
I.e. his solves a very small amount of the memory leaks in this area,
whereas this is something I've got running as part of end-to-end
SANITIZE=leak testing, so I think that the difference in approaches we
picked when it comes to fixing them is likely because of that.
E.g. continuing to allocate the "struct dir_struct" on the heap in his
version[4] in his is, I think, something that makes more sense for
fixes that haven't pulled at the thread of how much merge-recursive.c
is making that question of ownerhip confusing. There's also changen in
his that'll become simpler as the complexity of the underlying APIs
has gone away, e.g. [6].
1. https://lore.kernel.org/git/87ilyjviiy.fsf@evledraar.gmail.com/
2. https://lore.kernel.org/git/0c74285b25311c83bb158cf89a551160a0f3a5d3.1632760428.git.gitgitgadget@gmail.com/
3. https://lore.kernel.org/git/pull.1036.v3.git.1632760428.gitgitgadget@gmail.com/
4. https://lore.kernel.org/git/0d119142778dce8617dd9b2c102b5f5bfdc9dc0f.1632760428.git.gitgitgadget@gmail.com/
6. https://lore.kernel.org/git/f1a0700e598e52d6cdb507fe8a09c4fa9291c982.1632760428.git.gitgitgadget@gmail.com/
Comments on individual patches below:
Ævar Arnfjörð Bjarmason (10):
unpack-trees.[ch]: define and use a UNPACK_TREES_OPTIONS_INIT
I had this at the end of the v3 of my designated initializer cleanup
series[7]
I think Junio fairly commented that this in isolation looked like it
was going nowhere[8] since we didn' get past initializing "struct
unpack_trees_options" as "{ 0 }", but that'll soon be the case in this
series...
merge-recursive.c: call a new unpack_trees_options_init() function
Details how merge-recursive.c calls unpack_trees() differently than
every other caller when it comes to "struct unpack_trees_options"
setup.
unpack-trees.[ch]: embed "dir" in "struct unpack_trees_options"
Elijah's series ends up with a "dir" still heap-allocated in "struct
unpack_trees_options", just dynamically and "privately". Here it's
allocated on the stack (or for merge-recursive.c, as defined in
UNPACK_TREES_OPTIONS_INIT), because we could untangle the
merge-recursive.c edge-case earlier.
unpack-trees API: don't have clear_unpack_trees_porcelain() reset
Move merge-recursive.c special-snowflake behavior into
merge-recursive.c.
dir.[ch]: make DIR_INIT mandatory
dir.c: get rid of lazy initialization
The last caller not using "struct dir_struct" via DIR_INIT goes away,
allowing for untangling the mess I commented on at length in [9].
unpack-trees API: rename clear_unpack_trees_porcelain()
Just a s/clear_unpack_trees_porcelain/unpack_trees_options_release/g,
since that's what it does now.
unpack-trees: don't leak memory in verify_clean_subdirectory()
merge.c: avoid duplicate unpack_trees_options_release() code
built-ins: plug memory leaks with unpack_trees_options_release()
A lot of memory leak fixes both in unpack-trees.c and its users, only
a subset of this is in Elijah's series.
7. https://lore.kernel.org/git/cover-v3-0.6-00000000000-20211001T102056Z-avarab@gmail.com/
8. https://lore.kernel.org/git/xmqqk0iw4e7v.fsf@gitster.g/
9. https://lore.kernel.org/git/87sfxhohsj.fsf@evledraar.gmail.com/
add-interactive.c | 2 +-
archive.c | 12 ++++++++----
builtin/am.c | 23 ++++++++++++++---------
builtin/checkout.c | 22 ++++++++++++----------
builtin/clone.c | 4 ++--
builtin/commit.c | 9 ++++++---
builtin/merge.c | 9 +++++----
builtin/read-tree.c | 28 +++++++++++++++-------------
builtin/reset.c | 16 ++++++++++------
builtin/sparse-checkout.c | 5 ++---
builtin/stash.c | 20 ++++++++++++--------
diff-lib.c | 8 +++++---
dir.c | 8 --------
dir.h | 6 ++++--
merge-ort.c | 12 ++++--------
merge-recursive.c | 6 ++++--
merge.c | 23 +++++++++++------------
reset.c | 4 ++--
sequencer.c | 3 +--
unpack-trees.c | 24 +++++++++++++-----------
unpack-trees.h | 17 +++++++++++++----
21 files changed, 144 insertions(+), 117 deletions(-)
--
2.33.0.1404.g83021034c5d
In the preceding commit we introduced a new UNPACK_TREES_OPTIONS_INIT
macro, but "merge-recursive.c" could not be converted to it since
it (re-)initializes a "struct unpack_trees_options" on the heap.
In order to convert use the macro as the source of truth for
initialization we need to not only convert the initialization in
unpack_trees_start(), but also call the new
unpack_trees_options_init() just after the CALLOC_ARRAY() in
merge_start().
When we call merge_trees() we'll call merge_start() followed by
merge_trees_internal(), and it will call unpack_trees_start() before
it does much of anything. So it's covered by an initialization in
unpack_trees_start().
But when merge_recursive() is called it will call merge_start()
followed by merge_recursive_internal(), which won't call
unpack_trees_start() until its own call call to
merge_trees_internal(), but in the meantime it might end up using that
calloc'd "struct unpack_trees_options".
This was OK before, as setup_unpack_trees_porcelain() would call
strvec_init(), and our "struct dir_struct" in turn is OK with being
NULL'd. Let's convert the former to macro initialization, we'll deal
with the latter in a subsequent commit.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
merge-recursive.c | 3 ++-
unpack-trees.c | 8 ++++++--
unpack-trees.h | 5 ++++-
3 files changed, 12 insertions(+), 4 deletions(-)
@@ -108,8 +108,6 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,constchar**msgs=opts->msgs;constchar*msg;-strvec_init(&opts->msgs_to_free);-if(!strcmp(cmd,"checkout"))msg=advice_enabled(ADVICE_COMMIT_BEFORE_MERGE)?_("Your local changes to the following files would be overwritten by checkout:\n%%s"
Change the clear_unpack_trees_porcelain() to be like a *_release()
function, not a *_reset() (in strbuf.c terms). Let's move the only API
user that relied on the latter to doing its own
unpack_trees_options_init(). See the commit that introduced
unpack_trees_options_init() for details on the control flow involved
here.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
merge-recursive.c | 1 +
unpack-trees.c | 1 -
2 files changed, 1 insertion(+), 1 deletion(-)
Refactor code added in 1c41d2805e4 (unpack_trees_options: free
messages when done, 2018-05-21) to use a ret/goto pattern, rather than
duplicating the end cleanup in the function.
This control flow will be matched in subsequent commits by various
other similar code, which often needs to do more than just call
unpack_trees_options_release(). Let's change this to consistently use
the same pattern everywhere.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
merge.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
@@ -54,6 +54,7 @@ int checkout_fast_forward(struct repository *r,structtree_desct[MAX_UNPACK_TREES];inti,nr_trees=0;structlock_filelock_file=LOCK_INIT;+intret=0;refresh_index(r->index,REFRESH_QUIET,NULL,NULL,NULL);
@@ -95,12 +96,14 @@ int checkout_fast_forward(struct repository *r,if(unpack_trees(nr_trees,t,&opts)){rollback_lock_file(&lock_file);-unpack_trees_options_release(&opts);-return-1;+ret=-1;+gotocleanup;}-unpack_trees_options_release(&opts);if(write_locked_index(r->index,&lock_file,COMMIT_LOCK))-returnerror(_("unable to write new index file"));-return0;+ret=error(_("unable to write new index file"));++cleanup:+unpack_trees_options_release(&opts);+returnret;}
Change the "struct dir" in "struct unpack_trees_options" away from a
pointer to being embedded in the struct itself, this mean that we can
initialize it with our new UNPACK_TREES_OPTIONS_INIT macro.
As it turned out nothing actually needed to provide its own "struct
dir" to this API, there's another patch to "hide" the struct instead,
see the discussion at [1], but I think just allocating it on the stack
along with the rest makes more sense.
This fixes a memory leak in "builtin/checkout.c" that's been there
since the clear_unpack_trees_porcelain() function was added in
1c41d2805e4 (unpack_trees_options: free messages when done,
2018-05-21), i.e. we still had a memory leak from allocating the "dir"
member here. That "dir" member had in turn been with us ever since
782c2d65c24 (Build in checkout, 2008-02-07).
This fixes that memory leak, and allows us to remove the boilerplate
dir_clear() elsewhere in favor of just using
clear_unpack_trees_porcelain().
1. https://lore.kernel.org/git/87ilyjviiy.fsf@evledraar.gmail.com/
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/checkout.c | 5 ++---
builtin/read-tree.c | 11 ++++-------
merge-ort.c | 7 ++-----
merge.c | 7 ++-----
unpack-trees.c | 10 ++++------
unpack-trees.h | 4 +++-
6 files changed, 17 insertions(+), 27 deletions(-)
@@ -53,20 +53,17 @@ static int index_output_cb(const struct option *opt, const char *arg,staticintexclude_per_directory_cb(conststructoption*opt,constchar*arg,intunset){-structdir_struct*dir;structunpack_trees_options*opts;BUG_ON_OPT_NEG(unset);opts=(structunpack_trees_options*)opt->value;-if(opts->dir)+if(opts->dir.exclude_per_dir)die("more than one --exclude-per-directory given.");-dir=xcalloc(1,sizeof(*opts->dir));-dir->flags|=DIR_SHOW_IGNORED;-dir->exclude_per_dir=arg;-opts->dir=dir;+opts->dir.flags|=DIR_SHOW_IGNORED;+opts->dir.exclude_per_dir=arg;/* We do not need to nor want to do read-directory*here;wearemerelyinterestedinreusingthe*perdirectoryignorestackmechanism.
@@ -208,7 +205,7 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)if((opts.update||opts.index_only)&&!opts.merge)die("%s is meaningless without -m, --reset, or --prefix",opts.update?"-u":"-i");-if((opts.dir&&!opts.update))+if((opts.dir.exclude_per_dir&&!opts.update))die("--exclude-per-directory is meaningless unless -u");if(opts.merge&&!opts.index_only)setup_work_tree();
@@ -2085,7 +2086,7 @@ static int verify_clean_subdirectory(const struct cache_entry *ce,*/intnamelen;inti;-structdir_structd;+structdir_structd=DIR_INIT;char*pathbuf;intcnt=0;
@@ -2136,9 +2137,7 @@ static int verify_clean_subdirectory(const struct cache_entry *ce,*/pathbuf=xstrfmt("%.*s/",namelen,ce->name);-memset(&d,0,sizeof(d));-if(o->dir)-d.exclude_per_dir=o->dir->exclude_per_dir;+d.exclude_per_dir=o->dir.exclude_per_dir;i=read_directory(&d,o->src_index,pathbuf,namelen+1,NULL);if(i)returnadd_rejected_path(o,ERROR_NOT_UPTODATE_DIR,ce->name);
@@ -2179,8 +2178,7 @@ static int check_ok_to_remove(const char *name, int len, int dtype,if(ignore_case&&icase_exists(o,name,len,st))return0;-if(o->dir&&-is_excluded(o->dir,o->src_index,name,&dtype))+if(is_excluded(&o->dir,o->src_index,name,&dtype))/**ce->nameisexplicitlyexcluded,soitisOkto*overwriteit.
Almost all users of "struct unpack_trees_options" allocate it on the
stack already, let's provide an *_INIT macro for them to use. This
will make later changes that would like to initialize other members of
the struct easier[1][2], but for now we're keeping it compatible with
a memset() to "0" with an "{ 0 }" initialization.
This covers all callers except a special-case in "merge-recursive.c",
which will be dealt with in a subsequent commit.
1. https://lore.kernel.org/git/87k0ixrv23.fsf@evledraar.gmail.com/
2. https://lore.kernel.org/git/87fstlrumj.fsf@evledraar.gmail.com/
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
archive.c | 3 +--
builtin/am.c | 6 ++----
builtin/checkout.c | 6 ++----
builtin/clone.c | 3 +--
builtin/commit.c | 3 +--
builtin/merge.c | 3 +--
builtin/read-tree.c | 3 +--
builtin/reset.c | 3 +--
builtin/sparse-checkout.c | 3 +--
builtin/stash.c | 6 ++----
diff-lib.c | 3 +--
merge-ort.c | 3 +--
merge.c | 3 +--
reset.c | 2 +-
sequencer.c | 3 +--
unpack-trees.h | 2 ++
16 files changed, 20 insertions(+), 35 deletions(-)
@@ -1911,7 +1911,6 @@ static int fast_forward_to(struct tree *head, struct tree *remote, int reset)refresh_cache(REFRESH_QUIET);-memset(&opts,0,sizeof(opts));opts.head_idx=1;opts.src_index=&the_index;opts.dst_index=&the_index;
@@ -1940,7 +1939,7 @@ static int fast_forward_to(struct tree *head, struct tree *remote, int reset)staticintmerge_tree(structtree*tree){structlock_filelock_file=LOCK_INIT;-structunpack_trees_optionsopts;+structunpack_trees_optionsopts=UNPACK_TREES_OPTIONS_INIT;structtree_desct[1];if(parse_tree(tree))
@@ -1948,7 +1947,6 @@ static int merge_tree(struct tree *tree)hold_locked_index(&lock_file,LOCK_DIE_ON_ERROR);-memset(&opts,0,sizeof(opts));opts.head_idx=1;opts.src_index=&the_index;opts.dst_index=&the_index;
@@ -244,8 +244,6 @@ static int reset_tree(struct object_id *i_tree, int update, int reset)hold_locked_index(&lock_file,LOCK_DIE_ON_ERROR);-memset(&opts,0,sizeof(opts));-tree=parse_tree_indirect(i_tree);if(parse_tree(tree))return-1;
@@ -4021,9 +4021,8 @@ static int checkout(struct merge_options *opt,/* Switch the index/working copy from old to new */intret;structtree_desctrees[2];-structunpack_trees_optionsunpack_opts;+structunpack_trees_optionsunpack_opts=UNPACK_TREES_OPTIONS_INIT;-memset(&unpack_opts,0,sizeof(unpack_opts));unpack_opts.head_idx=-1;unpack_opts.src_index=opt->repo->index;unpack_opts.dst_index=opt->repo->index;
The dir_init() initializer has been documented as being mandatory
since eceba532141 (dir: fix problematic API to avoid memory leaks,
2020-08-18), but both it and my ce93a4c6127 (dir.[ch]: replace
dir_init() with DIR_INIT, 2021-07-01) managed to miss this callsite in
"add-interactive.c" added before those two commits in
ab1e1cccaf6 (built-in add -i: re-implement `add-untracked` in C,
2019-11-29).
In addition my change to remove dir_init() neglected to update this
documentation. Let's use "must be initialized with" in reference to
"DIR_INIT". We have one lazy initialization which pre-dates
eceba532141 in dir.c. Adjusting this callsite is a prerequisite for
removing it in favor of trusting the macro to initialize the "struct
dir_struct" correctly.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
add-interactive.c | 2 +-
dir.h | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
Remove the "Lazy initialization" in prep_exclude() left behind by
aceb9429b37 (prep_exclude: remove the artificial PATH_MAX limit,
2014-07-14).
Now that every caller who sets up a "struct dir_struct" is using the
DIR_INIT macro we can rely on it to have done the initialization. As
noted in an analysis of the previous control flow[1] an earlier
passing of of "dir->basebuf.buf" to strncmp() wasn't buggy, as we'd
only reach that code on subsequent invocations of prep_exclude(),
i.e. after this strbuf_init() had been run. But keeping track of that
makes for hard-to-read code. Let's just rely on the initialization
instead.
This does change the behavior of this code in that it won't be
pre-growing the strbuf to a size of PATH_MAX. I think that's OK.
That we were using PATH_MAX at all is just a relic from this being a
fixed buffer from way back in f87f9497486 (git-ls-files: --exclude
mechanism updates., 2005-07-24).
Pre-allocating PATH_MAX was the opposite of an optimization in this
case. I logged all "basebuf.buf" values when running the test suite,
and by far the most common one (around 80%) is "", which we now won't
allocate at all for, and just use the "strbuf_slopbuf".
The second most common one was "a/", followed by other common cases of
short relative paths. So using the default "struct strbuf" growth
pattern is a much better allocation optimization in this case.
1. https://lore.kernel.org/git/87sfxhohsj.fsf@evledraar.gmail.com/
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
dir.c | 8 --------
dir.h | 4 +++-
2 files changed, 3 insertions(+), 9 deletions(-)
@@ -1550,14 +1550,6 @@ static void prep_exclude(struct dir_struct *dir,if(dir->pattern)return;-/*-*Lazyinitialization.Allcallsitescurrentlyjust-*memset(dir,0,sizeof(*dir))beforeuse.Changingallof-*themseemslotsofworkforlittlebenefit.-*/-if(!dir->basebuf.buf)-strbuf_init(&dir->basebuf,PATH_MAX);-/* Read from the parent directories and push them down. */current=stk?stk->baselen:-1;strbuf_setlen(&dir->basebuf,current<0?0:current);
Since a preceding commit we've been using
clear_unpack_trees_porcelain() to call dir_clear(). So it's no longer
a function that corresponds to setup_unpack_trees_porcelain(), as it
was when it was added in 1c41d2805e4 (unpack_trees_options: free
messages when done, 2018-05-21).
Instead it's similar to strbuf_release() and other similar generic
"free" functions. Let's rename it to avoid any future confusion on the
topic.
Let's also update the API documentation for it to note this, and to
cover e.g. the code added around update_sparsity() in
4ee5d50fc39 (sparse-checkout: use improved unpack_trees porcelain
messages, 2020-03-27).
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/checkout.c | 2 +-
builtin/sparse-checkout.c | 2 +-
merge-ort.c | 2 +-
merge-recursive.c | 2 +-
merge.c | 4 ++--
reset.c | 2 +-
unpack-trees.c | 2 +-
unpack-trees.h | 8 +++++---
8 files changed, 13 insertions(+), 11 deletions(-)
@@ -95,10 +95,10 @@ int checkout_fast_forward(struct repository *r,if(unpack_trees(nr_trees,t,&opts)){rollback_lock_file(&lock_file);-clear_unpack_trees_porcelain(&opts);+unpack_trees_options_release(&opts);return-1;}-clear_unpack_trees_porcelain(&opts);+unpack_trees_options_release(&opts);if(write_locked_index(r->index,&lock_file,COMMIT_LOCK))returnerror(_("unable to write new index file"));
@@ -41,10 +41,12 @@ enum unpack_trees_error_types {voidsetup_unpack_trees_porcelain(structunpack_trees_options*opts,constchar*cmd);-/*-*Freesresourcesallocatedbysetup_unpack_trees_porcelain().+/**+*Freesresourcesallocatedbyfunctionthattakethe"struct+*unpack_trees_options". Always call this after using unpack_trees(),+*update_sparsity()etc.*/-voidclear_unpack_trees_porcelain(structunpack_trees_options*opts);+voidunpack_trees_options_release(structunpack_trees_options*opts);structunpack_trees_options{unsignedintreset,
Fix two different but related memory leaks in
verify_clean_subdirectory(). We leaked both the "pathbuf" if
read_directory() returned non-zero, and we never cleaned up our own
"struct dir_struct" either.
* "pathbuf": When the read_directory() call followed by the
free(pathbuf) was added in c81935348be (Fix switching to a branch
with D/F when current branch has file D., 2007-03-15) we didn't
bother to free() before we called die().
But when this code was later libified in 203a2fe1170 (Allow callers
of unpack_trees() to handle failure, 2008-02-07) we started to leak
as we returned data to the caller. This fixes that memory leak,
which can be observed under SANITIZE=leak with e.g. the
"t1001-read-tree-m-2way.sh" test.
* "struct dir_struct": We've leaked the dir_struct ever since this
code was added back in c81935348be.
When that commit was written there wasn't an equivalent of
dir_clear(). Since it was added in 270be816049 (dir.c: provide
clear_directory() for reclaiming dir_struct memory, 2013-01-06)
we've omitted freeing the memory allocated here.
This memory leak could also be observed under SANITIZE=leak and the
"t1001-read-tree-m-2way.sh" test.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
unpack-trees.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
Plug memory leaks in various built-ins that were missing
unpack_trees_options_release() calls. In an earlier commit these
functions were all made to use the "UNPACK_TREES_OPTIONS_INIT" macro,
now let's have the ones that didn't clean up their memory do so.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
archive.c | 9 +++++++--
builtin/am.c | 17 ++++++++++++-----
builtin/checkout.c | 9 +++++++--
builtin/clone.c | 1 +
builtin/commit.c | 6 +++++-
builtin/merge.c | 6 ++++--
builtin/read-tree.c | 14 ++++++++++----
builtin/reset.c | 13 +++++++++----
builtin/stash.c | 14 ++++++++++----
diff-lib.c | 5 ++++-
10 files changed, 69 insertions(+), 25 deletions(-)
@@ -1903,6 +1903,7 @@ static int fast_forward_to(struct tree *head, struct tree *remote, int reset)structlock_filelock_file=LOCK_INIT;structunpack_trees_optionsopts=UNPACK_TREES_OPTIONS_INIT;structtree_desct[2];+intret=0;if(parse_tree(head)||parse_tree(remote))return-1;
@@ -1923,13 +1924,15 @@ static int fast_forward_to(struct tree *head, struct tree *remote, int reset)if(unpack_trees(2,t,&opts)){rollback_lock_file(&lock_file);-return-1;+ret=-1;+gotocleanup;}if(write_locked_index(&the_index,&lock_file,COMMIT_LOCK))die(_("unable to write new index file"));--return0;+cleanup:+unpack_trees_options_release(&opts);+returnret;}/**
@@ -1941,6 +1944,7 @@ static int merge_tree(struct tree *tree)structlock_filelock_file=LOCK_INIT;structunpack_trees_optionsopts=UNPACK_TREES_OPTIONS_INIT;structtree_desct[1];+intret=0;if(parse_tree(tree))return-1;
@@ -1956,13 +1960,16 @@ static int merge_tree(struct tree *tree)if(unpack_trees(1,t,&opts)){rollback_lock_file(&lock_file);-return-1;+ret=-1;+gotocleanup;}if(write_locked_index(&the_index,&lock_file,COMMIT_LOCK))die(_("unable to write new index file"));-return0;+cleanup:+unpack_trees_options_release(&opts);+returnret;}/**
@@ -697,6 +697,7 @@ static int checkout(int submodule_progress)init_tree_desc(&t,tree->buffer,tree->size);if(unpack_trees(1,&t,&opts)<0)die(_("unable to checkout working tree"));+unpack_trees_options_release(&opts);free(head);
@@ -243,11 +244,13 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)parse_tree(tree);init_tree_desc(t+i,tree->buffer,tree->size);}-if(unpack_trees(nr_trees,t,&opts))-return128;+if(unpack_trees(nr_trees,t,&opts)){+ret=128;+gotocleanup;+}if(opts.debug_unpack||opts.dry_run)-return0;/* do not write the index out */+gotocleanup;/* do not write the index out *//**Whenreadingonlyonetree(eitherthemostbasicform,
@@ -262,5 +265,8 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)if(write_locked_index(&the_index,&lock_file,COMMIT_LOCK))die("unable to write new index file");-return0;++cleanup:+unpack_trees_options_release(&opts);+returnret;}
@@ -78,10 +78,14 @@ static int reset_index(const char *ref, const struct object_id *oid, int reset_tif(reset_type==KEEP){structobject_idhead_oid;-if(get_oid("HEAD",&head_oid))-returnerror(_("You do not have a valid HEAD."));-if(!fill_tree_descriptor(the_repository,desc+nr,&head_oid))-returnerror(_("Failed to find tree of HEAD."));+if(get_oid("HEAD",&head_oid)){+error(_("You do not have a valid HEAD."));+gotoout;+}+if(!fill_tree_descriptor(the_repository,desc+nr,&head_oid)){+error(_("Failed to find tree of HEAD."));+gotoout;+}nr++;opts.fn=twoway_merge;}
@@ -103,6 +107,7 @@ static int reset_index(const char *ref, const struct object_id *oid, int reset_tret=0;out:+unpack_trees_options_release(&opts);for(i=0;i<nr;i++)free((void*)desc[i].buffer);returnret;
@@ -237,6 +237,7 @@ static int reset_tree(struct object_id *i_tree, int update, int reset)structtree_desct[MAX_UNPACK_TREES];structtree*tree;structlock_filelock_file=LOCK_INIT;+intret=0;read_cache_preload(NULL);if(refresh_cache(REFRESH_QUIET))
@@ -258,13 +259,17 @@ static int reset_tree(struct object_id *i_tree, int update, int reset)opts.update=update;opts.fn=oneway_merge;-if(unpack_trees(nr_trees,t,&opts))-return-1;+if(unpack_trees(nr_trees,t,&opts)){+ret=-1;+gotocleanup;+}if(write_locked_index(&the_index,&lock_file,COMMIT_LOCK))-returnerror(_("unable to write new index file"));+ret=error(_("unable to write new index file"));-return0;+cleanup:+unpack_trees_options_release(&opts);+returnret;}staticintdiff_tree_binary(structstrbuf*out,structobject_id*w_commit)
Hi Ævar
On 04/10/2021 01:46, Ævar Arnfjörð Bjarmason wrote:
Change the clear_unpack_trees_porcelain() to be like a *_release()
function, not a *_reset() (in strbuf.c terms). Let's move the only API
user that relied on the latter to doing its own
unpack_trees_options_init(). See the commit that introduced
unpack_trees_options_init() for details on the control flow involved
here.
Before this change if there was a call to unpack_trees() after
clear_unpack_trees_porcelain() then that caller would get the default
error messages. After this change we end up with a use-after-free error
in that situation. I found the subject line of this patch hard to
understand, the commit message explains what it is doing but I'm still
not sure what the motivation for this change is.
Best Wishes
Phillip
Hi Ævar
On 04/10/2021 01:46, Ævar Arnfjörð Bjarmason wrote:
Since a preceding commit we've been using
clear_unpack_trees_porcelain() to call dir_clear(). So it's no longer
a function that corresponds to setup_unpack_trees_porcelain(), as it
was when it was added in 1c41d2805e4 (unpack_trees_options: free
messages when done, 2018-05-21).
Instead it's similar to strbuf_release() and other similar generic
"free" functions. Let's rename it to avoid any future confusion on the
topic.
Let's also update the API documentation for it to note this, and to
cover e.g. the code added around update_sparsity() in
4ee5d50fc39 (sparse-checkout: use improved unpack_trees porcelain
messages, 2020-03-27).
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/checkout.c | 2 +-
builtin/sparse-checkout.c | 2 +-
merge-ort.c | 2 +-
merge-recursive.c | 2 +-
merge.c | 4 ++--
reset.c | 2 +-
I was expecting to see sequencer.c here, but it appears it does not call
clear_unpack_trees_porcelain() to free the memory allocated with
setup_unpack_trees_porcelain(). If you're interested in fixing memory
leaks from unpack trees it might be worth checking that all the calls to
setup_unpack_trees_porcelain() have a matching call to
clear_unpack_trees_porcelain().
Best Wishes
Phillip
@@ -95,10 +95,10 @@ int checkout_fast_forward(struct repository *r,if(unpack_trees(nr_trees,t,&opts)){rollback_lock_file(&lock_file);-clear_unpack_trees_porcelain(&opts);+unpack_trees_options_release(&opts);return-1;}-clear_unpack_trees_porcelain(&opts);+unpack_trees_options_release(&opts);if(write_locked_index(r->index,&lock_file,COMMIT_LOCK))returnerror(_("unable to write new index file"));
@@ -41,10 +41,12 @@ enum unpack_trees_error_types {voidsetup_unpack_trees_porcelain(structunpack_trees_options*opts,constchar*cmd);-/*-*Freesresourcesallocatedbysetup_unpack_trees_porcelain().+/**+*Freesresourcesallocatedbyfunctionthattakethe"struct+*unpack_trees_options". Always call this after using unpack_trees(),+*update_sparsity()etc.*/-voidclear_unpack_trees_porcelain(structunpack_trees_options*opts);+voidunpack_trees_options_release(structunpack_trees_options*opts);structunpack_trees_options{unsignedintreset,
Hi Ævar
On 04/10/2021 01:46, Ævar Arnfjörð Bjarmason wrote:
quoted
Since a preceding commit we've been using
clear_unpack_trees_porcelain() to call dir_clear(). So it's no longer
a function that corresponds to setup_unpack_trees_porcelain(), as it
was when it was added in 1c41d2805e4 (unpack_trees_options: free
messages when done, 2018-05-21).
Instead it's similar to strbuf_release() and other similar generic
"free" functions. Let's rename it to avoid any future confusion on the
topic.
Let's also update the API documentation for it to note this, and to
cover e.g. the code added around update_sparsity() in
4ee5d50fc39 (sparse-checkout: use improved unpack_trees porcelain
messages, 2020-03-27).
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/checkout.c | 2 +-
builtin/sparse-checkout.c | 2 +-
merge-ort.c | 2 +-
merge-recursive.c | 2 +-
merge.c | 4 ++--
reset.c | 2 +-
I was expecting to see sequencer.c here, but it appears it does not
call clear_unpack_trees_porcelain() to free the memory allocated with
setup_unpack_trees_porcelain(). If you're interested in fixing memory
leaks from unpack trees it might be worth checking that all the calls
to setup_unpack_trees_porcelain() have a matching call to
clear_unpack_trees_porcelain().
Well spotted, I thought I was doing that, but see that I missed this one
special-case, will fix.
With this series grepping for "UNPACK_TREES_OPTIONS_INIT" is better (and
knowing about the one special-case of merge-recursive.c).
Hi Ævar
On 04/10/2021 01:46, Ævar Arnfjörð Bjarmason wrote:
quoted
Change the clear_unpack_trees_porcelain() to be like a *_release()
function, not a *_reset() (in strbuf.c terms). Let's move the only API
user that relied on the latter to doing its own
unpack_trees_options_init(). See the commit that introduced
unpack_trees_options_init() for details on the control flow involved
here.
Before this change if there was a call to unpack_trees() after
clear_unpack_trees_porcelain() then that caller would get the default
error messages. After this change we end up with a use-after-free
error in that situation. I found the subject line of this patch hard
to understand, the commit message explains what it is doing but I'm
still not sure what the motivation for this change is.
I'll work on the commit message part.
With this series such a caller is purely hypothetical, isn't it?
I.e. the journey in 02/10 & 04/10, and later in the 07/10 you commented
on is to make the API behave similarly to e.g. strbuf, where there's a
release() that leaves it in such a state, different from a "reset".
Perhaps this step in isolation is confusing at it leaves the function
named as clear_unpack_trees_porcelain(). I had this all in one change
initially, but figured having such a large rename diff & one behavior
change was worse.
We could just leave the "reset" semantics in place for everyone, but
just like "strbuf_release()" et al I think it's good for
self-documentation purposes to explicitly make clear if you're re-using
the struct, or just freeing it at the end.
For this API only one user of the API cares about the re-use case,
merge-recursive.c.
On Sun, Oct 3, 2021 at 5:46 PM Ævar Arnfjörð Bjarmason [off-list ref] wrote:
quoted hunk
Fix two different but related memory leaks in
verify_clean_subdirectory(). We leaked both the "pathbuf" if
read_directory() returned non-zero, and we never cleaned up our own
"struct dir_struct" either.
* "pathbuf": When the read_directory() call followed by the
free(pathbuf) was added in c81935348be (Fix switching to a branch
with D/F when current branch has file D., 2007-03-15) we didn't
bother to free() before we called die().
But when this code was later libified in 203a2fe1170 (Allow callers
of unpack_trees() to handle failure, 2008-02-07) we started to leak
as we returned data to the caller. This fixes that memory leak,
which can be observed under SANITIZE=leak with e.g. the
"t1001-read-tree-m-2way.sh" test.
* "struct dir_struct": We've leaked the dir_struct ever since this
code was added back in c81935348be.
When that commit was written there wasn't an equivalent of
dir_clear(). Since it was added in 270be816049 (dir.c: provide
clear_directory() for reclaiming dir_struct memory, 2013-01-06)
we've omitted freeing the memory allocated here.
This memory leak could also be observed under SANITIZE=leak and the
"t1001-read-tree-m-2way.sh" test.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
unpack-trees.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
On Sun, Oct 3, 2021 at 5:46 PM Ævar Arnfjörð Bjarmason [off-list ref] wrote:
Plug memory leaks in various built-ins that were missing
unpack_trees_options_release() calls. In an earlier commit these
functions were all made to use the "UNPACK_TREES_OPTIONS_INIT" macro,
now let's have the ones that didn't clean up their memory do so.
This commit message doesn't say anything about what was leaking. You
should really bring up that it was the dir_clear() you added to
unpack_trees_options_release() earlier in the series that is important
here (or at least that's what I presume is the important fix).
@@ -1903,6 +1903,7 @@ static int fast_forward_to(struct tree *head, struct tree *remote, int reset)structlock_filelock_file=LOCK_INIT;structunpack_trees_optionsopts=UNPACK_TREES_OPTIONS_INIT;structtree_desct[2];+intret=0;if(parse_tree(head)||parse_tree(remote))return-1;
@@ -1923,13 +1924,15 @@ static int fast_forward_to(struct tree *head, struct tree *remote, int reset)if(unpack_trees(2,t,&opts)){rollback_lock_file(&lock_file);-return-1;+ret=-1;+gotocleanup;}if(write_locked_index(&the_index,&lock_file,COMMIT_LOCK))die(_("unable to write new index file"));--return0;+cleanup:+unpack_trees_options_release(&opts);+returnret;}/**
@@ -1941,6 +1944,7 @@ static int merge_tree(struct tree *tree)structlock_filelock_file=LOCK_INIT;structunpack_trees_optionsopts=UNPACK_TREES_OPTIONS_INIT;structtree_desct[1];+intret=0;if(parse_tree(tree))return-1;
@@ -1956,13 +1960,16 @@ static int merge_tree(struct tree *tree)if(unpack_trees(1,t,&opts)){rollback_lock_file(&lock_file);-return-1;+ret=-1;+gotocleanup;}if(write_locked_index(&the_index,&lock_file,COMMIT_LOCK))die(_("unable to write new index file"));-return0;+cleanup:+unpack_trees_options_release(&opts);+returnret;}/**
@@ -697,6 +697,7 @@ static int checkout(int submodule_progress)init_tree_desc(&t,tree->buffer,tree->size);if(unpack_trees(1,&t,&opts)<0)die(_("unable to checkout working tree"));+unpack_trees_options_release(&opts);free(head);
@@ -243,11 +244,13 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)parse_tree(tree);init_tree_desc(t+i,tree->buffer,tree->size);}-if(unpack_trees(nr_trees,t,&opts))-return128;+if(unpack_trees(nr_trees,t,&opts)){+ret=128;+gotocleanup;+}if(opts.debug_unpack||opts.dry_run)-return0;/* do not write the index out */+gotocleanup;/* do not write the index out *//**Whenreadingonlyonetree(eitherthemostbasicform,
@@ -262,5 +265,8 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)if(write_locked_index(&the_index,&lock_file,COMMIT_LOCK))die("unable to write new index file");-return0;++cleanup:+unpack_trees_options_release(&opts);+returnret;}
@@ -78,10 +78,14 @@ static int reset_index(const char *ref, const struct object_id *oid, int reset_tif(reset_type==KEEP){structobject_idhead_oid;-if(get_oid("HEAD",&head_oid))-returnerror(_("You do not have a valid HEAD."));-if(!fill_tree_descriptor(the_repository,desc+nr,&head_oid))-returnerror(_("Failed to find tree of HEAD."));+if(get_oid("HEAD",&head_oid)){+error(_("You do not have a valid HEAD."));+gotoout;+}+if(!fill_tree_descriptor(the_repository,desc+nr,&head_oid)){+error(_("Failed to find tree of HEAD."));+gotoout;+}nr++;opts.fn=twoway_merge;}
@@ -103,6 +107,7 @@ static int reset_index(const char *ref, const struct object_id *oid, int reset_tret=0;out:+unpack_trees_options_release(&opts);for(i=0;i<nr;i++)free((void*)desc[i].buffer);returnret;
@@ -237,6 +237,7 @@ static int reset_tree(struct object_id *i_tree, int update, int reset)structtree_desct[MAX_UNPACK_TREES];structtree*tree;structlock_filelock_file=LOCK_INIT;+intret=0;read_cache_preload(NULL);if(refresh_cache(REFRESH_QUIET))
@@ -258,13 +259,17 @@ static int reset_tree(struct object_id *i_tree, int update, int reset)opts.update=update;opts.fn=oneway_merge;-if(unpack_trees(nr_trees,t,&opts))-return-1;+if(unpack_trees(nr_trees,t,&opts)){+ret=-1;+gotocleanup;+}if(write_locked_index(&the_index,&lock_file,COMMIT_LOCK))-returnerror(_("unable to write new index file"));+ret=error(_("unable to write new index file"));-return0;+cleanup:+unpack_trees_options_release(&opts);+returnret;}staticintdiff_tree_binary(structstrbuf*out,structobject_id*w_commit)
On Sun, Oct 3, 2021 at 5:46 PM Ævar Arnfjörð Bjarmason [off-list ref] wrote:
The subject seems misleading; this patch doesn't make DIR_INIT
mandatory, it is just updating the docs to reflect that and updating
an out-of-date callsite.
quoted hunk
The dir_init() initializer has been documented as being mandatory
since eceba532141 (dir: fix problematic API to avoid memory leaks,
2020-08-18), but both it and my ce93a4c6127 (dir.[ch]: replace
dir_init() with DIR_INIT, 2021-07-01) managed to miss this callsite in
"add-interactive.c" added before those two commits in
ab1e1cccaf6 (built-in add -i: re-implement `add-untracked` in C,
2019-11-29).
In addition my change to remove dir_init() neglected to update this
documentation. Let's use "must be initialized with" in reference to
"DIR_INIT". We have one lazy initialization which pre-dates
eceba532141 in dir.c. Adjusting this callsite is a prerequisite for
removing it in favor of trusting the macro to initialize the "struct
dir_struct" correctly.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
add-interactive.c | 2 +-
dir.h | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
On Sun, Oct 3, 2021 at 5:46 PM Ævar Arnfjörð Bjarmason [off-list ref] wrote:
quoted hunk
Change the clear_unpack_trees_porcelain() to be like a *_release()
function, not a *_reset() (in strbuf.c terms). Let's move the only API
user that relied on the latter to doing its own
unpack_trees_options_init(). See the commit that introduced
unpack_trees_options_init() for details on the control flow involved
here.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
merge-recursive.c | 1 +
unpack-trees.c | 1 -
2 files changed, 1 insertion(+), 1 deletion(-)
This is wrong. It suggests that unpack_opts is used after
unpack_trees_finish() (other than an outer merge first calling
unpack_trees_start() again), which can only serve to greatly confuse
future readers. Drop this hunk.
quoted hunk
}
static int save_files_dirs(const struct object_id *oid,
On Sun, Oct 3, 2021 at 5:46 PM Ævar Arnfjörð Bjarmason [off-list ref] wrote:
Remove the "Lazy initialization" in prep_exclude() left behind by
aceb9429b37 (prep_exclude: remove the artificial PATH_MAX limit,
2014-07-14).
Now that every caller who sets up a "struct dir_struct" is using the
DIR_INIT macro we can rely on it to have done the initialization. As
noted in an analysis of the previous control flow[1] an earlier
passing of of "dir->basebuf.buf" to strncmp() wasn't buggy, as we'd
only reach that code on subsequent invocations of prep_exclude(),
i.e. after this strbuf_init() had been run. But keeping track of that
makes for hard-to-read code. Let's just rely on the initialization
instead.
Having read through the link previously, this all makes sense to me,
but I'm not sure if this paragraph motivates the change without that
context. Maybe another reader can comment.
quoted hunk
This does change the behavior of this code in that it won't be
pre-growing the strbuf to a size of PATH_MAX. I think that's OK.
That we were using PATH_MAX at all is just a relic from this being a
fixed buffer from way back in f87f9497486 (git-ls-files: --exclude
mechanism updates., 2005-07-24).
Pre-allocating PATH_MAX was the opposite of an optimization in this
case. I logged all "basebuf.buf" values when running the test suite,
and by far the most common one (around 80%) is "", which we now won't
allocate at all for, and just use the "strbuf_slopbuf".
The second most common one was "a/", followed by other common cases of
short relative paths. So using the default "struct strbuf" growth
pattern is a much better allocation optimization in this case.
1. https://lore.kernel.org/git/87sfxhohsj.fsf@evledraar.gmail.com/
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
dir.c | 8 --------
dir.h | 4 +++-
2 files changed, 3 insertions(+), 9 deletions(-)
@@ -1550,14 +1550,6 @@ static void prep_exclude(struct dir_struct *dir,if(dir->pattern)return;-/*-*Lazyinitialization.Allcallsitescurrentlyjust-*memset(dir,0,sizeof(*dir))beforeuse.Changingallof-*themseemslotsofworkforlittlebenefit.-*/-if(!dir->basebuf.buf)-strbuf_init(&dir->basebuf,PATH_MAX);-/* Read from the parent directories and push them down. */current=stk?stk->baselen:-1;strbuf_setlen(&dir->basebuf,current<0?0:current);
On Sun, Oct 3, 2021 at 5:46 PM Ævar Arnfjörð Bjarmason [off-list ref] wrote:
quoted hunk
Change the "struct dir" in "struct unpack_trees_options" away from a
pointer to being embedded in the struct itself, this mean that we can
initialize it with our new UNPACK_TREES_OPTIONS_INIT macro.
As it turned out nothing actually needed to provide its own "struct
dir" to this API, there's another patch to "hide" the struct instead,
see the discussion at [1], but I think just allocating it on the stack
along with the rest makes more sense.
This fixes a memory leak in "builtin/checkout.c" that's been there
since the clear_unpack_trees_porcelain() function was added in
1c41d2805e4 (unpack_trees_options: free messages when done,
2018-05-21), i.e. we still had a memory leak from allocating the "dir"
member here. That "dir" member had in turn been with us ever since
782c2d65c24 (Build in checkout, 2008-02-07).
This fixes that memory leak, and allows us to remove the boilerplate
dir_clear() elsewhere in favor of just using
clear_unpack_trees_porcelain().
1. https://lore.kernel.org/git/87ilyjviiy.fsf@evledraar.gmail.com/
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/checkout.c | 5 ++---
builtin/read-tree.c | 11 ++++-------
merge-ort.c | 7 ++-----
merge.c | 7 ++-----
unpack-trees.c | 10 ++++------
unpack-trees.h | 4 +++-
6 files changed, 17 insertions(+), 27 deletions(-)
@@ -53,20 +53,17 @@ static int index_output_cb(const struct option *opt, const char *arg,staticintexclude_per_directory_cb(conststructoption*opt,constchar*arg,intunset){-structdir_struct*dir;structunpack_trees_options*opts;BUG_ON_OPT_NEG(unset);opts=(structunpack_trees_options*)opt->value;-if(opts->dir)+if(opts->dir.exclude_per_dir)die("more than one --exclude-per-directory given.");-dir=xcalloc(1,sizeof(*opts->dir));-dir->flags|=DIR_SHOW_IGNORED;-dir->exclude_per_dir=arg;-opts->dir=dir;+opts->dir.flags|=DIR_SHOW_IGNORED;+opts->dir.exclude_per_dir=arg;/* We do not need to nor want to do read-directory*here;wearemerelyinterestedinreusingthe*perdirectoryignorestackmechanism.
@@ -208,7 +205,7 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)if((opts.update||opts.index_only)&&!opts.merge)die("%s is meaningless without -m, --reset, or --prefix",opts.update?"-u":"-i");-if((opts.dir&&!opts.update))+if((opts.dir.exclude_per_dir&&!opts.update))die("--exclude-per-directory is meaningless unless -u");if(opts.merge&&!opts.index_only)setup_work_tree();
@@ -2085,7 +2086,7 @@ static int verify_clean_subdirectory(const struct cache_entry *ce,*/intnamelen;inti;-structdir_structd;+structdir_structd=DIR_INIT;char*pathbuf;intcnt=0;
@@ -2136,9 +2137,7 @@ static int verify_clean_subdirectory(const struct cache_entry *ce,*/pathbuf=xstrfmt("%.*s/",namelen,ce->name);-memset(&d,0,sizeof(d));-if(o->dir)-d.exclude_per_dir=o->dir->exclude_per_dir;+d.exclude_per_dir=o->dir.exclude_per_dir;i=read_directory(&d,o->src_index,pathbuf,namelen+1,NULL);if(i)returnadd_rejected_path(o,ERROR_NOT_UPTODATE_DIR,ce->name);
@@ -2179,8 +2178,7 @@ static int check_ok_to_remove(const char *name, int len, int dtype,if(ignore_case&&icase_exists(o,name,len,st))return0;-if(o->dir&&-is_excluded(o->dir,o->src_index,name,&dtype))+if(is_excluded(&o->dir,o->src_index,name,&dtype))/**ce->nameisexplicitlyexcluded,soitisOkto*overwriteit.
This patch is going to conflict HEAVILY with
en/removing-untracked-fixes, which Junio already said he'd merge to
next.
You should rebase your patches on that series.
On Sun, Oct 3, 2021 at 5:46 PM Ævar Arnfjörð Bjarmason [off-list ref] wrote:
quoted hunk
Refactor code added in 1c41d2805e4 (unpack_trees_options: free
messages when done, 2018-05-21) to use a ret/goto pattern, rather than
duplicating the end cleanup in the function.
This control flow will be matched in subsequent commits by various
other similar code, which often needs to do more than just call
unpack_trees_options_release(). Let's change this to consistently use
the same pattern everywhere.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
merge.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
@@ -54,6 +54,7 @@ int checkout_fast_forward(struct repository *r,structtree_desct[MAX_UNPACK_TREES];inti,nr_trees=0;structlock_filelock_file=LOCK_INIT;+intret=0;refresh_index(r->index,REFRESH_QUIET,NULL,NULL,NULL);
@@ -95,12 +96,14 @@ int checkout_fast_forward(struct repository *r,if(unpack_trees(nr_trees,t,&opts)){rollback_lock_file(&lock_file);-unpack_trees_options_release(&opts);-return-1;+ret=-1;+gotocleanup;}-unpack_trees_options_release(&opts);if(write_locked_index(r->index,&lock_file,COMMIT_LOCK))-returnerror(_("unable to write new index file"));-return0;+ret=error(_("unable to write new index file"));++cleanup:+unpack_trees_options_release(&opts);+returnret;}--
2.33.0.1404.g83021034c5d
I would say 'meh'...but the commit message is downright confusing. It
suggests that subsequent commits, plural, will be modifying this code
further. There is only one more commit in this series, and looking
ahead, it doesn't even touch this file. So, there actually isn't any
reason for these changes beyond what we see in this file, at least not
for this series. And as far as this series is concerned, I think it's
actually less readable. If there's a subsequent series that will use
this and make further changes I could be convinced, but then I'd say
this commit belongs as part of the later series, not this one.
On Sun, Oct 3, 2021 at 5:46 PM Ævar Arnfjörð Bjarmason [off-list ref] wrote:
In the preceding commit we introduced a new UNPACK_TREES_OPTIONS_INIT
macro, but "merge-recursive.c" could not be converted to it since
it (re-)initializes a "struct unpack_trees_options" on the heap.
In order to convert use the macro as the source of truth for
initialization we need to not only convert the initialization in
unpack_trees_start(), but also call the new
unpack_trees_options_init() just after the CALLOC_ARRAY() in
merge_start().
Um...why?
When we call merge_trees() we'll call merge_start() followed by
merge_trees_internal(), and it will call unpack_trees_start() before
it does much of anything. So it's covered by an initialization in
unpack_trees_start().
But when merge_recursive() is called it will call merge_start()
followed by merge_recursive_internal(), which won't call
unpack_trees_start() until its own call call to
merge_trees_internal(), but in the meantime it might end up using that
calloc'd "struct unpack_trees_options".
Nothing in merge-recursive.c usings unpack_opts before
unpack_trees_start() or after unpack_trees_finish(). If anyone
attempts to use it elsewhere, that would itself be a bug. So, I'd
replace the above three paragraphs with:
"Change the initialization of opt->priv_unpack_opts from using memset
to 0, with unpack_trees_options_init()."
or something like that, and then drop your change to merge_start().
This was OK before, as setup_unpack_trees_porcelain() would call
strvec_init(), and our "struct dir_struct" in turn is OK with being
NULL'd. Let's convert the former to macro initialization, we'll deal
with the latter in a subsequent commit.
This is quite confusing; it's really hard to understand how this
relates to the rest of the commit message. I have to read the code to
see what you're doing, and then write my own commit message in my head
because the wording in this paragraph still doesn't parse.
I'd make the change strvec_init/STRVEC_INIT changes be a separate
patch. I suspect it'll be easier to write the commit message for it
as a standalone commit as well.
@@ -108,8 +108,6 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,constchar**msgs=opts->msgs;constchar*msg;-strvec_init(&opts->msgs_to_free);-if(!strcmp(cmd,"checkout"))msg=advice_enabled(ADVICE_COMMIT_BEFORE_MERGE)?_("Your local changes to the following files would be overwritten by checkout:\n%%s"
On Sun, Oct 3, 2021 at 5:46 PM Ævar Arnfjörð Bjarmason [off-list ref] wrote:
This series fixes memory leaks in the unpack-trees and dir APIs for
all their callers.
There are several good fixes in this series. Thanks for working on them!
There's been a discussion between myself & Elijah
on his en/removing-untracked-fixes series[1] about the memory leak
fixing aspect of his series.
Not really, the memory leak fixing aspect of my series was patch 2;
most of our discussion was on patch 4, including your footnote link.
Patch 4 did not in any way involve fixing a memory leak, which you
yourself later acknowledged. So most of our discussion was mostly
about aspects _other_ than leak fixing.
I've got locally queued patches that fix widespread memory leaks in
the test suite and make much of it pass under SANITIZE=leak, once the
common leaks in revisions.c (git rev-list/show/log etc.), "checkout",
"dir" and "unpack-trees" are fixed a lot of tests become entirely
leak-free, as much code that needs to setup various basic things will
require one of those commands.
Yaay! This is great stuff!
I think that the more narrow fixes[2] to the memory leaks around
unpack-trees in Elijah's series[3] are better skipped and that series
rebased on top of this one (I'll submit an RFC version of his that is
rebased on this as a follow-up).
I *strongly* disagree.
I.e. his solves a very small amount of the memory leaks in this area,
whereas this is something I've got running as part of end-to-end
SANITIZE=leak testing, so I think that the difference in approaches we
picked when it comes to fixing them is likely because of that.
E.g. continuing to allocate the "struct dir_struct" on the heap in his
version[4] in his is, I think, something that makes more sense for
fixes that haven't pulled at the thread of how much merge-recursive.c
is making that question of ownerhip confusing. There's also changen in
his that'll become simpler as the complexity of the underlying APIs
has gone away, e.g. [6].
*Sigh*. unpack_trees_options->dir is not allocated on the heap at the
end of my series. I could understand missing that in the patches, but
I've also pointed it out to you two additional times in discussions on
the patches so far. And you supposedly looked at all the patches
again while rebasing and adding your signed-off-by.
You also continue to refer to our discussion as though it was about
leakfixes, even though the patch we discussed in my series did not
involve any leak fixing. I pointed that out and you said you stood
corrected (last comment at
https://lore.kernel.org/git/87k0ivpzfx.fsf@evledraar.gmail.com/), but
now you're referring to it that way again? Even after rebasing my
series and adding your Signed-off-by, suggesting you should understand
it? The leakfix was a different patch of the series, namely patch #2.
I agree that merge-recursive.c has confusing points. I totally agree.
Unfortunately, both your patches that touch merge-recursive.c make it
worse; more so than the problems you were trying to fix in that file.
merge-recursive.c: call a new unpack_trees_options_init() function
Details how merge-recursive.c calls unpack_trees() differently than
every other caller when it comes to "struct unpack_trees_options"
setup.
Saying:
"merge-recursive.c has a heap-allocated unpack_trees_options, and thus
can't naturally use UNPACK_TREES_OPTIONS_INIT"
would have been shorter, and also explained things in full detail.
Your version makes it sound like it's doing something really weird and
needs a much more expansive explanation.
unpack-trees.[ch]: embed "dir" in "struct unpack_trees_options"
Elijah's series ends up with a "dir" still heap-allocated in "struct
unpack_trees_options", just dynamically and "privately".
As noted above, this is not true. I'm confused why you try to claim
otherwise. (I mean, it's really not all that important, I'm just
confused why you find it important to call out, especially when the
stack-based point was highlighted multiple times before but you still
insist on referring to it as heap-allocated.)
...
unpack-trees: don't leak memory in verify_clean_subdirectory()
merge.c: avoid duplicate unpack_trees_options_release() code
built-ins: plug memory leaks with unpack_trees_options_release()
A lot of memory leak fixes both in unpack-trees.c and its users, only
a subset of this is in Elijah's series.
Not sure why you feel the need to include the final phrase there; it
almost feels like you're trying to portray my series as a leakfix,
which feels misleading. My series wasn't even about fixing leaks. In
my first round, I knew of leaks, and intentionally attempted to avoid
fixing them because it was orthogonal to the point of my series (I
figured I could come back in a follow-on series and deal with it). In
a subsequent round, I fixed one leak incidentally, in part because you
called it out, but more so because otherwise when I attempted to
consolidate code into one place it would appear to reviewers that the
consolidated code didn't match some of the callers. In particular,
some of the sites had a leak and others didn't. Adding a preparatory
leakfix (again, patch #2, NOT patch #4) made clear that the later
consolidation (in patch #4) really was just that -- moving several
identical code chunks into a single place.
....anyway, all that said, you've got some good fixes in this series.
You've also got three very problematic bits that need to be ripped
out. And you should rebase this series on top of v3 of
en/removing-untracked-fixes.
On Sun, Oct 3, 2021 at 5:46 PM Ævar Arnfjörð Bjarmason [off-list ref] wrote:
quoted hunk
Since a preceding commit we've been using
clear_unpack_trees_porcelain() to call dir_clear(). So it's no longer
a function that corresponds to setup_unpack_trees_porcelain(), as it
was when it was added in 1c41d2805e4 (unpack_trees_options: free
messages when done, 2018-05-21).
Instead it's similar to strbuf_release() and other similar generic
"free" functions. Let's rename it to avoid any future confusion on the
topic.
Let's also update the API documentation for it to note this, and to
cover e.g. the code added around update_sparsity() in
4ee5d50fc39 (sparse-checkout: use improved unpack_trees porcelain
messages, 2020-03-27).
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/checkout.c | 2 +-
builtin/sparse-checkout.c | 2 +-
merge-ort.c | 2 +-
merge-recursive.c | 2 +-
merge.c | 4 ++--
reset.c | 2 +-
unpack-trees.c | 2 +-
unpack-trees.h | 8 +++++---
8 files changed, 13 insertions(+), 11 deletions(-)
@@ -95,10 +95,10 @@ int checkout_fast_forward(struct repository *r,if(unpack_trees(nr_trees,t,&opts)){rollback_lock_file(&lock_file);-clear_unpack_trees_porcelain(&opts);+unpack_trees_options_release(&opts);return-1;}-clear_unpack_trees_porcelain(&opts);+unpack_trees_options_release(&opts);if(write_locked_index(r->index,&lock_file,COMMIT_LOCK))returnerror(_("unable to write new index file"));
@@ -41,10 +41,12 @@ enum unpack_trees_error_types {voidsetup_unpack_trees_porcelain(structunpack_trees_options*opts,constchar*cmd);-/*-*Freesresourcesallocatedbysetup_unpack_trees_porcelain().+/**+*Freesresourcesallocatedbyfunctionthattakethe"struct+*unpack_trees_options". Always call this after using unpack_trees(),+*update_sparsity()etc.*/-voidclear_unpack_trees_porcelain(structunpack_trees_options*opts);+voidunpack_trees_options_release(structunpack_trees_options*opts);structunpack_trees_options{unsignedintreset,--
On Sun, Oct 3, 2021 at 5:46 PM Ævar Arnfjörð Bjarmason [off-list ref] wrote:
quoted
In the preceding commit we introduced a new UNPACK_TREES_OPTIONS_INIT
macro, but "merge-recursive.c" could not be converted to it since
it (re-)initializes a "struct unpack_trees_options" on the heap.
In order to convert use the macro as the source of truth for
initialization we need to not only convert the initialization in
unpack_trees_start(), but also call the new
unpack_trees_options_init() just after the CALLOC_ARRAY() in
merge_start().
Um...why?
Replied below.
quoted
When we call merge_trees() we'll call merge_start() followed by
merge_trees_internal(), and it will call unpack_trees_start() before
it does much of anything. So it's covered by an initialization in
unpack_trees_start().
But when merge_recursive() is called it will call merge_start()
followed by merge_recursive_internal(), which won't call
unpack_trees_start() until its own call call to
merge_trees_internal(), but in the meantime it might end up using that
calloc'd "struct unpack_trees_options".
Nothing in merge-recursive.c usings unpack_opts before
unpack_trees_start() or after unpack_trees_finish(). If anyone
attempts to use it elsewhere, that would itself be a bug. So, I'd
replace the above three paragraphs with:
"Change the initialization of opt->priv_unpack_opts from using memset
to 0, with unpack_trees_options_init()."
or something like that, and then drop your change to merge_start().
Sure, I'll defer to you about being confident about that. I didn't want
to leave a copy if it hanging without the proper initialization in case
there were new callers.
quoted
This was OK before, as setup_unpack_trees_porcelain() would call
strvec_init(), and our "struct dir_struct" in turn is OK with being
NULL'd. Let's convert the former to macro initialization, we'll deal
with the latter in a subsequent commit.
This is quite confusing; it's really hard to understand how this
relates to the rest of the commit message. I have to read the code to
see what you're doing, and then write my own commit message in my head
because the wording in this paragraph still doesn't parse.
I'd make the change strvec_init/STRVEC_INIT changes be a separate
patch. I suspect it'll be easier to write the commit message for it
as a standalone commit as well.
Sure, FWIW I had it split up locally, and figured it would be easier to
explain if the API usage change came with the initialization change that
made it possible.
@@ -405,7 +405,7 @@ static int unpack_trees_start(struct merge_options *opt,structtree_desct[3];structindex_statetmp_index={NULL};-memset(&opt->priv->unpack_opts,0,sizeof(opt->priv->unpack_opts));+unpack_trees_options_init(&opt->priv->unpack_opts);if(opt->priv->call_depth)opt->priv->unpack_opts.index_only=1;else
@@ -3704,6 +3704,7 @@ static int merge_start(struct merge_options *opt, struct tree *head)CALLOC_ARRAY(opt->priv,1);string_list_init_dup(&opt->priv->df_conflict_file_set);+unpack_trees_options_init(&opt->priv->unpack_opts);
Drop this hunk.
Speaking of splitting things up in more understandable ways: If we're
going to hard rely on the interaction between merge_start() and
unpack_trees_start() wouldn't it make sense to lead with a change that
dropped that memset, which if this invariant holds is also redundant to
the CALLOC() of opt->priv in merge_start() in the pre-image.
On Sun, Oct 3, 2021 at 5:46 PM Ævar Arnfjörð Bjarmason [off-list ref] wrote:
quoted
Refactor code added in 1c41d2805e4 (unpack_trees_options: free
messages when done, 2018-05-21) to use a ret/goto pattern, rather than
duplicating the end cleanup in the function.
This control flow will be matched in subsequent commits by various
other similar code, which often needs to do more than just call
unpack_trees_options_release(). Let's change this to consistently use
the same pattern everywhere.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
merge.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
@@ -54,6 +54,7 @@ int checkout_fast_forward(struct repository *r,structtree_desct[MAX_UNPACK_TREES];inti,nr_trees=0;structlock_filelock_file=LOCK_INIT;+intret=0;refresh_index(r->index,REFRESH_QUIET,NULL,NULL,NULL);
@@ -95,12 +96,14 @@ int checkout_fast_forward(struct repository *r,if(unpack_trees(nr_trees,t,&opts)){rollback_lock_file(&lock_file);-unpack_trees_options_release(&opts);-return-1;+ret=-1;+gotocleanup;}-unpack_trees_options_release(&opts);if(write_locked_index(r->index,&lock_file,COMMIT_LOCK))-returnerror(_("unable to write new index file"));-return0;+ret=error(_("unable to write new index file"));++cleanup:+unpack_trees_options_release(&opts);+returnret;}--
2.33.0.1404.g83021034c5d
I would say 'meh'...but the commit message is downright confusing. It
suggests that subsequent commits, plural, will be modifying this code
further. There is only one more commit in this series, and looking
ahead, it doesn't even touch this file. So, there actually isn't any
reason for these changes beyond what we see in this file, at least not
for this series. And as far as this series is concerned, I think it's
actually less readable. If there's a subsequent series that will use
this and make further changes I could be convinced, but then I'd say
this commit belongs as part of the later series, not this one.
Will fix, initially had these built-in fixes split into one commit per
built-in, but decided it was too verbose and squashed them all, and
didn't copyedit the commit message properly.
On Sun, Oct 3, 2021 at 5:46 PM Ævar Arnfjörð Bjarmason [off-list ref] wrote:
quoted
Plug memory leaks in various built-ins that were missing
unpack_trees_options_release() calls. In an earlier commit these
functions were all made to use the "UNPACK_TREES_OPTIONS_INIT" macro,
now let's have the ones that didn't clean up their memory do so.
This commit message doesn't say anything about what was leaking. You
should really bring up that it was the dir_clear() you added to
unpack_trees_options_release() earlier in the series that is important
here (or at least that's what I presume is the important fix).
*nod*, willdo.
Looks good to me; thanks for catching and fixing these leaks.
On Mon, Oct 4, 2021 at 7:49 AM Ævar Arnfjörð Bjarmason [off-list ref] wrote:
On Mon, Oct 04 2021, Elijah Newren wrote:
quoted
On Sun, Oct 3, 2021 at 5:46 PM Ævar Arnfjörð Bjarmason [off-list ref] wrote:
quoted
In the preceding commit we introduced a new UNPACK_TREES_OPTIONS_INIT
macro, but "merge-recursive.c" could not be converted to it since
it (re-)initializes a "struct unpack_trees_options" on the heap.
In order to convert use the macro as the source of truth for
initialization we need to not only convert the initialization in
unpack_trees_start(), but also call the new
unpack_trees_options_init() just after the CALLOC_ARRAY() in
merge_start().
Um...why?
Replied below.
quoted
quoted
When we call merge_trees() we'll call merge_start() followed by
merge_trees_internal(), and it will call unpack_trees_start() before
it does much of anything. So it's covered by an initialization in
unpack_trees_start().
But when merge_recursive() is called it will call merge_start()
followed by merge_recursive_internal(), which won't call
unpack_trees_start() until its own call call to
merge_trees_internal(), but in the meantime it might end up using that
calloc'd "struct unpack_trees_options".
Nothing in merge-recursive.c usings unpack_opts before
unpack_trees_start() or after unpack_trees_finish(). If anyone
attempts to use it elsewhere, that would itself be a bug. So, I'd
replace the above three paragraphs with:
"Change the initialization of opt->priv_unpack_opts from using memset
to 0, with unpack_trees_options_init()."
or something like that, and then drop your change to merge_start().
Sure, I'll defer to you about being confident about that. I didn't want
to leave a copy if it hanging without the proper initialization in case
there were new callers.
quoted
quoted
This was OK before, as setup_unpack_trees_porcelain() would call
strvec_init(), and our "struct dir_struct" in turn is OK with being
NULL'd. Let's convert the former to macro initialization, we'll deal
with the latter in a subsequent commit.
This is quite confusing; it's really hard to understand how this
relates to the rest of the commit message. I have to read the code to
see what you're doing, and then write my own commit message in my head
because the wording in this paragraph still doesn't parse.
I'd make the change strvec_init/STRVEC_INIT changes be a separate
patch. I suspect it'll be easier to write the commit message for it
as a standalone commit as well.
Sure, FWIW I had it split up locally, and figured it would be easier to
explain if the API usage change came with the initialization change that
made it possible.
@@ -405,7 +405,7 @@ static int unpack_trees_start(struct merge_options *opt,structtree_desct[3];structindex_statetmp_index={NULL};-memset(&opt->priv->unpack_opts,0,sizeof(opt->priv->unpack_opts));+unpack_trees_options_init(&opt->priv->unpack_opts);if(opt->priv->call_depth)opt->priv->unpack_opts.index_only=1;else
@@ -3704,6 +3704,7 @@ static int merge_start(struct merge_options *opt, struct tree *head)CALLOC_ARRAY(opt->priv,1);string_list_init_dup(&opt->priv->df_conflict_file_set);+unpack_trees_options_init(&opt->priv->unpack_opts);
Drop this hunk.
Speaking of splitting things up in more understandable ways: If we're
going to hard rely on the interaction between merge_start() and
unpack_trees_start() wouldn't it make sense to lead with a change that
dropped that memset, which if this invariant holds is also redundant to
the CALLOC() of opt->priv in merge_start() in the pre-image.
It is not redundant, and that would be a very confusing change.
merge-recursive is so named because its main driving function contains
recursive calls to itself. merge_start() is not involved in that
recursion. For readability, we should initialize unpack_opts before
each use.
On Sun, Oct 3, 2021 at 5:46 PM Ævar Arnfjörð Bjarmason [off-list ref] wrote:
quoted
Change the clear_unpack_trees_porcelain() to be like a *_release()
function, not a *_reset() (in strbuf.c terms). Let's move the only API
user that relied on the latter to doing its own
unpack_trees_options_init(). See the commit that introduced
unpack_trees_options_init() for details on the control flow involved
here.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
merge-recursive.c | 1 +
unpack-trees.c | 1 -
2 files changed, 1 insertion(+), 1 deletion(-)
This is wrong. It suggests that unpack_opts is used after
unpack_trees_finish() (other than an outer merge first calling
unpack_trees_start() again), which can only serve to greatly confuse
future readers. Drop this hunk.
Sure, but (and also re:
https://lore.kernel.org/git/CABPp-BEA2myh2Np_YpFWnE+jqmT5vz7ohigZ0=2tL-wizgYQmg@mail.gmail.com/)
if you'd like not initialize things in merge_start() just for good
measure wouldn't the diff-at-the-end on top of your 5bf7e5779ec
(merge-recursive: split internal fields into a separate struct,
2019-08-17) also make sense?
I.e. the reason I entered this particular rabbit hole was in looking at
existing members of "struct merge_options_internal" & past commits and
seeing how we did its initialization. That canary on top passes all our
tests, and per my reading we also don't use "df_conflict_file_set" until
as late as the things we setup in unpack_trees_start(). Should those be
moved to do the post-merge_start() setup at the same time?
quoted
}
static int save_files_dirs(const struct object_id *oid,
This seems like a very dangerous change. You want to leave opts->msgs
pointing at freed memory?
Yes, as argued in
http://lore.kernel.org/git/87bl45niqs.fsf@evledraar.gmail.com; In this
series we can see that nothing re-uses it, so it's as safe as our
strbuf_release(), or a plain free().
Maybe I'm misunderstanding what you're getting at, and I could
understand a "let's just reset it for good measure" POV. But I can't
square your view that we shouldn't do setup in merge_start() for good
measure in case some new future code accidentally uses the data earlier
(which I'm fine with), but then also not finding it OK to skip the
memset() here ...
@@ -869,6 +870,7 @@ static int make_room_for_path(struct merge_options *opt, const char *path)constchar*msg=_("failed to create path '%s'%s");/* Unlink any D/F conflict files that are in the way */+assert(opt->priv->df_conflict_file_set.nr!=123456);for(i=0;i<opt->priv->df_conflict_file_set.nr;i++){constchar*df_path=opt->priv->df_conflict_file_set.items[i].string;size_tpathlen=strlen(path);
@@ -3467,6 +3469,7 @@ static int merge_trees_internal(struct merge_options *opt,return1;}+string_list_init_dup(&opt->priv->df_conflict_file_set);code=unpack_trees_start(opt,merge_base,head,merge);if(code!=0){
@@ -3703,7 +3706,7 @@ static int merge_start(struct merge_options *opt, struct tree *head)}CALLOC_ARRAY(opt->priv,1);-string_list_init_dup(&opt->priv->df_conflict_file_set);+opt->priv->df_conflict_file_set.nr=123456;return0;}
On Mon, Oct 4, 2021 at 8:42 AM Ævar Arnfjörð Bjarmason [off-list ref] wrote:
On Mon, Oct 04 2021, Elijah Newren wrote:
quoted
On Sun, Oct 3, 2021 at 5:46 PM Ævar Arnfjörð Bjarmason [off-list ref] wrote:
quoted
Change the clear_unpack_trees_porcelain() to be like a *_release()
function, not a *_reset() (in strbuf.c terms). Let's move the only API
user that relied on the latter to doing its own
unpack_trees_options_init(). See the commit that introduced
unpack_trees_options_init() for details on the control flow involved
here.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
merge-recursive.c | 1 +
unpack-trees.c | 1 -
2 files changed, 1 insertion(+), 1 deletion(-)
This is wrong. It suggests that unpack_opts is used after
unpack_trees_finish() (other than an outer merge first calling
unpack_trees_start() again), which can only serve to greatly confuse
future readers. Drop this hunk.
Sorry, I can't parse this sentence. Could you retry?
I.e. the reason I entered this particular rabbit hole was in looking at
existing members of "struct merge_options_internal" & past commits and
seeing how we did its initialization. That canary on top passes all our
tests, and per my reading we also don't use "df_conflict_file_set" until
as late as the things we setup in unpack_trees_start(). Should those be
moved to do the post-merge_start() setup at the same time?
It appears df_conflict_file_set has some theoretical memory leaks
(though in practice unlikely and quite small in the few cases that
could be constructed to trigger it). Initializing it nearer to use
and free'ing when done (in merge_trees_internal()) would make more
sense, yes.
But, merge-recursive.c right now is supposed to be the stable fallback
in case someone runs into an issue with merge-ort. I'd rather keep it
stable in preparation for deleting it, not churning its code
unnecessarily.
quoted
quoted
}
static int save_files_dirs(const struct object_id *oid,
strbuf_release() sets sb->buf to strbuf_slopbuf, and sets sb->len =
sb->alloc = 0. The strbuf can thus be reused after calling
strbuf_release().
strvec_clear() also calls strvec_init() afterwards to set the vector
to be usable though 0-sized.
hashmap_clear() also clears out existing data, but makes it ready for
reuse (as per 6da1a25814)
strmap_clear(), strintmap_clear(), and strset_clear() also set up the
data structure for reuse.
There's a longstanding presumption that something named `*_clear()`
will make it still usable afterwards. Rename it to end with `_free`
if you want it to be an analogy to free() where usage afterwards would
cause use-after-free errors.
Maybe I'm misunderstanding what you're getting at, and I could
understand a "let's just reset it for good measure" POV. But I can't
square your view that we shouldn't do setup in merge_start() for good
measure in case some new future code accidentally uses the data earlier
(which I'm fine with), but then also not finding it OK to skip the
memset() here ...
No existing caller needs to make use of the fact that it's a `_clear`
function rather than a `_free` function, but if you want to take
advantage of that to do less work, you should both call it out in your
commit message and rename the function. You didn't do either. In
fact, your existing commit message mentions strbuf_release(), which
reinforces the `_clear` presumption of reusability and thus makes me
flag the change as dangerous.