| Message ID | 4912af5f2b949b9944b37843a9ebabdd33e66215.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 and trace2.advertiseSID is true, advertise > upload-pack's trace2 session ID via the new trace2-sid capability. I would have imagined when advertiseSID is enabled, trace2, at least the part that allocates and assigns the session ID, ought to be enabled automatically. But the above goes in a different direction and requires both to be enabled. Any compelling reason behind the choice? Does the documentation added by this series make it clear that asking for advertiseSID does NOT automatically enable allocation of session IDs (even if it does not explain why it does not happen)? Thanks.
On 2020.11.03 13:48, Junio C Hamano wrote: > Josh Steadmon <steadmon@google.com> writes: > > > When trace2 is enabled and trace2.advertiseSID is true, advertise > > upload-pack's trace2 session ID via the new trace2-sid capability. > > I would have imagined when advertiseSID is enabled, trace2, at least > the part that allocates and assigns the session ID, ought to be > enabled automatically. > > But the above goes in a different direction and requires both to be > enabled. Any compelling reason behind the choice? My reasoning was that by advertising the capability, you are telling the remote side "I have definitely produced a log using this session ID. If you need it later, you can find it with this key". If we advertise a session ID even when trace2 is not enabled, the remote side can't be as sure that the received session ID actually points to any useful logs on the other side. Of course, this is a weak guarantee since a client could send whatever it likes regardless of whether anything was logged, or one side could delete or lose its logs before the other decides it needs to view them. I think your idea in a different subthread about having a general session ID not tied to trace2 is interesting, and would also be a point in favor of changing the current behavior here, but I have some thoughts on that point that I'll add in the other subthread. I'm still leaning towards advertising a session ID only if we actually produced logs locally, but I'm open to further discussion. > Does the documentation added by this series make it clear that > asking for advertiseSID does NOT automatically enable allocation of > session IDs (even if it does not explain why it does not happen)? In V3 I'll update the docs to call out whichever decision we reach on this point. > Thanks. > >
diff --git a/upload-pack.c b/upload-pack.c index 3b858eb457..3bb01c5427 100644 --- a/upload-pack.c +++ b/upload-pack.c @@ -110,6 +110,7 @@ struct upload_pack_data { unsigned done : 1; /* v2 only */ unsigned allow_ref_in_want : 1; /* v2 only */ unsigned allow_sideband_all : 1; /* v2 only */ + unsigned advertise_trace2_sid : 1; }; static void upload_pack_data_init(struct upload_pack_data *data) @@ -141,6 +142,7 @@ static void upload_pack_data_init(struct upload_pack_data *data) packet_writer_init(&data->writer, 1); data->keepalive = 5; + data->advertise_trace2_sid = 0; } static void upload_pack_data_clear(struct upload_pack_data *data) @@ -1178,6 +1180,11 @@ static void format_symref_info(struct strbuf *buf, struct string_list *symref) strbuf_addf(buf, " symref=%s:%s", item->string, (char *)item->util); } +static void format_trace2_info(struct strbuf *buf, struct upload_pack_data *d) { + if (d->advertise_trace2_sid && trace2_is_enabled()) + strbuf_addf(buf, " trace2-sid=%s", trace2_session_id()); +} + static int send_ref(const char *refname, const struct object_id *oid, int flag, void *cb_data) { @@ -1193,9 +1200,11 @@ static int send_ref(const char *refname, const struct object_id *oid, if (capabilities) { struct strbuf symref_info = STRBUF_INIT; + struct strbuf trace2_info = STRBUF_INIT; format_symref_info(&symref_info, &data->symref); - packet_write_fmt(1, "%s %s%c%s%s%s%s%s%s object-format=%s agent=%s\n", + format_trace2_info(&trace2_info, data); + packet_write_fmt(1, "%s %s%c%s%s%s%s%s%s%s object-format=%s agent=%s\n", oid_to_hex(oid), refname_nons, 0, capabilities, (data->allow_uor & ALLOW_TIP_SHA1) ? @@ -1205,9 +1214,11 @@ static int send_ref(const char *refname, const struct object_id *oid, data->stateless_rpc ? " no-done" : "", symref_info.buf, data->allow_filter ? " filter" : "", + trace2_info.buf, the_hash_algo->name, git_user_agent_sanitized()); strbuf_release(&symref_info); + strbuf_release(&trace2_info); } else { packet_write_fmt(1, "%s %s\n", oid_to_hex(oid), refname_nons); } @@ -1299,6 +1310,8 @@ static int upload_pack_config(const char *var, const char *value, void *cb_data) data->allow_sideband_all = git_config_bool(var, value); } else if (!strcmp("core.precomposeunicode", var)) { precomposed_unicode = git_config_bool(var, value); + } else if (!strcmp("trace2.advertisesid", var)) { + data->advertise_trace2_sid = git_config_bool(var, value); } if (current_config_scope() != CONFIG_SCOPE_LOCAL &&
When trace2 is enabled and trace2.advertiseSID is true, advertise upload-pack's trace2 session ID via the new trace2-sid capability. Signed-off-by: Josh Steadmon <steadmon@google.com> --- upload-pack.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-)