Re: [PATCH v4 13/27] revisions API users: use release_revisions() in builtin/log.c

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Apr 3, 2022, 14:07 UTC
Message-ID: <220403.865ynqmeun.gmgdl@evledraar.gmail.com>
In-Reply-To: <14c17fa9-e9ff-ac5f-dbda-4a566ed09fd3@gmail.com>

On Sat, Apr 02 2022, Phillip Wood wrote:

[A comment on v4, but also applies to v5 I think]

Show 23 quoted lines

> 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.

Show 15 quoted lines

>>   /*
>>    * 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.