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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 9, 2016, 20:09 UTC
Message-ID: <xmqq37l8bzsb.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <ac55dc281e6875df8abcc6ed06d5f258a53dd251.1473441620.git.jonathantanmy@google.com>

Jonathan Tan <jonathantanmy@google.com> writes:

Show 5 quoted lines

> Git advertises the same capabilities^{} ref in its ref advertisement for push
> but since it never did so for fetch, the client didn't need to handle this
> case. Handle it.
>
> In this aspect, JGit is compliant with the specification in pack-protocol.txt.

The last sentence somehow looks out of place.

Show 12 quoted lines

> +	int got_dummy_ref_with_capabilities_declaration = 0;
>  
>  	*list = NULL;
>  	for (saw_response = 0; ; saw_response = 1) {
> @@ -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");

OK. saw_response tells us that we saw ".have", a valid ref, or "shallow", but "capabilities^{}" should happen before any of them, so that is a protocol violation. Makes perfect sense.

Show 13 quoted lines

> +			if (got_dummy_ref_with_capabilities_declaration)
> +				warning("protocol error: multiple capabilities^{}, "
> +					"continuing anyway");
> +			got_dummy_ref_with_capabilities_declaration = 1;
> +			continue;
> +		}
> +
>  		if (!check_ref(name, flags))
>  			continue;
> +
> +		if (got_dummy_ref_with_capabilities_declaration)
> +			warning("protocol error: unexpected ref after capabilities^{}, "
> +				"using this ref and continuing anyway");

Likewise. "capabilities^{}" is used when we cannot piggyback the capability list after a real ref, so it is unusual to see a real ref after seeing one. Makes perfect sense.

Do we want to abort the connection in these cases, I wonder, though?

Thanks.