Re: [PATCH 3/7] pack-protocol.txt: Mark all LFs in push-cert as required

[Dave Borowitz <dborowitz@google.com>]

On Mon, Jul 6, 2015 at 10:46 AM, Dave Borowitz <dborowitz@google.com> wrote:
> On Wed, Jul 1, 2015 at 4:49 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Dave Borowitz <dborowitz@google.com> writes:
>>
>> >> I am moderately negative about this; wouldn't it make the end result
>> >> cleaner to fix the implementation?
>> >
>> > I'm not sure I understand your suggestion. Are you saying, you would
>> > prefer to make LFs optional in the push cert, for consistency with LFs
>> > being optional elsewhere?
>>
>> Absolutely.  It is not "make" it optional, but "even though it is
>> optional, the receiver has not been following the spec, and it is
>> not too late to fix it".
>>
>> The earliest these documentation updates can hit the public is 2.6;
>> by that time I'd expect the deployed receivers would be fixed with
>> 2.5.1 and 2.4.7 maintenance releases.
>>
>> If some third-party reimplemented their client not to terminate
>> with LF, they wouldn't be working correctly with the deployed
>> servers right now *anyway*.  And with the more lenient receive-pack
>> in 2.5.1 or 2.4.7, they will start working.
>>
>> And we will not change our client to drop LF termination.  So
>> overall I do not see that it is too much a price to pay for
>> consistency across the protocol.
>
> Ok, I understand your proposal now, thank you. I will drop this
> documentation patch from this series, and abandon
> https://git.eclipse.org/r/51071 in JGit. I am not volunteering to
> rewrite push cert handling in git-core though ;)

Unfortunately, optional LFs still make the stored certs for later
auditing and parsing a bit illegible. This is one way in which push
certs are fundamentally different from the rest of the wire protocol,
which is not intended to be persisted.

The corner case I pointed out before where nonce runs into commands is
not the only one.

Consider the following cert fragment:
001fpushee git://localhost/repo
0029nonce 1433954361-bde756572d665bba81d8

A naive cert storage/auditing implementation would store the raw
payload that needs to be verified, without the pkt-line framing. In
this case:
pushee git://localhost/repononce 1433954361-bde756572d665bba81d8

A naive parser that wants to find the pushee would look for "pushee
<urlish>", which would be wrong in this case. (To say nothing of the
fact that "pushee" might actually be "-0700pushee".)

The alternatives for someone writing a parser are:
a. Store the original pkt-line framing.
b. Write a parser in some other clever way, e.g. parsing the entire
cert in reverse might work.

Neither of these is very satisfying, and both reduce human legibility
of the stored payload.

>> > If LF is optional, then with that approach you might end up with a
>> > section of that buffer like:
>>
>> I think I touched on this in my previous message.  You cannot send
>> an empty line anywhere, and this is not limited to push-cert section
>> of the protocol.  Strictly speaking, the wire level allows it, but I
>> do not think the deployed client APIs easily lets you deal with it.
>>
>> So you must follow the "SHOULD terminate with LF" for an empty line,
>> even when you choose to ignore the "SHOULD" in most other places.
>>
>> I do not think it is such a big loss, as long as it is properly
>> documented.