Re: [PATCH 1/7] pack-protocol.txt: Add warning about protocol inaccuracies

From: Dave Borowitz <dborowitz@google.com>
Date: Jul 1, 2015, 19:56 UTC
Message-ID: <CAD0k6qTtvfKgJX1LyaO3i6as1Av9T=cZrwput-1HnTJuek5HoA@mail.gmail.com>
In-Reply-To: <20150701193949.GC4865@google.com>

On Wed, Jul 1, 2015 at 12:39 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:

Show 31 quoted lines

> Hi,
>
> Dave Borowitz wrote:
>
>> --- a/Documentation/technical/pack-protocol.txt
>> +++ b/Documentation/technical/pack-protocol.txt
>> @@ -14,6 +14,17 @@ data.  The protocol functions to have a server tell a client what is
>>  currently on the server, then for the two to negotiate the smallest amount
>>  of data to send in order to fully update one or the other.
>>
>> +Notes to Implementors
>> +---------------------
>> +
>> +WARNING: This document is a work in progress. Some of the protocol
>> +specifications below may be incomplete or inaccurate. When in doubt,
>> +refer to the C code.
>
> If we include this warning, can it also say to contact
> git@vger.kernel.org for inaccuracies?
>
> Otherwise it is easy to misread as "Some of this document may be
> inaccurate, and we're working on fixing that.  Don't bug me --- I
> already know about the problems --- just be patient!"
>
> I would rather that it would say something like
>
>         Caveat
>         ------
>         This document contains some inaccuracies.  Do not forget to also
>         check against the C code, and please contact git@vger.kernel.org
>         if you run into any unclear or inaccurate passages in this spec.

Agreed with your rationale and suggested wording, thanks.

Show 13 quoted lines

>> +
>> +One particular example is that many of the LFs referenced in the
>> +specifications are optional, but may (yet) not be marked as such. If not
>> +explicitly marked one way or the other, double-check with the C code.
>
> The 'Reference Discovery' section says:
>
>         Server SHOULD terminate each non-flush line using LF ("\n") terminator;
>         client MUST NOT complain if there is no terminator.
>
> Unfortunately that's not explained in a section with broader scope.
>
> Isn't that actually the rule everywhere except for in push certs?

It's certainly the rule in more places. I personally have partially audited send-pack.c, but there are many places that are not yet audited. I don't feel comfortable making a broader claim without having done so, and I do not have the time to do so at the moment.

> The documentation will be easier to use if it says so instead of
> asking implementers to check the source of all implementations
> (since interoperability with only one isn't enough).

Unfortunately, the trust I have in this document at this point is less than zero. A handful of spot fixes, while useful, does not serve as any sort of assurance that we've gotten all or even most of the problems. Until such time as this document is actually reliable, implementors must check the source of all implementations if they want to be accurate. That sucks for implementors (believe me, I know), but it's the truth.

> Thanks,
> Jonathan