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
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.
I.e. there's a few changes that are in this series just so it can pass
in that "GIT_TEST_PASSING_SANITIZE_LEAK=check" mode, but it would still
pass in "GIT_TEST_PASSING_SANITIZE_LEAK=true", i.e. because we'd make
some new test pass unexpectedly.
But I think maintaining the 1=1 correspondance really helps to follow
along with this, i.e. tests are tweaked as they become leak-free, and we
(or well, mostly I) can be confident that I marked all the relevant
newlry passing ones, and that there are no regressions in-between.
>> /*
>> * 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?
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.
