git-format-patch should include a checksum

Hi,

I'm seeing Git patches being corrupted by mailers and still apply correctly. It would be great if git-format-patch could include a hash of the patch body (and commit message); git-am should check the hash, and refuse to commit if the patch was corrupted (--force should override that, of course).

                                        Juliusz

Heya,

On Tue, Jan 26, 2010 at 23:34, Juliusz Chroboczek <jch@pps.jussieu.fr> wrote:

Sounds like a good idea, have a look at cmd_format_patch in builtin-log.c. Assuming you want to add the hash as a X- mail header (e.g. X-Git-Message-Hash), you should probably add it before line 1019 (where rev.extra_headers is set to the current contents of 'buf'). For the hash itself have a look at git_SHA1_{Init,Update,Final} in various files, csum-file.c would seem like an excellent candidate, as it writes a sha1-summed file, which is sortof what you want to do.

For the git-am part look at 'git-am.sh', you should have checking logic warn only at first, and add a configuration option that allows enforcing it and warn when the option isn't set, instructing the user how to configure the option, see warn_unconfigured_deny_msg in builtin-receive-pack.c. A few releases from now we can set it from warn to abort (or deny, whatever the enforcing option is called).

Good luck!

Juliusz Chroboczek <jch@pps.jussieu.fr> writes:

Do you have an example of such corrupted and incorrectly applied patches? What kind of corruption are you talking about?

format-patch/am pair is designed to be lenient, allowing people to write additional messages after the three-dash lines after the output is made but before it is given to the MUA for sending the result out, for example, so adding a checksum over the entire output and forcing a check upon application is really a bad idea, even though, provided if the patch is done cleanly, it might be acceptable as an optional feature.

Heya,

On Wed, Jan 27, 2010 at 00:21, Junio C Hamano <gitster@pobox.com> wrote:

I would imagine that the checksum is taken over just the actual commit message, perhaps author information, and use the patch-id for the patch itself, that way any comments after triple dash would be ignored, right?

On Wed, 27 Jan 2010, Sverre Rabbelier wrote:

That wouldn't work either. People can, should, and do add extra things to the message before applying it.

Examples of things I tend to add/change in the commit message:
 - add ack's from people in the same thread
 - add "Cc: stable@kernel.org" 
 - re-flow paragraphs when somebody uses a mailer that makes a mess of it.
 - occasionally fix spelling and grammar

so if there is some checksum that screws that up and requires me to then use a "--force" flag to apply it, that would be a bad thing.

I also do edit patches manually too. Having lived with people sending me patches for the last almost twenty years, I can edit patches in my sleep. Doing things like renaming new variables etc by search-and-replace on the patch may not be something I do _often_, but it happens.

In short, it might make sense to have some anti-corruption logic, but I suspect it needs a lot of thought.

		Linus

Heya,

On Wed, Jan 27, 2010 at 01:45, Linus Torvalds <torvalds@linux-foundation.org> wrote:

Ah, that's a fair point.

Perhaps it makes sense to make it a separate mode to git am, such that it only checks that the patch is not corrupted, but does not apply it. That way it would be possible to download the patch, check that it arrived unscathed, and then do your usual patch handling. Those who do not edit patches before applying it would be convenient to set a configuration option that automatically does it when applying the patch, either warning about it or aborting (as Juliusz suggested).

On Wed, 27 Jan 2010, Sverre Rabbelier wrote:

FWIW, I do manually edit both incoming and outgoing patches from time to time as well.

I think what would be even more useful at first is to find out why corrupted patches still apply.

And yet without any changes in the patch format, it should be possible to test the validity of a patch whenever the blob for the preimage SHA1 from the index line in the patch header is available locally. Just applying the patch to that blob and confirming it matches the postimage SHA1 should cover many cases already.

Nicolas

Nicolas Pitre <nico@fluxnic.net> writes:

Me three.

Exactly.  That is why I asked that question at the very beginning.

That would work when you are/have the sole authority (so contributors won't send patches based on some other trees), you push out often (to keep the length of the patch queue contributors keep short).

That would make it more likely that others base their work on what you published, and send patches from their base version all the way (not skipping "this is what I sent earlier but haven't been accepted nor pushed out"). Otherwise it will be unlikely that you have the object recorded as the preimage. Often when I receive follow-up patches from people, some are based on what was committed by me (possibly with tweaks), and some others are based on what was seen on the list (lacking the tweaks), yet some others are based on random other versions. I'll have preimages only in the first case.

Usually while editing incoming patch text (not log message), you mostly touch postimage, but when fixing up a diff that was based on a bit stale version, you need to touch preimage as well. In these cases, the blob object names recorded in the patch wouldn't be very useful.

The commit message getting rewrapped. For some reason, the patch itself was not corrupted.

Another case is that of the commit message having its non-ASCII characters corrupted.

The part I really care about is that git-format-patch should include a checksum by default.

I'd be quite happy if git-am only warned about a checksum mismatch.

Linus:

Shouldn't they remove the checksum line at the same time as they edit a patch?

                                        Juliusz

Juliusz Chroboczek <Juliusz.Chroboczek@pps.jussieu.fr> writes:

Why force extra work on people, especially the ones who _receive_ patches, when they say they _don't_ want to have such a noise by default?

Juliusz Chroboczek <Juliusz.Chroboczek@pps.jussieu.fr> writes:

You hopefully check the message (and of course the patch) before applying, so "automatically reject and require force" wouldn't help you much in this case. Either you reject and tell the sender to resend, or you reflow it yourself to save the sender (and yourself) the hassle of round-trip.

And if you choose to do the latter, having to force it would actively inconvenience you.

I've seen this one. It often is that the commit object records UTF-8, but somehow the MUA didn't mark it as such (or incorrectly marked it as ISO-8859-1), and mailinfo ended up doing unnecessary conversion. When this happens, not just the message but also the patch text is affected.

But to use "checksumming" as a solution for this, you need to think about what you checksum. Output from format-patch is "text/plain;charset=UTF-8" and requires 8-bit clean transport, but by cutting and pasting that into MUA you often end up with whatever MIME B/Q-quoting your MUA gives you, and mailinfo is actively unwraps them, so the right place to add an optional check _might_ be immediately after we run mailinfo. At that point, however, the author name and the subject line is in separate records from the body of the commit log message.

More realistically, the kind of MUA corruption I personally see most often is "text/plain; format-flowed"; I have a pre-applypatch hook to reject them altogether, but they tend to corrupt leading whitespaces so badly that the patch seldome applies. This is not something "checksumming" is the right tool to solve.

One thing that we _might_ consider doing is to reduce the default length of the function name we place on the hunk header line. For some reason, I see they get wrapped in messages I see, even when the proposed commit log message has overlong lines.