{"thread":{"id":"31226","subject":"[PATCH 0/4] jk/version-string and google code","startedAt":"2012-08-10T07:53:42Z","lastAt":"2012-08-14T04:56:57Z","messageCount":37,"participants":["Jeff King","Eric Sunshine","Junio C Hamano","Dave Borowitz","Shawn Pearce"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"196780","messageId":"20120810075342.GA30072@sigill.intra.peff.net","threadId":"31226","inReplyTo":null,"subject":"[PATCH 0/4] jk/version-string and google code","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-08-10T07:53:42Z","receivedAt":"2012-08-10T07:53:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 03, 2012 at 12:19:16PM -0400, Jeff King wrote:\n\n> Instead of having the client advertise a particular version\n> number in the git protocol, we have managed extensions and\n> backwards compatibility by having clients and servers\n> advertise capabilities that they support. This is far more\n> robust than having each side consult a table of\n> known versions, and provides sufficient information for the\n> protocol interaction to complete.\n> \n> However, it does not allow servers to keep statistics on\n> which client versions are being used. This information is\n> not necessary to complete the network request (the\n> capabilities provide enough information for that), but it\n> may be helpful to conduct a general survey of client\n> versions in use.\n> \n> We already send the client version in the user-agent header\n> for http requests; adding it here allows us to gather\n> similar statistics for non-http requests.\n\nUgh, the jk/version-string topic breaks fetching from Google Code. With\nmy patch, the client unconditionally sends an \"agent=foo\" capability,\nbut the server does not like seeing the unknown capability and ends the\nconnection (I'm guessing with some kind of internal exception, since it\nspews \"Internal server error\" over the protocol channel).\n\nThis is the right thing to do according to protocol-capabilities.txt,\nwhich says:\n\n Client will then send a space separated list of capabilities it wants\n to be in effect. The client MUST NOT ask for capabilities the server\n did not say it supports.\n\n Server MUST diagnose and abort if capabilities it does not understand\n was sent. Server MUST NOT ignore capabilities that client requested\n and server advertised. As a consequence of these rules, server MUST\n NOT advertise capabilities it does not understand.\n\nHowever, that is not how git-core behaves. Its server side will ignore\nan unknown capability coming from the client (so not only is it more\nlenient about what the client does, but it does not follow the \"MUST\"\ndirectives in the second paragraph).\n\nThis isn't a huge deal for this topic; any server that is collecting the\ndata should be advertising anyway. The only ones who would miss out are\nhumans trying to debug client behavior via tcpdump or similar, when the\nserver side is an older version of git.\n\nThat's probably acceptable, given that the alternative is changing Google\nCode's implementation, along with finding out how many other\nimplementations might have followed that spec strictly. We might or\nmight not want to loosen the \"MUST\" bits in that document, since git\nitself does not follow them.\n\nHere's a patch series that goes on top of jk/version-string:\n\n [1/4]: send-pack: fix capability-sending logic\n\nThis one is a minor bug fix in the same area.\n\n [2/4]: do not send client agent unless server does first\n\nThe actual fix.\n\n [3/4]: connect: learn to parse capabilities with values\n [4/4]: fetch-pack: mention server version with verbose output\n\nA bonus feature. I'm not sure if they are worth doing or not. I'd\nreally expect somebody debugging a protocol issue to just use\nGIT_TRACE_PACKET, and then they can read it straight from the packet\ndump themselves.\n\n-Peff\n"},{"id":"196781","messageId":"20120810075731.GA8399@sigill.intra.peff.net","threadId":"31226","inReplyTo":"20120810075342.GA30072@sigill.intra.peff.net","subject":"[PATCH 1/4] send-pack: fix capability-sending logic","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-08-10T07:57:31Z","receivedAt":"2012-08-10T07:57:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If we have capabilities to send to the server, we send the\nregular \"want\" line followed by a NUL, then the\ncapabilities; otherwise, we do not even send the NUL.\n\nHowever, when checking whether we want to send the \"quiet\"\ncapability, we check args->quiet, which is wrong. That flag\nonly tells us whether the client side wanted to be quiet,\nnot whether the server supports it (originally, in c207e34f,\nit meant both; however, that was later split into two flags\nby 01fdc21f).\n\nWe still check the right flag when actually printing\n\"quiet\", so this could only have two effects:\n\n 1. We might send the trailing NUL when we do not otherwise\n need to. In theory, an antique pre-capability\n implementation of git might choke on this (since the\n client is instructed never to respond with capabilities\n that the server has not first advertised).\n\n 2. We might also want to send the quiet flag if the\n args->progress flag is false, but this code path would\n not trigger in that instance.\n\nIn practice, it almost certainly never matters. The\nreport-status capability dates back to 2005. Any real-world\nserver is going to advertise that, and we will always\nrespond with at least that capability.\n\nSigned-off-by: Jeff King \n---\nI'm tempted to remove this part of the conditional entirely to make the\ncode simpler, which would mean that we always send the extra NUL, even\nif there are no capabilities. But I'm not 100% sure that pre-1.1.0\nversions of git actually handle that, and who knows if there are other\nimplementations. This fix is the safe, conservative route.\n\n builtin/send-pack.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/send-pack.c b/builtin/send-pack.c\nindex c4d4211..5c69995 100644\n--- a/builtin/send-pack.c\n+++ b/builtin/send-pack.c\n@@ -306,7 +306,7 @@ int send_pack(struct send_pack_args *args,\n \t\t\tchar *new_hex = sha1_to_hex(ref->new_sha1);\n \t\t\tint quiet = quiet_supported && (args->quiet || !args->progress);\n \n-\t\t\tif (!cmds_sent && (status_report || use_sideband || args->quiet)) {\n+\t\t\tif (!cmds_sent && (status_report || use_sideband || quiet)) {\n \t\t\t\tpacket_buf_write(&req_buf,\n \t\t\t\t\t\t \"%s %s %s%c%s%s%s agent=%s\",\n \t\t\t\t\t\t old_hex, new_hex, ref->name, 0,\n-- \n1.7.12.rc2.4.g7f05cf9\n"},{"id":"196782","messageId":"20120810075743.GB8399@sigill.intra.peff.net","threadId":"31226","inReplyTo":"20120810075342.GA30072@sigill.intra.peff.net","subject":"[PATCH 2/4] do not send client agent unless server does first","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-08-10T07:57:43Z","receivedAt":"2012-08-10T07:57:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Commit ff5effdf taught both clients and servers of the git\nprotocol to send an \"agent\" capability that just advertises\ntheir version for statistics and debugging purposes.\nHowever, the protocol-capabilities.txt document indicates\nthat the client's advertisement is actually a response, and\nshould never include capabilities not mentioned in the\nserver's advertisement.\n\nAdding the unconditional advertisement in the server\nprograms was OK, then, but the clients broke the protocol.\nThe server implementation of git-core itself does not care,\nbut at least one does: the Google Code git server will hang\nup with an internal error upon seeing an unknown capability.\n\nInstead, each client must record whether we saw an agent\nstring from the server, and respond with its agent only if\nthe server mentioned it first.\n\nSigned-off-by: Jeff King \n---\n builtin/fetch-pack.c | 7 ++++++-\n builtin/send-pack.c | 12 +++++++++---\n 2 files changed, 15 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex fe56596..bc7a0f9 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -19,6 +19,7 @@ static int prefer_ofs_delta = 1;\n static int no_done;\n static int fetch_fsck_objects = -1;\n static int transfer_fsck_objects = -1;\n+static int agent_supported;\n static struct fetch_pack_args args = {\n \t/* .uploadpack = */ \"git-upload-pack\",\n };\n@@ -328,7 +329,8 @@ static int find_common(int fd[2], unsigned char *result_sha1,\n \t\t\tif (args.no_progress) strbuf_addstr(&c, \" no-progress\");\n \t\t\tif (args.include_tag) strbuf_addstr(&c, \" include-tag\");\n \t\t\tif (prefer_ofs_delta) strbuf_addstr(&c, \" ofs-delta\");\n-\t\t\tstrbuf_addf(&c, \" agent=%s\", git_user_agent_sanitized());\n+\t\t\tif (agent_supported) strbuf_addf(&c, \" agent=%s\",\n+\t\t\t\t\t\t\t git_user_agent_sanitized());\n \t\t\tpacket_buf_write(&req_buf, \"want %s%s\\n\", remote_hex, c.buf);\n \t\t\tstrbuf_release(&c);\n \t\t} else\n@@ -821,6 +823,9 @@ static struct ref *do_fetch_pack(int fd[2],\n \t\t\tfprintf(stderr, \"Server supports ofs-delta\\n\");\n \t} else\n \t\tprefer_ofs_delta = 0;\n+\tif (server_supports(\"agent\"))\n+\t\tagent_supported = 1;\n+\n \tif (everything_local(&ref, nr_match, match)) {\n \t\tpacket_flush(fd[1]);\n \t\tgoto all_done;\ndiff --git a/builtin/send-pack.c b/builtin/send-pack.c\nindex 5c69995..7d05064 100644\n--- a/builtin/send-pack.c\n+++ b/builtin/send-pack.c\n@@ -252,6 +252,7 @@ int send_pack(struct send_pack_args *args,\n \tint status_report = 0;\n \tint use_sideband = 0;\n \tint quiet_supported = 0;\n+\tint agent_supported = 0;\n \tunsigned cmds_sent = 0;\n \tint ret;\n \tstruct async demux;\n@@ -267,6 +268,8 @@ int send_pack(struct send_pack_args *args,\n \t\tuse_sideband = 1;\n \tif (server_supports(\"quiet\"))\n \t\tquiet_supported = 1;\n+\tif (server_supports(\"agent\"))\n+\t\tagent_supported = 1;\n \n \tif (!remote_refs) {\n \t\tfprintf(stderr, \"No refs in common and none specified; doing nothing.\\n\"\n@@ -306,14 +309,17 @@ int send_pack(struct send_pack_args *args,\n \t\t\tchar *new_hex = sha1_to_hex(ref->new_sha1);\n \t\t\tint quiet = quiet_supported && (args->quiet || !args->progress);\n \n-\t\t\tif (!cmds_sent && (status_report || use_sideband || quiet)) {\n+\t\t\tif (!cmds_sent && (status_report || use_sideband ||\n+\t\t\t\t\t quiet || agent_supported)) {\n \t\t\t\tpacket_buf_write(&req_buf,\n-\t\t\t\t\t\t \"%s %s %s%c%s%s%s agent=%s\",\n+\t\t\t\t\t\t \"%s %s %s%c%s%s%s%s%s\",\n \t\t\t\t\t\t old_hex, new_hex, ref->name, 0,\n \t\t\t\t\t\t status_report ? \" report-status\" : \"\",\n \t\t\t\t\t\t use_sideband ? \" side-band-64k\" : \"\",\n \t\t\t\t\t\t quiet ? \" quiet\" : \"\",\n-\t\t\t\t\t\t git_user_agent_sanitized());\n+\t\t\t\t\t\t agent_supported ? \" agent=\" : \"\",\n+\t\t\t\t\t\t agent_supported ? git_user_agent_sanitized() : \"\"\n+\t\t\t\t\t\t);\n \t\t\t}\n \t\t\telse\n \t\t\t\tpacket_buf_write(&req_buf, \"%s %s %s\",\n-- \n1.7.12.rc2.4.g7f05cf9\n"},{"id":"196783","messageId":"20120810075816.GC8399@sigill.intra.peff.net","threadId":"31226","inReplyTo":"20120810075342.GA30072@sigill.intra.peff.net","subject":"[PATCH 3/4] connect: learn to parse capabilities with values","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-08-10T07:58:16Z","receivedAt":"2012-08-10T07:58:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We already take care to parse a capability like \"foo=bar\"\nproperly, but the code does not provide a good way of\nactually finding out what is on the right-hand side of the\n\"=\".\n\nSigned-off-by: Jeff King \n---\n cache.h | 2 ++\n connect.c | 32 ++++++++++++++++++++++++++++++++\n 2 files changed, 34 insertions(+)\n\ndiff --git a/cache.h b/cache.h\nindex 06413e1..3811c66 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1030,7 +1030,9 @@ struct extra_have_objects {\n };\n extern struct ref **get_remote_heads(int in, struct ref **list, unsigned int flags, struct extra_have_objects *);\n extern int server_supports(const char *feature);\n+extern char *server_feature_value(const char *feature);\n extern const char *parse_feature_request(const char *features, const char *feature);\n+extern char *parse_feature_request_value(const char *features, const char *feature);\n \n extern struct packed_git *parse_pack_index(unsigned char *sha1, const char *idx_path);\n \ndiff --git a/connect.c b/connect.c\nindex 912cdde..6bd2531 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -104,6 +104,11 @@ int server_supports(const char *feature)\n \treturn !!parse_feature_request(server_capabilities, feature);\n }\n \n+char *server_feature_value(const char *feature)\n+{\n+\treturn parse_feature_request_value(server_capabilities, feature);\n+}\n+\n const char *parse_feature_request(const char *feature_list, const char *feature)\n {\n \tint len;\n@@ -124,6 +129,33 @@ const char *parse_feature_request(const char *feature_list, const char *feature)\n \treturn NULL;\n }\n \n+/*\n+ * Parse features of the form \"feature=value\". Returns NULL if the feature\n+ * does not exist, the empty string if it exists but does not have an \"=\", or\n+ * the content to the right of the \"=\" until the first space (or end of\n+ * string). The cannot contain literal spaces; double-quoting or similar\n+ * schemes would break compatibility, since older versions of git treat the\n+ * space as a hard-delimiter without any context.\n+ *\n+ * The return value (if non-NULL) is newly allocated on the heap and belongs to\n+ * the caller.\n+ */\n+char *parse_feature_request_value(const char *feature_list, const char *feature)\n+{\n+\tconst char *start = parse_feature_request(feature_list, feature);\n+\tconst char *end;\n+\n+\tif (!start || prefixcmp(start, feature))\n+\t\treturn NULL;\n+\tstart += strlen(feature);\n+\n+\tif (*start == '=')\n+\t\tstart++;\n+\tend = strchrnul(start, ' ');\n+\n+\treturn xmemdupz(start, end - start);\n+}\n+\n enum protocol {\n \tPROTO_LOCAL = 1,\n \tPROTO_SSH,\n-- \n1.7.12.rc2.4.g7f05cf9\n"},{"id":"196784","messageId":"20120810075929.GD8399@sigill.intra.peff.net","threadId":"31226","inReplyTo":"20120810075342.GA30072@sigill.intra.peff.net","subject":"[PATCH 4/4] fetch-pack: mention server version with verbose output","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-08-10T07:59:29Z","receivedAt":"2012-08-10T07:59:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Fetch-pack's verbose mode is more of a debugging mode (and\nin fact takes two \"-v\" arguments to trigger via the\nporcelain layer). Let's mention the server version as\nanother possible item of interest.\n\nSigned-off-by: Jeff King \n---\nLike I mentioned before, I'm lukewarm on this one, because I doubt\nanybody actually uses this debugging information. If we drop it, it\nmakes sense to drop 3/4, too, which is just infrastructure for this.\n\n builtin/fetch-pack.c | 8 +++++++-\n 1 file changed, 7 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex bc7a0f9..bfe31ee 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -823,8 +823,14 @@ static struct ref *do_fetch_pack(int fd[2],\n \t\t\tfprintf(stderr, \"Server supports ofs-delta\\n\");\n \t} else\n \t\tprefer_ofs_delta = 0;\n-\tif (server_supports(\"agent\"))\n+\tif (server_supports(\"agent\")) {\n \t\tagent_supported = 1;\n+\t\tif (args.verbose) {\n+\t\t\tchar *v = server_feature_value(\"agent\");\n+\t\t\tfprintf(stderr, \"Server version is %s\\n\", v);\n+\t\t\tfree(v);\n+\t\t}\n+\t}\n \n \tif (everything_local(&ref, nr_match, match)) {\n \t\tpacket_flush(fd[1]);\n-- \n1.7.12.rc2.4.g7f05cf9\n"},{"id":"196785","messageId":"CAPig+cT2Dtvbm=mDxYvgn8CA_zBYVcQiiMv+8pPjUvBsFAGLYA@mail.gmail.com","threadId":"31226","inReplyTo":"20120810075816.GC8399@sigill.intra.peff.net","subject":"Re: [PATCH 3/4] connect: learn to parse capabilities with values","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2012-08-10T08:06:40Z","receivedAt":"2012-08-10T08:06:40Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Aug 10, 2012 at 3:58 AM, Jeff King wrote:\n> + * Parse features of the form \"feature=value\". Returns NULL if the feature\n> + * does not exist, the empty string if it exists but does not have an \"=\", or\n> + * the content to the right of the \"=\" until the first space (or end of\n> + * string). The cannot contain literal spaces; double-quoting or similar\n\ns/The cannot/The value cannot/\n\n-- ES\n"},{"id":"196800","messageId":"7vfw7ulq0a.fsf@alter.siamese.dyndns.org","threadId":"31226","inReplyTo":"20120810075342.GA30072@sigill.intra.peff.net","subject":"Re: [PATCH 0/4] jk/version-string and google code","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-10T15:34:45Z","receivedAt":"2012-08-10T15:34:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Ugh, the jk/version-string topic breaks fetching from Google Code. With\n> my patch, the client unconditionally sends an \"agent=foo\" capability,\n> but the server does not like seeing the unknown capability and ends the\n> connection (I'm guessing with some kind of internal exception, since it\n> spews \"Internal server error\" over the protocol channel).\n\nThe fix looks sane and the right thing to do.\n\nI've been using 'next' and started seeing this breakage when pushing\nto code.google.com only a few days ago. My reflog tells that\nff5effd (include agent identifier in capability string, 2012-08-03)\nwas merged to my everyday-work branch on this Monday, which is more\nor less consistent with what I am observing.\n\n> This is the right thing to do according to protocol-capabilities.txt,\n> which says:\n>\n> Client will then send a space separated list of capabilities it wants\n> to be in effect. The client MUST NOT ask for capabilities the server\n> did not say it supports.\n>\n> Server MUST diagnose and abort if capabilities it does not understand\n> was sent. Server MUST NOT ignore capabilities that client requested\n> and server advertised. As a consequence of these rules, server MUST\n> NOT advertise capabilities it does not understand.\n>\n> However, that is not how git-core behaves. Its server side will ignore\n> an unknown capability coming from the client (so not only is it more\n> lenient about what the client does, but it does not follow the \"MUST\"\n> directives in the second paragraph).\n\nYeah, we probably should fix in the implementation to honor the\n\"MUST\". When various implementations of clients can start asking\nsomething the server does not support and still yet expect the\nrequest to cause the server do unusual things, it will lead to\nchaos, and honoring that \"MUST\" is a good way to catch and help\ndiagnosing such a breakage early.\n\nBut that is a separate topic and a longer term direction item.\n\nThanks.\n"},{"id":"196801","messageId":"7vboiilpvs.fsf@alter.siamese.dyndns.org","threadId":"31226","inReplyTo":"20120810075342.GA30072@sigill.intra.peff.net","subject":"Re: [PATCH 0/4] jk/version-string and google code","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-10T15:37:27Z","receivedAt":"2012-08-10T15:37:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Ugh, the jk/version-string topic breaks fetching from Google Code. With\n> my patch, the client unconditionally sends an \"agent=foo\" capability,\n> but the server does not like seeing the unknown capability and ends the\n> connection (I'm guessing with some kind of internal exception, since it\n> spews \"Internal server error\" over the protocol channel).\n\nI asked the folks who run code.google.com and they are indeed seeing\nsomething like these in their logs:\n\n >> Client asked for capability agent=git/1.7.12.rc2.79.g86c1702 that was not advertised.\n\nSo please consider your conjecture confirmed, and thanks for a\nprompt fix.\n"},{"id":"196813","messageId":"20120810174605.GA29113@sigill.intra.peff.net","threadId":"31226","inReplyTo":"7vfw7ulq0a.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 0/4] jk/version-string and google code","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-08-10T17:46:05Z","receivedAt":"2012-08-10T17:46:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 10, 2012 at 08:34:45AM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > Ugh, the jk/version-string topic breaks fetching from Google Code. With\n> > my patch, the client unconditionally sends an \"agent=foo\" capability,\n> > but the server does not like seeing the unknown capability and ends the\n> > connection (I'm guessing with some kind of internal exception, since it\n> > spews \"Internal server error\" over the protocol channel).\n> \n> The fix looks sane and the right thing to do.\n> \n> I've been using 'next' and started seeing this breakage when pushing\n> to code.google.com only a few days ago. My reflog tells that\n> ff5effd (include agent identifier in capability string, 2012-08-03)\n> was merged to my everyday-work branch on this Monday, which is more\n> or less consistent with what I am observing.\n\nThanks for confirming the push side. I have been running with the patch\nfor months, but only recently happened to try cloning something from\ncode.google.com. I assumed the push side had the same problem, but did\nnot want to make a new project just to test (and it seemed fairly\nobvious that it would have the same issue).\n\nIt was pure coincidence that it happened at the same time you were\ngraduating the topic to next. But I'm glad I was able to catch it before\nanybody started complaining. :)\n\n-Peff\n"},{"id":"196815","messageId":"CAD0k6qTnQ+b8e2wt4WZ2HhOdsKXzcMkGbUZBkbw2j6_mQdE9GA@mail.gmail.com","threadId":"31226","inReplyTo":"7vboiilpvs.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 0/4] jk/version-string and google code","fromName":"Dave Borowitz","fromEmail":"dborowitz@google.com","sentAt":"2012-08-10T18:06:08Z","receivedAt":"2012-08-10T18:06:08Z","isPatch":true,"sender":{"key":"dborowitz@google.com","avatar":"https://avatars.githubusercontent.com/u/194927?v=4"},"body":"On Fri, Aug 10, 2012 at 8:37 AM, Junio C Hamano wrote:\n> Jeff King writes:\n>\n>> Ugh, the jk/version-string topic breaks fetching from Google Code. With\n>> my patch, the client unconditionally sends an \"agent=foo\" capability,\n>> but the server does not like seeing the unknown capability and ends the\n>> connection (I'm guessing with some kind of internal exception, since it\n>> spews \"Internal server error\" over the protocol channel).\n>\n> I asked the folks who run code.google.com and they are indeed seeing\n> something like these in their logs:\n>\n> >> Client asked for capability agent=git/1.7.12.rc2.79.g86c1702 that was not advertised.\n\nFWIW, this error comes from Dulwich:\nhttps://github.com/jelmer/dulwich/blob/25250c1694dac343d469742aeafa139f37fc4ec6/dulwich/server.py#L196\n\nSo any servers running Dulwich would be affected by this...though I'm\nnot aware of any large-scale Dulwich installations other than Google\nCode.\n\n> So please consider your conjecture confirmed, and thanks for a\n> prompt fix.\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n"},{"id":"196816","messageId":"20120810180836.GA29597@sigill.intra.peff.net","threadId":"31226","inReplyTo":"CAD0k6qTnQ+b8e2wt4WZ2HhOdsKXzcMkGbUZBkbw2j6_mQdE9GA@mail.gmail.com","subject":"Re: [PATCH 0/4] jk/version-string and google code","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-08-10T18:08:36Z","receivedAt":"2012-08-10T18:08:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 10, 2012 at 11:06:08AM -0700, Dave Borowitz wrote:\n\n> > I asked the folks who run code.google.com and they are indeed seeing\n> > something like these in their logs:\n> >\n> > >> Client asked for capability agent=git/1.7.12.rc2.79.g86c1702 that was not advertised.\n> \n> FWIW, this error comes from Dulwich:\n> https://github.com/jelmer/dulwich/blob/25250c1694dac343d469742aeafa139f37fc4ec6/dulwich/server.py#L196\n\nThanks for the data point. I knew you guys ran some custom code, so I\nwasn't sure how widespread this is. The fact that other dulwich-based\nservers would see the same issue makes me doubly sure that my fix is the\nright direction.\n\n> So any servers running Dulwich would be affected by this...though I'm\n> not aware of any large-scale Dulwich installations other than Google\n> Code.\n\nI'd rather not break small-scale installations, either. :)\n\n-Peff\n"},{"id":"196817","messageId":"CAD0k6qSZYH5mvwR5PKhs1FFAPqMfRryXooxd_qhtt+eGXU7pUA@mail.gmail.com","threadId":"31226","inReplyTo":"20120810180836.GA29597@sigill.intra.peff.net","subject":"Re: [PATCH 0/4] jk/version-string and google code","fromName":"Dave Borowitz","fromEmail":"dborowitz@google.com","sentAt":"2012-08-10T18:13:30Z","receivedAt":"2012-08-10T18:13:30Z","isPatch":true,"sender":{"key":"dborowitz@google.com","avatar":"https://avatars.githubusercontent.com/u/194927?v=4"},"body":"On Fri, Aug 10, 2012 at 11:08 AM, Jeff King wrote:\n> On Fri, Aug 10, 2012 at 11:06:08AM -0700, Dave Borowitz wrote:\n>\n>> > I asked the folks who run code.google.com and they are indeed seeing\n>> > something like these in their logs:\n>> >\n>> > >> Client asked for capability agent=git/1.7.12.rc2.79.g86c1702 that was not advertised.\n>>\n>> FWIW, this error comes from Dulwich:\n>> https://github.com/jelmer/dulwich/blob/25250c1694dac343d469742aeafa139f37fc4ec6/dulwich/server.py#L196\n>\n> Thanks for the data point. I knew you guys ran some custom code, so I\n> wasn't sure how widespread this is. The fact that other dulwich-based\n> servers would see the same issue makes me doubly sure that my fix is the\n> right direction.\n\nYou may also notice in that code a set of innocuous_capabilities,\nwhich IIRC is the complete set of capabilities, at the time of\nwriting, that the C git client may send without the server advertising\nthem. Such a set (painstakingly assembled, I assure you :) may be\nuseful as we move further in this direction.\n\n>> So any servers running Dulwich would be affected by this...though I'm\n>> not aware of any large-scale Dulwich installations other than Google\n>> Code.\n>\n> I'd rather not break small-scale installations, either. :)\n>\n> -Peff\n"},{"id":"196819","messageId":"20120810182555.GA29707@sigill.intra.peff.net","threadId":"31226","inReplyTo":"CAD0k6qSZYH5mvwR5PKhs1FFAPqMfRryXooxd_qhtt+eGXU7pUA@mail.gmail.com","subject":"Re: [PATCH 0/4] jk/version-string and google code","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-08-10T18:25:55Z","receivedAt":"2012-08-10T18:25:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 10, 2012 at 11:13:30AM -0700, Dave Borowitz wrote:\n\n> > Thanks for the data point. I knew you guys ran some custom code, so I\n> > wasn't sure how widespread this is. The fact that other dulwich-based\n> > servers would see the same issue makes me doubly sure that my fix is the\n> > right direction.\n> \n> You may also notice in that code a set of innocuous_capabilities,\n> which IIRC is the complete set of capabilities, at the time of\n> writing, that the C git client may send without the server advertising\n> them. Such a set (painstakingly assembled, I assure you :) may be\n> useful as we move further in this direction.\n\nOh, hmm. When initially writing my message I thought that might be the\ncase, but I checked to see that the features were sent only when the\nserver had first advertised them. However, I didn't notice that is true\nonly in _some_ of these lines from fetch-pack.c:\n\n if (multi_ack == 2) strbuf_addstr(&c, \" multi_ack_detailed\");\n if (multi_ack == 1) strbuf_addstr(&c, \" multi_ack\");\n if (no_done) strbuf_addstr(&c, \" no-done\");\n if (use_sideband == 2) strbuf_addstr(&c, \" side-band-64k\");\n if (use_sideband == 1) strbuf_addstr(&c, \" side-band\");\n if (args.use_thin_pack) strbuf_addstr(&c, \" thin-pack\");\n if (args.no_progress) strbuf_addstr(&c, \" no-progress\");\n if (args.include_tag) strbuf_addstr(&c, \" include-tag\");\n\nThe early ones are checking that the server claimed support, but all of\nthe args.* ones are influenced directly by the arguments, whether the\nserver supports it or not.\n\nI don't think there's any bug here. They are all of a class of features\nwhere the client can handle the case where the server simply ignores the\nrequest. However it is certainly food for thought if we are considering\ntightening git's server side (even if we fix these, we have to support\nthe innocuous capabilities list forever for older clients).\n\n-Peff\n"},{"id":"196821","messageId":"7vlihmk2ab.fsf@alter.siamese.dyndns.org","threadId":"31226","inReplyTo":"20120810174605.GA29113@sigill.intra.peff.net","subject":"Re: [PATCH 0/4] jk/version-string and google code","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-10T18:52:28Z","receivedAt":"2012-08-10T18:52:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Thanks for confirming the push side. I have been running with the patch\n> for months, but only recently happened to try cloning something from\n> code.google.com.\n\nNote that I didn't \"confirm\" the fix. I only confirmed the\nexistence of the breakage (not that I have any reason to doubt the\ncompetence your \"fix\" patches were made with).\n\nWhen evaluating a change in the interoperability area, it does not\nadd much more confidence to the correctness that the change has been\nin use for months with the same partner than that it has been used\nto talk to many different partners even for a short period of time,\nI guess.\n\n> It was pure coincidence that it happened at the same time you were\n> graduating the topic to next. But I'm glad I was able to catch it before\n> anybody started complaining. :)\n\nYeah, the problematic one is not in 'master'.\n"},{"id":"196822","messageId":"7vhasak1el.fsf@alter.siamese.dyndns.org","threadId":"31226","inReplyTo":"CAD0k6qSZYH5mvwR5PKhs1FFAPqMfRryXooxd_qhtt+eGXU7pUA@mail.gmail.com","subject":"Re: [PATCH 0/4] jk/version-string and google code","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-10T19:11:30Z","receivedAt":"2012-08-10T19:11:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dave Borowitz writes:\n\n> You may also notice in that code a set of innocuous_capabilities,\n> which IIRC is the complete set of capabilities, at the time of\n> writing, that the C git client may send without the server advertising\n> them. Such a set (painstakingly assembled, I assure you :) may be\n> useful as we move further in this direction.\n\nIn builtin/fetch-pack.c, we find this:\n\n\t\tif (!fetching) {\n\t\t\tstruct strbuf c = STRBUF_INIT;\n\t\t\tif (multi_ack == 2) strbuf_addstr(&c, \" multi_ack_detailed\");\n\t\t\tif (multi_ack == 1) strbuf_addstr(&c, \" multi_ack\");\n\t\t\tif (no_done) strbuf_addstr(&c, \" no-done\");\n\t\t\tif (use_sideband == 2) strbuf_addstr(&c, \" side-band-64k\");\n\t\t\tif (use_sideband == 1) strbuf_addstr(&c, \" side-band\");\n\t\t\tif (args.use_thin_pack) strbuf_addstr(&c, \" thin-pack\");\n\t\t\tif (args.no_progress) strbuf_addstr(&c, \" no-progress\");\n\t\t\tif (args.include_tag) strbuf_addstr(&c, \" include-tag\");\n\t\t\tif (prefer_ofs_delta) strbuf_addstr(&c, \" ofs-delta\");\n\t\t\tpacket_buf_write(&req_buf, \"want %s%s\\n\", remote_hex, c.buf);\n\t\t\tstrbuf_release(&c);\n\t\t} else\n\t\t\tpacket_buf_write(&req_buf, \"want %s\\n\", remote_hex);\n\nThe ones we choose to throw at the other end based on \"args.foo\" are\nnot protected by \"server_supports()\" at all, which is where the\nhardcoded list of \"innocuous capabilities\" comes from. I would say\nthis is a client bug. I wish Dulwich folks didn't choose to be\nsilent when they added that hardcoded list as a workaround.\n\nIf a client threw a request X at a server that does not support it,\nand relied on a server bug that does not reject such a request to\nallow it send a pack stream that does not conform to what X asked,\nand handled the pack stream assuming that the server did X, it would\nbe a triple bug on the client's end. Depending on the nature of X,\nthe end result may be broken. Dulwich is correct to raise an\nexception upon seeing agent=foo.\n\nOne could argue that from correctness standpoint, being asked to\nsend ofs-delta and using only ref-delta does not make a corrupt\npackfile (it just makes things less efficient), but we cannot\nguarantee that all protocol capabilities will be \"innocuous\" that\nway. Longer term direction should be to reduce the \"innocuous\" set.\n\nThanks.\n"},{"id":"196826","messageId":"7vd32yjzu8.fsf@alter.siamese.dyndns.org","threadId":"31226","inReplyTo":"20120810075743.GB8399@sigill.intra.peff.net","subject":"Re: [PATCH 2/4] do not send client agent unless server does first","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-10T19:45:19Z","receivedAt":"2012-08-10T19:45:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Instead, each client must record whether we saw an agent\n> string from the server, and respond with its agent only if\n> the server mentioned it first.\n\nJust a couple of minor comments.\n\n> diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\n> index fe56596..bc7a0f9 100644\n> --- a/builtin/fetch-pack.c\n> +++ b/builtin/fetch-pack.c\n> @@ -19,6 +19,7 @@ static int prefer_ofs_delta = 1;\n> static int no_done;\n> static int fetch_fsck_objects = -1;\n> static int transfer_fsck_objects = -1;\n> +static int agent_supported;\n> static struct fetch_pack_args args = {\n> \t/* .uploadpack = */ \"git-upload-pack\",\n> };\n\nThis is only set to false once per process invocation. We do not\ncurrently talk with more than one remote from the same process in\nthe \"fetch\" codepath, and we must maintain that. This fix will be\nbroken otherwise (\"recursive submodule fetch\" comes to mind; one\nmore reason it should do its submodule business in a separate\nprocess).\n\n> @@ -328,7 +329,8 @@ static int find_common(int fd[2], unsigned char *result_sha1,\n> \t\t\tif (args.no_progress) strbuf_addstr(&c, \" no-progress\");\n> \t\t\tif (args.include_tag) strbuf_addstr(&c, \" include-tag\");\n\nThis codepath still forgets to check if the other side advertised\n\"thin-pack\", \"no-progress\", and \"include-tag\", no?\n\n> \t\t\tif (prefer_ofs_delta) strbuf_addstr(&c, \" ofs-delta\");\n> -\t\t\tstrbuf_addf(&c, \" agent=%s\", git_user_agent_sanitized());\n> +\t\t\tif (agent_supported) strbuf_addf(&c, \" agent=%s\",\n> +\t\t\t\t\t\t\t git_user_agent_sanitized());\n> diff --git a/builtin/send-pack.c b/builtin/send-pack.c\n> index 5c69995..7d05064 100644\n> --- a/builtin/send-pack.c\n> +++ b/builtin/send-pack.c\n> @@ -252,6 +252,7 @@ int send_pack(struct send_pack_args *args,\n> \tint status_report = 0;\n> \tint use_sideband = 0;\n> \tint quiet_supported = 0;\n> +\tint agent_supported = 0;\n> \tunsigned cmds_sent = 0;\n> \tint ret;\n> \tstruct async demux;\n\nThis is initialied to 0 per communication, so having multiple\nremote.$there.pushURL configuration variables will work correctly.\n"},{"id":"196828","messageId":"7v7gt6jz3s.fsf@alter.siamese.dyndns.org","threadId":"31226","inReplyTo":"20120810075816.GC8399@sigill.intra.peff.net","subject":"Re: [PATCH 3/4] connect: learn to parse capabilities with values","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-10T20:01:11Z","receivedAt":"2012-08-10T20:01:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> +/*\n> + * Parse features of the form \"feature=value\". Returns NULL if the feature\n> + * does not exist, the empty string if it exists but does not have an \"=\", or\n> + * the content to the right of the \"=\" until the first space (or end of\n> + * string). The cannot contain literal spaces; double-quoting or similar\n> + * schemes would break compatibility, since older versions of git treat the\n> + * space as a hard-delimiter without any context.\n> + *\n> + * The return value (if non-NULL) is newly allocated on the heap and belongs to\n> + * the caller.\n> + */\n> +char *parse_feature_request_value(const char *feature_list, const char *feature)\n> +{\n> +\tconst char *start = parse_feature_request(feature_list, feature);\n> +\tconst char *end;\n> +\n> +\tif (!start || prefixcmp(start, feature))\n> +\t\treturn NULL;\n> +\tstart += strlen(feature);\n> +\n> +\tif (*start == '=')\n> +\t\tstart++;\n> +\tend = strchrnul(start, ' ');\n> +\n> +\treturn xmemdupz(start, end - start);\n> +}\n\nHaving to run strlen(feature) three times in this function (once in\nparse_feature_request() as part of strstr() and the edge check of\nthe found string, once as part of prefixcmp() here, and then an\nexplicit strlen() to skip it) makes me feel dirty.\n\nIt is not wrong per-se, but it is likely that the caller has a\nconstant string as the feature when it called this function, so\nperhaps just changing the function signature of server_supports,\ni.e.\n\n const char *server_supports(const char *feature)\n {\n\treturn parse_feature_request(server_capabilities, feature);\n }\n\nto return \"var=val \" would be more than sufficient.\n\nThen the existing callers can keep doing\n\n\tif (server_supports(\"thin-pack\"))\n if (!server_supports(\"quiet\"))\n\nand a new caller can do something like\n\n\tagent = server_supports(\"agent\");\n if (!agent || !agent[5])\n \t... no agent ...\n\telse {\n \tint span = strcspn(agent + 6, \" \\t\\n\");\n printf(\"I found agent=<%.*s>!\\n\", span, agent + 6);\n\t}\n\nwhich doesn't look too bad.\n"},{"id":"196834","messageId":"20120810210953.GA888@sigill.intra.peff.net","threadId":"31226","inReplyTo":"7vd32yjzu8.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/4] do not send client agent unless server does first","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-08-10T21:09:53Z","receivedAt":"2012-08-10T21:09:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 10, 2012 at 12:45:19PM -0700, Junio C Hamano wrote:\n\n> > diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\n> > index fe56596..bc7a0f9 100644\n> > --- a/builtin/fetch-pack.c\n> > +++ b/builtin/fetch-pack.c\n> > @@ -19,6 +19,7 @@ static int prefer_ofs_delta = 1;\n> > static int no_done;\n> > static int fetch_fsck_objects = -1;\n> > static int transfer_fsck_objects = -1;\n> > +static int agent_supported;\n> > static struct fetch_pack_args args = {\n> > \t/* .uploadpack = */ \"git-upload-pack\",\n> > };\n> \n> This is only set to false once per process invocation. We do not\n> currently talk with more than one remote from the same process in\n> the \"fetch\" codepath, and we must maintain that. This fix will be\n> broken otherwise (\"recursive submodule fetch\" comes to mind; one\n> more reason it should do its submodule business in a separate\n> process).\n\nRight; I followed the existing options in both fetch-pack and send-pack\n(use_sideband, no_done, and others here have the same issue), with the\nassumption that the current code was not broken (which may not be true,\nif it is simply masked by the fact that in practice everybody happens to\nsupport those older features).\n\nI don't know off-hand whether that is actually a trigger-able bug in the\ncurrent code or not. It's probably a good topic for a follow-on series.\n\n> > @@ -328,7 +329,8 @@ static int find_common(int fd[2], unsigned char *result_sha1,\n> > \t\t\tif (args.no_progress) strbuf_addstr(&c, \" no-progress\");\n> > \t\t\tif (args.include_tag) strbuf_addstr(&c, \" include-tag\");\n> \n> This codepath still forgets to check if the other side advertised\n> \"thin-pack\", \"no-progress\", and \"include-tag\", no?\n\nYes. I didn't realize it until Dave mentioned the \"innocuous\" list. I\nthink cleaning them up is reasonable, but probably a separate topic.\n\n> > diff --git a/builtin/send-pack.c b/builtin/send-pack.c\n> > index 5c69995..7d05064 100644\n> > --- a/builtin/send-pack.c\n> > +++ b/builtin/send-pack.c\n> > @@ -252,6 +252,7 @@ int send_pack(struct send_pack_args *args,\n> > \tint status_report = 0;\n> > \tint use_sideband = 0;\n> > \tint quiet_supported = 0;\n> > +\tint agent_supported = 0;\n> > \tunsigned cmds_sent = 0;\n> > \tint ret;\n> > \tstruct async demux;\n> \n> This is initialied to 0 per communication, so having multiple\n> remote.$there.pushURL configuration variables will work correctly.\n\nRight. Not through me having thought about it, though, but by following\nthe existing convention.\n\n-Peff\n"},{"id":"196836","messageId":"20120810211509.GB888@sigill.intra.peff.net","threadId":"31226","inReplyTo":"7v7gt6jz3s.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 3/4] connect: learn to parse capabilities with values","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-08-10T21:15:09Z","receivedAt":"2012-08-10T21:15:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 10, 2012 at 01:01:11PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > +/*\n> > + * Parse features of the form \"feature=value\". Returns NULL if the feature\n> > + * does not exist, the empty string if it exists but does not have an \"=\", or\n> > + * the content to the right of the \"=\" until the first space (or end of\n> > + * string). The cannot contain literal spaces; double-quoting or similar\n> > + * schemes would break compatibility, since older versions of git treat the\n> > + * space as a hard-delimiter without any context.\n> > + *\n> > + * The return value (if non-NULL) is newly allocated on the heap and belongs to\n> > + * the caller.\n> > + */\n> > +char *parse_feature_request_value(const char *feature_list, const char *feature)\n> > +{\n> > +\tconst char *start = parse_feature_request(feature_list, feature);\n> > +\tconst char *end;\n> > +\n> > +\tif (!start || prefixcmp(start, feature))\n> > +\t\treturn NULL;\n> > +\tstart += strlen(feature);\n> > +\n> > +\tif (*start == '=')\n> > +\t\tstart++;\n> > +\tend = strchrnul(start, ' ');\n> > +\n> > +\treturn xmemdupz(start, end - start);\n> > +}\n> \n> Having to run strlen(feature) three times in this function (once in\n> parse_feature_request() as part of strstr() and the edge check of\n> the found string, once as part of prefixcmp() here, and then an\n> explicit strlen() to skip it) makes me feel dirty.\n\nI thought about that, but it seems like a quite premature optimization.\nIt is three extra strlens per network conversation. _If_ you have turned\non double-verbosity in fetch-pack. I considered reusing the existing\nparse_feature_request function more valuable from a maintenance\nstandpoint.\n\nI would think the extra memory allocation would dwarf it, anyway.\n\n> It is not wrong per-se, but it is likely that the caller has a\n> constant string as the feature when it called this function, so\n> perhaps just changing the function signature of server_supports,\n> i.e.\n> \n> const char *server_supports(const char *feature)\n> {\n> \treturn parse_feature_request(server_capabilities, feature);\n> }\n> \n> to return \"var=val \" would be more than sufficient.\n\nThat was in fact my first iteration, but...\n\n> Then the existing callers can keep doing\n> \n> \tif (server_supports(\"thin-pack\"))\n> if (!server_supports(\"quiet\"))\n> \n> and a new caller can do something like\n> \n> \tagent = server_supports(\"agent\");\n> if (!agent || !agent[5])\n> \t... no agent ...\n> \telse {\n> \tint span = strcspn(agent + 6, \" \\t\\n\");\n> printf(\"I found agent=<%.*s>!\\n\", span, agent + 6);\n> \t}\n> \n> which doesn't look too bad.\n\nI didn't want to force callers to have to deal with ad-hoc parsing.\n\nAnyway, do you think this is even worth doing at this point? I'm\nlukewarm on the final two patches due to the existence of\nGIT_TRACE_PACKET, which is much more likely to be useful.\n\n-Peff\n"},{"id":"196837","messageId":"7vr4reigm8.fsf@alter.siamese.dyndns.org","threadId":"31226","inReplyTo":"20120810182555.GA29707@sigill.intra.peff.net","subject":"Re: [PATCH 0/4] jk/version-string and google code","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-10T21:25:51Z","receivedAt":"2012-08-10T21:25:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Fri, Aug 10, 2012 at 11:13:30AM -0700, Dave Borowitz wrote:\n>\n>> > Thanks for the data point. I knew you guys ran some custom code, so I\n>> > wasn't sure how widespread this is. The fact that other dulwich-based\n>> > servers would see the same issue makes me doubly sure that my fix is the\n>> > right direction.\n>> \n>> You may also notice in that code a set of innocuous_capabilities,\n>> which IIRC is the complete set of capabilities, at the time of\n>> writing, that the C git client may send without the server advertising\n>> them. Such a set (painstakingly assembled, I assure you :) may be\n>> useful as we move further in this direction.\n>\n> Oh, hmm. When initially writing my message I thought that might be the\n> case, but I checked to see that the features were sent only when the\n> server had first advertised them. However, I didn't notice that is true\n> only in _some_ of these lines from fetch-pack.c:\n>\n> if (multi_ack == 2) strbuf_addstr(&c, \" multi_ack_detailed\");\n> if (multi_ack == 1) strbuf_addstr(&c, \" multi_ack\");\n> if (no_done) strbuf_addstr(&c, \" no-done\");\n> if (use_sideband == 2) strbuf_addstr(&c, \" side-band-64k\");\n> if (use_sideband == 1) strbuf_addstr(&c, \" side-band\");\n> if (args.use_thin_pack) strbuf_addstr(&c, \" thin-pack\");\n> if (args.no_progress) strbuf_addstr(&c, \" no-progress\");\n> if (args.include_tag) strbuf_addstr(&c, \" include-tag\");\n>\n> The early ones are checking that the server claimed support, but all of\n> the args.* ones are influenced directly by the arguments, whether the\n> server supports it or not.\n\n> I don't think there's any bug here. They are all of a class of features\n> where the client can handle the case where the server simply ignores the\n> request. However it is certainly food for thought if we are considering\n> tightening git's server side (even if we fix these, we have to support\n> the innocuous capabilities list forever for older clients).\n\nI doubt the \"innocuous\" approach is really viable, unless we have an\nautoritative documentation that tells which ones are and which ones\nare not innocuous, and everybody follows it, so that everybody's\nserver and client understands the same set of capabilities as such.\n\nWhich is not likely to happen. So in that sense, the above have\nthree bugs. A new person that starts writing his server without\nknowing the workaround Dulwich used that has been hidden from the\nGit community until today will have to rediscover the \"innocuous\"\nworkaround on his server, unless such buggy clients die out.\n\nI'd rather make sure that 10 years on, the maintainer does not have\nto worry about interoperating with a new server written by some\nthird-party.\n\nSomething like this, perhaps.\n\n builtin/fetch-pack.c | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex bc7a0f9..fdec7f6 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -818,6 +818,12 @@ static struct ref *do_fetch_pack(int fd[2],\n \t\t\tfprintf(stderr, \"Server supports side-band\\n\");\n \t\tuse_sideband = 1;\n \t}\n+\tif (!server_supports(\"thin-pack\"))\n+\t\targs.use_thin_pack = 0;\n+\tif (!server_supports(\"no-progress\"))\n+\t\targs.no_progress = 0;\n+\tif (!server_supports(\"include-tag\"))\n+\t\targs.include_tag = 0;\n \tif (server_supports(\"ofs-delta\")) {\n \t\tif (args.verbose)\n \t\t\tfprintf(stderr, \"Server supports ofs-delta\\n\");\n"},{"id":"196840","messageId":"20120810213508.GC888@sigill.intra.peff.net","threadId":"31226","inReplyTo":"7vr4reigm8.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 0/4] jk/version-string and google code","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-08-10T21:35:08Z","receivedAt":"2012-08-10T21:35:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 10, 2012 at 02:25:51PM -0700, Junio C Hamano wrote:\n\n> > I don't think there's any bug here. They are all of a class of features\n> > where the client can handle the case where the server simply ignores the\n> > request. However it is certainly food for thought if we are considering\n> > tightening git's server side (even if we fix these, we have to support\n> > the innocuous capabilities list forever for older clients).\n> \n> I doubt the \"innocuous\" approach is really viable, unless we have an\n> autoritative documentation that tells which ones are and which ones\n> are not innocuous, and everybody follows it, so that everybody's\n> server and client understands the same set of capabilities as such.\n\nI think the point is that the ordering is something like:\n\n 1. New client features get implemented wrongly. Nobody notices because\n the server side is lax.\n\n 2. Somebody writes a new server (dulwich), or tightens the existing\n code (what we are thinking of). They create the innocuous list\n because they must deal with older clients from (1).\n\n 3. Somebody tries to implement a new client feature wrongly. They\n notice because strict servers actually exist, and are told their\n client is buggy and wrong. The innocuous list never grows.\n\nSo we are at step (2), and are just realizing the client problem. Even\nif we fix it, we still need the current innocuous list to handle\nexisting clients.\n\nAlthough I would think you do not have to worry about the innocuous list\nif you always advertise those features. Which I'm surprised dulwich does\nnot do (IOW, why do they even need the innocuous list?).\n\n> Which is not likely to happen. So in that sense, the above have\n> three bugs. A new person that starts writing his server without\n> knowing the workaround Dulwich used that has been hidden from the\n> Git community until today will have to rediscover the \"innocuous\"\n> workaround on his server, unless such buggy clients die out.\n> \n> I'd rather make sure that 10 years on, the maintainer does not have\n> to worry about interoperating with a new server written by some\n> third-party.\n\nOh, definitely. I wasn't arguing that we shouldn't fix the clients. Just\nthat we need to make sure that the current list continues working if we\ndecide to tighten the server side.\n\n> Something like this, perhaps.\n> \n> builtin/fetch-pack.c | 6 ++++++\n> 1 file changed, 6 insertions(+)\n> \n> diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\n> index bc7a0f9..fdec7f6 100644\n> --- a/builtin/fetch-pack.c\n> +++ b/builtin/fetch-pack.c\n> @@ -818,6 +818,12 @@ static struct ref *do_fetch_pack(int fd[2],\n> \t\t\tfprintf(stderr, \"Server supports side-band\\n\");\n> \t\tuse_sideband = 1;\n> \t}\n> +\tif (!server_supports(\"thin-pack\"))\n> +\t\targs.use_thin_pack = 0;\n> +\tif (!server_supports(\"no-progress\"))\n> +\t\targs.no_progress = 0;\n> +\tif (!server_supports(\"include-tag\"))\n> +\t\targs.include_tag = 0;\n> \tif (server_supports(\"ofs-delta\")) {\n> \t\tif (args.verbose)\n> \t\t\tfprintf(stderr, \"Server supports ofs-delta\\n\");\n\nYes, I think that is all that is necessary to fix the immediate issue.\nThe protocol-capabilities document talks about what to do when\ninclude-tag is not available (\"SHOULD issue a subsequent fetch to\nacquire the tags that include-tag would have otherwise given the\nclient\"), but I am not sure how well we handle that (in theory we should\nbe handling it already, but I didn't look).\n\n-Peff\n"},{"id":"196842","messageId":"7vboiiifu4.fsf@alter.siamese.dyndns.org","threadId":"31226","inReplyTo":"20120810213508.GC888@sigill.intra.peff.net","subject":"Re: [PATCH 0/4] jk/version-string and google code","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-10T21:42:43Z","receivedAt":"2012-08-10T21:42:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Yes, I think that is all that is necessary to fix the immediate issue.\n> The protocol-capabilities document talks about what to do when\n> include-tag is not available (\"SHOULD issue a subsequent fetch to\n> acquire the tags that include-tag would have otherwise given the\n> client\"), but I am not sure how well we handle that (in theory we should\n> be handling it already, but I didn't look).\n\nYeah, it is more like \"A separate fetch only to follow tags was the\nonly way we handled it before we added include-tag, so we know it\nused to work, and also we know the four combinations of new/old *\nfetch/upload all used to work\".\n\nWe may have broken some combinations over time, though.\n"},{"id":"196844","messageId":"20120810215020.GA1841@sigill.intra.peff.net","threadId":"31226","inReplyTo":"7vlihmk2ab.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 0/4] jk/version-string and google code","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-08-10T21:50:20Z","receivedAt":"2012-08-10T21:50:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 10, 2012 at 11:52:28AM -0700, Junio C Hamano wrote:\n\n> When evaluating a change in the interoperability area, it does not\n> add much more confidence to the correctness that the change has been\n> in use for months with the same partner than that it has been used\n> to talk to many different partners even for a short period of time,\n> I guess.\n\nTraditionally our interoperability testing has been to cook things in\n\"next\" and \"master\" and see if anybody complains. It would be nice to\nhave an interoperability test suite that could hit some common hosting\nsites, as well as older versions of git-core itself. I suspect\nautomating that would be a big pain, though.\n\n-Peff\n"},{"id":"196845","messageId":"7v393uif9g.fsf@alter.siamese.dyndns.org","threadId":"31226","inReplyTo":"20120810211509.GB888@sigill.intra.peff.net","subject":"Re: [PATCH 3/4] connect: learn to parse capabilities with values","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-10T21:55:07Z","receivedAt":"2012-08-10T21:55:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I would think the extra memory allocation would dwarf it, anyway.\n>> ...\n>> and a new caller can do something like\n>> \n>> \tagent = server_supports(\"agent\");\n>> if (!agent || !agent[5])\n>> \t... no agent ...\n>> \telse {\n>> \tint span = strcspn(agent + 6, \" \\t\\n\");\n>> printf(\"I found agent=<%.*s>!\\n\", span, agent + 6);\n>> \t}\n>> \n>> which doesn't look too bad.\n\nI forgot to mention it, but the above was done also to make it\n\"possible but not mandatory\" to pay extra allocation penalty. The\ncaller can choose to parse the string into an int, for example,\nwithout extra allocation. Only the ones that want a string value\nand keep a copy around do have to do xmemdupz().\n\n> Anyway, do you think this is even worth doing at this point? I'm\n> lukewarm on the final two patches due to the existence of\n> GIT_TRACE_PACKET, which is much more likely to be useful.\n\nIn the longer term, I think giving callers access to the parameter\nvalue given to a capability is necessary. If we had this facility\nin the old days, we wouldn't have done side-band-64k but spelled it\nas side-band=64k.\n\nFor the agent=, certainly we don't need it.\n"},{"id":"196847","messageId":"CAJo=hJsaQyBEjH8uWHqnR9Sk9HwFqCwe81HRN3HxUi2j76YNAg@mail.gmail.com","threadId":"31226","inReplyTo":"20120810215020.GA1841@sigill.intra.peff.net","subject":"Re: [PATCH 0/4] jk/version-string and google code","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2012-08-10T22:29:24Z","receivedAt":"2012-08-10T22:29:24Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"On Fri, Aug 10, 2012 at 2:50 PM, Jeff King wrote:\n> On Fri, Aug 10, 2012 at 11:52:28AM -0700, Junio C Hamano wrote:\n>\n>> When evaluating a change in the interoperability area, it does not\n>> add much more confidence to the correctness that the change has been\n>> in use for months with the same partner than that it has been used\n>> to talk to many different partners even for a short period of time,\n>> I guess.\n>\n> Traditionally our interoperability testing has been to cook things in\n> \"next\" and \"master\" and see if anybody complains. It would be nice to\n> have an interoperability test suite that could hit some common hosting\n> sites, as well as older versions of git-core itself. I suspect\n> automating that would be a big pain, though.\n\nI don't know that you need to hit the hosting sites themselves, just\nthe implementations they are using. Dulwich and JGit are both open\nsource. It should be possible to build a smallish compatibility test\nsuite that grabs everyone into one tree, compiles a small test server\nfrom each, and runs the matrix locally. Then its up to the hosting\nsites to worry about making sure their implementations are included in\nthis suite, and up-to-date. :-)\n"},{"id":"196848","messageId":"7vy5lmgyra.fsf@alter.siamese.dyndns.org","threadId":"31226","inReplyTo":"CAJo=hJsaQyBEjH8uWHqnR9Sk9HwFqCwe81HRN3HxUi2j76YNAg@mail.gmail.com","subject":"Re: [PATCH 0/4] jk/version-string and google code","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-10T22:36:57Z","receivedAt":"2012-08-10T22:36:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shawn Pearce writes:\n\n> On Fri, Aug 10, 2012 at 2:50 PM, Jeff King wrote:\n>> On Fri, Aug 10, 2012 at 11:52:28AM -0700, Junio C Hamano wrote:\n>>\n>>> When evaluating a change in the interoperability area, it does not\n>>> add much more confidence to the correctness that the change has been\n>>> in use for months with the same partner than that it has been used\n>>> to talk to many different partners even for a short period of time,\n>>> I guess.\n>>\n>> Traditionally our interoperability testing has been to cook things in\n>> \"next\" and \"master\" and see if anybody complains. It would be nice to\n>> have an interoperability test suite that could hit some common hosting\n>> sites, as well as older versions of git-core itself. I suspect\n>> automating that would be a big pain, though.\n>\n> I don't know that you need to hit the hosting sites themselves, just\n> the implementations they are using. Dulwich and JGit are both open\n> source. It should be possible to build a smallish compatibility test\n> suite that grabs everyone into one tree, compiles a small test server\n> from each, and runs the matrix locally. Then its up to the hosting\n> sites to worry about making sure their implementations are included in\n> this suite, and up-to-date. :-)\n\nYeah, and do that for all the major versions of these\nimplementations.\n"},{"id":"196926","messageId":"7vboieehrf.fsf@alter.siamese.dyndns.org","threadId":"31226","inReplyTo":"7v393uif9g.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 3/4] connect: learn to parse capabilities with values","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-13T19:03:48Z","receivedAt":"2012-08-13T19:03:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> I forgot to mention it, but the above was done also to make it\n> \"possible but not mandatory\" to pay extra allocation penalty. The\n> caller can choose to parse the string into an int, for example,\n> without extra allocation. Only the ones that want a string value\n> and keep a copy around do have to do xmemdupz().\n>\n>> Anyway, do you think this is even worth doing at this point? I'm\n>> lukewarm on the final two patches due to the existence of\n>> GIT_TRACE_PACKET, which is much more likely to be useful.\n>\n> In the longer term, I think giving callers access to the parameter\n> value given to a capability is necessary. If we had this facility\n> in the old days, we wouldn't have done side-band-64k but spelled it\n> as side-band=64k.\n>\n> For the agent=, certainly we don't need it.\n\nHere are the first of two patches to replace your 3 and 4 without\nextra allocations, primarily for further discussion.\n\n-- >8 --\nSubject: [PATCH 3/4] connect: expose the parameter to a capability\n\nWe already take care to parse a capability like \"foo=bar\" properly,\nbut the code does not provide a good way of actually finding out\nwhat is on the right-hand side of the \"=\".\n\nBased on the patch by Jeff King \n\nSigned-off-by: Junio C Hamano \n---\n cache.h | 1 +\n connect.c | 27 ++++++++++++++++++++-------\n 2 files changed, 21 insertions(+), 7 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 06413e1..d239cee 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1030,6 +1030,7 @@ struct extra_have_objects {\n };\n extern struct ref **get_remote_heads(int in, struct ref **list, unsigned int flags, struct extra_have_objects *);\n extern int server_supports(const char *feature);\n+extern const char *server_feature(const char *feature, int *len_ret);\n extern const char *parse_feature_request(const char *features, const char *feature);\n \n extern struct packed_git *parse_pack_index(unsigned char *sha1, const char *idx_path);\ndiff --git a/connect.c b/connect.c\nindex 912cdde..42640bc 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -99,12 +99,7 @@ struct ref **get_remote_heads(int in, struct ref **list,\n \treturn list;\n }\n \n-int server_supports(const char *feature)\n-{\n-\treturn !!parse_feature_request(server_capabilities, feature);\n-}\n-\n-const char *parse_feature_request(const char *feature_list, const char *feature)\n+static const char *parse_feature_request_1(const char *feature_list, const char *feature, int *lenp)\n {\n \tint len;\n \n@@ -117,13 +112,31 @@ const char *parse_feature_request(const char *feature_list, const char *feature)\n \t\tif (!found)\n \t\t\treturn NULL;\n \t\tif ((feature_list == found || isspace(found[-1])) &&\n-\t\t (!found[len] || isspace(found[len]) || found[len] == '='))\n+\t\t (!found[len] || isspace(found[len]) || found[len] == '=')) {\n+\t\t\tif (lenp)\n+\t\t\t\t*lenp = strcspn(found, \" \\t\\n\");\n \t\t\treturn found;\n+\t\t}\n \t\tfeature_list = found + 1;\n \t}\n \treturn NULL;\n }\n \n+const char *parse_feature_request(const char *feature_list, const char *feature)\n+{\n+\treturn parse_feature_request_1(feature_list, feature, NULL);\n+}\n+\n+const char *server_feature(const char *feature, int *len)\n+{\n+\treturn parse_feature_request_1(server_capabilities, feature, len);\n+}\n+\n+int server_supports(const char *feature)\n+{\n+\treturn !!server_feature(feature, NULL);\n+}\n+\n enum protocol {\n \tPROTO_LOCAL = 1,\n \tPROTO_SSH,\n-- \n1.7.12.rc2.85.g1de7134\n"},{"id":"196927","messageId":"7v7gt2ehl4.fsf_-_@alter.siamese.dyndns.org","threadId":"31226","inReplyTo":"7vboieehrf.fsf@alter.siamese.dyndns.org","subject":"[PATCH 4/4] fetch-pack: mention server version with verbose output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-13T19:07:35Z","receivedAt":"2012-08-13T19:07:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Fetch-pack's verbose mode is more of a debugging mode (and in fact\ntakes two \"-v\" arguments to trigger via the porcelain layer). Let's\nmention the server version as another possible item of interest.\n\nSigned-off-by: Jeff King \nSigned-off-by: Junio C Hamano \n---\n\n * And this is your 4 adjusted for the previous one, releaving the\n caller from having to figure out where the capability string\n ends.\n\n builtin/fetch-pack.c | 11 ++++++++++-\n 1 file changed, 10 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex fdec7f6..1633aa3 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -787,6 +787,8 @@ static struct ref *do_fetch_pack(int fd[2],\n {\n \tstruct ref *ref = copy_ref_list(orig_ref);\n \tunsigned char sha1[20];\n+\tconst char *agent_feature = NULL;\n+\tint agent_len;\n \n \tsort_ref_list(&ref, ref_compare_name);\n \n@@ -829,8 +831,15 @@ static struct ref *do_fetch_pack(int fd[2],\n \t\t\tfprintf(stderr, \"Server supports ofs-delta\\n\");\n \t} else\n \t\tprefer_ofs_delta = 0;\n-\tif (server_supports(\"agent\"))\n+\n+\tif ((agent_feature = server_feature(\"agent\", &agent_len)) != NULL &&\n+\t 5 < agent_len && agent_feature[5] == '=') {\n \t\tagent_supported = 1;\n+\t\tif (args.verbose) {\n+\t\t\tfprintf(stderr, \"Server version is %.*s\\n\",\n+\t\t\t\tagent_len - 6, agent_feature + 6);\n+\t\t}\n+\t}\n \n \tif (everything_local(&ref, nr_match, match)) {\n \t\tpacket_flush(fd[1]);\n-- \n1.7.12.rc2.85.g1de7134\n"},{"id":"196928","messageId":"7vy5lid1d6.fsf@alter.siamese.dyndns.org","threadId":"31226","inReplyTo":"7v7gt2ehl4.fsf_-_@alter.siamese.dyndns.org","subject":"Re: [PATCH 4/4] fetch-pack: mention server version with verbose output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-13T19:43:17Z","receivedAt":"2012-08-13T19:43:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Fetch-pack's verbose mode is more of a debugging mode (and in fact\n> takes two \"-v\" arguments to trigger via the porcelain layer). Let's\n> mention the server version as another possible item of interest.\n>\n> Signed-off-by: Jeff King \n> Signed-off-by: Junio C Hamano \n> ---\n>\n> * And this is your 4 adjusted for the previous one, releaving the\n> caller from having to figure out where the capability string\n> ends.\n\nOops; this was a cut and paste error. There are these four\n(counting the blank after \"Subject:\") lines before the description.\n\n From: Jeff King \n Date: Fri, 10 Aug 2012 03:59:29 -0400\n Subject: [PATCH] fetch-pack: mention server version with verbose output\n"},{"id":"196936","messageId":"20120813205423.GA31630@sigill.intra.peff.net","threadId":"31226","inReplyTo":"7v7gt2ehl4.fsf_-_@alter.siamese.dyndns.org","subject":"Re: [PATCH 4/4] fetch-pack: mention server version with verbose output","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-08-13T20:54:24Z","receivedAt":"2012-08-13T20:54:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 13, 2012 at 12:07:35PM -0700, Junio C Hamano wrote:\n\n> * And this is your 4 adjusted for the previous one, releaving the\n> caller from having to figure out where the capability string\n> ends.\n> [...]\n> @@ -829,8 +831,15 @@ static struct ref *do_fetch_pack(int fd[2],\n> \t\t\tfprintf(stderr, \"Server supports ofs-delta\\n\");\n> \t} else\n> \t\tprefer_ofs_delta = 0;\n> -\tif (server_supports(\"agent\"))\n> +\n> +\tif ((agent_feature = server_feature(\"agent\", &agent_len)) != NULL &&\n> +\t 5 < agent_len && agent_feature[5] == '=') {\n> \t\tagent_supported = 1;\n> +\t\tif (args.verbose) {\n> +\t\t\tfprintf(stderr, \"Server version is %.*s\\n\",\n> +\t\t\t\tagent_len - 6, agent_feature + 6);\n> +\t\t}\n> +\t}\n\nYeah, this is exactly the kind of ugliness I was trying to avoid with my\nallocating wrapper. Still, there is only one call site, so I do not care\noverly much (and I as I've already said, I'm lukewarm on the final two\npatches, anyway).\n\nThere is one difference in your code and mine. With mine, the server can\nsay simply \"agent\" to tell the client that it understands the extension\nbut does not wish to disclose its version. That might be considered\nunfriendly (why would the client show theirs if the server is not\nwilling to do the same?), but it may be a practical decision (e.g.,\nsecurity policies may say that servers are higher-risk targets[1]).\nOf course, a server can also say \"agent=git/none-of-your-business\"; this\nis just a syntactic question.\n\n-Peff\n\n[1] I think you and I both agreed earlier that the \"sharing versions is\n a security risk\" line of argument is not that compelling, but that\n does not mean it is not made all the time.\n\n-Peff\n"},{"id":"196938","messageId":"7va9xycxh1.fsf@alter.siamese.dyndns.org","threadId":"31226","inReplyTo":"20120813205423.GA31630@sigill.intra.peff.net","subject":"Re: [PATCH 4/4] fetch-pack: mention server version with verbose output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-13T21:07:22Z","receivedAt":"2012-08-13T21:07:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Of course, a server can also say \"agent=git/none-of-your-business\"; this\n> is just a syntactic question.\n\nYou do not even have to advertise it in the first place, no?\n"},{"id":"196939","messageId":"20120813210757.GA32599@sigill.intra.peff.net","threadId":"31226","inReplyTo":"7va9xycxh1.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 4/4] fetch-pack: mention server version with verbose output","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-08-13T21:07:57Z","receivedAt":"2012-08-13T21:07:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 13, 2012 at 02:07:22PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > Of course, a server can also say \"agent=git/none-of-your-business\"; this\n> > is just a syntactic question.\n> \n> You do not even have to advertise it in the first place, no?\n\nIf you want the client to respond with theirs, you do.\n\n-Peff\n"},{"id":"196940","messageId":"7v628mcxdf.fsf@alter.siamese.dyndns.org","threadId":"31226","inReplyTo":"20120813205423.GA31630@sigill.intra.peff.net","subject":"Re: [PATCH 4/4] fetch-pack: mention server version with verbose output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-13T21:09:32Z","receivedAt":"2012-08-13T21:09:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Mon, Aug 13, 2012 at 12:07:35PM -0700, Junio C Hamano wrote:\n>\n>> * And this is your 4 adjusted for the previous one, releaving the\n>> caller from having to figure out where the capability string\n>> ends.\n>> [...]\n>> @@ -829,8 +831,15 @@ static struct ref *do_fetch_pack(int fd[2],\n>> \t\t\tfprintf(stderr, \"Server supports ofs-delta\\n\");\n>> \t} else\n>> \t\tprefer_ofs_delta = 0;\n>> -\tif (server_supports(\"agent\"))\n>> +\n>> +\tif ((agent_feature = server_feature(\"agent\", &agent_len)) != NULL &&\n>> +\t 5 < agent_len && agent_feature[5] == '=') {\n>> \t\tagent_supported = 1;\n>> +\t\tif (args.verbose) {\n>> +\t\t\tfprintf(stderr, \"Server version is %.*s\\n\",\n>> +\t\t\t\tagent_len - 6, agent_feature + 6);\n>> +\t\t}\n>> +\t}\n>\n> Yeah, this is exactly the kind of ugliness I was trying to avoid with my\n> allocating wrapper. Still, there is only one call site, so I do not care\n> overly much (and I as I've already said, I'm lukewarm on the final two\n> patches, anyway).\n\nActually, the above is vastly superiour compared to the allocating\nkind. Be honest and think about it. If the caller wants to\nallocate, it could, and it does not even have to count. If the\ncaller does not want to allocate, it does not have to pay the price.\n"},{"id":"196941","messageId":"20120813211109.GA32688@sigill.intra.peff.net","threadId":"31226","inReplyTo":"7v628mcxdf.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 4/4] fetch-pack: mention server version with verbose output","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-08-13T21:11:10Z","receivedAt":"2012-08-13T21:11:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 13, 2012 at 02:09:32PM -0700, Junio C Hamano wrote:\n\n> >> +\tif ((agent_feature = server_feature(\"agent\", &agent_len)) != NULL &&\n> >> +\t 5 < agent_len && agent_feature[5] == '=') {\n> >> \t\tagent_supported = 1;\n> >> +\t\tif (args.verbose) {\n> >> +\t\t\tfprintf(stderr, \"Server version is %.*s\\n\",\n> >> +\t\t\t\tagent_len - 6, agent_feature + 6);\n> >> +\t\t}\n> >> +\t}\n> >\n> > Yeah, this is exactly the kind of ugliness I was trying to avoid with my\n> > allocating wrapper. Still, there is only one call site, so I do not care\n> > overly much (and I as I've already said, I'm lukewarm on the final two\n> > patches, anyway).\n> \n> Actually, the above is vastly superiour compared to the allocating\n> kind. Be honest and think about it. If the caller wants to\n> allocate, it could, and it does not even have to count. If the\n> caller does not want to allocate, it does not have to pay the price.\n\nMy point is that the run-time allocation price is quite small, but the\nreadability cost of that ugly conditional with the magic \"5\" is\nnon-trivial. But they are apples and oranges, so it is hard to compare\ntheir amounts directly.\n\n-Peff\n"},{"id":"196952","messageId":"20120814015927.GA7891@sigill.intra.peff.net","threadId":"31226","inReplyTo":"20120813211109.GA32688@sigill.intra.peff.net","subject":"Re: [PATCH 4/4] fetch-pack: mention server version with verbose output","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-08-14T01:59:27Z","receivedAt":"2012-08-14T01:59:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 13, 2012 at 05:11:10PM -0400, Jeff King wrote:\n\n> On Mon, Aug 13, 2012 at 02:09:32PM -0700, Junio C Hamano wrote:\n> \n> > >> +\tif ((agent_feature = server_feature(\"agent\", &agent_len)) != NULL &&\n> > >> +\t 5 < agent_len && agent_feature[5] == '=') {\n> > >> \t\tagent_supported = 1;\n> > >> +\t\tif (args.verbose) {\n> > >> +\t\t\tfprintf(stderr, \"Server version is %.*s\\n\",\n> > >> +\t\t\t\tagent_len - 6, agent_feature + 6);\n> > >> +\t\t}\n> > >> +\t}\n> > >\n> > > Yeah, this is exactly the kind of ugliness I was trying to avoid with my\n> > > allocating wrapper. Still, there is only one call site, so I do not care\n> > > overly much (and I as I've already said, I'm lukewarm on the final two\n> > > patches, anyway).\n> > \n> > Actually, the above is vastly superiour compared to the allocating\n> > kind. Be honest and think about it. If the caller wants to\n> > allocate, it could, and it does not even have to count. If the\n> > caller does not want to allocate, it does not have to pay the price.\n> \n> My point is that the run-time allocation price is quite small, but the\n> readability cost of that ugly conditional with the magic \"5\" is\n> non-trivial. But they are apples and oranges, so it is hard to compare\n> their amounts directly.\n\nSo if we want to avoid the allocation, then this is how I would do it:\nby returning the feature's _value_ and not the whole key. Since we know\nthat the beginning part must obviously match what we fed it anyway, it\nis not that interesting.\n\n-- >8 --\nSubject: [PATCH] parse_feature_request: make it easier to see feature values\n\nWe already take care to parse key/value capabilities like\n\"foo=bar\", but the code does not provide a good way of\nactually finding out what is on the right-hand side of the\n\"=\".\n\nA server using \"parse_feature_request\" could accomplish this\nwith some extra parsing. You must skip past the \"key\"\nportion manually, check for \"=\" versus NUL or space, and\nthen find the length by searching for the next space (or\nNUL). But clients can't even do that, since the\n\"server_supports\" interface does not even return the\npointer.\n\nInstead, let's have our parser share more information by\nproviding a pointer to the value and its length. The\n\"parse_feature_value\" function returns a pointer to the\nfeature's value portion, along with the length of the value.\nIf the feature is missing, NULL is returned. If it does not\nhave an \"=\", then a zero-length value is returned.\n\nSimilarly, \"server_feature_value\" behaves in the same way,\nbut always checks the static server_feature_list variable.\n\nWe can then implement \"server_supports\" in terms of\n\"server_feature_value\". We cannot implement the original\n\"parse_feature_request\" in terms of our new function,\nbecause it returned a pointer to the beginning of the\nfeature. However, no callers actually cared about the value\nof the returned pointer, so we can simplify it to a boolean\njust as we do for \"server_supports\".\n\nSigned-off-by: Jeff King \n---\n cache.h | 4 +++-\n connect.c | 45 ++++++++++++++++++++++++++++++++++++---------\n 2 files changed, 39 insertions(+), 10 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 67f28b4..95daa69 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1038,7 +1038,9 @@ struct extra_have_objects {\n };\n extern struct ref **get_remote_heads(int in, struct ref **list, unsigned int flags, struct extra_have_objects *);\n extern int server_supports(const char *feature);\n-extern const char *parse_feature_request(const char *features, const char *feature);\n+extern int parse_feature_request(const char *features, const char *feature);\n+extern const char *server_feature_value(const char *feature, int *len_ret);\n+extern const char *parse_feature_value(const char *feature_list, const char *feature, int *len_ret);\n \n extern struct packed_git *parse_pack_index(unsigned char *sha1, const char *idx_path);\n \ndiff --git a/connect.c b/connect.c\nindex 55a85ad..49e56ba 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -115,12 +115,7 @@ struct ref **get_remote_heads(int in, struct ref **list,\n \treturn list;\n }\n \n-int server_supports(const char *feature)\n-{\n-\treturn !!parse_feature_request(server_capabilities, feature);\n-}\n-\n-const char *parse_feature_request(const char *feature_list, const char *feature)\n+const char *parse_feature_value(const char *feature_list, const char *feature, int *lenp)\n {\n \tint len;\n \n@@ -132,14 +127,46 @@ const char *parse_feature_request(const char *feature_list, const char *feature)\n \t\tconst char *found = strstr(feature_list, feature);\n \t\tif (!found)\n \t\t\treturn NULL;\n-\t\tif ((feature_list == found || isspace(found[-1])) &&\n-\t\t (!found[len] || isspace(found[len]) || found[len] == '='))\n-\t\t\treturn found;\n+\t\tif (feature_list == found || isspace(found[-1])) {\n+\t\t\tconst char *value = found + len;\n+\t\t\t/* feature with no value (e.g., \"thin-pack\") */\n+\t\t\tif (!*value || isspace(*value)) {\n+\t\t\t\tif (lenp)\n+\t\t\t\t\t*lenp = 0;\n+\t\t\t\treturn value;\n+\t\t\t}\n+\t\t\t/* feature with a value (e.g., \"agent=git/1.2.3\") */\n+\t\t\telse if (*value == '=') {\n+\t\t\t\tvalue++;\n+\t\t\t\tif (lenp)\n+\t\t\t\t\t*lenp = strcspn(value, \" \\t\\n\");\n+\t\t\t\treturn value;\n+\t\t\t}\n+\t\t\t/*\n+\t\t\t * otherwise we matched a substring of another feature;\n+\t\t\t * keep looking\n+\t\t\t */\n+\t\t}\n \t\tfeature_list = found + 1;\n \t}\n \treturn NULL;\n }\n \n+int parse_feature_request(const char *feature_list, const char *feature)\n+{\n+\treturn !!parse_feature_value(feature_list, feature, NULL);\n+}\n+\n+const char *server_feature_value(const char *feature, int *len)\n+{\n+\treturn parse_feature_value(server_capabilities, feature, len);\n+}\n+\n+int server_supports(const char *feature)\n+{\n+\treturn !!server_feature_value(feature, NULL);\n+}\n+\n enum protocol {\n \tPROTO_LOCAL = 1,\n \tPROTO_SSH,\n-- \n1.7.12.rc2.11.gf0a1e27\n"},{"id":"196953","messageId":"20120814020210.GB7891@sigill.intra.peff.net","threadId":"31226","inReplyTo":"20120814015927.GA7891@sigill.intra.peff.net","subject":"Re: [PATCH 4/4] fetch-pack: mention server version with verbose output","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-08-14T02:02:10Z","receivedAt":"2012-08-14T02:02:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 13, 2012 at 09:59:27PM -0400, Jeff King wrote:\n\n> So if we want to avoid the allocation, then this is how I would do it:\n> by returning the feature's _value_ and not the whole key. Since we know\n> that the beginning part must obviously match what we fed it anyway, it\n> is not that interesting.\n> \n> -- >8 --\n> Subject: [PATCH] parse_feature_request: make it easier to see feature values\n\nAnd here is the rebased 4/4 on top of that.\n\nAt this point, I think this part of the topic has received more than\nenough attention. Please feel free to apply these patches, your patches,\nor even just drop it altogether (and when somebody has a more compelling\nreason to actually parse such a value, they can resurrect the\ninfrastructure patch).\n\n-- >8 --\nSubject: [PATCH] fetch-pack: mention server version with verbose output\n\nFetch-pack's verbose mode is more of a debugging mode (and\nin fact takes two \"-v\" arguments to trigger via the\nporcelain layer). Let's mention the server version as\nanother possible item of interest.\n\nSigned-off-by: Jeff King \n---\n builtin/fetch-pack.c | 10 +++++++++-\n 1 file changed, 9 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex bc7a0f9..3b2b5a4 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -787,6 +787,8 @@ static struct ref *do_fetch_pack(int fd[2],\n {\n \tstruct ref *ref = copy_ref_list(orig_ref);\n \tunsigned char sha1[20];\n+\tconst char *agent_feature;\n+\tint agent_len;\n \n \tsort_ref_list(&ref, ref_compare_name);\n \n@@ -823,8 +825,14 @@ static struct ref *do_fetch_pack(int fd[2],\n \t\t\tfprintf(stderr, \"Server supports ofs-delta\\n\");\n \t} else\n \t\tprefer_ofs_delta = 0;\n-\tif (server_supports(\"agent\"))\n+\n+\tif ((agent_feature = server_feature_value(\"agent\", &agent_len))) {\n \t\tagent_supported = 1;\n+\t\tif (args.verbose && agent_len) {\n+\t\t\tfprintf(stderr, \"Server version is %.*s\\n\",\n+\t\t\t\tagent_len, agent_feature);\n+\t\t}\n+\t}\n \n \tif (everything_local(&ref, nr_match, match)) {\n \t\tpacket_flush(fd[1]);\n-- \n1.7.12.rc2.11.gf0a1e27\n"},{"id":"196956","messageId":"7v8vdiax5y.fsf@alter.siamese.dyndns.org","threadId":"31226","inReplyTo":"20120814020210.GB7891@sigill.intra.peff.net","subject":"Re: [PATCH 4/4] fetch-pack: mention server version with verbose output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-14T04:56:57Z","receivedAt":"2012-08-14T04:56:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> +\tif ((agent_feature = server_feature_value(\"agent\", &agent_len))) {\n> \t\tagent_supported = 1;\n> +\t\tif (args.verbose && agent_len) {\n> +\t\t\tfprintf(stderr, \"Server version is %.*s\\n\",\n> +\t\t\t\tagent_len, agent_feature);\n> +\t\t}\n> +\t}\n\nOK. The one I queued in 'pu' said \"Server version not disclosed\"\nwhen length was 0, but I think I like this one better.\n\nAlso I like the the update to the parsing code in the previous\npatch. It makes the logic clearer.\n\nThanks.\n"}]}