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

On Mon, Jul 6, 2015 at 1:30 PM, Junio C Hamano [off-list ref] wrote:

Dave Borowitz [off-list ref] writes:

quoted

I think I understand the confusion now. I think you and I are working
from different assumptions about the client behavior.

I agree that we now both understand where we come from ;-)

And sorry for not being clear when I did the "push-cert" originally
in the documentation.  As I already said, "packets between push-cert
and push-cert-end are contents of individual lines of the GPG signed
push certificate"

This sentence makes sense to me now, but only because we now agree
that "contents" does not include the LF. Different people may have
different initial assumptions about whether the "contents" of a line
includes the trailing newline or not.

was the design meant from day one, and a85b377d
(push: the beginning of "git push --signed", 2014-09-12) could have
made it clearer.

quoted

The problem with the documentation, then, is that the documentation
does not say anything to indicate that the signed payload is anything
other than what is on the wire.

Yeah, that was untold assumption, as I considered "what is on the
wire" to include pkt-line framing when I wrote a85b377d (push: the
beginning of "git push --signed", 2014-09-12).

quoted

So maybe this series should include an explicit description of the
singed payload outside of the context of a push. Then, in the push
section, we can describe the set of transformations that the client
MUST perform (splitting on LF; adding pkt-line headers) and MAY
perform (adding LFs).

Yes, and the latter is not limited to push-cert but anything sent on
pkt-line.

That incidentally is the only point I deeply care about.  I just
want to minimize "the protocol is this way in general, but only for
this one you must do it differently".

Understood, and I'm glad we have finally come to an arrangement that
is both consistent and easy to implement on the server side.

One example of "only for this one you must do it differently" is
another caveat for protocol implementors for the sending side (again
not limited to "push cert").

That existing implementations of the receivers treat an empty packet
(i.e. "0004")

or "0005\n" ;)

as if it is the same as a flush packet (i.e. "0000"),
so even if the sending side chooses to ignore the "SHOULD terminate
each non-flush line using LF", it is strongly advised not to do so
when it wants to send an empty payload.  This needs to be documented.

The receiving end SHOULD NOT treat "0004" the same way as "0000".
I think that must be documented and implementations (including our
own) should be fixed.

Agreed.

Thanks.