On Mon, Jul 6, 2015 at 10:46 AM, Dave Borowitz <dborowitz@xxxxxxxxxx> wrote: > On Wed, Jul 1, 2015 at 4:49 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote: >> >> Dave Borowitz <dborowitz@xxxxxxxxxx> 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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html