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

[Phillip Wood <phillip.wood123@gmail.com>]

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