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

[Jonathan Nieder <jrnieder@gmail.com>]

Junio C Hamano wrote:
> Jonathan Tan <jonathantanmy@google.com> writes:

>> diff --git a/connect.c b/connect.c
>> index 722dc3f..0c2221e 100644
>> --- a/connect.c
>> +++ b/connect.c
>> @@ -165,6 +165,9 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
>>  			continue;
>>  		}
>>  
>> +		if (!strcmp(name, "capabilities^{}"))
>> +			continue;
>
> While it is true that ignoring this line anywhere in the ref
> advertisement is safe, it feels a bit strange to do so, when we know
> that it can appear _only_ when there is no other ref advertised.  I
> guess you can argue that it is good to be lenient to accept what
> others produce, but on the other hand, it can also be argued that
> having this among real ref advertisement would be a protocol
> violation that we may want to diagnose and prod the other side to
> fix their software (but still not fail).

By "it can also be argued", do you mean that you would prefer that
behavior?

It sounds like the worst of both worlds to me --- git would allow the
buggy server behavior, leading people not to fix their servers, but it
would print an ugly error message, so end-users would associate git
with confusing messages.

Given that there aren't any servers that are going to produce this
kind of bad input anyway, I prefer a die().

Thanks,
Jonathan