Re: [PATCH] trace2: log progress time and throughput
- From
Junio C Hamano <gitster@pobox.com>
- Date
- May 15, 2020, 16:09 UTC
- Message-ID
- <xmqqtv0hmb7r.fsf@gitster.c.googlers.com>
- In-Reply-To
- <xmqqzha9md5d.fsf@gitster.c.googlers.com>
Junio C Hamano <gitster@pobox.com> writes:
Show 19 quoted lines
> Derrick Stolee <stolee@gmail.com> 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-<.
Something like this?
-- >8 -- Subject: progress: call trace2_region_leave() only after calling _enter() From: Derrick Stolee <dstolee@microsoft.com>
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@microsoft.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> --- 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")); }