| Message ID | c804ba3620476713bd0535a315876378149ad7dd.1683581621.git.me@ttaylorr.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | refs: implement skip lists for packed backend | expand |
Taylor Blau <me@ttaylorr.com> writes: > From: Jeff King <peff@peff.net> > > We did not bother to clean up at all in branch/tag, and for-each-ref > only hit a few elements. So this is probably cleaning up leaks, but I > didn't check yet. > > Note that the reachable_from and unreachable_from lists should be > cleaned as they are used. So this is just covering any case where we > might bail before running the reachability check. > --- Not signed-off? > +void ref_filter_clear(struct ref_filter *filter) > +{ > + oid_array_clear(&filter->points_at); > + free_commit_list(filter->with_commit); > + free_commit_list(filter->no_commit); > + free_commit_list(filter->reachable_from); > + free_commit_list(filter->unreachable_from); > + ref_filter_init(filter); > +} And the previous step matters here---otherwise we will end up walking two commit lists whose elements have all been popped in reach_filter(). Makes sense. > +void ref_filter_init(struct ref_filter *filter) > +{ > + struct ref_filter blank = REF_FILTER_INIT; > + memcpy(filter, &blank, sizeof(blank)); > +} I wonder if structure assignment "*filter = blank" is easier to see but I think we've seen this "_INIT; memcpy()" dance before.
On Mon, May 08, 2023 at 03:29:22PM -0700, Junio C Hamano wrote: > Taylor Blau <me@ttaylorr.com> writes: > > > From: Jeff King <peff@peff.net> > > > > We did not bother to clean up at all in branch/tag, and for-each-ref > > only hit a few elements. So this is probably cleaning up leaks, but I > > didn't check yet. > > > > Note that the reachable_from and unreachable_from lists should be > > cleaned as they are used. So this is just covering any case where we > > might bail before running the reachability check. > > --- > > Not signed-off? Oops. Sorry about that, this should be: Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Taylor Blau <me@ttaylorr.com> > I wonder if structure assignment "*filter = blank" is easier to see > but I think we've seen this "_INIT; memcpy()" dance before. Yeah, this matches how many other _init() functions work (just looking through the output of `git grep -B3 memcpy | grep -A3 _init`). Thanks, Taylor
On Mon, May 08, 2023 at 05:59:52PM -0400, Taylor Blau wrote: > From: Jeff King <peff@peff.net> > > We did not bother to clean up at all in branch/tag, and for-each-ref > only hit a few elements. So this is probably cleaning up leaks, but I > didn't check yet. Nit: it sounds like there still is the intent to check whether this does fix leaks. How about updating the commit message to either not mention the intent or perform the check? Patrick
On Tue, May 09, 2023 at 05:14:49PM +0200, Patrick Steinhardt wrote: > On Mon, May 08, 2023 at 05:59:52PM -0400, Taylor Blau wrote: > > From: Jeff King <peff@peff.net> > > > > We did not bother to clean up at all in branch/tag, and for-each-ref > > only hit a few elements. So this is probably cleaning up leaks, but I > > didn't check yet. > > Nit: it sounds like there still is the intent to check whether this does > fix leaks. How about updating the commit message to either not mention > the intent or perform the check? Oops. Thanks for the reminder. Peff sent this patch to me while we were working on this topic together, and I forgot to come back and actually perform this check. Luckily, it helps out quite a bit: t/t0041-usage.sh | 1 + t/t2016-checkout-patch.sh | 1 + t/t3402-rebase-merge.sh | 1 + t/t3407-rebase-abort.sh | 1 + t/t4058-diff-duplicates.sh | 2 ++ t/t5407-post-rewrite-hook.sh | 1 + t/t5811-proto-disable-git.sh | 2 ++ t/t6001-rev-list-graft.sh | 1 + t/t7008-filter-branch-null-sha1.sh | 1 + t/t7408-submodule-reference.sh | 2 ++ t/t9502-gitweb-standalone-parse-output.sh | 1 + 11 new leak-free tests! I'll take it ;-). Thanks, Taylor
diff --git a/builtin/branch.c b/builtin/branch.c index 03bb8e414c..c201f0cb0b 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -813,6 +813,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix) print_columns(&output, colopts, NULL); string_list_clear(&output, 0); ref_sorting_release(sorting); + ref_filter_clear(&filter); return 0; } else if (edit_description) { const char *branch_name; diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c index 99ccb73518..c01fa6fefe 100644 --- a/builtin/for-each-ref.c +++ b/builtin/for-each-ref.c @@ -120,8 +120,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix) strbuf_release(&err); strbuf_release(&output); ref_array_clear(&array); - free_commit_list(filter.with_commit); - free_commit_list(filter.no_commit); + ref_filter_clear(&filter); ref_sorting_release(sorting); strvec_clear(&vec); return 0; diff --git a/builtin/tag.c b/builtin/tag.c index 6b41bb7374..aab5e693fe 100644 --- a/builtin/tag.c +++ b/builtin/tag.c @@ -645,6 +645,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix) cleanup: ref_sorting_release(sorting); + ref_filter_clear(&filter); strbuf_release(&buf); strbuf_release(&ref); strbuf_release(&reflog_msg); diff --git a/ref-filter.c b/ref-filter.c index b1d5022a51..9ea92b9637 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -2864,3 +2864,19 @@ int parse_opt_merge_filter(const struct option *opt, const char *arg, int unset) return 0; } + +void ref_filter_init(struct ref_filter *filter) +{ + struct ref_filter blank = REF_FILTER_INIT; + memcpy(filter, &blank, sizeof(blank)); +} + +void ref_filter_clear(struct ref_filter *filter) +{ + oid_array_clear(&filter->points_at); + free_commit_list(filter->with_commit); + free_commit_list(filter->no_commit); + free_commit_list(filter->reachable_from); + free_commit_list(filter->unreachable_from); + ref_filter_init(filter); +} diff --git a/ref-filter.h b/ref-filter.h index a920f73b29..160b807224 100644 --- a/ref-filter.h +++ b/ref-filter.h @@ -170,4 +170,7 @@ void filter_ahead_behind(struct repository *r, struct ref_format *format, struct ref_array *array); +void ref_filter_init(struct ref_filter *filter); +void ref_filter_clear(struct ref_filter *filter); + #endif /* REF_FILTER_H */
From: Jeff King <peff@peff.net> We did not bother to clean up at all in branch/tag, and for-each-ref only hit a few elements. So this is probably cleaning up leaks, but I didn't check yet. Note that the reachable_from and unreachable_from lists should be cleaned as they are used. So this is just covering any case where we might bail before running the reachability check. --- builtin/branch.c | 1 + builtin/for-each-ref.c | 3 +-- builtin/tag.c | 1 + ref-filter.c | 16 ++++++++++++++++ ref-filter.h | 3 +++ 5 files changed, 22 insertions(+), 2 deletions(-)