[PATCH 0/4] jk/version-string and google code

[PATCH 0/4] jk/version-string and google code

[Jeff King]

On Fri, Aug 03, 2012 at 12:19:16PM -0400, Jeff King wrote:

> Instead of having the client advertise a particular version
> number in the git protocol, we have managed extensions and
> backwards compatibility by having clients and servers
> advertise capabilities that they support. This is far more
> robust than having each side consult a table of
> known versions, and provides sufficient information for the
> protocol interaction to complete.
> 
> However, it does not allow servers to keep statistics on
> which client versions are being used. This information is
> not necessary to complete the network request (the
> capabilities provide enough information for that), but it
> may be helpful to conduct a general survey of client
> versions in use.
> 
> We already send the client version in the user-agent header
> for http requests; adding it here allows us to gather
> similar statistics for non-http requests.

Ugh, the jk/version-string topic breaks fetching from Google Code. With
my patch, the client unconditionally sends an "agent=foo" capability,
but the server does not like seeing the unknown capability and ends the
connection (I'm guessing with some kind of internal exception, since it
spews "Internal server error" over the protocol channel).

This is the right thing to do according to protocol-capabilities.txt,
which says:

  Client will then send a space separated list of capabilities it wants
  to be in effect. The client MUST NOT ask for capabilities the server
  did not say it supports.

  Server MUST diagnose and abort if capabilities it does not understand
  was sent.  Server MUST NOT ignore capabilities that client requested
  and server advertised.  As a consequence of these rules, server MUST
  NOT advertise capabilities it does not understand.

However, that is not how git-core behaves. Its server side will ignore
an unknown capability coming from the client (so not only is it more
lenient about what the client does, but it does not follow the "MUST"
directives in the second paragraph).

This isn't a huge deal for this topic; any server that is collecting the
data should be advertising anyway. The only ones who would miss out are
humans trying to debug client behavior via tcpdump or similar, when the
server side is an older version of git.

That's probably acceptable, given that the alternative is changing Google
Code's implementation, along with finding out how many other
implementations might have followed that spec strictly. We might or
might not want to loosen the "MUST" bits in that document, since git
itself does not follow them.

Here's a patch series that goes on top of jk/version-string:

  [1/4]: send-pack: fix capability-sending logic

This one is a minor bug fix in the same area.

  [2/4]: do not send client agent unless server does first

The actual fix.

  [3/4]: connect: learn to parse capabilities with values
  [4/4]: fetch-pack: mention server version with verbose output

A bonus feature. I'm not sure if they are worth doing or not. I'd
really expect somebody debugging a protocol issue to just use
GIT_TRACE_PACKET, and then they can read it straight from the packet
dump themselves.

-Peff

[PATCH 1/4] send-pack: fix capability-sending logic

[Jeff King]

If we have capabilities to send to the server, we send the
regular "want" line followed by a NUL, then the
capabilities; otherwise, we do not even send the NUL.

However, when checking whether we want to send the "quiet"
capability, we check args->quiet, which is wrong. That flag
only tells us whether the client side wanted to be quiet,
not whether the server supports it (originally, in c207e34f,
it meant both; however, that was later split into two flags
by 01fdc21f).

We still check the right flag when actually printing
"quiet", so this could only have two effects:

  1. We might send the trailing NUL when we do not otherwise
     need to. In theory, an antique pre-capability
     implementation of git might choke on this (since the
     client is instructed never to respond with capabilities
     that the server has not first advertised).

  2. We might also want to send the quiet flag if the
     args->progress flag is false, but this code path would
     not trigger in that instance.

In practice, it almost certainly never matters. The
report-status capability dates back to 2005. Any real-world
server is going to advertise that, and we will always
respond with at least that capability.

Signed-off-by: Jeff King <peff@peff.net>
---
I'm tempted to remove this part of the conditional entirely to make the
code simpler, which would mean that we always send the extra NUL, even
if there are no capabilities. But I'm not 100% sure that pre-1.1.0
versions of git actually handle that, and who knows if there are other
implementations. This fix is the safe, conservative route.

 builtin/send-pack.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index c4d4211..5c69995 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -306,7 +306,7 @@ int send_pack(struct send_pack_args *args,
 			char *new_hex = sha1_to_hex(ref->new_sha1);
 			int quiet = quiet_supported && (args->quiet || !args->progress);
 
-			if (!cmds_sent && (status_report || use_sideband || args->quiet)) {
+			if (!cmds_sent && (status_report || use_sideband || quiet)) {
 				packet_buf_write(&req_buf,
 						 "%s %s %s%c%s%s%s agent=%s",
 						 old_hex, new_hex, ref->name, 0,
-- 
1.7.12.rc2.4.g7f05cf9

[PATCH 2/4] do not send client agent unless server does first

[Jeff King]

Commit ff5effdf taught both clients and servers of the git
protocol to send an "agent" capability that just advertises
their version for statistics and debugging purposes.
However, the protocol-capabilities.txt document indicates
that the client's advertisement is actually a response, and
should never include capabilities not mentioned in the
server's advertisement.

Adding the unconditional advertisement in the server
programs was OK, then, but the clients broke the protocol.
The server implementation of git-core itself does not care,
but at least one does: the Google Code git server will hang
up with an internal error upon seeing an unknown capability.

Instead, each client must record whether we saw an agent
string from the server, and respond with its agent only if
the server mentioned it first.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/fetch-pack.c |  7 ++++++-
 builtin/send-pack.c  | 12 +++++++++---
 2 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index fe56596..bc7a0f9 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -19,6 +19,7 @@ static int prefer_ofs_delta = 1;
 static int no_done;
 static int fetch_fsck_objects = -1;
 static int transfer_fsck_objects = -1;
+static int agent_supported;
 static struct fetch_pack_args args = {
 	/* .uploadpack = */ "git-upload-pack",
 };
@@ -328,7 +329,8 @@ static int find_common(int fd[2], unsigned char *result_sha1,
 			if (args.no_progress)   strbuf_addstr(&c, " no-progress");
 			if (args.include_tag)   strbuf_addstr(&c, " include-tag");
 			if (prefer_ofs_delta)   strbuf_addstr(&c, " ofs-delta");
-			strbuf_addf(&c, " agent=%s", git_user_agent_sanitized());
+			if (agent_supported)    strbuf_addf(&c, " agent=%s",
+							    git_user_agent_sanitized());
 			packet_buf_write(&req_buf, "want %s%s\n", remote_hex, c.buf);
 			strbuf_release(&c);
 		} else
@@ -821,6 +823,9 @@ static struct ref *do_fetch_pack(int fd[2],
 			fprintf(stderr, "Server supports ofs-delta\n");
 	} else
 		prefer_ofs_delta = 0;
+	if (server_supports("agent"))
+		agent_supported = 1;
+
 	if (everything_local(&ref, nr_match, match)) {
 		packet_flush(fd[1]);
 		goto all_done;
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index 5c69995..7d05064 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -252,6 +252,7 @@ int send_pack(struct send_pack_args *args,
 	int status_report = 0;
 	int use_sideband = 0;
 	int quiet_supported = 0;
+	int agent_supported = 0;
 	unsigned cmds_sent = 0;
 	int ret;
 	struct async demux;
@@ -267,6 +268,8 @@ int send_pack(struct send_pack_args *args,
 		use_sideband = 1;
 	if (server_supports("quiet"))
 		quiet_supported = 1;
+	if (server_supports("agent"))
+		agent_supported = 1;
 
 	if (!remote_refs) {
 		fprintf(stderr, "No refs in common and none specified; doing nothing.\n"
@@ -306,14 +309,17 @@ int send_pack(struct send_pack_args *args,
 			char *new_hex = sha1_to_hex(ref->new_sha1);
 			int quiet = quiet_supported && (args->quiet || !args->progress);
 
-			if (!cmds_sent && (status_report || use_sideband || quiet)) {
+			if (!cmds_sent && (status_report || use_sideband ||
+					   quiet || agent_supported)) {
 				packet_buf_write(&req_buf,
-						 "%s %s %s%c%s%s%s agent=%s",
+						 "%s %s %s%c%s%s%s%s%s",
 						 old_hex, new_hex, ref->name, 0,
 						 status_report ? " report-status" : "",
 						 use_sideband ? " side-band-64k" : "",
 						 quiet ? " quiet" : "",
-						 git_user_agent_sanitized());
+						 agent_supported ? " agent=" : "",
+						 agent_supported ? git_user_agent_sanitized() : ""
+						);
 			}
 			else
 				packet_buf_write(&req_buf, "%s %s %s",
-- 
1.7.12.rc2.4.g7f05cf9

[PATCH 3/4] connect: learn to parse capabilities with values

[Jeff King]

We already take care to parse a capability like "foo=bar"
properly, but the code does not provide a good way of
actually finding out what is on the right-hand side of the
"=".

Signed-off-by: Jeff King <peff@peff.net>
---
 cache.h   |  2 ++
 connect.c | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 34 insertions(+)

diff --git a/cache.h b/cache.h
index 06413e1..3811c66 100644
--- a/cache.h
+++ b/cache.h
@@ -1030,7 +1030,9 @@ struct extra_have_objects {
 };
 extern struct ref **get_remote_heads(int in, struct ref **list, unsigned int flags, struct extra_have_objects *);
 extern int server_supports(const char *feature);
+extern char *server_feature_value(const char *feature);
 extern const char *parse_feature_request(const char *features, const char *feature);
+extern char *parse_feature_request_value(const char *features, const char *feature);
 
 extern struct packed_git *parse_pack_index(unsigned char *sha1, const char *idx_path);
 
diff --git a/connect.c b/connect.c
index 912cdde..6bd2531 100644
--- a/connect.c
+++ b/connect.c
@@ -104,6 +104,11 @@ int server_supports(const char *feature)
 	return !!parse_feature_request(server_capabilities, feature);
 }
 
+char *server_feature_value(const char *feature)
+{
+	return parse_feature_request_value(server_capabilities, feature);
+}
+
 const char *parse_feature_request(const char *feature_list, const char *feature)
 {
 	int len;
@@ -124,6 +129,33 @@ const char *parse_feature_request(const char *feature_list, const char *feature)
 	return NULL;
 }
 
+/*
+ * Parse features of the form "feature=value".  Returns NULL if the feature
+ * does not exist, the empty string if it exists but does not have an "=", or
+ * the content to the right of the "=" until the first space (or end of
+ * string).  The cannot contain literal spaces; double-quoting or similar
+ * schemes would break compatibility, since older versions of git treat the
+ * space as a hard-delimiter without any context.
+ *
+ * The return value (if non-NULL) is newly allocated on the heap and belongs to
+ * the caller.
+ */
+char *parse_feature_request_value(const char *feature_list, const char *feature)
+{
+	const char *start = parse_feature_request(feature_list, feature);
+	const char *end;
+
+	if (!start || prefixcmp(start, feature))
+		return NULL;
+	start += strlen(feature);
+
+	if (*start == '=')
+		start++;
+	end = strchrnul(start, ' ');
+
+	return xmemdupz(start, end - start);
+}
+
 enum protocol {
 	PROTO_LOCAL = 1,
 	PROTO_SSH,
-- 
1.7.12.rc2.4.g7f05cf9

[PATCH 4/4] fetch-pack: mention server version with verbose output

[Jeff King]

Fetch-pack's verbose mode is more of a debugging mode (and
in fact takes two "-v" arguments to trigger via the
porcelain layer). Let's mention the server version as
another possible item of interest.

Signed-off-by: Jeff King <peff@peff.net>
---
Like I mentioned before, I'm lukewarm on this one, because I doubt
anybody actually uses this debugging information. If we drop it, it
makes sense to drop 3/4, too, which is just infrastructure for this.

 builtin/fetch-pack.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index bc7a0f9..bfe31ee 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -823,8 +823,14 @@ static struct ref *do_fetch_pack(int fd[2],
 			fprintf(stderr, "Server supports ofs-delta\n");
 	} else
 		prefer_ofs_delta = 0;
-	if (server_supports("agent"))
+	if (server_supports("agent")) {
 		agent_supported = 1;
+		if (args.verbose) {
+			char *v = server_feature_value("agent");
+			fprintf(stderr, "Server version is %s\n", v);
+			free(v);
+		}
+	}
 
 	if (everything_local(&ref, nr_match, match)) {
 		packet_flush(fd[1]);
-- 
1.7.12.rc2.4.g7f05cf9

Re: [PATCH 3/4] connect: learn to parse capabilities with values

[Eric Sunshine]

On Fri, Aug 10, 2012 at 3:58 AM, Jeff King <peff@peff.net> wrote:
> + * Parse features of the form "feature=value".  Returns NULL if the feature
> + * does not exist, the empty string if it exists but does not have an "=", or
> + * the content to the right of the "=" until the first space (or end of
> + * string).  The cannot contain literal spaces; double-quoting or similar

s/The cannot/The value cannot/

-- ES

Re: [PATCH 0/4] jk/version-string and google code

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Ugh, the jk/version-string topic breaks fetching from Google Code. With
> my patch, the client unconditionally sends an "agent=foo" capability,
> but the server does not like seeing the unknown capability and ends the
> connection (I'm guessing with some kind of internal exception, since it
> spews "Internal server error" over the protocol channel).

The fix looks sane and the right thing to do.

I've been using 'next' and started seeing this breakage when pushing
to code.google.com only a few days ago.  My reflog tells that
ff5effd (include agent identifier in capability string, 2012-08-03)
was merged to my everyday-work branch on this Monday, which is more
or less consistent with what I am observing.

> This is the right thing to do according to protocol-capabilities.txt,
> which says:
>
>   Client will then send a space separated list of capabilities it wants
>   to be in effect. The client MUST NOT ask for capabilities the server
>   did not say it supports.
>
>   Server MUST diagnose and abort if capabilities it does not understand
>   was sent.  Server MUST NOT ignore capabilities that client requested
>   and server advertised.  As a consequence of these rules, server MUST
>   NOT advertise capabilities it does not understand.
>
> However, that is not how git-core behaves. Its server side will ignore
> an unknown capability coming from the client (so not only is it more
> lenient about what the client does, but it does not follow the "MUST"
> directives in the second paragraph).

Yeah, we probably should fix in the implementation to honor the
"MUST".  When various implementations of clients can start asking
something the server does not support and still yet expect the
request to cause the server do unusual things, it will lead to
chaos, and honoring that "MUST" is a good way to catch and help
diagnosing such a breakage early.

But that is a separate topic and a longer term direction item.

Thanks.

Re: [PATCH 0/4] jk/version-string and google code

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Ugh, the jk/version-string topic breaks fetching from Google Code. With
> my patch, the client unconditionally sends an "agent=foo" capability,
> but the server does not like seeing the unknown capability and ends the
> connection (I'm guessing with some kind of internal exception, since it
> spews "Internal server error" over the protocol channel).

I asked the folks who run code.google.com and they are indeed seeing
something like these in their logs:

 >> Client asked for capability agent=git/1.7.12.rc2.79.g86c1702 that was not advertised.

So please consider your conjecture confirmed, and thanks for a
prompt fix.

Re: [PATCH 0/4] jk/version-string and google code

[Jeff King]

On Fri, Aug 10, 2012 at 08:34:45AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > Ugh, the jk/version-string topic breaks fetching from Google Code. With
> > my patch, the client unconditionally sends an "agent=foo" capability,
> > but the server does not like seeing the unknown capability and ends the
> > connection (I'm guessing with some kind of internal exception, since it
> > spews "Internal server error" over the protocol channel).
> 
> The fix looks sane and the right thing to do.
> 
> I've been using 'next' and started seeing this breakage when pushing
> to code.google.com only a few days ago.  My reflog tells that
> ff5effd (include agent identifier in capability string, 2012-08-03)
> was merged to my everyday-work branch on this Monday, which is more
> or less consistent with what I am observing.

Thanks for confirming the push side. I have been running with the patch
for months, but only recently happened to try cloning something from
code.google.com. I assumed the push side had the same problem, but did
not want to make a new project just to test (and it seemed fairly
obvious that it would have the same issue).

It was pure coincidence that it happened at the same time you were
graduating the topic to next. But I'm glad I was able to catch it before
anybody started complaining. :)

-Peff

Re: [PATCH 0/4] jk/version-string and google code

[Dave Borowitz]

On Fri, Aug 10, 2012 at 8:37 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Jeff King <peff@peff.net> writes:
>
>> Ugh, the jk/version-string topic breaks fetching from Google Code. With
>> my patch, the client unconditionally sends an "agent=foo" capability,
>> but the server does not like seeing the unknown capability and ends the
>> connection (I'm guessing with some kind of internal exception, since it
>> spews "Internal server error" over the protocol channel).
>
> I asked the folks who run code.google.com and they are indeed seeing
> something like these in their logs:
>
>  >> Client asked for capability agent=git/1.7.12.rc2.79.g86c1702 that was not advertised.

FWIW, this error comes from Dulwich:
https://github.com/jelmer/dulwich/blob/25250c1694dac343d469742aeafa139f37fc4ec6/dulwich/server.py#L196

So any servers running Dulwich would be affected by this...though I'm
not aware of any large-scale Dulwich installations other than Google
Code.

> So please consider your conjecture confirmed, and thanks for a
> prompt fix.
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/4] jk/version-string and google code

[Jeff King]

On Fri, Aug 10, 2012 at 11:06:08AM -0700, Dave Borowitz wrote:

> > I asked the folks who run code.google.com and they are indeed seeing
> > something like these in their logs:
> >
> >  >> Client asked for capability agent=git/1.7.12.rc2.79.g86c1702 that was not advertised.
> 
> FWIW, this error comes from Dulwich:
> https://github.com/jelmer/dulwich/blob/25250c1694dac343d469742aeafa139f37fc4ec6/dulwich/server.py#L196

Thanks for the data point. I knew you guys ran some custom code, so I
wasn't sure how widespread this is. The fact that other dulwich-based
servers would see the same issue makes me doubly sure that my fix is the
right direction.

> So any servers running Dulwich would be affected by this...though I'm
> not aware of any large-scale Dulwich installations other than Google
> Code.

I'd rather not break small-scale installations, either. :)

-Peff

Re: [PATCH 0/4] jk/version-string and google code

[Dave Borowitz]

On Fri, Aug 10, 2012 at 11:08 AM, Jeff King <peff@peff.net> wrote:
> On Fri, Aug 10, 2012 at 11:06:08AM -0700, Dave Borowitz wrote:
>
>> > I asked the folks who run code.google.com and they are indeed seeing
>> > something like these in their logs:
>> >
>> >  >> Client asked for capability agent=git/1.7.12.rc2.79.g86c1702 that was not advertised.
>>
>> FWIW, this error comes from Dulwich:
>> https://github.com/jelmer/dulwich/blob/25250c1694dac343d469742aeafa139f37fc4ec6/dulwich/server.py#L196
>
> Thanks for the data point. I knew you guys ran some custom code, so I
> wasn't sure how widespread this is. The fact that other dulwich-based
> servers would see the same issue makes me doubly sure that my fix is the
> right direction.

You may also notice in that code a set of innocuous_capabilities,
which IIRC is the complete set of capabilities, at the time of
writing, that the C git client may send without the server advertising
them. Such a set (painstakingly assembled, I assure you :) may be
useful as we move further in this direction.

>> So any servers running Dulwich would be affected by this...though I'm
>> not aware of any large-scale Dulwich installations other than Google
>> Code.
>
> I'd rather not break small-scale installations, either. :)
>
> -Peff

Re: [PATCH 0/4] jk/version-string and google code

[Jeff King]

On Fri, Aug 10, 2012 at 11:13:30AM -0700, Dave Borowitz wrote:

> > Thanks for the data point. I knew you guys ran some custom code, so I
> > wasn't sure how widespread this is. The fact that other dulwich-based
> > servers would see the same issue makes me doubly sure that my fix is the
> > right direction.
> 
> You may also notice in that code a set of innocuous_capabilities,
> which IIRC is the complete set of capabilities, at the time of
> writing, that the C git client may send without the server advertising
> them. Such a set (painstakingly assembled, I assure you :) may be
> useful as we move further in this direction.

Oh, hmm. When initially writing my message I thought that might be the
case, but I checked to see that the features were sent only when the
server had first advertised them. However, I didn't notice that is true
only in _some_ of these lines from fetch-pack.c:

    if (multi_ack == 2)     strbuf_addstr(&c, " multi_ack_detailed");
    if (multi_ack == 1)     strbuf_addstr(&c, " multi_ack");
    if (no_done)            strbuf_addstr(&c, " no-done");
    if (use_sideband == 2)  strbuf_addstr(&c, " side-band-64k");
    if (use_sideband == 1)  strbuf_addstr(&c, " side-band");
    if (args.use_thin_pack) strbuf_addstr(&c, " thin-pack");
    if (args.no_progress)   strbuf_addstr(&c, " no-progress");
    if (args.include_tag)   strbuf_addstr(&c, " include-tag");

The early ones are checking that the server claimed support, but all of
the args.* ones are influenced directly by the arguments, whether the
server supports it or not.

I don't think there's any bug here. They are all of a class of features
where the client can handle the case where the server simply ignores the
request. However it is certainly food for thought if we are considering
tightening git's server side (even if we fix these, we have to support
the innocuous capabilities list forever for older clients).

-Peff

Re: [PATCH 0/4] jk/version-string and google code

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Thanks for confirming the push side. I have been running with the patch
> for months, but only recently happened to try cloning something from
> code.google.com.

Note that I didn't "confirm" the fix.  I only confirmed the
existence of the breakage (not that I have any reason to doubt the
competence your "fix" patches were made with).

When evaluating a change in the interoperability area, it does not
add much more confidence to the correctness that the change has been
in use for months with the same partner than that it has been used
to talk to many different partners even for a short period of time,
I guess.

> It was pure coincidence that it happened at the same time you were
> graduating the topic to next. But I'm glad I was able to catch it before
> anybody started complaining. :)

Yeah, the problematic one is not in 'master'.

Re: [PATCH 0/4] jk/version-string and google code

[Junio C Hamano]

Dave Borowitz <dborowitz@google.com> writes:

> You may also notice in that code a set of innocuous_capabilities,
> which IIRC is the complete set of capabilities, at the time of
> writing, that the C git client may send without the server advertising
> them. Such a set (painstakingly assembled, I assure you :) may be
> useful as we move further in this direction.

In builtin/fetch-pack.c, we find this:

		if (!fetching) {
			struct strbuf c = STRBUF_INIT;
			if (multi_ack == 2)     strbuf_addstr(&c, " multi_ack_detailed");
			if (multi_ack == 1)     strbuf_addstr(&c, " multi_ack");
			if (no_done)            strbuf_addstr(&c, " no-done");
			if (use_sideband == 2)  strbuf_addstr(&c, " side-band-64k");
			if (use_sideband == 1)  strbuf_addstr(&c, " side-band");
			if (args.use_thin_pack) strbuf_addstr(&c, " thin-pack");
			if (args.no_progress)   strbuf_addstr(&c, " no-progress");
			if (args.include_tag)   strbuf_addstr(&c, " include-tag");
			if (prefer_ofs_delta)   strbuf_addstr(&c, " ofs-delta");
			packet_buf_write(&req_buf, "want %s%s\n", remote_hex, c.buf);
			strbuf_release(&c);
		} else
			packet_buf_write(&req_buf, "want %s\n", remote_hex);

The ones we choose to throw at the other end based on "args.foo" are
not protected by "server_supports()" at all, which is where the
hardcoded list of "innocuous capabilities" comes from.  I would say
this is a client bug.  I wish Dulwich folks didn't choose to be
silent when they added that hardcoded list as a workaround.

If a client threw a request X at a server that does not support it,
and relied on a server bug that does not reject such a request to
allow it send a pack stream that does not conform to what X asked,
and handled the pack stream assuming that the server did X, it would
be a triple bug on the client's end.  Depending on the nature of X,
the end result may be broken. Dulwich is correct to raise an
exception upon seeing agent=foo.

One could argue that from correctness standpoint, being asked to
send ofs-delta and using only ref-delta does not make a corrupt
packfile (it just makes things less efficient), but we cannot
guarantee that all protocol capabilities will be "innocuous" that
way. Longer term direction should be to reduce the "innocuous" set.

Thanks.

Re: [PATCH 2/4] do not send client agent unless server does first

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Instead, each client must record whether we saw an agent
> string from the server, and respond with its agent only if
> the server mentioned it first.

Just a couple of minor comments.

> diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
> index fe56596..bc7a0f9 100644
> --- a/builtin/fetch-pack.c
> +++ b/builtin/fetch-pack.c
> @@ -19,6 +19,7 @@ static int prefer_ofs_delta = 1;
>  static int no_done;
>  static int fetch_fsck_objects = -1;
>  static int transfer_fsck_objects = -1;
> +static int agent_supported;
>  static struct fetch_pack_args args = {
>  	/* .uploadpack = */ "git-upload-pack",
>  };

This is only set to false once per process invocation.  We do not
currently talk with more than one remote from the same process in
the "fetch" codepath, and we must maintain that. This fix will be
broken otherwise ("recursive submodule fetch" comes to mind; one
more reason it should do its submodule business in a separate
process).

> @@ -328,7 +329,8 @@ static int find_common(int fd[2], unsigned char *result_sha1,
>  			if (args.no_progress)   strbuf_addstr(&c, " no-progress");
>  			if (args.include_tag)   strbuf_addstr(&c, " include-tag");

This codepath still forgets to check if the other side advertised
"thin-pack", "no-progress", and "include-tag", no?

>  			if (prefer_ofs_delta)   strbuf_addstr(&c, " ofs-delta");
> -			strbuf_addf(&c, " agent=%s", git_user_agent_sanitized());
> +			if (agent_supported)    strbuf_addf(&c, " agent=%s",
> +							    git_user_agent_sanitized());
> diff --git a/builtin/send-pack.c b/builtin/send-pack.c
> index 5c69995..7d05064 100644
> --- a/builtin/send-pack.c
> +++ b/builtin/send-pack.c
> @@ -252,6 +252,7 @@ int send_pack(struct send_pack_args *args,
>  	int status_report = 0;
>  	int use_sideband = 0;
>  	int quiet_supported = 0;
> +	int agent_supported = 0;
>  	unsigned cmds_sent = 0;
>  	int ret;
>  	struct async demux;

This is initialied to 0 per communication, so having multiple
remote.$there.pushURL configuration variables will work correctly.

Re: [PATCH 3/4] connect: learn to parse capabilities with values

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> +/*
> + * Parse features of the form "feature=value".  Returns NULL if the feature
> + * does not exist, the empty string if it exists but does not have an "=", or
> + * the content to the right of the "=" until the first space (or end of
> + * string).  The cannot contain literal spaces; double-quoting or similar
> + * schemes would break compatibility, since older versions of git treat the
> + * space as a hard-delimiter without any context.
> + *
> + * The return value (if non-NULL) is newly allocated on the heap and belongs to
> + * the caller.
> + */
> +char *parse_feature_request_value(const char *feature_list, const char *feature)
> +{
> +	const char *start = parse_feature_request(feature_list, feature);
> +	const char *end;
> +
> +	if (!start || prefixcmp(start, feature))
> +		return NULL;
> +	start += strlen(feature);
> +
> +	if (*start == '=')
> +		start++;
> +	end = strchrnul(start, ' ');
> +
> +	return xmemdupz(start, end - start);
> +}

Having to run strlen(feature) three times in this function (once in
parse_feature_request() as part of strstr() and the edge check of
the found string, once as part of prefixcmp() here, and then an
explicit strlen() to skip it) makes me feel dirty.

It is not wrong per-se, but it is likely that the caller has a
constant string as the feature when it called this function, so
perhaps just changing the function signature of server_supports,
i.e.

    const char *server_supports(const char *feature)
    {
	return parse_feature_request(server_capabilities, feature);
    }

to return "var=val " would be more than sufficient.

Then the existing callers can keep doing

	if (server_supports("thin-pack"))
        if (!server_supports("quiet"))

and a new caller can do something like

	agent = server_supports("agent");
        if (!agent || !agent[5])
        	... no agent ...
	else {
        	int span = strcspn(agent + 6, " \t\n");
                printf("I found agent=<%.*s>!\n", span, agent + 6);
	}

which doesn't look too bad.

Re: [PATCH 2/4] do not send client agent unless server does first

[Jeff King]

On Fri, Aug 10, 2012 at 12:45:19PM -0700, Junio C Hamano wrote:

> > diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
> > index fe56596..bc7a0f9 100644
> > --- a/builtin/fetch-pack.c
> > +++ b/builtin/fetch-pack.c
> > @@ -19,6 +19,7 @@ static int prefer_ofs_delta = 1;
> >  static int no_done;
> >  static int fetch_fsck_objects = -1;
> >  static int transfer_fsck_objects = -1;
> > +static int agent_supported;
> >  static struct fetch_pack_args args = {
> >  	/* .uploadpack = */ "git-upload-pack",
> >  };
> 
> This is only set to false once per process invocation.  We do not
> currently talk with more than one remote from the same process in
> the "fetch" codepath, and we must maintain that. This fix will be
> broken otherwise ("recursive submodule fetch" comes to mind; one
> more reason it should do its submodule business in a separate
> process).

Right; I followed the existing options in both fetch-pack and send-pack
(use_sideband, no_done, and others here have the same issue), with the
assumption that the current code was not broken (which may not be true,
if it is simply masked by the fact that in practice everybody happens to
support those older features).

I don't know off-hand whether that is actually a trigger-able bug in the
current code or not. It's probably a good topic for a follow-on series.

> > @@ -328,7 +329,8 @@ static int find_common(int fd[2], unsigned char *result_sha1,
> >  			if (args.no_progress)   strbuf_addstr(&c, " no-progress");
> >  			if (args.include_tag)   strbuf_addstr(&c, " include-tag");
> 
> This codepath still forgets to check if the other side advertised
> "thin-pack", "no-progress", and "include-tag", no?

Yes. I didn't realize it until Dave mentioned the "innocuous" list. I
think cleaning them up is reasonable, but probably a separate topic.

> > diff --git a/builtin/send-pack.c b/builtin/send-pack.c
> > index 5c69995..7d05064 100644
> > --- a/builtin/send-pack.c
> > +++ b/builtin/send-pack.c
> > @@ -252,6 +252,7 @@ int send_pack(struct send_pack_args *args,
> >  	int status_report = 0;
> >  	int use_sideband = 0;
> >  	int quiet_supported = 0;
> > +	int agent_supported = 0;
> >  	unsigned cmds_sent = 0;
> >  	int ret;
> >  	struct async demux;
> 
> This is initialied to 0 per communication, so having multiple
> remote.$there.pushURL configuration variables will work correctly.

Right. Not through me having thought about it, though, but by following
the existing convention.

-Peff

Re: [PATCH 3/4] connect: learn to parse capabilities with values

[Jeff King]

On Fri, Aug 10, 2012 at 01:01:11PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > +/*
> > + * Parse features of the form "feature=value".  Returns NULL if the feature
> > + * does not exist, the empty string if it exists but does not have an "=", or
> > + * the content to the right of the "=" until the first space (or end of
> > + * string).  The cannot contain literal spaces; double-quoting or similar
> > + * schemes would break compatibility, since older versions of git treat the
> > + * space as a hard-delimiter without any context.
> > + *
> > + * The return value (if non-NULL) is newly allocated on the heap and belongs to
> > + * the caller.
> > + */
> > +char *parse_feature_request_value(const char *feature_list, const char *feature)
> > +{
> > +	const char *start = parse_feature_request(feature_list, feature);
> > +	const char *end;
> > +
> > +	if (!start || prefixcmp(start, feature))
> > +		return NULL;
> > +	start += strlen(feature);
> > +
> > +	if (*start == '=')
> > +		start++;
> > +	end = strchrnul(start, ' ');
> > +
> > +	return xmemdupz(start, end - start);
> > +}
> 
> Having to run strlen(feature) three times in this function (once in
> parse_feature_request() as part of strstr() and the edge check of
> the found string, once as part of prefixcmp() here, and then an
> explicit strlen() to skip it) makes me feel dirty.

I thought about that, but it seems like a quite premature optimization.
It is three extra strlens per network conversation. _If_ you have turned
on double-verbosity in fetch-pack. I considered reusing the existing
parse_feature_request function more valuable from a maintenance
standpoint.

I would think the extra memory allocation would dwarf it, anyway.

> It is not wrong per-se, but it is likely that the caller has a
> constant string as the feature when it called this function, so
> perhaps just changing the function signature of server_supports,
> i.e.
> 
>     const char *server_supports(const char *feature)
>     {
> 	return parse_feature_request(server_capabilities, feature);
>     }
> 
> to return "var=val " would be more than sufficient.

That was in fact my first iteration, but...

> Then the existing callers can keep doing
> 
> 	if (server_supports("thin-pack"))
>         if (!server_supports("quiet"))
> 
> and a new caller can do something like
> 
> 	agent = server_supports("agent");
>         if (!agent || !agent[5])
>         	... no agent ...
> 	else {
>         	int span = strcspn(agent + 6, " \t\n");
>                 printf("I found agent=<%.*s>!\n", span, agent + 6);
> 	}
> 
> which doesn't look too bad.

I didn't want to force callers to have to deal with ad-hoc parsing.

Anyway, do you think this is even worth doing at this point? I'm
lukewarm on the final two patches due to the existence of
GIT_TRACE_PACKET, which is much more likely to be useful.

-Peff

Re: [PATCH 0/4] jk/version-string and google code

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Fri, Aug 10, 2012 at 11:13:30AM -0700, Dave Borowitz wrote:
>
>> > Thanks for the data point. I knew you guys ran some custom code, so I
>> > wasn't sure how widespread this is. The fact that other dulwich-based
>> > servers would see the same issue makes me doubly sure that my fix is the
>> > right direction.
>> 
>> You may also notice in that code a set of innocuous_capabilities,
>> which IIRC is the complete set of capabilities, at the time of
>> writing, that the C git client may send without the server advertising
>> them. Such a set (painstakingly assembled, I assure you :) may be
>> useful as we move further in this direction.
>
> Oh, hmm. When initially writing my message I thought that might be the
> case, but I checked to see that the features were sent only when the
> server had first advertised them. However, I didn't notice that is true
> only in _some_ of these lines from fetch-pack.c:
>
>     if (multi_ack == 2)     strbuf_addstr(&c, " multi_ack_detailed");
>     if (multi_ack == 1)     strbuf_addstr(&c, " multi_ack");
>     if (no_done)            strbuf_addstr(&c, " no-done");
>     if (use_sideband == 2)  strbuf_addstr(&c, " side-band-64k");
>     if (use_sideband == 1)  strbuf_addstr(&c, " side-band");
>     if (args.use_thin_pack) strbuf_addstr(&c, " thin-pack");
>     if (args.no_progress)   strbuf_addstr(&c, " no-progress");
>     if (args.include_tag)   strbuf_addstr(&c, " include-tag");
>
> The early ones are checking that the server claimed support, but all of
> the args.* ones are influenced directly by the arguments, whether the
> server supports it or not.

> I don't think there's any bug here. They are all of a class of features
> where the client can handle the case where the server simply ignores the
> request. However it is certainly food for thought if we are considering
> tightening git's server side (even if we fix these, we have to support
> the innocuous capabilities list forever for older clients).

I doubt the "innocuous" approach is really viable, unless we have an
autoritative documentation that tells which ones are and which ones
are not innocuous, and everybody follows it, so that everybody's
server and client understands the same set of capabilities as such.

Which is not likely to happen.  So in that sense, the above have
three bugs.  A new person that starts writing his server without
knowing the workaround Dulwich used that has been hidden from the
Git community until today will have to rediscover the "innocuous"
workaround on his server, unless such buggy clients die out.

I'd rather make sure that 10 years on, the maintainer does not have
to worry about interoperating with a new server written by some
third-party.

Something like this, perhaps.

 builtin/fetch-pack.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index bc7a0f9..fdec7f6 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -818,6 +818,12 @@ static struct ref *do_fetch_pack(int fd[2],
 			fprintf(stderr, "Server supports side-band\n");
 		use_sideband = 1;
 	}
+	if (!server_supports("thin-pack"))
+		args.use_thin_pack = 0;
+	if (!server_supports("no-progress"))
+		args.no_progress = 0;
+	if (!server_supports("include-tag"))
+		args.include_tag = 0;
 	if (server_supports("ofs-delta")) {
 		if (args.verbose)
 			fprintf(stderr, "Server supports ofs-delta\n");

Re: [PATCH 0/4] jk/version-string and google code

[Jeff King]

On Fri, Aug 10, 2012 at 02:25:51PM -0700, Junio C Hamano wrote:

> > I don't think there's any bug here. They are all of a class of features
> > where the client can handle the case where the server simply ignores the
> > request. However it is certainly food for thought if we are considering
> > tightening git's server side (even if we fix these, we have to support
> > the innocuous capabilities list forever for older clients).
> 
> I doubt the "innocuous" approach is really viable, unless we have an
> autoritative documentation that tells which ones are and which ones
> are not innocuous, and everybody follows it, so that everybody's
> server and client understands the same set of capabilities as such.

I think the point is that the ordering is something like:

  1. New client features get implemented wrongly. Nobody notices because
     the server side is lax.

  2. Somebody writes a new server (dulwich), or tightens the existing
     code (what we are thinking of). They create the innocuous list
     because they must deal with older clients from (1).

  3. Somebody tries to implement a new client feature wrongly. They
     notice because strict servers actually exist, and are told their
     client is buggy and wrong. The innocuous list never grows.

So we are at step (2), and are just realizing the client problem. Even
if we fix it, we still need the current innocuous list to handle
existing clients.

Although I would think you do not have to worry about the innocuous list
if you always advertise those features. Which I'm surprised dulwich does
not do (IOW, why do they even need the innocuous list?).

> Which is not likely to happen.  So in that sense, the above have
> three bugs.  A new person that starts writing his server without
> knowing the workaround Dulwich used that has been hidden from the
> Git community until today will have to rediscover the "innocuous"
> workaround on his server, unless such buggy clients die out.
> 
> I'd rather make sure that 10 years on, the maintainer does not have
> to worry about interoperating with a new server written by some
> third-party.

Oh, definitely. I wasn't arguing that we shouldn't fix the clients. Just
that we need to make sure that the current list continues working if we
decide to tighten the server side.

> Something like this, perhaps.
> 
>  builtin/fetch-pack.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
> index bc7a0f9..fdec7f6 100644
> --- a/builtin/fetch-pack.c
> +++ b/builtin/fetch-pack.c
> @@ -818,6 +818,12 @@ static struct ref *do_fetch_pack(int fd[2],
>  			fprintf(stderr, "Server supports side-band\n");
>  		use_sideband = 1;
>  	}
> +	if (!server_supports("thin-pack"))
> +		args.use_thin_pack = 0;
> +	if (!server_supports("no-progress"))
> +		args.no_progress = 0;
> +	if (!server_supports("include-tag"))
> +		args.include_tag = 0;
>  	if (server_supports("ofs-delta")) {
>  		if (args.verbose)
>  			fprintf(stderr, "Server supports ofs-delta\n");

Yes, I think that is all that is necessary to fix the immediate issue.
The protocol-capabilities document talks about what to do when
include-tag is not available ("SHOULD issue a subsequent fetch to
acquire the tags that include-tag would have otherwise given the
client"), but I am not sure how well we handle that (in theory we should
be handling it already, but I didn't look).

-Peff

Re: [PATCH 0/4] jk/version-string and google code

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Yes, I think that is all that is necessary to fix the immediate issue.
> The protocol-capabilities document talks about what to do when
> include-tag is not available ("SHOULD issue a subsequent fetch to
> acquire the tags that include-tag would have otherwise given the
> client"), but I am not sure how well we handle that (in theory we should
> be handling it already, but I didn't look).

Yeah, it is more like "A separate fetch only to follow tags was the
only way we handled it before we added include-tag, so we know it
used to work, and also we know the four combinations of new/old *
fetch/upload all used to work".

We may have broken some combinations over time, though.

Re: [PATCH 0/4] jk/version-string and google code

[Jeff King]

On Fri, Aug 10, 2012 at 11:52:28AM -0700, Junio C Hamano wrote:

> When evaluating a change in the interoperability area, it does not
> add much more confidence to the correctness that the change has been
> in use for months with the same partner than that it has been used
> to talk to many different partners even for a short period of time,
> I guess.

Traditionally our interoperability testing has been to cook things in
"next" and "master" and see if anybody complains. It would be nice to
have an interoperability test suite that could hit some common hosting
sites, as well as older versions of git-core itself. I suspect
automating that would be a big pain, though.

-Peff

Re: [PATCH 3/4] connect: learn to parse capabilities with values

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I would think the extra memory allocation would dwarf it, anyway.
>> ...
>> and a new caller can do something like
>> 
>> 	agent = server_supports("agent");
>>         if (!agent || !agent[5])
>>         	... no agent ...
>> 	else {
>>         	int span = strcspn(agent + 6, " \t\n");
>>                 printf("I found agent=<%.*s>!\n", span, agent + 6);
>> 	}
>> 
>> which doesn't look too bad.

I forgot to mention it, but the above was done also to make it
"possible but not mandatory" to pay extra allocation penalty.  The
caller can choose to parse the string into an int, for example,
without extra allocation.  Only the ones that want a string value
and keep a copy around do have to do xmemdupz().

> Anyway, do you think this is even worth doing at this point? I'm
> lukewarm on the final two patches due to the existence of
> GIT_TRACE_PACKET, which is much more likely to be useful.

In the longer term, I think giving callers access to the parameter
value given to a capability is necessary.  If we had this facility
in the old days, we wouldn't have done side-band-64k but spelled it
as side-band=64k.

For the agent=<foo>, certainly we don't need it.

Re: [PATCH 0/4] jk/version-string and google code

[Shawn Pearce]

On Fri, Aug 10, 2012 at 2:50 PM, Jeff King <peff@peff.net> wrote:
> On Fri, Aug 10, 2012 at 11:52:28AM -0700, Junio C Hamano wrote:
>
>> When evaluating a change in the interoperability area, it does not
>> add much more confidence to the correctness that the change has been
>> in use for months with the same partner than that it has been used
>> to talk to many different partners even for a short period of time,
>> I guess.
>
> Traditionally our interoperability testing has been to cook things in
> "next" and "master" and see if anybody complains. It would be nice to
> have an interoperability test suite that could hit some common hosting
> sites, as well as older versions of git-core itself. I suspect
> automating that would be a big pain, though.

I don't know that you need to hit the hosting sites themselves, just
the implementations they are using. Dulwich and JGit are both open
source. It should be possible to build a smallish compatibility test
suite that grabs everyone into one tree, compiles a small test server
from each, and runs the matrix locally. Then its up to the hosting
sites to worry about making sure their implementations are included in
this suite, and up-to-date.  :-)

Re: [PATCH 0/4] jk/version-string and google code

[Junio C Hamano]

Shawn Pearce <spearce@spearce.org> writes:

> On Fri, Aug 10, 2012 at 2:50 PM, Jeff King <peff@peff.net> wrote:
>> On Fri, Aug 10, 2012 at 11:52:28AM -0700, Junio C Hamano wrote:
>>
>>> When evaluating a change in the interoperability area, it does not
>>> add much more confidence to the correctness that the change has been
>>> in use for months with the same partner than that it has been used
>>> to talk to many different partners even for a short period of time,
>>> I guess.
>>
>> Traditionally our interoperability testing has been to cook things in
>> "next" and "master" and see if anybody complains. It would be nice to
>> have an interoperability test suite that could hit some common hosting
>> sites, as well as older versions of git-core itself. I suspect
>> automating that would be a big pain, though.
>
> I don't know that you need to hit the hosting sites themselves, just
> the implementations they are using. Dulwich and JGit are both open
> source. It should be possible to build a smallish compatibility test
> suite that grabs everyone into one tree, compiles a small test server
> from each, and runs the matrix locally. Then its up to the hosting
> sites to worry about making sure their implementations are included in
> this suite, and up-to-date.  :-)

Yeah, and do that for all the major versions of these
implementations.

Re: [PATCH 3/4] connect: learn to parse capabilities with values

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> I forgot to mention it, but the above was done also to make it
> "possible but not mandatory" to pay extra allocation penalty.  The
> caller can choose to parse the string into an int, for example,
> without extra allocation.  Only the ones that want a string value
> and keep a copy around do have to do xmemdupz().
>
>> Anyway, do you think this is even worth doing at this point? I'm
>> lukewarm on the final two patches due to the existence of
>> GIT_TRACE_PACKET, which is much more likely to be useful.
>
> In the longer term, I think giving callers access to the parameter
> value given to a capability is necessary.  If we had this facility
> in the old days, we wouldn't have done side-band-64k but spelled it
> as side-band=64k.
>
> For the agent=<foo>, certainly we don't need it.

Here are the first of two patches to replace your 3 and 4 without
extra allocations, primarily for further discussion.

-- >8 --
Subject: [PATCH 3/4] connect: expose the parameter to a capability

We already take care to parse a capability like "foo=bar" properly,
but the code does not provide a good way of actually finding out
what is on the right-hand side of the "=".

Based on the patch by Jeff King <peff@peff.net>

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 cache.h   |  1 +
 connect.c | 27 ++++++++++++++++++++-------
 2 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/cache.h b/cache.h
index 06413e1..d239cee 100644
--- a/cache.h
+++ b/cache.h
@@ -1030,6 +1030,7 @@ struct extra_have_objects {
 };
 extern struct ref **get_remote_heads(int in, struct ref **list, unsigned int flags, struct extra_have_objects *);
 extern int server_supports(const char *feature);
+extern const char *server_feature(const char *feature, int *len_ret);
 extern const char *parse_feature_request(const char *features, const char *feature);
 
 extern struct packed_git *parse_pack_index(unsigned char *sha1, const char *idx_path);
diff --git a/connect.c b/connect.c
index 912cdde..42640bc 100644
--- a/connect.c
+++ b/connect.c
@@ -99,12 +99,7 @@ struct ref **get_remote_heads(int in, struct ref **list,
 	return list;
 }
 
-int server_supports(const char *feature)
-{
-	return !!parse_feature_request(server_capabilities, feature);
-}
-
-const char *parse_feature_request(const char *feature_list, const char *feature)
+static const char *parse_feature_request_1(const char *feature_list, const char *feature, int *lenp)
 {
 	int len;
 
@@ -117,13 +112,31 @@ const char *parse_feature_request(const char *feature_list, const char *feature)
 		if (!found)
 			return NULL;
 		if ((feature_list == found || isspace(found[-1])) &&
-		    (!found[len] || isspace(found[len]) || found[len] == '='))
+		    (!found[len] || isspace(found[len]) || found[len] == '=')) {
+			if (lenp)
+				*lenp = strcspn(found, " \t\n");
 			return found;
+		}
 		feature_list = found + 1;
 	}
 	return NULL;
 }
 
+const char *parse_feature_request(const char *feature_list, const char *feature)
+{
+	return parse_feature_request_1(feature_list, feature, NULL);
+}
+
+const char *server_feature(const char *feature, int *len)
+{
+	return parse_feature_request_1(server_capabilities, feature, len);
+}
+
+int server_supports(const char *feature)
+{
+	return !!server_feature(feature, NULL);
+}
+
 enum protocol {
 	PROTO_LOCAL = 1,
 	PROTO_SSH,
-- 
1.7.12.rc2.85.g1de7134

[PATCH 4/4] fetch-pack: mention server version with verbose output

[Junio C Hamano]

Fetch-pack's verbose mode is more of a debugging mode (and in fact
takes two "-v" arguments to trigger via the porcelain layer). Let's
mention the server version as another possible item of interest.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * And this is your 4 adjusted for the previous one, releaving the
   caller from having to figure out where the capability string
   ends.

 builtin/fetch-pack.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index fdec7f6..1633aa3 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -787,6 +787,8 @@ static struct ref *do_fetch_pack(int fd[2],
 {
 	struct ref *ref = copy_ref_list(orig_ref);
 	unsigned char sha1[20];
+	const char *agent_feature = NULL;
+	int agent_len;
 
 	sort_ref_list(&ref, ref_compare_name);
 
@@ -829,8 +831,15 @@ static struct ref *do_fetch_pack(int fd[2],
 			fprintf(stderr, "Server supports ofs-delta\n");
 	} else
 		prefer_ofs_delta = 0;
-	if (server_supports("agent"))
+
+	if ((agent_feature = server_feature("agent", &agent_len)) != NULL &&
+	    5 < agent_len && agent_feature[5] == '=') {
 		agent_supported = 1;
+		if (args.verbose) {
+			fprintf(stderr, "Server version is %.*s\n",
+				agent_len - 6, agent_feature + 6);
+		}
+	}
 
 	if (everything_local(&ref, nr_match, match)) {
 		packet_flush(fd[1]);
-- 
1.7.12.rc2.85.g1de7134

Re: [PATCH 4/4] fetch-pack: mention server version with verbose output

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Fetch-pack's verbose mode is more of a debugging mode (and in fact
> takes two "-v" arguments to trigger via the porcelain layer). Let's
> mention the server version as another possible item of interest.
>
> Signed-off-by: Jeff King <peff@peff.net>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>
>  * And this is your 4 adjusted for the previous one, releaving the
>    caller from having to figure out where the capability string
>    ends.

Oops; this was a cut and paste error.  There are these four
(counting the blank after "Subject:") lines before the description.

    From: Jeff King <peff@peff.net>
    Date: Fri, 10 Aug 2012 03:59:29 -0400
    Subject: [PATCH] fetch-pack: mention server version with verbose output

Re: [PATCH 4/4] fetch-pack: mention server version with verbose output

[Jeff King]

On Mon, Aug 13, 2012 at 12:07:35PM -0700, Junio C Hamano wrote:

>  * And this is your 4 adjusted for the previous one, releaving the
>    caller from having to figure out where the capability string
>    ends.
> [...]
> @@ -829,8 +831,15 @@ static struct ref *do_fetch_pack(int fd[2],
>  			fprintf(stderr, "Server supports ofs-delta\n");
>  	} else
>  		prefer_ofs_delta = 0;
> -	if (server_supports("agent"))
> +
> +	if ((agent_feature = server_feature("agent", &agent_len)) != NULL &&
> +	    5 < agent_len && agent_feature[5] == '=') {
>  		agent_supported = 1;
> +		if (args.verbose) {
> +			fprintf(stderr, "Server version is %.*s\n",
> +				agent_len - 6, agent_feature + 6);
> +		}
> +	}

Yeah, this is exactly the kind of ugliness I was trying to avoid with my
allocating wrapper. Still, there is only one call site, so I do not care
overly much (and I as I've already said, I'm lukewarm on the final two
patches, anyway).

There is one difference in your code and mine. With mine, the server can
say simply "agent" to tell the client that it understands the extension
but does not wish to disclose its version. That might be considered
unfriendly (why would the client show theirs if the server is not
willing to do the same?), but it may be a practical decision (e.g.,
security policies may say that servers are higher-risk targets[1]).
Of course, a server can also say "agent=git/none-of-your-business"; this
is just a syntactic question.

-Peff

[1] I think you and I both agreed earlier that the "sharing versions is
    a security risk" line of argument is not that compelling, but that
    does not mean it is not made all the time.

-Peff

Re: [PATCH 4/4] fetch-pack: mention server version with verbose output

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Of course, a server can also say "agent=git/none-of-your-business"; this
> is just a syntactic question.

You do not even have to advertise it in the first place, no?

Re: [PATCH 4/4] fetch-pack: mention server version with verbose output

[Jeff King]

On Mon, Aug 13, 2012 at 02:07:22PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > Of course, a server can also say "agent=git/none-of-your-business"; this
> > is just a syntactic question.
> 
> You do not even have to advertise it in the first place, no?

If you want the client to respond with theirs, you do.

-Peff

Re: [PATCH 4/4] fetch-pack: mention server version with verbose output

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, Aug 13, 2012 at 12:07:35PM -0700, Junio C Hamano wrote:
>
>>  * And this is your 4 adjusted for the previous one, releaving the
>>    caller from having to figure out where the capability string
>>    ends.
>> [...]
>> @@ -829,8 +831,15 @@ static struct ref *do_fetch_pack(int fd[2],
>>  			fprintf(stderr, "Server supports ofs-delta\n");
>>  	} else
>>  		prefer_ofs_delta = 0;
>> -	if (server_supports("agent"))
>> +
>> +	if ((agent_feature = server_feature("agent", &agent_len)) != NULL &&
>> +	    5 < agent_len && agent_feature[5] == '=') {
>>  		agent_supported = 1;
>> +		if (args.verbose) {
>> +			fprintf(stderr, "Server version is %.*s\n",
>> +				agent_len - 6, agent_feature + 6);
>> +		}
>> +	}
>
> Yeah, this is exactly the kind of ugliness I was trying to avoid with my
> allocating wrapper. Still, there is only one call site, so I do not care
> overly much (and I as I've already said, I'm lukewarm on the final two
> patches, anyway).

Actually, the above is vastly superiour compared to the allocating
kind.  Be honest and think about it.  If the caller wants to
allocate, it could, and it does not even have to count.  If the
caller does not want to allocate, it does not have to pay the price.

Re: [PATCH 4/4] fetch-pack: mention server version with verbose output

[Jeff King]

On Mon, Aug 13, 2012 at 02:09:32PM -0700, Junio C Hamano wrote:

> >> +	if ((agent_feature = server_feature("agent", &agent_len)) != NULL &&
> >> +	    5 < agent_len && agent_feature[5] == '=') {
> >>  		agent_supported = 1;
> >> +		if (args.verbose) {
> >> +			fprintf(stderr, "Server version is %.*s\n",
> >> +				agent_len - 6, agent_feature + 6);
> >> +		}
> >> +	}
> >
> > Yeah, this is exactly the kind of ugliness I was trying to avoid with my
> > allocating wrapper. Still, there is only one call site, so I do not care
> > overly much (and I as I've already said, I'm lukewarm on the final two
> > patches, anyway).
> 
> Actually, the above is vastly superiour compared to the allocating
> kind.  Be honest and think about it.  If the caller wants to
> allocate, it could, and it does not even have to count.  If the
> caller does not want to allocate, it does not have to pay the price.

My point is that the run-time allocation price is quite small, but the
readability cost of that ugly conditional with the magic "5" is
non-trivial. But they are apples and oranges, so it is hard to compare
their amounts directly.

-Peff

Re: [PATCH 4/4] fetch-pack: mention server version with verbose output

[Jeff King]

On Mon, Aug 13, 2012 at 05:11:10PM -0400, Jeff King wrote:

> On Mon, Aug 13, 2012 at 02:09:32PM -0700, Junio C Hamano wrote:
> 
> > >> +	if ((agent_feature = server_feature("agent", &agent_len)) != NULL &&
> > >> +	    5 < agent_len && agent_feature[5] == '=') {
> > >>  		agent_supported = 1;
> > >> +		if (args.verbose) {
> > >> +			fprintf(stderr, "Server version is %.*s\n",
> > >> +				agent_len - 6, agent_feature + 6);
> > >> +		}
> > >> +	}
> > >
> > > Yeah, this is exactly the kind of ugliness I was trying to avoid with my
> > > allocating wrapper. Still, there is only one call site, so I do not care
> > > overly much (and I as I've already said, I'm lukewarm on the final two
> > > patches, anyway).
> > 
> > Actually, the above is vastly superiour compared to the allocating
> > kind.  Be honest and think about it.  If the caller wants to
> > allocate, it could, and it does not even have to count.  If the
> > caller does not want to allocate, it does not have to pay the price.
> 
> My point is that the run-time allocation price is quite small, but the
> readability cost of that ugly conditional with the magic "5" is
> non-trivial. But they are apples and oranges, so it is hard to compare
> their amounts directly.

So if we want to avoid the allocation, then this is how I would do it:
by returning the feature's _value_ and not the whole key. Since we know
that the beginning part must obviously match what we fed it anyway, it
is not that interesting.

-- >8 --
Subject: [PATCH] parse_feature_request: make it easier to see feature values

We already take care to parse key/value capabilities like
"foo=bar", but the code does not provide a good way of
actually finding out what is on the right-hand side of the
"=".

A server using "parse_feature_request" could accomplish this
with some extra parsing. You must skip past the "key"
portion manually, check for "=" versus NUL or space, and
then find the length by searching for the next space (or
NUL).  But clients can't even do that, since the
"server_supports" interface does not even return the
pointer.

Instead, let's have our parser share more information by
providing a pointer to the value and its length. The
"parse_feature_value" function returns a pointer to the
feature's value portion, along with the length of the value.
If the feature is missing, NULL is returned. If it does not
have an "=", then a zero-length value is returned.

Similarly, "server_feature_value" behaves in the same way,
but always checks the static server_feature_list variable.

We can then implement "server_supports" in terms of
"server_feature_value". We cannot implement the original
"parse_feature_request" in terms of our new function,
because it returned a pointer to the beginning of the
feature. However, no callers actually cared about the value
of the returned pointer, so we can simplify it to a boolean
just as we do for "server_supports".

Signed-off-by: Jeff King <peff@peff.net>
---
 cache.h   |  4 +++-
 connect.c | 45 ++++++++++++++++++++++++++++++++++++---------
 2 files changed, 39 insertions(+), 10 deletions(-)

diff --git a/cache.h b/cache.h
index 67f28b4..95daa69 100644
--- a/cache.h
+++ b/cache.h
@@ -1038,7 +1038,9 @@ struct extra_have_objects {
 };
 extern struct ref **get_remote_heads(int in, struct ref **list, unsigned int flags, struct extra_have_objects *);
 extern int server_supports(const char *feature);
-extern const char *parse_feature_request(const char *features, const char *feature);
+extern int parse_feature_request(const char *features, const char *feature);
+extern const char *server_feature_value(const char *feature, int *len_ret);
+extern const char *parse_feature_value(const char *feature_list, const char *feature, int *len_ret);
 
 extern struct packed_git *parse_pack_index(unsigned char *sha1, const char *idx_path);
 
diff --git a/connect.c b/connect.c
index 55a85ad..49e56ba 100644
--- a/connect.c
+++ b/connect.c
@@ -115,12 +115,7 @@ struct ref **get_remote_heads(int in, struct ref **list,
 	return list;
 }
 
-int server_supports(const char *feature)
-{
-	return !!parse_feature_request(server_capabilities, feature);
-}
-
-const char *parse_feature_request(const char *feature_list, const char *feature)
+const char *parse_feature_value(const char *feature_list, const char *feature, int *lenp)
 {
 	int len;
 
@@ -132,14 +127,46 @@ const char *parse_feature_request(const char *feature_list, const char *feature)
 		const char *found = strstr(feature_list, feature);
 		if (!found)
 			return NULL;
-		if ((feature_list == found || isspace(found[-1])) &&
-		    (!found[len] || isspace(found[len]) || found[len] == '='))
-			return found;
+		if (feature_list == found || isspace(found[-1])) {
+			const char *value = found + len;
+			/* feature with no value (e.g., "thin-pack") */
+			if (!*value || isspace(*value)) {
+				if (lenp)
+					*lenp = 0;
+				return value;
+			}
+			/* feature with a value (e.g., "agent=git/1.2.3") */
+			else if (*value == '=') {
+				value++;
+				if (lenp)
+					*lenp = strcspn(value, " \t\n");
+				return value;
+			}
+			/*
+			 * otherwise we matched a substring of another feature;
+			 * keep looking
+			 */
+		}
 		feature_list = found + 1;
 	}
 	return NULL;
 }
 
+int parse_feature_request(const char *feature_list, const char *feature)
+{
+	return !!parse_feature_value(feature_list, feature, NULL);
+}
+
+const char *server_feature_value(const char *feature, int *len)
+{
+	return parse_feature_value(server_capabilities, feature, len);
+}
+
+int server_supports(const char *feature)
+{
+	return !!server_feature_value(feature, NULL);
+}
+
 enum protocol {
 	PROTO_LOCAL = 1,
 	PROTO_SSH,
-- 
1.7.12.rc2.11.gf0a1e27

Re: [PATCH 4/4] fetch-pack: mention server version with verbose output

[Jeff King]

On Mon, Aug 13, 2012 at 09:59:27PM -0400, Jeff King wrote:

> So if we want to avoid the allocation, then this is how I would do it:
> by returning the feature's _value_ and not the whole key. Since we know
> that the beginning part must obviously match what we fed it anyway, it
> is not that interesting.
> 
> -- >8 --
> Subject: [PATCH] parse_feature_request: make it easier to see feature values

And here is the rebased 4/4 on top of that.

At this point, I think this part of the topic has received more than
enough attention. Please feel free to apply these patches, your patches,
or even just drop it altogether (and when somebody has a more compelling
reason to actually parse such a value, they can resurrect the
infrastructure patch).

-- >8 --
Subject: [PATCH] fetch-pack: mention server version with verbose output

Fetch-pack's verbose mode is more of a debugging mode (and
in fact takes two "-v" arguments to trigger via the
porcelain layer). Let's mention the server version as
another possible item of interest.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/fetch-pack.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index bc7a0f9..3b2b5a4 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -787,6 +787,8 @@ static struct ref *do_fetch_pack(int fd[2],
 {
 	struct ref *ref = copy_ref_list(orig_ref);
 	unsigned char sha1[20];
+	const char *agent_feature;
+	int agent_len;
 
 	sort_ref_list(&ref, ref_compare_name);
 
@@ -823,8 +825,14 @@ static struct ref *do_fetch_pack(int fd[2],
 			fprintf(stderr, "Server supports ofs-delta\n");
 	} else
 		prefer_ofs_delta = 0;
-	if (server_supports("agent"))
+
+	if ((agent_feature = server_feature_value("agent", &agent_len))) {
 		agent_supported = 1;
+		if (args.verbose && agent_len) {
+			fprintf(stderr, "Server version is %.*s\n",
+				agent_len, agent_feature);
+		}
+	}
 
 	if (everything_local(&ref, nr_match, match)) {
 		packet_flush(fd[1]);
-- 
1.7.12.rc2.11.gf0a1e27

Re: [PATCH 4/4] fetch-pack: mention server version with verbose output

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> +	if ((agent_feature = server_feature_value("agent", &agent_len))) {
>  		agent_supported = 1;
> +		if (args.verbose && agent_len) {
> +			fprintf(stderr, "Server version is %.*s\n",
> +				agent_len, agent_feature);
> +		}
> +	}

OK.  The one I queued in 'pu' said "Server version not disclosed"
when length was 0, but I think I like this one better.

Also I like the the update to the parsing code in the previous
patch.  It makes the logic clearer.

Thanks.