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

On Thu, Aug 05, 2021 at 01:40:51AM +0200, Ævar Arnfjörð Bjarmason wrote:

On Wed, Aug 04 2021, Josh Steadmon wrote:

quoted

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 <redacted>
---

I know Taylor asked you to change it to a string from in int upthread in
[ref], but I really don't see the point. But am
willing to be convinced otherwise.

The conversion to log a string instead of an integer is necessary if
Josh wants to write "<unknown>" instead of -1. To me, that seemed
clearer, and I like that it makes the trace2 representation for a
protocol version separate from the protocol_version enum.

It seems to me that both of these codepaths will never usefully use this
new "UNKNOWN_VERSION" string you added, i.e.:

quoted

 connect.c                             |  3 +++
 protocol.c                            |  3 +++
 t/t5705-session-id-in-capabilities.sh | 12 ++++++++++++
 3 files changed, 18 insertions(+)

diff --git a/connect.c b/connect.c
index 70b13389ba..5f0e113625 100644
--- a/connect.c
+++ b/connect.c

@@ -150,6 +150,9 @@ enum protocol_version discover_version(struct packet_reader *reader)
 		break;
 	}

+	trace2_data_string("transfer", NULL, "negotiated-version",
+			   format_protocol_version(version));

Right after this.

quoted

 	switch (version) {
 	case protocol_v2:
 		process_capabilities_v2(reader);

We'll die here with BUG("unknown protocol version") if it's unknown..

Good eyes. In fact, the second switch statement shouldn't even need a
case-arm for protocol_unknown_version (but has it to cover all
enumerated values).

I didn't realize before that the unknown case really is dead code, so
we'll never log "<unknown>". And since the mapping from protocol_version
to string is identical for known values, we could probably do without
it.

And I don't much care either way. I think the benefit is really pretty
slim, and arguably my code is just adding unnecessary overhead. So I'm
happy to go with or without it, but I'd be rather sad to spend much more
of our collective time discussing it.

Thanks,
Taylor