Re: [PATCH v7 5/7] progress.c: add temporary variable from progress struct

From: Johannes Altmanninger <aclopte@gmail.com>
Date: Dec 27, 2021, 01:11 UTC
Message-ID: <20211227011125.bbczjt3wctfobfso@gmail.com>
In-Reply-To: <patch-v7-5.7-c2303bfd130-20211217T041945Z-avarab@gmail.com>

On Fri, Dec 17, 2021 at 05:25:00AM +0100, Ævar Arnfjörð Bjarmason wrote:

Show 6 quoted lines

> 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.

This message contains lots of nice details, but some of that is only really useful after reading the diff. You mention the parameter's name but not the function name (other combinations would make more sense). I'd maybe write it like this, trying to follow our usual cadence of diagnosis -> action:

	Since 98a13647408 (trace2: log progress time and throughput, 2020-05-12)
	stop_progress() dereferences a "struct progress **" parameter in several
	places. Extract a dereferenced variable (like in stop_progress_msg()) to
	reduce clutter and make it clearer who needs to write to this parameter.

Show 15 quoted lines

> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  progress.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/progress.c b/progress.c
> index 680c6a8bf93..76a95cb7322 100644
> --- a/progress.c
> +++ b/progress.c
> @@ -325,15 +325,16 @@ void stop_progress(struct progress **p_progress)
>  	finish_if_sparse(*p_progress);
>  
>  	if (*p_progress) {
> +		struct progress *progress = *p_progress;

Yeah, it's a good idea to reduce uses of the struct progress ** parameter. Why not do this even earlier in this function:

	struct progress *progress;

	if (!p_progress)
		BUG("don't provide NULL to stop_progress");
	progress = *p_progress;

	finish_if_sparse(progress);

	if (progress) {
		...
	}

	stop_progress_msg(p_progress, _("done"));

this way we only need to dereference once (right next to the null check) and it's clearer that stop_progress_msg() is the only one who wants to modify the parameter.

>  		trace2_data_intmax("progress", the_repository, "total_objects",
>  				   (*p_progress)->total);

s/(\*p_progress)/progress/, same in the next line

Show 15 quoted lines

>  
>  		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);
>  	}
>  
>  	stop_progress_msg(p_progress, _("done"));
> -- 
> 2.34.1.1119.g7a3fc8778ee
>