On 18/09/2021 22:06, 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.
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);
^~~~~~~~~~~~~~~~~
Casting the pointer a la "free((void *) ...)" seems to be a common
pattern in git, and seems like a reasonable option here. AFAIUI the
const is still needed because clients of rev_cmdline_info shouldn't be
changing name. But since we own and created rev_cmdline_info, we also
know it's safe to clean it up. For comparison, here's an example of
submodule_entry being cleaned up - all members end up needing a cast:
static void free_one_config(struct submodule_entry *entry)
{
free((void *) entry->config->path);
free((void *) entry->config->name);
free((void *) entry->config->branch);
free((void *) entry->config->update_strategy.command);
free(entry->config);
}
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
This patch looks good to me (modulo adding the cast as discussed above),
and is obviously much better than my approach of using an UNLEAK!
ATB,
Andrzej
