On 5/15/2020 12:09 PM, Junio C Hamano wrote:
> Junio C Hamano <gitster@xxxxxxxxx> writes:
>
>> Derrick Stolee <stolee@xxxxxxxxx> writes:
>> ...
>>> This trace2_region_leave() needs to be conditional on the progress
>>> being non-null. The pattern used by consumers of the progress API is:
>>>
>>> if (my_progress_condition)
>>> start_progress(&progress);
>>>
>>> do {
>>> display_progress(&progress, count);
>>> } while (condition);
>>>
>>> stop_progress(&progress);
>>>
>>> The condition to show progress or not is conditional on an option that
>>> is external to the progress API.
>>>
>>> The fix for this patch is simple: make the trace2_region_leave() be
>>> conditional on "p_progress && *p_progress".
>
> Oops. That means that we need to apply the fix before -rc1 to
> 'master' X-<.
Sorry I didn't catch this in time for rc0!
> Something like this?
The patch below is exactly what I would have recommended. It looks like
I wrote it, too! ;)
> -- >8 --
> Subject: progress: call trace2_region_leave() only after calling _enter()
> From: Derrick Stolee <dstolee@xxxxxxxxxxxxx>
>
> A user of progress API calls start_progress() conditionally and
> depends on the display_progress() and stop_progress() functions to
> become no-op when start_progress() hasn't been called.
>
> As we added a call to trace2_region_enter() to start_progress(), the
> calls to other trace2 API calls from the progress API functions must
> make sure that these trace2 calls are skipped when start_progress()
> hasn't been called on the progress struct. Specifically, do not
> call trace2_region_leave() from stop_progress() when we haven't
> called start_progress(), which would have called the matching
> trace2_region_enter().
>
> Signed-off-by: Derrick Stolee <dstolee@xxxxxxxxxxxxx>
> Signed-off-by: Junio C Hamano <gitster@xxxxxxxxx>
> ---
> progress.c | 8 ++------
> 1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/progress.c b/progress.c
> index 6d2dcff0b6..3eda914518 100644
> --- a/progress.c
> +++ b/progress.c
> @@ -329,13 +329,9 @@ void stop_progress(struct progress **p_progress)
> trace2_data_intmax("progress", the_repository,
> "total_bytes",
> (*p_progress)->throughput->curr_total);
> - }
>
> - trace2_region_leave("progress",
> - p_progress && *p_progress
> - ? (*p_progress)->title
> - : NULL,
> - the_repository);
> + trace2_region_leave("progress", (*p_progress)->title, the_repository);
> + }
>
> stop_progress_msg(p_progress, _("done"));
> }
Thanks,
-Stolee
