On Sat, Sep 18 2021, Carlo Marcelo Arenas Belón wrote: > My equivalent version for these fixes is obviously more verbose but IMHO > not that ugly (and as safe) > > It avoids the need to UNLEAK early by changing the program flow also for > the early return so the cleanup could be centralized in one single > function. > > Both, the cmdline and mailmap arrays (and the objects they accumulate) > are cleaned in a "reusable" way. > > Note that the cleaning of the "name" in the cmdline item throws a warning > as shown below which I intentionally didn't fix, as it would seem that > either the use of const there or the need to strdup is wrong. So hope > someone that knows this code better could chime in. It should just be a "char *", I got that wrong in my version posted in the side-thread[1] & mentioned in the side-reply[2]. (I think I got it right in some earlier version days ago, it should be a 'char *' like anyting we malloc, but brainfart when re-doing/re-basing those changes). Yours here below has a bug where you free() rev_cmdline_info items, you need to use release_revisions_cmdline_rev(). I should have said in [2], but thanks a lot to you and Andrzej for following up on the mess in t0000-basic.sh addressed by my v7 re-roll. It'll be really nice to get some of these leaks fixed & tested for. I think I was rather curt in [2] after a long debugging session, just saying I appreciate it. Hopefully we can figure out some plan for mostly pulling in the same direction with regards to the way forward. 1. https://github.com/git/git/commit/06380cd4f56f4c542685eb7aa79e28285fe02c55 2. https://lore.kernel.org/git/87a6k8daeu.fsf@xxxxxxxxxxxxxxxxxxx/ > Carlo > ------ >8 ------ > Subject: [PATCH] builtin/log: leaks from `git show` in t0000 > > obviously not ready, since the following will need to be corrected: > > revision.c:1496:8: warning: passing 'const char *' to parameter of type 'void *' discards qualifiers [-Wincompatible-pointer-types-discards-qualifiers] > free(info->rev[i].name); > ^~~~~~~~~~~~~~~~~ > > Signed-off-by: Carlo Marcelo Arenas Belón <carenas@xxxxxxxxx> > --- > builtin/log.c | 8 ++++++-- > revision.c | 20 ++++++++++++++++++++ > revision.h | 5 +++++ > 3 files changed, 31 insertions(+), 2 deletions(-) > > diff --git a/builtin/log.c b/builtin/log.c > index f75d87e8d7..1b1c1f53f4 100644 > --- a/builtin/log.c > +++ b/builtin/log.c > @@ -645,8 +645,10 @@ int cmd_show(int argc, const char **argv, const char *prefix) > opt.tweak = show_setup_revisions_tweak; > cmd_log_init(argc, argv, prefix, &rev, &opt); > > - if (!rev.no_walk) > - return cmd_log_walk(&rev); > + if (!rev.no_walk) { > + ret = cmd_log_walk(&rev); > + goto done; > + } > > count = rev.pending.nr; > objects = rev.pending.objects; > @@ -702,6 +704,8 @@ int cmd_show(int argc, const char **argv, const char *prefix) > } > } > free(objects); > +done: > + repo_clear_revisions(&rev); > return ret; > } > > diff --git a/revision.c b/revision.c > index 0dabb5a0bc..ce62192dd8 100644 > --- a/revision.c > +++ b/revision.c > @@ -1487,6 +1487,18 @@ static void add_rev_cmdline(struct rev_info *revs, > info->nr++; > } > > +static void clear_rev_cmdline(struct rev_info *revs) > +{ > + struct rev_cmdline_info *info = &revs->cmdline; > + size_t i, nr = info->nr; > + > + for (i = 0; i < nr; i++) > + free(info->rev[i].name); > + > + FREE_AND_NULL(info->rev); > + info->nr = info->alloc = 0; > +} > + > static void add_rev_cmdline_list(struct rev_info *revs, > struct commit_list *commit_list, > int whence, > @@ -1845,6 +1857,14 @@ void repo_init_revisions(struct repository *r, > init_display_notes(&revs->notes_opt); > } > > +void repo_clear_revisions(struct rev_info *revs) > +{ > + if (revs->mailmap) > + clear_mailmap(revs->mailmap); > + FREE_AND_NULL(revs->mailmap); > + clear_rev_cmdline(revs); > +} > + > static void add_pending_commit_list(struct rev_info *revs, > struct commit_list *commit_list, > unsigned int flags) > diff --git a/revision.h b/revision.h > index 0c65a760ee..f695c41cee 100644 > --- a/revision.h > +++ b/revision.h > @@ -358,6 +358,11 @@ void repo_init_revisions(struct repository *r, > struct rev_info *revs, > const char *prefix); > > +/* > + * Free all structures dynamically allocated for the provided rev_info > + */ > +void repo_clear_revisions(struct rev_info *revs); > + > /** > * Parse revision information, filling in the `rev_info` structure, and > * removing the used arguments from the argument list. Returns the number
