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 <avarab@xxxxxxxxx>
> ---
> 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
