Hi Ævar
On 03/04/2022 15:07, Ævar Arnfjörð Bjarmason wrote:
On Sat, Apr 02 2022, Phillip Wood wrote:
[A comment on v4, but also applies to v5 I think]
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.
It doesn't, for this series each individual commit passes with
Oh I'd missed that, thanks for explaining
make test
GIT_TEST_PASSING_SANITIZE_LEAK=true make test SANITIZE=leak
And also in a stricter mode that I have locally (not in git yet):
make test
GIT_TEST_PASSING_SANITIZE_LEAK=check make test SANITIZE=leak
Which ensures not only that the tests we marked as leak free pass, but
that no other tests we *haven't* marked pass unexpectedly (requires prep
changes before this series to mark the still-not-marked-but-should-be
tests).
I think that should address/help explain things re your questions about
some of the UNLEAK() back-and-forth.
Yes it does, the next patch makes sense to me now as well thanks
[...]
@@ -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?
It's just a way to save every single call to this callsite a change on
top like this:
diff --git a/builtin/log.c b/builtin/log.c
index 5dad70aa47e..ece03536bed 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -684,8 +684,11 @@ 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_deinit(cmd_log_walk(&rev), &rev);
+ if (!rev.no_walk) {
+ ret = cmd_log_walk(&rev);
+ release_revisions(&rev);
+ return ret;
+ }
count = rev.pending.nr;
objects = rev.pending.objects;
Which, given that there's 6 of them nicely cuts down on the resulting
verbosity.
If you want to adopt this pattern more widely then if there was a helper
function in revisions.c it could be used in patches 11 and 14 as well as
this one I think.
Best Wishes
Phillip
