Fix empty message serialization

[castedo]

Fixes #1429, incorrect signature verification for commits with an empty message, as encoded by CGit.

[castedo]

@jelmer I've made this a draft, because the change still seems terrifying to me. I haven't read and understood enough of the code to feel great about this change. I'm planning to read and understand some more of the code.

What do you think? You see any issues?

An addition improvement to consider is to strongly type the message attribute of commit to be a non-optional bytes string and only a string. In other words, it can not be None. I think the idea of a missing message in a commit is more error prone than a required message, but the message can be empty. But this re-typing of message in commit is probably not worth the time/effort.

[jelmer]

@jelmer I've made this a draft, because the change still seems terrifying to me. I haven't read and understood enough of the code to feel great about this change. I'm planning to read and understand some more of the code.

What do you think? You see any issues?

An addition improvement to consider is to strongly type the message attribute of commit to be a non-optional bytes string and only a string. In other words, it can not be None. I think the idea of a missing message in a commit is more error prone than a required message, but the message can be empty. But this re-typing of message in commit is probably not worth the time/effort.

This looks correct to me; if you create a commit with an empty commit message in C git, you still end up with two newlines after the last header.

[castedo]

Reading the code, I'm seeing that this change is affecting a code path when a tag lacks a non-empty message and lacks a signature. So this code change is also going to cause an extra newline in a tag corner case.

I'm going peek at some more unit tests to see if they can help confirm that a new newline is appropriate in the tag case.

[castedo]

Doing git cat-file -p ... | xxd indicates that the logic is appropriate for tags too. When there is no message, cgit still places two newlines after the headers.

[castedo]

OK, i've looked over the code/tests, looked at git cat-file -d .. | xxd with some tags and commits with empty messages and this looks good to me. I give a 👍 to merge!

[jelmer]

Thanks for the careful checking!