Re: [PATCH v8 5/7] progress.c: add temporary variable from progress struct
From: René Scharfe <hidden>
Date: 2021-12-28 16:05:37
Am 28.12.21 um 16:19 schrieb Ævar Arnfjörð Bjarmason:
quoted hunk ↗ jump to hunk
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. Now instead of using "*p_progress" several times in stop_progress() we check it once for NULL and then use a dereferenced "progress" variable thereafter. This continues the same pattern used in the above stop_progress() function, see ac900fddb7f (progress: don't dereference before checking for NULL, 2020-08-10). Signed-off-by: Ævar Arnfjörð Bjarmason <redacted> --- progress.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)diff --git a/progress.c b/progress.c index 680c6a8bf93..688749648be 100644 --- a/progress.c +++ b/progress.c@@ -319,21 +319,23 @@ static void finish_if_sparse(struct progress *progress) void stop_progress(struct progress **p_progress) { + struct progress *progress; if (!p_progress) BUG("don't provide NULL to stop_progress"); + progress = *p_progress; - finish_if_sparse(*p_progress); + finish_if_sparse(progress); - if (*p_progress) { + if (progress) { trace2_data_intmax("progress", the_repository, "total_objects", - (*p_progress)->total); + progress->total); - if ((*p_progress)->throughput) + if (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"));
This patch is trivially correct, but I wonder why all that code is here instead of in stop_progress_msg(). I would expect stop_progress() to be a thin wrapper that just provides a default message, but actually it handles sparse progress and tracing. Isn't both necessary even with a custom message? In any case, moving the code there becomes easier after this patch. René