Re: [PATCH v4 3/3] connect: advertized capability is not a ref

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Sep 9, 2016, 19:40 UTC
Message-ID: <20160909194037.GG25016@google.com>
In-Reply-To: <ac55dc281e6875df8abcc6ed06d5f258a53dd251.1473441620.git.jonathantanmy@google.com>

Jonathan Tan wrote:

Show 10 quoted lines

> --- a/connect.c
> +++ b/connect.c
> @@ -172,8 +173,24 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
>  			continue;
>  		}
>  
> +		if (!strcmp(name, "capabilities^{}")) {
> +			if (saw_response)
> +				warning("protocol error: unexpected capabilities^{}, "
> +					"continuing anyway");

Please use die() for these.

The warning is directed at the wrong user. The end-user isn't going to be able to fix the server. The server owner is going to say "Git works fine --- I'll ignore this". Client authors are going to *eventually* discover the bad server and have to work around it. So everyone suffers.

I feel strongly about this: because there are no servers that violate this, it should be a fatal error. If we find a server that violates this, we should weaken the spec and make all violations of the spec still a fatal error.

The rest looks good.

Thanks for your patience, Jonathan