Subject: Re: [PATCH v3 07/10] progress.c: add temporary variable from progress struct

From: Taylor Blau <[email-masked]>
Date: Fri, 22 Oct 2021 18:37:29 -0400

On Thu, Oct 14, 2021 at 12:28:23AM +0200, Ævar Arnfjörð Bjarmason wrote:
> Add a temporary "progress" variable for the dereferenced p_progress
> pointer to a "struct progress *". Before 98a13647408 (trace2: log
> progress time and throughput, 2020-05-12) we didn't dereference
> "p_progress" in this function, now that we do it's easier to read the
> code if we work with a "progress" struct pointer like everywhere else,
> instead of a pointer to a pointer.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <[email-masked]>
> ---
>  progress.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/progress.c b/progress.c
> index 7fcc513717a..b9369e9a264 100644
> --- a/progress.c
> +++ b/progress.c
> @@ -329,15 +329,16 @@ void stop_progress(struct progress **p_progress)
>  	finish_if_sparse(*p_progress);

This spot also wants to dereference p_progress, but doesn't access any
of the fields by dereferencing the second pointer. So this could
certainly have passed `progress` (had you scoped its declaration to the
whole function).

I don't think it hurts readability, and I definitely don't have a strong
opinion about it. Just an idle thought while reading this patch...

>  	if (*p_progress) {
> +		struct progress *progress = *p_progress;
>  		trace2_data_intmax("progress", the_repository, "total_objects",
>  				   (*p_progress)->total);
>
>  		if ((*p_progress)->throughput)
>  			trace2_data_intmax("progress", the_repository,
>  					   "total_bytes",
> -					   (*p_progress)->throughput->curr_total);
> +					   progress->throughput->curr_total);
>
> -		trace2_region_leave("progress", (*p_progress)->title, the_repository);
> +		trace2_region_leave("progress", progress->title, the_repository);

This all looks much better to me. Thanks.

Thanks,
Taylor