{"thread":{"id":"22401","subject":"git-format-patch should include a checksum","startedAt":"2010-01-26T22:34:56Z","lastAt":"2010-01-27T03:01:07Z","messageCount":11,"participants":["Juliusz Chroboczek","Sverre Rabbelier","Junio C Hamano","Linus Torvalds","Nicolas Pitre"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"132723","messageId":"871vhcmr5b.fsf@trurl.pps.jussieu.fr","threadId":"22401","inReplyTo":null,"subject":"git-format-patch should include a checksum","fromName":"Juliusz Chroboczek","fromEmail":"jch@pps.jussieu.fr","sentAt":"2010-01-26T22:34:56Z","receivedAt":"2010-01-26T22:34:56Z","isPatch":false,"sender":{"key":"jch@pps.jussieu.fr","avatar":null},"body":"Hi,\n\nI'm seeing Git patches being corrupted by mailers and still apply\ncorrectly.  It would be great if git-format-patch could include a hash\nof the patch body (and commit message); git-am should check the hash,\nand refuse to commit if the patch was corrupted (--force should override\nthat, of course).\n\n                                        Juliusz\n"},{"id":"132725","messageId":"fabb9a1e1001261515w44ccf7a4le6a49724164e902c@mail.gmail.com","threadId":"22401","inReplyTo":"871vhcmr5b.fsf@trurl.pps.jussieu.fr","subject":"Re: git-format-patch should include a checksum","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2010-01-26T23:15:28Z","receivedAt":"2010-01-26T23:15:28Z","isPatch":false,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Tue, Jan 26, 2010 at 23:34, Juliusz Chroboczek <jch@pps.jussieu.fr> wrote:\n> I'm seeing Git patches being corrupted by mailers and still apply\n> correctly.  It would be great if git-format-patch could include a hash\n> of the patch body (and commit message); git-am should check the hash,\n> and refuse to commit if the patch was corrupted (--force should override\n> that, of course).\n\nSounds like a good idea, have a look at cmd_format_patch in\nbuiltin-log.c. Assuming you want to add the hash as a X- mail header\n(e.g. X-Git-Message-Hash), you should probably add it before line 1019\n(where rev.extra_headers is set to the current contents of 'buf'). For\nthe hash itself have a look at git_SHA1_{Init,Update,Final} in various\nfiles, csum-file.c would seem like an excellent candidate, as it\nwrites a sha1-summed file, which is sortof what you want to do.\n\nFor the git-am part look at 'git-am.sh', you should have checking\nlogic warn only at first, and add a configuration option that allows\nenforcing it and warn when the option isn't set, instructing the user\nhow to configure the option, see warn_unconfigured_deny_msg in\nbuiltin-receive-pack.c. A few releases from now we can set it from\nwarn to abort (or deny, whatever the enforcing option is called).\n\nGood luck!\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"132726","messageId":"7vljfkxxj9.fsf@alter.siamese.dyndns.org","threadId":"22401","inReplyTo":"871vhcmr5b.fsf@trurl.pps.jussieu.fr","subject":"Re: git-format-patch should include a checksum","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-01-26T23:21:30Z","receivedAt":"2010-01-26T23:21:30Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Juliusz Chroboczek <jch@pps.jussieu.fr> writes:\n\n> I'm seeing Git patches being corrupted by mailers and still apply\n> correctly.  It would be great if git-format-patch could include a hash\n> of the patch body (and commit message); git-am should check the hash,\n> and refuse to commit if the patch was corrupted (--force should override\n> that, of course).\n\nDo you have an example of such corrupted and incorrectly applied patches?\nWhat kind of corruption are you talking about?\n\nformat-patch/am pair is designed to be lenient, allowing people to write\nadditional messages after the three-dash lines after the output is made\nbut before it is given to the MUA for sending the result out, for example,\nso adding a checksum over the entire output and forcing a check upon\napplication is really a bad idea, even though, provided if the patch is\ndone cleanly, it might be acceptable as an optional feature.\n"},{"id":"132727","messageId":"fabb9a1e1001261526tc86c04em4c6ede23e109e66@mail.gmail.com","threadId":"22401","inReplyTo":"7vljfkxxj9.fsf@alter.siamese.dyndns.org","subject":"Re: git-format-patch should include a checksum","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2010-01-26T23:26:44Z","receivedAt":"2010-01-26T23:26:44Z","isPatch":false,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Wed, Jan 27, 2010 at 00:21, Junio C Hamano <gitster@pobox.com> wrote:\n> format-patch/am pair is designed to be lenient, allowing people to write\n> additional messages after the three-dash lines after the output is made\n> but before it is given to the MUA for sending the result out, for example,\n> so adding a checksum over the entire output and forcing a check upon\n> application is really a bad idea, even though, provided if the patch is\n> done cleanly, it might be acceptable as an optional feature.\n\nI would imagine that the checksum is taken over just the actual commit\nmessage, perhaps author information, and use the patch-id for the\npatch itself, that way any comments after triple dash would be ignored, right?\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"132732","messageId":"alpine.LFD.2.00.1001261639550.17519@localhost.localdomain","threadId":"22401","inReplyTo":"fabb9a1e1001261526tc86c04em4c6ede23e109e66@mail.gmail.com","subject":"Re: git-format-patch should include a checksum","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2010-01-27T00:45:07Z","receivedAt":"2010-01-27T00:45:07Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Wed, 27 Jan 2010, Sverre Rabbelier wrote:\n> \n> I would imagine that the checksum is taken over just the actual commit\n> message, perhaps author information, and use the patch-id for the\n> patch itself, that way any comments after triple dash would be ignored, right?\n\nThat wouldn't work either. People can, should, and do add extra things to \nthe message before applying it.\n\nExamples of things I tend to add/change in the commit message:\n - add ack's from people in the same thread\n - add \"Cc: stable@kernel.org\" \n - re-flow paragraphs when somebody uses a mailer that makes a mess of it.\n - occasionally fix spelling and grammar\n\nso if there is some checksum that screws that up and requires me to then \nuse a \"--force\" flag to apply it, that would be a bad thing.\n\nI also do edit patches manually too. Having lived with people sending me \npatches for the last almost twenty years, I can edit patches in my sleep. \nDoing things like renaming new variables etc by search-and-replace on the \npatch may not be something I do _often_, but it happens.\n\nIn short, it might make sense to have some anti-corruption logic, but I \nsuspect it needs a lot of thought. \n\n\t\tLinus\n"},{"id":"132733","messageId":"fabb9a1e1001261650r18e04e3cw2efade6072a426b@mail.gmail.com","threadId":"22401","inReplyTo":"alpine.LFD.2.00.1001261639550.17519@localhost.localdomain","subject":"Re: git-format-patch should include a checksum","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2010-01-27T00:50:57Z","receivedAt":"2010-01-27T00:50:57Z","isPatch":false,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Wed, Jan 27, 2010 at 01:45, Linus Torvalds\n<torvalds@linux-foundation.org> wrote:\n> That wouldn't work either. People can, should, and do add extra things to\n> the message before applying it.\n\nAh, that's a fair point.\n\n> In short, it might make sense to have some anti-corruption logic, but I\n> suspect it needs a lot of thought.\n\nPerhaps it makes sense to make it a separate mode to git am, such that\nit only checks that the patch is not corrupted, but does not apply it.\nThat way it would be possible to download the patch, check that it\narrived unscathed, and then do your usual patch handling. Those who do\nnot edit patches before applying it would be convenient to set a\nconfiguration option that automatically does it when applying the\npatch, either warning about it or aborting (as Juliusz suggested).\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"132735","messageId":"alpine.LFD.2.00.1001262006150.1681@xanadu.home","threadId":"22401","inReplyTo":"fabb9a1e1001261650r18e04e3cw2efade6072a426b@mail.gmail.com","subject":"Re: git-format-patch should include a checksum","fromName":"Nicolas Pitre","fromEmail":"nico@fluxnic.net","sentAt":"2010-01-27T01:13:56Z","receivedAt":"2010-01-27T01:13:56Z","isPatch":false,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Wed, 27 Jan 2010, Sverre Rabbelier wrote:\n\n> Heya,\n> \n> On Wed, Jan 27, 2010 at 01:45, Linus Torvalds\n> <torvalds@linux-foundation.org> wrote:\n> > That wouldn't work either. People can, should, and do add extra things to\n> > the message before applying it.\n> \n> Ah, that's a fair point.\n\nFWIW, I do manually edit both incoming and outgoing patches from time to \ntime as well.\n\n> > In short, it might make sense to have some anti-corruption logic, but I\n> > suspect it needs a lot of thought.\n> \n> Perhaps it makes sense to make it a separate mode to git am, such that\n> it only checks that the patch is not corrupted, but does not apply it.\n> That way it would be possible to download the patch, check that it\n> arrived unscathed, and then do your usual patch handling. Those who do\n> not edit patches before applying it would be convenient to set a\n> configuration option that automatically does it when applying the\n> patch, either warning about it or aborting (as Juliusz suggested).\n\nI think what would be even more useful at first is to find out why \ncorrupted patches still apply.\n\nAnd yet without any changes in the patch format, it should be possible \nto test the validity of a patch whenever the blob for the preimage SHA1 \nfrom the index line in the patch header is available locally.  Just \napplying the patch to that blob and confirming it matches the postimage \nSHA1 should cover many cases already.\n\n\nNicolas\n"},{"id":"132739","messageId":"7ir5pccp9n.fsf@lanthane.pps.jussieu.fr","threadId":"22401","inReplyTo":"7vljfkxxj9.fsf@alter.siamese.dyndns.org","subject":"Re: git-format-patch should include a checksum","fromName":"Juliusz Chroboczek","fromEmail":"juliusz.chroboczek@pps.jussieu.fr","sentAt":"2010-01-27T01:25:40Z","receivedAt":"2010-01-27T01:25:40Z","isPatch":false,"sender":{"key":"juliusz.chroboczek@pps.jussieu.fr","avatar":null},"body":"> Do you have an example of such corrupted and incorrectly applied patches?\n> What kind of corruption are you talking about?\n\nThe commit message getting rewrapped.  For some reason, the patch itself\nwas not corrupted.\n\nAnother case is that of the commit message having its non-ASCII\ncharacters corrupted.\n\n> adding a checksum over the entire output and forcing a check upon\n> application is really a bad idea, even though, provided if the patch\n> is done cleanly, it might be acceptable as an optional feature.\n\nThe part I really care about is that git-format-patch should include\na checksum by default.\n\nI'd be quite happy if git-am only warned about a checksum mismatch.\n\nLinus:\n\n> That wouldn't work either. People can, should, and do add extra things to \n> the message before applying it.\n\nShouldn't they remove the checksum line at the same time as they edit\na patch?\n\n                                        Juliusz\n"},{"id":"132737","messageId":"7v4om8xrs7.fsf@alter.siamese.dyndns.org","threadId":"22401","inReplyTo":"alpine.LFD.2.00.1001262006150.1681@xanadu.home","subject":"Re: git-format-patch should include a checksum","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-01-27T01:25:44Z","receivedAt":"2010-01-27T01:25:44Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nicolas Pitre <nico@fluxnic.net> writes:\n\n> FWIW, I do manually edit both incoming and outgoing patches from time to \n> time as well.\n\nMe three.\n\n> I think what would be even more useful at first is to find out why \n> corrupted patches still apply.\n\nExactly.  That is why I asked that question at the very beginning.\n\n> ... Just \n> applying the patch to that blob and confirming it matches the postimage \n> SHA1 should cover many cases already.\n\nThat would work when you are/have the sole authority (so contributors\nwon't send patches based on some other trees), you push out often (to keep\nthe length of the patch queue contributors keep short).\n\nThat would make it more likely that others base their work on what you\npublished, and send patches from their base version all the way (not\nskipping \"this is what I sent earlier but haven't been accepted nor pushed\nout\").  Otherwise it will be unlikely that you have the object recorded as\nthe preimage.  Often when I receive follow-up patches from people, some\nare based on what was committed by me (possibly with tweaks), and some\nothers are based on what was seen on the list (lacking the tweaks), yet\nsome others are based on random other versions.  I'll have preimages only\nin the first case.\n\nUsually while editing incoming patch text (not log message), you mostly\ntouch postimage, but when fixing up a diff that was based on a bit stale\nversion, you need to touch preimage as well.  In these cases, the blob\nobject names recorded in the patch wouldn't be very useful.\n"},{"id":"132740","messageId":"7vvdeowcry.fsf@alter.siamese.dyndns.org","threadId":"22401","inReplyTo":"7ir5pccp9n.fsf@lanthane.pps.jussieu.fr","subject":"Re: git-format-patch should include a checksum","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-01-27T01:35:13Z","receivedAt":"2010-01-27T01:35:13Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Juliusz Chroboczek <Juliusz.Chroboczek@pps.jussieu.fr> writes:\n\n>> That wouldn't work either. People can, should, and do add extra things to \n>> the message before applying it.\n>\n> Shouldn't they remove the checksum line at the same time as they edit\n> a patch?\n\nWhy force extra work on people, especially the ones who _receive_ patches,\nwhen they say they _don't_ want to have such a noise by default?\n"},{"id":"132743","messageId":"7vljfkw8ss.fsf@alter.siamese.dyndns.org","threadId":"22401","inReplyTo":"7ir5pccp9n.fsf@lanthane.pps.jussieu.fr","subject":"Re: git-format-patch should include a checksum","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-01-27T03:01:07Z","receivedAt":"2010-01-27T03:01:07Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Juliusz Chroboczek <Juliusz.Chroboczek@pps.jussieu.fr> writes:\n\n>> Do you have an example of such corrupted and incorrectly applied patches?\n>> What kind of corruption are you talking about?\n>\n> The commit message getting rewrapped.  For some reason, the patch itself\n> was not corrupted.\n\nYou hopefully check the message (and of course the patch) before applying,\nso \"automatically reject and require force\" wouldn't help you much in this\ncase.  Either you reject and tell the sender to resend, or you reflow it\nyourself to save the sender (and yourself) the hassle of round-trip.\n\nAnd if you choose to do the latter, having to force it would actively\ninconvenience you.\n\n> Another case is that of the commit message having its non-ASCII\n> characters corrupted.\n\nI've seen this one.  It often is that the commit object records UTF-8, but\nsomehow the MUA didn't mark it as such (or incorrectly marked it as\nISO-8859-1), and mailinfo ended up doing unnecessary conversion.  When\nthis happens, not just the message but also the patch text is affected.\n\nBut to use \"checksumming\" as a solution for this, you need to think about\nwhat you checksum.  Output from format-patch is \"text/plain;charset=UTF-8\"\nand requires 8-bit clean transport, but by cutting and pasting that into\nMUA you often end up with whatever MIME B/Q-quoting your MUA gives you,\nand mailinfo is actively unwraps them, so the right place to add an\noptional check _might_ be immediately after we run mailinfo.  At that\npoint, however, the author name and the subject line is in separate\nrecords from the body of the commit log message.\n\nMore realistically, the kind of MUA corruption I personally see most often\nis \"text/plain; format-flowed\"; I have a pre-applypatch hook to reject\nthem altogether, but they tend to corrupt leading whitespaces so badly\nthat the patch seldome applies.  This is not something \"checksumming\" is\nthe right tool to solve.\n\nOne thing that we _might_ consider doing is to reduce the default length\nof the function name we place on the hunk header line.  For some reason, I\nsee they get wrapped in messages I see, even when the proposed commit log\nmessage has overlong lines.\n"}]}