Hi Ævar
On 31/03/2022 02:11, Ævar Arnfjörð Bjarmason wrote:
In preparation for having the "log" family of functions make wider use
of release_revisions() let's have them call it just before
exiting. This changes the "log", "whatchanged", "show",
"format-patch", etc. commands, all of which live in this file.
The release_revisions() API still only frees the "pending" member, but
will learn to release more members of "struct rev_info" in subsequent
commits.
In the case of "format-patch" revert the addition of UNLEAK() in
dee839a2633 (format-patch: mark rev_info with UNLEAK, 2021-12-16),
which will cause several tests that previously passed under
"TEST_PASSES_SANITIZE_LEAK=true" to start failing.
In subsequent commits we'll now be able to use those tests to check
whether that part of the API is really leaking memory, and will fix
all of those memory leaks. Removing the UNLEAK() allows us to make
incremental progress in that direction. See [1] for further details
about this approach.
This breaks "git bisect" but only when running the test suite to detect
leaks so I guess that's not too bad. An alternative would be to manually
remove the UNLEAK() when you're testing rather than committing the change.
Note that the release_revisions() will not be sufficient to deal with
the code in cmd_show() added in 5d7eeee2ac6 (git-show: grok blobs,
trees and tags, too, 2006-12-14) which clobbers the "pending" array in
the case of "OBJ_COMMIT". That will need to be dealt with by some
future follow-up work.
1. https://lore.kernel.org/git/220218.861r00ib86.gmgdl@xxxxxxxxxxxxxxxxxxx/
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx>
---
builtin/log.c | 20 ++++++++++++--------
t/t4126-apply-empty.sh | 2 --
2 files changed, 12 insertions(+), 10 deletions(-)
diff --git a/builtin/log.c b/builtin/log.c
index 6f9928fabfe..c40ebe1c3f4 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -295,6 +295,12 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix,
cmd_log_init_finish(argc, argv, prefix, rev, opt);
}
+static int cmd_log_deinit(int ret, struct rev_info *rev)
+{
+ release_revisions(rev);
+ return ret;
+}
/*
* This gives a rough estimate for how many commits we
* will print out in the list.
@@ -558,7 +564,7 @@ int cmd_whatchanged(int argc, const char **argv, const char *prefix)
cmd_log_init(argc, argv, prefix, &rev, &opt);
if (!rev.diffopt.output_format)
rev.diffopt.output_format = DIFF_FORMAT_RAW;
- return cmd_log_walk(&rev);
+ return cmd_log_deinit(cmd_log_walk(&rev), &rev);
This is a rather unusual pattern, at first I wondered if there were
going to be more added to the body of cmd_log_deinit() in later commits
but there isn't so why not just call release_revisions() here to be
consistent with the other release_revisions() call that are added in
other patches?
Best Wishes
Phillip
