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
