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

[Junio C Hamano <gitster@pobox.com>]

Jonathan Tan <jonathantanmy@google.com> writes:

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

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

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