From: Phillip Wood Date: Fri, 01 Apr 2022 15:17:21 GMT Subject: Re: [PATCH v4 06/27] revision.[ch]: provide and start using a release_revisions() Message-ID: In-Reply-To: Hi Ævar On 31/03/2022 02:11, Ævar Arnfjörð Bjarmason wrote: > The users of the revision.[ch] API's "struct rev_info" are a major > source of memory leaks in the test suite under SANITIZE=leak, which in > turn adds a lot of noise when trying to mark up tests with > "TEST_PASSES_SANITIZE_LEAK=true". > > The users of that API are largely one-shot, e.g. "git log", so > arguably freeing the memory is a waste of time, but in many cases > they've actually been trying to free the memory, and just doing that > in a buggy manner. I've not looked that closely at these changes but builtin/log.c is not modified so I'm a bit confused by the mention of "git log" above. Best Wishes Phillip > Let's provide a release_revisions() function for these users, and > start migrating them over per the plan outlined in [1]. Right now this > only handles the "pending" member of the struct, but more will be > added in subsequent commits. > > Even though we only clear the "pending" member now, let's not leave a > trap in code like the pre-image of index_differs_from(), where we'd > start doing the wrong thing as soon as the release_revisions() learned > to clear its "diffopt". I.e. we need to call release_revisions() after > we've inspected any state in "struct rev_info". > > This leaves in place e.g. clear_pathspec(&rev.prune_data) in > stash_working_tree() in builtin/stash.c, subsequent commits will teach > release_revisions() to free "prune_data" and other members that in > some cases are individually cleared by users of "struct rev_info" by > reaching into its members. Those subsequent commits will remove the > relevant calls to e.g. clear_pathspec(). > > We avoid amending code in index_differs_from() in diff-lib.c as well > as wt_status_collect_changes_index(), has_unstaged_changes() and > has_uncommitted_changes() in wt-status.c in a way that assumes that we > are already clearing the "diffopt" member. That will be handled in a > subsequent commit. > > 1. https://lore.kernel.org/git/87a6k8daeu.fsf@evledraar.gmail.com/ > > Signed-off-by: Ævar Arnfjörð Bjarmason > --- > builtin/checkout.c | 2 +- > builtin/stash.c | 3 +-- > diff-lib.c | 2 +- > range-diff.c | 2 +- > revision.c | 5 +++++ > revision.h | 6 ++++++ > wt-status.c | 5 +++-- > 7 files changed, 18 insertions(+), 7 deletions(-) > > diff --git a/builtin/checkout.c b/builtin/checkout.c > index 797681481d1..4d9e0bd3ac1 100644 > --- a/builtin/checkout.c > +++ b/builtin/checkout.c > @@ -629,7 +629,7 @@ static void show_local_changes(struct object *head, > diff_setup_done(&rev.diffopt); > add_pending_object(&rev, head, NULL); > run_diff_index(&rev, 0); > - object_array_clear(&rev.pending); > + release_revisions(&rev); > } > > static void describe_detached_head(const char *msg, struct commit *commit) > diff --git a/builtin/stash.c b/builtin/stash.c > index ccdfdab44be..ad74624c2f7 100644 > --- a/builtin/stash.c > +++ b/builtin/stash.c > @@ -1266,9 +1266,8 @@ static int stash_working_tree(struct stash_info *info, const struct pathspec *ps > > done: > discard_index(&istate); > - UNLEAK(rev); > - object_array_clear(&rev.pending); > clear_pathspec(&rev.prune_data); > + release_revisions(&rev); > strbuf_release(&diff_output); > remove_path(stash_index_path.buf); > return ret; > diff --git a/diff-lib.c b/diff-lib.c > index ca085a03efc..d6800274bd5 100644 > --- a/diff-lib.c > +++ b/diff-lib.c > @@ -662,7 +662,7 @@ int index_differs_from(struct repository *r, > diff_flags_or(&rev.diffopt.flags, flags); > rev.diffopt.ita_invisible_in_index = ita_invisible_in_index; > run_diff_index(&rev, 1); > - object_array_clear(&rev.pending); > + release_revisions(&rev); > return (rev.diffopt.flags.has_changes != 0); > } > > diff --git a/range-diff.c b/range-diff.c > index b72eb9fdbee..39cc010c628 100644 > --- a/range-diff.c > +++ b/range-diff.c > @@ -596,6 +596,6 @@ int is_range_diff_range(const char *arg) > } > > free(copy); > - object_array_clear(&revs.pending); > + release_revisions(&revs); > return negative > 0 && positive > 0; > } > diff --git a/revision.c b/revision.c > index 29f102cb83d..9a3c05b69a5 100644 > --- a/revision.c > +++ b/revision.c > @@ -2931,6 +2931,11 @@ static void release_revisions_commit_list(struct rev_info *revs) > } > } > > +void release_revisions(struct rev_info *revs) > +{ > + object_array_clear(&revs->pending); > +} > + > static void add_child(struct rev_info *revs, struct commit *parent, struct commit *child) > { > struct commit_list *l = xcalloc(1, sizeof(*l)); > diff --git a/revision.h b/revision.h > index 5bc59c7bfe1..61c780fc4cd 100644 > --- a/revision.h > +++ b/revision.h > @@ -377,6 +377,12 @@ void repo_init_revisions(struct repository *r, > int setup_revisions(int argc, const char **argv, struct rev_info *revs, > struct setup_revision_opt *); > > +/** > + * Free data allocated in a "struct rev_info" after it's been > + * initialized with repo_init_revisions(). > + */ > +void release_revisions(struct rev_info *revs); > + > void parse_revision_opt(struct rev_info *revs, struct parse_opt_ctx_t *ctx, > const struct option *options, > const char * const usagestr[]); > diff --git a/wt-status.c b/wt-status.c > index d33f9272b72..922cf787f95 100644 > --- a/wt-status.c > +++ b/wt-status.c > @@ -662,7 +662,7 @@ static void wt_status_collect_changes_index(struct wt_status *s) > > copy_pathspec(&rev.prune_data, &s->pathspec); > run_diff_index(&rev, 1); > - object_array_clear(&rev.pending); > + release_revisions(&rev); > clear_pathspec(&rev.prune_data); > } > > @@ -2545,6 +2545,7 @@ int has_unstaged_changes(struct repository *r, int ignore_submodules) > rev_info.diffopt.flags.quick = 1; > diff_setup_done(&rev_info.diffopt); > result = run_diff_files(&rev_info, 0); > + release_revisions(&rev_info); > return diff_result_code(&rev_info.diffopt, result); > } > > @@ -2577,7 +2578,7 @@ int has_uncommitted_changes(struct repository *r, > > diff_setup_done(&rev_info.diffopt); > result = run_diff_index(&rev_info, 1); > - object_array_clear(&rev_info.pending); > + release_revisions(&rev_info); > return diff_result_code(&rev_info.diffopt, result); > } >