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

Jeff King [off-list ref] writes:

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

quoted

quoted

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");

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

quoted

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.

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