From: Dave Borowitz Date: Mon, 06 Jul 2015 15:22:02 GMT Subject: Re: [PATCH 3/7] pack-protocol.txt: Mark all LFs in push-cert as required Message-ID: In-Reply-To: On Mon, Jul 6, 2015 at 10:46 AM, Dave Borowitz wrote: > On Wed, Jul 1, 2015 at 4:49 PM, Junio C Hamano wrote: >> >> Dave Borowitz 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 ", 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.