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

[Jonathan Nieder <jrnieder@gmail.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.

[...]
> @@ -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