From: Phillip Wood <phillip.wood123@gmail.com>
Date: Sat, 02 Apr 2022 09:22:44 GMT
Subject: Re: [PATCH v4 13/27] revisions API users: use release_revisions() in builtin/log.c
Message-ID: <14c17fa9-e9ff-ac5f-dbda-4a566ed09fd3@gmail.com>
In-Reply-To: <patch-v4-13.27-02ca92660af-20220331T005325Z-avarab@gmail.com>

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@evledraar.gmail.com/
> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>   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