Re: [PATCH] trace2: log progress time and throughput

[Junio C Hamano <gitster@pobox.com>]

Junio C Hamano <gitster@pobox.com> writes:

> 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"));
 }