{"thread":{"id":"58341","subject":"`git patch-id --stable' vs quoted-printable","startedAt":"2022-08-22T02:25:09Z","lastAt":"2022-08-22T18:25:17Z","messageCount":8,"participants":["Eric Wong","Junio C Hamano","René Scharfe"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"461741","messageId":"20220822022503.M873583@dcvr","threadId":"58341","inReplyTo":null,"subject":"`git patch-id --stable' vs quoted-printable","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2022-08-22T02:25:03Z","receivedAt":"2022-08-22T02:25:09Z","isPatch":false,"sender":{"key":"e@80x24.org","avatar":null},"body":"While poking around at the newish patchid indexing support in\npublic-inbox[1], I noticed an inconsistency in how it seems to\nmishandle quoted-printable messages.\n\nFor instance, René's message can apply fine with `git am':\n\nhttps://public-inbox.org/git/6727daf1-f077-7319-187e-ab4e55de3b2d@web.de/raw\n\nHowever, note the blank context lines are truly blank, as in /^$/,\nnot /^ $/. Running `git patch-id --stable' on the decoded content\nof that message gives me: a245e99dbd2ce6e319955569eb8a111cb044f474\nAnd that's the value public-inbox indexes.\n\nThat patch ends up being commit fc0f8bcd64eb0a03a7098f72da9c4008bd48cf11\nin git.git.\n\nHowever,\n`git show fc0f8bcd64eb0a03a7098f72da9c4008bd48cf11 |git patch-id --stable'\ngives me: fcac4cf581e11b703c229a129072c95c79b68bf\n\nSo, I'm wondering if the search indexing code of public-inbox\nshould s/^$/ /mgs before feeding stuff to `git patch-id'; and/or\nif `git patch-id' should be assuming empty lines and lines with a\nsingle SP are the same...\n\n\n[1] https://public-inbox.org/meta/20220620192730.550803-3-e@80x24.org/\n"},{"id":"461743","messageId":"xmqqczcsgbvn.fsf@gitster.g","threadId":"58341","inReplyTo":"20220822022503.M873583@dcvr","subject":"Re: `git patch-id --stable' vs quoted-printable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-22T04:06:52Z","receivedAt":"2022-08-22T04:07:01Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Wong writes:\n\n> While poking around at the newish patchid indexing support in\n> public-inbox[1], I noticed an inconsistency in how it seems to\n> mishandle quoted-printable messages.\n> ...\n> So, I'm wondering if the search indexing code of public-inbox\n> should s/^$/ /mgs before feeding stuff to `git patch-id'; and/or\n> if `git patch-id' should be assuming empty lines and lines with a\n> single SP are the same...\n\nI suspect that QP is a red herring. I haven't looked at relevant\ncode at all for a while, but what I think is going on is:\n\n * patch-id algorithm was written back when \"unified\" format of\n \"diff\" did not have the extension of GNU origin to allow an empty\n context line to be expressed as a truely empty line, not a single\n whitespace that signals it is a context line, followed by the\n contents of the line that is empty\n\n * \"git apply\" hence \"git am\" was taught to grok the empty context\n line extention, https://pubs.opengroup.org/onlinepubs/9699919799/utilities/diff.html\n has this:\n\n It is implementation-defined whether an empty unaffected line is\n written as an empty line or a line containing a single character.\n\n IIRC, this was added after GNU diff started emitting such an\n output (--suppress-blank-empty) and people complained that such a\n patch is not understood by us.\n\n * \"git diff\" was updated to allow this with diff.suppressBlankEmpty\n configuration , but that is never turned on by default.\n\nSo, if a patch producer runs \"git diff\" with diff.suppressBlankEmpty\nturned on, \"git am\" accepts it, and then you run \"git show\" without\nthe configuration, then the \"shape\" of the patch text would be\nslightly different. I do not offhand know if we added configuration\nsupport to \"patch-id\", but even with a configuration knob, because\nonce you turn incoming e-mail into a commit, the single bit (i.e.\nwhether suppressBlankEmpty was in use or not) is forever lost, it\nwould not be of much help. After all, the incoming patch can be\nhand munged to use both \"single whitespace and the end of line\" and\n\"a completely empty line\" to record an empty context line, and \"am\"\nhas to take such a patch happily.\n\nI *think* the right thing to do is for patch-id that takes text\ninput to normalize the empty context line into one form or the other\n(as a conservatist, I would say we should probably pretend as if an\nempty context line is always expressed as a single whitespace on a\nline by itself) before computing the ID.\n\nRené, do you remember if you used diff.suppressBlankEmpty\nconfiguration when generating the patch in question at:\n\n https://public-inbox.org/git/6727daf1-f077-7319-187e-ab4e55de3b2d@web.de/raw\n\nby the way?\n\nThanks.\n"},{"id":"461744","messageId":"xmqq7d30gbcd.fsf@gitster.g","threadId":"58341","inReplyTo":"xmqqczcsgbvn.fsf@gitster.g","subject":"Re: `git patch-id --stable' vs quoted-printable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-22T04:18:26Z","receivedAt":"2022-08-22T04:18:39Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> So, I'm wondering if the search indexing code of public-inbox\n>> should s/^$/ /mgs before feeding stuff to `git patch-id'; and/or\n>> if `git patch-id' should be assuming empty lines and lines with a\n>> single SP are the same...\n\nAnother potential source of issues (not for the patch from René that\nwas used as an example) is that a patch producer can use different\ndiff algorithm from the setting you would use to index resulting\ncommits via \"git show | git patch-id\".\n\n $ git show -U5 | git patch-id\n $ git show | git patch-id\n\nlikely result in different patch IDs. \n\nThe --patience and the --histogram options affect how common lines\nare matched up, again affecting the shape of the patches you compute\npatch-ids over.\n"},{"id":"461745","messageId":"20220822045728.GA453@dcvr","threadId":"58341","inReplyTo":"xmqq7d30gbcd.fsf@gitster.g","subject":"Re: `git patch-id --stable' vs quoted-printable","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2022-08-22T04:57:28Z","receivedAt":"2022-08-22T04:57:31Z","isPatch":false,"sender":{"key":"e@80x24.org","avatar":null},"body":"Junio C Hamano wrote:\n> Junio C Hamano writes:\n> \n> >> So, I'm wondering if the search indexing code of public-inbox\n> >> should s/^$/ /mgs before feeding stuff to `git patch-id'; and/or\n> >> if `git patch-id' should be assuming empty lines and lines with a\n> >> single SP are the same...\n> \n> Another potential source of issues (not for the patch from René that\n> was used as an example) is that a patch producer can use different\n> diff algorithm from the setting you would use to index resulting\n> commits via \"git show | git patch-id\".\n> \n> $ git show -U5 | git patch-id\n> $ git show | git patch-id\n> \n> likely result in different patch IDs. \n> \n> The --patience and the --histogram options affect how common lines\n> are matched up, again affecting the shape of the patches you compute\n> patch-ids over.\n\nYes, you're right; though I suppose most users use git defaults.\nI'm not sure how useful patchid ends up being, actually...\nThe old use of dfpre/dfpost blob OIDs seems to have been working\nwell for years, already.\n\nPossibly OR-ing them with patchid in Xapian will end up getting\ngood enough search engine coverage.\n"},{"id":"461770","messageId":"4e3edfa1-9c04-7a63-30ee-65fe6f2de62f@web.de","threadId":"58341","inReplyTo":"xmqqczcsgbvn.fsf@gitster.g","subject":"Re: `git patch-id --stable' vs quoted-printable","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-08-22T15:58:36Z","receivedAt":"2022-08-22T15:58:53Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 22.08.22 um 06:06 schrieb Junio C Hamano:\n> René, do you remember if you used diff.suppressBlankEmpty\n> configuration when generating the patch in question at:\n>\n> https://public-inbox.org/git/6727daf1-f077-7319-187e-ab4e55de3b2d@web.de/raw\n>\n> by the way?\n\nI did not use that option. Attached the copy from my Sent folder, which\nhas spaces at the start of the blank lines in the config.txt hunk.\n\nRené\n\n\nAm 22.05.22 um 06:32 schrieb Junio C Hamano:\n> ---\n> Documentation/config/revert.txt | 3 +++\n> Documentation/git-revert.txt | 9 +++++++++\n> builtin/revert.c | 2 ++\n> sequencer.c | 32 +++++++++++++++++++++++++++-----\n> sequencer.h | 1 +\n> t/t3501-revert-cherry-pick.sh | 31 +++++++++++++++++++++++++++++++\n> 6 files changed, 73 insertions(+), 5 deletions(-)\n> create mode 100644 Documentation/config/revert.txt\n>\n> diff --git a/Documentation/config/revert.txt b/Documentation/config/revert.txt\n> new file mode 100644\n> index 0000000000..797bfb6d62\n> --- /dev/null\n> +++ b/Documentation/config/revert.txt\n> @@ -0,0 +1,3 @@\n> +revert.reference::\n> +\tSetting this variable to true makes `git revert` to behave\n> +\tas if the `--reference` option is given.\n\nShouldn't this be \"were\" instead of \"is\"? Not fully sure. Anyway:\n\n--- >8 ---\nSubject: [PATCH] revert: config documentation fixes\n\n43966ab315 (revert: optionally refer to commit in the \"reference\"\nformat, 2022-05-26) added the documentation file config/revert.txt.\nActually include it in config.txt.\n\nMake is used with a bare infinitive after the object; remove the \"to\".\n\nSigned-off-by: René Scharfe \n---\n Documentation/config.txt | 2 ++\n Documentation/config/revert.txt | 2 +-\n 2 files changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex e284b042f2..e376d547ce 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -495,6 +495,8 @@ include::config/repack.txt[]\n\n include::config/rerere.txt[]\n\n+include::config/revert.txt[]\n+\n include::config/safe.txt[]\n\n include::config/sendemail.txt[]\ndiff --git a/Documentation/config/revert.txt b/Documentation/config/revert.txt\nindex 797bfb6d62..802d6faca2 100644\n--- a/Documentation/config/revert.txt\n+++ b/Documentation/config/revert.txt\n@@ -1,3 +1,3 @@\n revert.reference::\n-\tSetting this variable to true makes `git revert` to behave\n+\tSetting this variable to true makes `git revert` behave\n \tas if the `--reference` option is given.\n--\n2.36.1\n"},{"id":"461776","messageId":"xmqqmtbwdzbj.fsf@gitster.g","threadId":"58341","inReplyTo":"4e3edfa1-9c04-7a63-30ee-65fe6f2de62f@web.de","subject":"Re: `git patch-id --stable' vs quoted-printable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-22T16:21:04Z","receivedAt":"2022-08-22T16:21:11Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> Am 22.08.22 um 06:06 schrieb Junio C Hamano:\n>> René, do you remember if you used diff.suppressBlankEmpty\n>> configuration when generating the patch in question at:\n>>\n>> https://public-inbox.org/git/6727daf1-f077-7319-187e-ab4e55de3b2d@web.de/raw\n>>\n>> by the way?\n>\n> I did not use that option. Attached the copy from my Sent folder, which\n> has spaces at the start of the blank lines in the config.txt hunk.\n\nHmph, so I cannot quite explain what removed the leading spaces from\nthe copy public-inbox archived.\n\nThanks.\n"},{"id":"461777","messageId":"20220822170114.M247768@dcvr","threadId":"58341","inReplyTo":"xmqqmtbwdzbj.fsf@gitster.g","subject":"Re: `git patch-id --stable' vs quoted-printable","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2022-08-22T17:01:14Z","receivedAt":"2022-08-22T17:02:09Z","isPatch":false,"sender":{"key":"e@80x24.org","avatar":null},"body":"Junio C Hamano wrote:\n> René Scharfe writes:\n> \n> > Am 22.08.22 um 06:06 schrieb Junio C Hamano:\n> >> René, do you remember if you used diff.suppressBlankEmpty\n> >> configuration when generating the patch in question at:\n> >>\n> >> https://public-inbox.org/git/6727daf1-f077-7319-187e-ab4e55de3b2d@web.de/raw\n> >>\n> >> by the way?\n> >\n> > I did not use that option. Attached the copy from my Sent folder, which\n> > has spaces at the start of the blank lines in the config.txt hunk.\n> \n> Hmph, so I cannot quite explain what removed the leading spaces from\n> the copy public-inbox archived.\n\nBoth the June 26 message and the latest which arrived directly\nin my IMAP inbox (w/o public-inbox) are missing the space in\nblank lines, matching what public-inbox.org and lore have.\n\nI don't think postfix, SpamAssassin, or dovecot strip that..\n\nI would expect the blank context lines in the QP version to have\n\"=20\". I sent this to test@public-inbox.org using\n`git send-email --transfer-encoding=quoted-printable' and it\nconfirms the blank context lines having \"=20\" which are missing\nin René's messages:\nhttps://try.public-inbox.org/test/20220822165736.23246-1-e@80x24.org/raw\n"},{"id":"461787","messageId":"xmqqtu64cf08.fsf@gitster.g","threadId":"58341","inReplyTo":"20220822170114.M247768@dcvr","subject":"Re: `git patch-id --stable' vs quoted-printable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-22T18:25:11Z","receivedAt":"2022-08-22T18:25:17Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Wong writes:\n\n> I don't think postfix, SpamAssassin, or dovecot strip that..\n> ...\n> I would expect the blank context lines in the QP version to have\n> \"=20\". I sent this to test@public-inbox.org using\n> `git send-email --transfer-encoding=quoted-printable' and it\n> confirms the blank context lines having \"=20\" which are missing\n> in René's messages:\n> https://try.public-inbox.org/test/20220822165736.23246-1-e@80x24.org/raw\n\nYeah, that is a mystery. Unlike format=flawed, I think CTE-QP was\ndesigned to be reversible without information loss.\n"}]}