Re: [PATCH] push: region_leave trace for negotiate_using_fetch

[Sam Delmerico <delmerico@google.com>]

* I don't exactly remember how I noticed it. I was doing some
debugging around the push negotiation code and either 1) saw this
region get entered twice in the trace output, or 2) I was just reading
the code around here and saw two enters.

* Perhaps there could be a check before the last git process ends that
checks that all opened trace regions have been closed? I'm not sure
how much work this would involve. It's probably also not a very
proactive way to catch these bugs since it would only get triggered
when a *user* hits a code path with a trace region that never exits.

* There could also be a test that checks that every region_enter trace
log has a corresponding region_leave. But I'm not sure how to ensure
that every code path is checked.

Overall, I'm not sure how much benefit there is from checking for
this. I'm not sure that it would have a large impact if it were to
happen again. For example, I think that it could be noticed relatively
quickly by a person/system looking at metrics like I was (e.g. if the
time spent in a region is infinite or zero).

FWIW I didn't see any other examples of this when going through logs.

Sam
<br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed,
Jan 3, 2024 at 3:37 PM Junio C Hamano &lt;gitster@pobox.com&gt;
wrote:<br></div><blockquote class="gmail_quote" style="margin: 0px 0px
0px 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left:
1ex;">Sam Delmerico &lt;<a href="mailto:delmerico@google.com"
target="_blank">delmerico@google.com</a>&gt; writes:<br>
<br>
&gt; There were two region_enter events for negotiate_using_fetch instead of<br>
&gt; one enter and one leave. This commit replaces the second region_enter<br>
&gt; event with a region_leave.<br>
&gt;<br>
&gt; Signed-off-by: Sam Delmerico &lt;<a
href="mailto:delmerico@google.com"
target="_blank">delmerico@google.com</a>&gt;<br>
&gt; ---<br>
&gt;&nbsp; fetch-pack.c | 2 +-<br>
&gt;&nbsp; 1 file changed, 1 insertion(+), 1 deletion(-)<br>
<br>
Looks right, after skimming a29263cf (fetch-pack: add tracing for<br>
negotiation rounds, 2022-08-02).&nbsp; Two questions and a half.<br>
<br>
&nbsp;* How did you find it?&nbsp; Code inspection?&nbsp; While
writing a script to<br>
&nbsp; &nbsp;parse the output from around this area, your script noticed the<br>
&nbsp; &nbsp;ever-increasing nesting level?&nbsp; Something else?<br>
<br>
&nbsp;* Would it be feasible to write some tests or tools that find<br>
&nbsp; &nbsp;similar problems (semi-)automatically?<br>
<br>
&nbsp;* Is the breakage (before this patch) something easily demonstrated<br>
&nbsp; &nbsp;in a new test in t/ somewhere?&nbsp; And if so, would it
be worth<br>
&nbsp; &nbsp;doing?<br>
<br>
Thanks.&nbsp; Will queue.<br>
<br>
<br>
&gt;<br>
&gt; diff --git a/fetch-pack.c b/fetch-pack.c<br>
&gt; index 31a72d43de..dba6d79944 100644<br>
&gt; --- a/fetch-pack.c<br>
&gt; +++ b/fetch-pack.c<br>
&gt; @@ -2218,7 +2218,7 @@ void negotiate_using_fetch(const struct
oid_array *negotiation_tips,<br>
&gt;&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
&nbsp; &nbsp; the_repository, "%d",<br>
&gt;&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
&nbsp; &nbsp; negotiation_round);<br>
&gt;&nbsp; &nbsp; &nbsp; &nbsp;}<br>
&gt; -&nbsp; &nbsp; &nbsp;trace2_region_enter("fetch-pa<wbr>ck",
"negotiate_using_fetch", the_repository);<br>
&gt; +&nbsp; &nbsp; &nbsp;trace2_region_leave("fetch-pa<wbr>ck",
"negotiate_using_fetch", the_repository);<br>
&gt;&nbsp; &nbsp; &nbsp;
&nbsp;trace2_data_intmax("<wbr>negotiate_using_fetch",
the_repository,<br>
&gt;&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
&nbsp; &nbsp; &nbsp; &nbsp; "total_rounds", negotiation_round);<br>
&gt;&nbsp; &nbsp; &nbsp; &nbsp;clear_common_flag(acked_commi<wbr>ts);<br>
&gt;<br>
&gt; base-commit: a26002b62827b89a19b1084bd75d93<wbr>71d565d03c<br>
</blockquote></div>