Re: [PATCH v2 2/2] connect, protocol: log negotiated protocol version

From: Josh Steadmon <steadmon@google.com>
To: Eric Sunshine <sunshine@sunshineco.com>
Cc: Git List <git@vger.kernel.org>,
	Junio C Hamano <gitster@pobox.com>, Taylor Blau <me@ttaylorr.com>
Subject: Re: [PATCH v2 2/2] connect, protocol: log negotiated protocol version
Date: Fri, 6 Aug 2021 14:15:58 -0700	[thread overview]
Message-ID: <YQ2mjmwoRYO4jqaH@google.com> (raw)
In-Reply-To: <CAPig+cSqm5cUmAVG6f73oZ+mi9pBJrOg2va2a_Op4=wNFyHfZQ@mail.gmail.com>

On 2021.08.04 18:28, Eric Sunshine wrote:
> On Wed, Aug 4, 2021 at 6:17 PM Josh Steadmon <steadmon@google.com> wrote:
> > It is useful for performance monitoring and debugging purposes to know
> > the wire protocol used for remote operations. This may differ from the
> > version set in local configuration due to differences in version and/or
> > configuration between the server and the client. Therefore, log the
> > negotiated wire protocol version via trace2, for both clients and
> > servers.
> >
> > Signed-off-by: Josh Steadmon <steadmon@google.com>
> > ---
> > diff --git a/t/t5705-session-id-in-capabilities.sh b/t/t5705-session-id-in-capabilities.sh
> > @@ -73,6 +74,17 @@ do
> > +       test_expect_success "client & server log negotiated version (v${PROTO})" '
> > +               test_when_finished "rm -rf local tr2-client-events tr2-server-events" &&
> > +               cp -r "$LOCAL_PRISTINE" local &&
> > +               GIT_TRACE2_EVENT="$(pwd)/tr2-client-events" \
> > +               git -c protocol.version=$PROTO -C local fetch \
> > +                       --upload-pack "GIT_TRACE2_EVENT=\"$(pwd)/tr2-server-events\" git-upload-pack" \
> > +                       origin &&
> > +               test "$(grep \"key\":\"negotiated-version\",\"value\":\"$PROTO\" tr2-client-events)" &&
> > +               test "$(grep \"key\":\"negotiated-version\",\"value\":\"$PROTO\" tr2-server-events)"
> > +       '
> >  done
> 
> What are these `test` commands actually testing? Did you mean `test
> -n`? Or, even better, just plain `grep` (not within a
> command-substitution)?

Yes, sorry about that, just plain grep is best here. This was due to a
quick copy & paste modification, I should have spent a bit more time
thinking about the test case.

Will be fixed in V3.