Re: [PATCH v4 08/19] worktree: fix a trivial leak in prune_worktrees()

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Tue, Jan 17 2023, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> We were leaking both the "struct strbuf" in prune_worktrees(), as well
>> as the "path" we got from should_prune_worktree(). Since these were
>> the only two uses of the "struct string_list" let's change it to a
>> "DUP" and push these to it with "string_list_append_nodup()".
>> ...
>> diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh
>> index d34d77f8934..ba8d929d189 100755
>> --- a/t/t3203-branch-output.sh
>> +++ b/t/t3203-branch-output.sh
>> @@ -1,6 +1,8 @@
>>  #!/bin/sh
>>  
>>  test_description='git branch display tests'
>> +
>> +TEST_PASSES_SANITIZE_LEAK=true
>>  . ./test-lib.sh
>>  . "$TEST_DIRECTORY"/lib-terminal.sh
>
> This is wrong, isn't it?
>
> t3203 uses --points-at, which populates filter.points_at by calling
> parse_opt_object_name().  Various members of the ref-filter
> structure is never freed (and there is no API helper function in
> ref-filter subsystem).
>
> Other tests that use --points-at (e.g. t6302 and t7004) are not
> marked with "passes_sanitize_leak", and this one shouldn't be,
> either.
>
> With the following squashed in, the branch seems to pass, but I am
> not sure which is lessor of the two evils.  From the point of view
> of the code maintenance, UNLEAK() to mark this singleton variable is
> far cleaner to deal with than selectively running the leak checks
> with the "passes_sanitize_leak" mechanism (which always feels like a
> losing whack-a-mole hack).
>
>  builtin/branch.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git c/builtin/branch.c w/builtin/branch.c
> index f63fd45edb..4fe7757670 100644
> --- c/builtin/branch.c
> +++ w/builtin/branch.c
> @@ -742,6 +742,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
>  	if (filter.abbrev == -1)
>  		filter.abbrev = DEFAULT_ABBREV;
>  	filter.ignore_case = icase;
> +	UNLEAK(filter);
>  
>  	finalize_colopts(&colopts, -1);
>  	if (filter.verbose) {

I'll send a v5 re-roll without this change, sorry.

This is a case where the version of GCC I was testing with doesn't
report the leak, but clang does (and probably other versions of GCC),
sorry.