From: Phillip Wood <phillip.wood123@gmail.com>
Date: Fri, 15 Apr 2022 13:58:37 GMT
Subject: Re: [PATCH v6 14/27] revisions API users: use release_revisions() with UNLEAK()
Message-ID: <2cc2d026-1496-1ed9-838b-6fdf8cfba285@gmail.com>
In-Reply-To: <patch-v6-14.27-31ce58da43d-20220413T195935Z-avarab@gmail.com>

Hi Ævar

On 13/04/2022 21:01, Ævar Arnfjörð Bjarmason wrote:
> Use a release_revisions() with those "struct rev_list" users which
> already "UNLEAK" the struct. It may seem odd to simultaneously attempt
> to free() memory, but also to explicitly ignore whether we have memory
> leaks in the same.
> 
> As explained in preceding commits this is being done to use the
> built-in commands as a guinea pig for whether the release_revisions()
> function works as expected, we'd like to test e.g. whether we segfault
> as we change it. In subsequent commits we'll then remove these
> UNLEAK() as the function is made to free the memory that caused us to
> add them in the first place.

It would have been nice to reword this to avoid the confusion that 
prompted my comments on the previous version. Saying "As explained in 
the preceding commits" is confusing when those commits remove UNLEAK() 
and this commit leaves it in. I now understand why you are leaving 
UNLEAK() here thanks to your explanation on the previous series and I 
think it makes sense to do so but I don't think this message does a good 
job of explaining it.

The rest of the series looks good

Best Wishes

Phillip

> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>   builtin/diff-index.c | 4 +++-
>   builtin/diff.c       | 1 +
>   2 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/builtin/diff-index.c b/builtin/diff-index.c
> index 5fd23ab5b6c..3a83183c312 100644
> --- a/builtin/diff-index.c
> +++ b/builtin/diff-index.c
> @@ -71,5 +71,7 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)
>   	}
>   	result = run_diff_index(&rev, option);
>   	UNLEAK(rev);
> -	return diff_result_code(&rev.diffopt, result);
> +	result = diff_result_code(&rev.diffopt, result);
> +	release_revisions(&rev);
> +	return result;
>   }
> diff --git a/builtin/diff.c b/builtin/diff.c
> index bb7fafca618..dd48336da56 100644
> --- a/builtin/diff.c
> +++ b/builtin/diff.c
> @@ -595,6 +595,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
>   	if (1 < rev.diffopt.skip_stat_unmatch)
>   		refresh_index_quietly();
>   	UNLEAK(rev);
> +	release_revisions(&rev);
>   	UNLEAK(ent);
>   	UNLEAK(blob);
>   	return result;