| Message ID | 11b5b1b54f14354f08c9eb230d5b4e6a3de1996b.1604355792.git.steadmon@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Advertise trace2 SID in protocol capabilities | expand |
Josh Steadmon <steadmon@google.com> writes: > When trace2 is enabled, the server sent a trace2-sid capability, and > trace2.advertiseSID is true, advertise fetch-pack's own session ID back > to the server. > > Signed-off-by: Josh Steadmon <steadmon@google.com> > --- > fetch-pack.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/fetch-pack.c b/fetch-pack.c > index b10c432315..7fbefa7b8e 100644 > --- a/fetch-pack.c > +++ b/fetch-pack.c > @@ -35,6 +35,8 @@ static int fetch_fsck_objects = -1; > static int transfer_fsck_objects = -1; > static int agent_supported; > static int server_supports_filtering; > +static int server_sent_trace2_sid; > +static int advertise_trace2_sid; > static struct shallow_lock shallow_lock; > static const char *alternate_shallow_file; > static struct strbuf fsck_msg_types = STRBUF_INIT; > @@ -326,6 +328,8 @@ static int find_common(struct fetch_negotiator *negotiator, > if (deepen_not_ok) strbuf_addstr(&c, " deepen-not"); > if (agent_supported) strbuf_addf(&c, " agent=%s", > git_user_agent_sanitized()); > + if (advertise_trace2_sid && server_sent_trace2_sid && trace2_is_enabled()) > + strbuf_addf(&c, " trace2-sid=%s", trace2_session_id()); > if (args->filter_options.choice) > strbuf_addstr(&c, " filter"); > packet_buf_write(&req_buf, "want %s%s\n", remote_hex, c.buf); > @@ -979,6 +983,9 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args, > agent_len, agent_feature); > } > > + if (server_supports("trace2-sid")) > + server_sent_trace2_sid = 1; > + > if (server_supports("shallow")) > print_verbose(args, _("Server supports %s"), "shallow"); > else if (args->depth > 0 || is_repository_shallow(r)) > @@ -1191,6 +1198,8 @@ static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out, > packet_buf_write(&req_buf, "command=fetch"); > if (server_supports_v2("agent", 0)) > packet_buf_write(&req_buf, "agent=%s", git_user_agent_sanitized()); > + if (advertise_trace2_sid && server_supports_v2("trace2-sid", 0) && trace2_is_enabled()) > + packet_buf_write(&req_buf, "trace2-sid=%s", trace2_session_id()); > if (args->server_options && args->server_options->nr && > server_supports_v2("server-option", 1)) { > int i; > @@ -1711,6 +1720,7 @@ static void fetch_pack_config(void) > git_config_get_bool("repack.usedeltabaseoffset", &prefer_ofs_delta); > git_config_get_bool("fetch.fsckobjects", &fetch_fsck_objects); > git_config_get_bool("transfer.fsckobjects", &transfer_fsck_objects); > + git_config_get_bool("trace2.advertisesid", &advertise_trace2_sid); > if (!uri_protocols.nr) { > char *str; The same comment as 05/11 and 06/11 applies to this repeated appearance of this boolean expression: advertise_trace2_sid && trace2_is_enabled() If we are committed to stick to the "even if we were told to advertise, do not alllocate a session ID" design, perhaps it is cleaner to clear advertise_trace2_sid at the very beginning, immediately after we learn that the tracing is disabled and the advertiseSID configuration is read. That way, everybody needs to only care about advertise_trace2_sid variable. Incidentally, if we decide to change the semantics to auto allocate the session ID if advertiseSID configuration asks us to advertise (it is OK if we do not enable the full trace2 suite), we can still make the code only check advertise_trace2_sid variable, without adding repeated check of trace2_is_enabled() everywhere at all.
On 2020.11.04 13:22, Junio C Hamano wrote: > Josh Steadmon <steadmon@google.com> writes: > > > When trace2 is enabled, the server sent a trace2-sid capability, and > > trace2.advertiseSID is true, advertise fetch-pack's own session ID back > > to the server. > > > > Signed-off-by: Josh Steadmon <steadmon@google.com> > > --- > > fetch-pack.c | 10 ++++++++++ > > 1 file changed, 10 insertions(+) > > > > diff --git a/fetch-pack.c b/fetch-pack.c > > index b10c432315..7fbefa7b8e 100644 > > --- a/fetch-pack.c > > +++ b/fetch-pack.c > > @@ -35,6 +35,8 @@ static int fetch_fsck_objects = -1; > > static int transfer_fsck_objects = -1; > > static int agent_supported; > > static int server_supports_filtering; > > +static int server_sent_trace2_sid; > > +static int advertise_trace2_sid; > > static struct shallow_lock shallow_lock; > > static const char *alternate_shallow_file; > > static struct strbuf fsck_msg_types = STRBUF_INIT; > > @@ -326,6 +328,8 @@ static int find_common(struct fetch_negotiator *negotiator, > > if (deepen_not_ok) strbuf_addstr(&c, " deepen-not"); > > if (agent_supported) strbuf_addf(&c, " agent=%s", > > git_user_agent_sanitized()); > > + if (advertise_trace2_sid && server_sent_trace2_sid && trace2_is_enabled()) > > + strbuf_addf(&c, " trace2-sid=%s", trace2_session_id()); > > if (args->filter_options.choice) > > strbuf_addstr(&c, " filter"); > > packet_buf_write(&req_buf, "want %s%s\n", remote_hex, c.buf); > > @@ -979,6 +983,9 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args, > > agent_len, agent_feature); > > } > > > > + if (server_supports("trace2-sid")) > > + server_sent_trace2_sid = 1; > > + > > if (server_supports("shallow")) > > print_verbose(args, _("Server supports %s"), "shallow"); > > else if (args->depth > 0 || is_repository_shallow(r)) > > @@ -1191,6 +1198,8 @@ static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out, > > packet_buf_write(&req_buf, "command=fetch"); > > if (server_supports_v2("agent", 0)) > > packet_buf_write(&req_buf, "agent=%s", git_user_agent_sanitized()); > > + if (advertise_trace2_sid && server_supports_v2("trace2-sid", 0) && trace2_is_enabled()) > > + packet_buf_write(&req_buf, "trace2-sid=%s", trace2_session_id()); > > if (args->server_options && args->server_options->nr && > > server_supports_v2("server-option", 1)) { > > int i; > > @@ -1711,6 +1720,7 @@ static void fetch_pack_config(void) > > git_config_get_bool("repack.usedeltabaseoffset", &prefer_ofs_delta); > > git_config_get_bool("fetch.fsckobjects", &fetch_fsck_objects); > > git_config_get_bool("transfer.fsckobjects", &transfer_fsck_objects); > > + git_config_get_bool("trace2.advertisesid", &advertise_trace2_sid); > > if (!uri_protocols.nr) { > > char *str; > > The same comment as 05/11 and 06/11 applies to this repeated > appearance of this boolean expression: > > advertise_trace2_sid && trace2_is_enabled() > > If we are committed to stick to the "even if we were told to > advertise, do not alllocate a session ID" design, perhaps it is > cleaner to clear advertise_trace2_sid at the very beginning, > immediately after we learn that the tracing is disabled and the > advertiseSID configuration is read. That way, everybody needs to > only care about advertise_trace2_sid variable. > > Incidentally, if we decide to change the semantics to auto allocate > the session ID if advertiseSID configuration asks us to advertise > (it is OK if we do not enable the full trace2 suite), we can still > make the code only check advertise_trace2_sid variable, without > adding repeated check of trace2_is_enabled() everywhere at all. Good point. Once we settle on whether or not to advertise when tracing is enabled, I'll update these conditionals in V3.
Josh Steadmon <steadmon@google.com> writes: >> The same comment as 05/11 and 06/11 applies to this repeated >> appearance of this boolean expression: >> >> advertise_trace2_sid && trace2_is_enabled() >> >> If we are committed to stick to the "even if we were told to >> advertise, do not alllocate a session ID" design, perhaps it is >> cleaner to clear advertise_trace2_sid at the very beginning, >> immediately after we learn that the tracing is disabled and the >> advertiseSID configuration is read. That way, everybody needs to >> only care about advertise_trace2_sid variable. >> >> Incidentally, if we decide to change the semantics to auto allocate >> the session ID if advertiseSID configuration asks us to advertise >> (it is OK if we do not enable the full trace2 suite), we can still >> make the code only check advertise_trace2_sid variable, without >> adding repeated check of trace2_is_enabled() everywhere at all. > > Good point. Once we settle on whether or not to advertise when tracing > is enabled, I'll update these conditionals in V3. Well, we can update these conditionals _before_ deciding that, and that is the whole point of the part of my message you are responding to. Whether the advertise_trace2_sid means (1) config asked and tracing enabled, or (2) config asked and we do not care whether tracing is enabled or not it makes it easier to flip between (1) and (2) later if you clean up the conditional first. Thanks.
On 2020.11.05 11:21, Junio C Hamano wrote: > Josh Steadmon <steadmon@google.com> writes: > > >> The same comment as 05/11 and 06/11 applies to this repeated > >> appearance of this boolean expression: > >> > >> advertise_trace2_sid && trace2_is_enabled() > >> > >> If we are committed to stick to the "even if we were told to > >> advertise, do not alllocate a session ID" design, perhaps it is > >> cleaner to clear advertise_trace2_sid at the very beginning, > >> immediately after we learn that the tracing is disabled and the > >> advertiseSID configuration is read. That way, everybody needs to > >> only care about advertise_trace2_sid variable. > >> > >> Incidentally, if we decide to change the semantics to auto allocate > >> the session ID if advertiseSID configuration asks us to advertise > >> (it is OK if we do not enable the full trace2 suite), we can still > >> make the code only check advertise_trace2_sid variable, without > >> adding repeated check of trace2_is_enabled() everywhere at all. > > > > Good point. Once we settle on whether or not to advertise when tracing > > is enabled, I'll update these conditionals in V3. > > Well, we can update these conditionals _before_ deciding that, and > that is the whole point of the part of my message you are responding > to. > > Whether the advertise_trace2_sid means > > (1) config asked and tracing enabled, or > > (2) config asked and we do not care whether tracing is enabled or not > > it makes it easier to flip between (1) and (2) later if you clean up > the conditional first. > > Thanks. > Done in V3. After some additional offline discussions, I've been convinced that it makes sense to advertise the session-id capability even when trace2 is not enabled. Specifically, it can be useful on the server side to identify multiple connections that make up a single larger operation, such as in a single `repo` invocation.
diff --git a/fetch-pack.c b/fetch-pack.c index b10c432315..7fbefa7b8e 100644 --- a/fetch-pack.c +++ b/fetch-pack.c @@ -35,6 +35,8 @@ static int fetch_fsck_objects = -1; static int transfer_fsck_objects = -1; static int agent_supported; static int server_supports_filtering; +static int server_sent_trace2_sid; +static int advertise_trace2_sid; static struct shallow_lock shallow_lock; static const char *alternate_shallow_file; static struct strbuf fsck_msg_types = STRBUF_INIT; @@ -326,6 +328,8 @@ static int find_common(struct fetch_negotiator *negotiator, if (deepen_not_ok) strbuf_addstr(&c, " deepen-not"); if (agent_supported) strbuf_addf(&c, " agent=%s", git_user_agent_sanitized()); + if (advertise_trace2_sid && server_sent_trace2_sid && trace2_is_enabled()) + strbuf_addf(&c, " trace2-sid=%s", trace2_session_id()); if (args->filter_options.choice) strbuf_addstr(&c, " filter"); packet_buf_write(&req_buf, "want %s%s\n", remote_hex, c.buf); @@ -979,6 +983,9 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args, agent_len, agent_feature); } + if (server_supports("trace2-sid")) + server_sent_trace2_sid = 1; + if (server_supports("shallow")) print_verbose(args, _("Server supports %s"), "shallow"); else if (args->depth > 0 || is_repository_shallow(r)) @@ -1191,6 +1198,8 @@ static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out, packet_buf_write(&req_buf, "command=fetch"); if (server_supports_v2("agent", 0)) packet_buf_write(&req_buf, "agent=%s", git_user_agent_sanitized()); + if (advertise_trace2_sid && server_supports_v2("trace2-sid", 0) && trace2_is_enabled()) + packet_buf_write(&req_buf, "trace2-sid=%s", trace2_session_id()); if (args->server_options && args->server_options->nr && server_supports_v2("server-option", 1)) { int i; @@ -1711,6 +1720,7 @@ static void fetch_pack_config(void) git_config_get_bool("repack.usedeltabaseoffset", &prefer_ofs_delta); git_config_get_bool("fetch.fsckobjects", &fetch_fsck_objects); git_config_get_bool("transfer.fsckobjects", &transfer_fsck_objects); + git_config_get_bool("trace2.advertisesid", &advertise_trace2_sid); if (!uri_protocols.nr) { char *str;
When trace2 is enabled, the server sent a trace2-sid capability, and trace2.advertiseSID is true, advertise fetch-pack's own session ID back to the server. Signed-off-by: Josh Steadmon <steadmon@google.com> --- fetch-pack.c | 10 ++++++++++ 1 file changed, 10 insertions(+)