Re: [PATCH v3 2/2] connect: advertized capability is not a ref

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Sep 8, 2016, 01:34 UTC
Message-ID: <20160908013431.GC25016@google.com>
In-Reply-To: <4b09bb7a5b7f4eb5fc31df3d98ce7ffc042eb367.1473291819.git.jonathantanmy@google.com>

Jonathan Tan wrote:

> Git advertises the same capabilities^{} ref in its ref advertisement for push
> but since it never remembered to do so for fetch, the client forgot to handle
> this case. Handle it.

The comment in the previous review was that this doesn't describe the history correctly. It can instead say something like

	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.

[...]

Show 8 quoted lines

> @@ -165,8 +166,24 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
>  			continue;
>  		}
>  
> +		if (!strcmp(name, "capabilities^{}")) {
> +			if (got_at_least_one_head)
> +				warning("protocol error: unexpected dummy ref for "
> +				        "capabilities declaration, continuing anyway");

Can this die() instead of warning?

I think mentioning capabilities^{} in the error message would make it easier to debug.

> +			if (got_dummy_ref_with_capabilities_declaration)
> +				warning("protocol error: multiple dummy refs for "
> +				        "capabilities declaration, continuing anyway");

Likewise.

> +			got_dummy_ref_with_capabilities_declaration = 1;
> +			continue;

I think we can make this stricter. The capabilities^{} line is supposed to be the first advertised ref, before any 'shallow' lines or .have extra refs.

(Alas, Documentation/technical/pack-protocol.txt doesn't describe .have refs --- v1.6.1-rc1~203^2~1, push: prepare sender to receive extended ref information from the receiver, 2008-09-09.)

'die_initial_contact' uses got_at_least_one_head to determine whether it was on the first line but code paths added later that use 'continue' don't populate it properly (see b06dcd7d, 40c155ff, and 1a7141ff). We could do

	int first_line = 1;

	for (;; first_line = 0) {
		...
	}

and use !first_line instead of got_at_least_one_head (removing got_at_least_one_head in the process since it has no other purpose).

Thanks, Jonathan