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

From: Jeff King <peff@peff.net>
Date: Aug 10, 2012, 21:35 UTC
Message-ID: <20120810213508.GC888@sigill.intra.peff.net>
In-Reply-To: <7vr4reigm8.fsf@alter.siamese.dyndns.org>

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

Show 10 quoted lines

> > 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?).

Show 9 quoted lines

> 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.

Show 22 quoted lines

> 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