Re: [PATCH] push: region_leave trace for negotiate_using_fetch

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 3, 2024, 23:37 UTC
Message-ID: <xmqqbka27zu9.fsf@gitster.g>
In-Reply-To: <20240103224054.1940209-1-delmerico@google.com>

Sam Delmerico <delmerico@google.com> writes:

Show 8 quoted lines

> There were two region_enter events for negotiate_using_fetch instead of
> one enter and one leave. This commit replaces the second region_enter
> event with a region_leave.
>
> Signed-off-by: Sam Delmerico <delmerico@google.com>
> ---
>  fetch-pack.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Looks right, after skimming a29263cf (fetch-pack: add tracing for negotiation rounds, 2022-08-02). Two questions and a half.

 * How did you find it?  Code inspection?  While writing a script to
   parse the output from around this area, your script noticed the
   ever-increasing nesting level?  Something else?

 * Would it be feasible to write some tests or tools that find
   similar problems (semi-)automatically?

 * Is the breakage (before this patch) something easily demonstrated
   in a new test in t/ somewhere?  And if so, would it be worth
   doing?

Thanks.  Will queue.

Show 16 quoted lines

>
> diff --git a/fetch-pack.c b/fetch-pack.c
> index 31a72d43de..dba6d79944 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -2218,7 +2218,7 @@ void negotiate_using_fetch(const struct oid_array *negotiation_tips,
>  					   the_repository, "%d",
>  					   negotiation_round);
>  	}
> -	trace2_region_enter("fetch-pack", "negotiate_using_fetch", the_repository);
> +	trace2_region_leave("fetch-pack", "negotiate_using_fetch", the_repository);
>  	trace2_data_intmax("negotiate_using_fetch", the_repository,
>  			   "total_rounds", negotiation_round);
>  	clear_common_flag(acked_commits);
>
> base-commit: a26002b62827b89a19b1084bd75d9371d565d03c