{"thread":{"id":"59760","subject":"bug report: cover letter is inheriting last patch's message ID with send-email","startedAt":"2023-05-17T18:38:24Z","lastAt":"2023-05-18T01:06:53Z","messageCount":10,"participants":["Emily Shaffer","Junio C Hamano","Doug Anderson","Michael Strawbridge"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"477471","messageId":"CAJoAoZ=GGgjGOeaeo6RFBO7=6msdRf-Ze6XcnL04K5ugupLUJA@mail.gmail.com","threadId":"59760","inReplyTo":null,"subject":"bug report: cover letter is inheriting last patch's message ID with send-email","fromName":"Emily Shaffer","fromEmail":"nasamuffin@google.com","sentAt":"2023-05-17T18:38:01Z","receivedAt":"2023-05-17T18:38:24Z","isPatch":false,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"Following is a report from inside of Google:\n\n**What did you do before the bug happened? (Steps to reproduce your issue)**\n\n```\n# With the attached patches, where all of the patches have a\n# Message-Id but the cover letter doesn't.\ngit send-email *.patch\n```\n\nSpecifically, you can see me doing it:\n\n```\n$ git send-email *.patch\n0000-cover-letter.patch\n0001-dt-bindings-interrupt-controller-arm-gic-v3-Add-quir.patch\n0002-irqchip-gic-v3-Disable-pseudo-NMIs-on-Mediatek-devic.patch\n0003-arm64-dts-mediatek-mt8183-Add-mediatek-gicr-save-qui.patch\n0004-arm64-dts-mediatek-mt8186-Add-mediatek-gicr-save-qui.patch\n0005-arm64-dts-mediatek-mt8192-Add-mediatek-gicr-save-qui.patch\n0006-arm64-dts-mediatek-mt8195-Add-mediatek-gicr-save-qui.patch\nTo whom should the emails be sent (if anyone)?\nMessage-ID to be used as In-Reply-To for the first email (if any)?\n(mbox) Adding cc: Douglas Anderson from line\n'From: Douglas Anderson '\n\nFrom: Douglas Anderson \nTo:\nCc: Douglas Anderson \nSubject: [PATCH 0/6] irqchip/gic-v3: Disable pseudo NMIs on Mediatek\nChromebooks w/ bad FW\nDate: Thu, 11 May 2023 15:25:55 -0700\nMessage-ID: <20230511151719.6.Ia0b6ebbaa351e3cd67e201355b9ae67783c7d718@changeid>\n```\n\nIf you look at `0000-cover-letter.patch` you can see that it has no\nMessage-ID, but the above clearly shows that the cover letter is being\nsent with a Message-ID (and the one from the last patch).\n\n\n**What did you expect to happen? (Expected behavior)**\n\nThe cover letter should get an auto-generated Message-Id like it always used to.\n\n**What happened instead? (Actual behavior)**\n\nThe cover letter ends up with the same Message-Id as the last patch.\n\n**What's different between what you expected and what actually happened?**\n\nSee above.\n\n**Anything else you want to add:**\n\nThis happens when using the `patman` tool to send patches with a cover\nletter. The individual patches encode the \"Change-Id\" in the\nMessage-ID but the cover letter doesn't need any special Message-Id.\n\nI didn't notice this until after I sent a patch series and thus:\n\nhttps://lore.kernel.org/r/20230511150539.6.Ia0b6ebbaa351e3cd67e201355b9ae67783c7d718@changeid/\n\n...refers to both my cover letter and the last patch in the series.\n\n**Please review the rest of the bug report below.\nYou can delete any lines you don't wish to share.**\n\n```\n[System Info]\ngit version:\ngit version 2.40.1.606.ga4b1b128d6-goog\ncpu: x86_64\nno commit associated with this build\nsizeof-long: 8\nsizeof-size_t: 8\nshell-path: /bin/sh\nuname: Linux 6.1.15-1rodete3-amd64 #1 SMP PREEMPT_DYNAMIC Debian\n6.1.15-1rodete3 (2023-03-28) x86_64\ncompiler info: gnuc: 12.2\nlibc info: glibc: 2.36\n$SHELL (typically, interactive shell): /bin/bash\n\n\n[Enabled Hooks]\ncommit-msg\npre-auto-gc\npre-commit\nprepare-commit-msg\n```\n\nThe report references an internal build based on 2.40.1, but we were\nable to reproduce on upstream git version 2.41.0.rc0.4.g004e0f790f\n(without using `patman`).\n\nNote that the internal report did come with the output of `git\nformat-patch` as a zipped attachment, which I tried to attach and am\nhoping won't get filtered. As described in the report, the patches\nthemselves already contain Message-IDs, but the cover letter does not.\nAt the mail linked in the bug, you can notice that patch 6 has two\nMessage-IDs for some reason, and the one that's clearly not generated\nby the `patman` helper is appended to patch 6 instead of patch 0.\n\nBisecting shows that this bug was introduced at a8022c5f7b\n(send-email: expose header information to git-send-email's\nsendemail-validate hook, 2023-04-19), thanks Josh for bisecting it.\n\n - Emily\n"},{"id":"477473","messageId":"xmqqo7mipyt0.fsf@gitster.g","threadId":"59760","inReplyTo":"CAJoAoZ=GGgjGOeaeo6RFBO7=6msdRf-Ze6XcnL04K5ugupLUJA@mail.gmail.com","subject":"Re: bug report: cover letter is inheriting last patch's message ID with send-email","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-05-17T19:01:31Z","receivedAt":"2023-05-17T19:01:37Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Emily Shaffer writes:\n\n> Following is a report from inside of Google:\n>\n> **What did you do before the bug happened? (Steps to reproduce your issue)**\n>\n> ```\n> # With the attached patches, where all of the patches have a\n> # Message-Id but the cover letter doesn't.\n> git send-email *.patch\n> ```\n>\n> Specifically, you can see me doing it:\n>\n> ```\n> $ git send-email *.patch\n> 0000-cover-letter.patch\n> 0001-dt-bindings-interrupt-controller-arm-gic-v3-Add-quir.patch\n> ...\n> 0006-arm64-dts-mediatek-mt8195-Add-mediatek-gicr-save-qui.patch\n> To whom should the emails be sent (if anyone)?\n> Message-ID to be used as In-Reply-To for the first email (if any)?\n> (mbox) Adding cc: Douglas Anderson from line\n> 'From: Douglas Anderson '\n>\n> From: Douglas Anderson \n> To:\n> Cc: Douglas Anderson \n> Subject: [PATCH 0/6] irqchip/gic-v3: Disable pseudo NMIs on Mediatek\n> Chromebooks w/ bad FW\n> Date: Thu, 11 May 2023 15:25:55 -0700\n> Message-ID: <20230511151719.6.Ia0b6ebbaa351e3cd67e201355b9ae67783c7d718@changeid>\n> ```\n>\n> If you look at `0000-cover-letter.patch` you can see that it has no\n> Message-ID, but the above clearly shows that the cover letter is being\n> sent with a Message-ID (and the one from the last patch).\n\nIt is correct that Message-ID needs to be assigned by send-email if\nthe outgoing message lacks one. I am not sure what is meant by\n\"from the last patch\". Do you mean that Message-ID exists in\n0006-*.patch but not in 0000-cover-letter.patch [*]? I suspect that\nis the root cause of the problem; if 000[1-6]-*.patch already has\ntheir own Message-ID: because --thread is used when running\ngit-format-patch, they would also have In-Reply-To: and References:,\nbut there is no way for them to reference 0000-cover-letter.patch\n(because format-patch did not get a chance to generate Message-ID to\nit), is there?\n\nIs this because format-patch was used without --cover-letter but\nwith --thread to prepare 000[1-6]*.patch and the cover letter was\ncreated separately, or something?\n\nThe simplest fix I can think of is to stop using Message-ID related\noptions when running format-patch, and let send-email do the\nthreading. It would avoid problems coming from mixing output from\nmultiple format-patch runs.\n\n\n[Footnote]\n\n * As a reproduction recipe, the report should tell how these files\n were prepared (format-patch with what arguments to get there).\n"},{"id":"477475","messageId":"xmqqjzx6pxuu.fsf@gitster.g","threadId":"59760","inReplyTo":"xmqqo7mipyt0.fsf@gitster.g","subject":"Re: bug report: cover letter is inheriting last patch's message ID with send-email","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-05-17T19:22:01Z","receivedAt":"2023-05-17T19:22:12Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> # With the attached patches, where all of the patches have a\n>> # Message-Id but the cover letter doesn't.\n>> git send-email *.patch\n\nI suspect this is a recent regression with the addition of the\npre_process_file step. 56adddaa (send-email: refactor header\ngeneration functions, 2023-04-19) makes all messages parsed\nbefore the first message is sent out, by calling a sub\n\"pre_process_file\" before invoking the validate hook. The same sub\nis called again for each message when it is sent out, as the\nprocessing in that step is shared between the time the message gets\nvetted and the time the message gets sent.\n\nUnfortunately, $message_id variable is assigned to in that sub. So\nit is very much understandable why this happens.\n\nI wonder if it is just doing something silly like this?\n\n--- >8 ---\nSubject: [PATCH] send-email: clear the $message_id after validation\n\nRecently git-send-email started parsing the same message twice, once\nto validate _all_ the message before sending even the first one, and\nthen after the validation hook is happy and each message gets sent,\nto read the contents to find out where to send to etc.\n\nUnfortunately, the effect of reading the messages for validation\nlingered even after the validation is done. Namely $message_id gets\nassigned if exists in the input files but the variable is global,\nand it is not cleared before pre_process_file runs. This causes\nreading a message without a message-id followed by reading a message\nwith a message-id to misbehave---the sub reports as if the message\nhad the same id as the previously written one.\n\nClear the variable before starting to read the headers in\npre_process_file\n\nSigned-off-by: Junio C Hamano \n---\n\n * I am not surprised at all if there are similar problems in this\n function around variables other than $message_id; this patch is\n merely reacting to the bug report and not systematically hunting\n and fixing the bugs coming from the same root cause. If the\n original author of the pre_process_file change is still around,\n the second sets of eyes from them is very much appreciated.\n\n git-send-email.perl | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git c/git-send-email.perl w/git-send-email.perl\nindex 89d8237e89..889ef388c8 100755\n--- c/git-send-email.perl\n+++ w/git-send-email.perl\n@@ -1771,6 +1771,7 @@ sub send_message {\n sub pre_process_file {\n \tmy ($t, $quiet) = @_;\n \n+\tundef $message_id;\n \topen my $fh, \"<\", $t or die sprintf(__(\"can't open file %s\"), $t);\n \n \tmy $author = undef;\n"},{"id":"477476","messageId":"CAD=FV=XnzFrczC1dvsHYgNabZMhC7-K1uG8=MH20qNE25o0CEA@mail.gmail.com","threadId":"59760","inReplyTo":"xmqqo7mipyt0.fsf@gitster.g","subject":"Re: bug report: cover letter is inheriting last patch's message ID with send-email","fromName":"Doug Anderson","fromEmail":"dianders@chromium.org","sentAt":"2023-05-17T19:24:35Z","receivedAt":"2023-05-17T19:25:34Z","isPatch":false,"sender":{"key":"dianders@chromium.org","avatar":null},"body":"Hi,\n\nOn Wed, May 17, 2023 at 12:01 PM Junio C Hamano wrote:\n>\n> Emily Shaffer writes:\n>\n> > Following is a report from inside of Google:\n> >\n> > **What did you do before the bug happened? (Steps to reproduce your issue)**\n> >\n> > ```\n> > # With the attached patches, where all of the patches have a\n> > # Message-Id but the cover letter doesn't.\n> > git send-email *.patch\n> > ```\n> >\n> > Specifically, you can see me doing it:\n> >\n> > ```\n> > $ git send-email *.patch\n> > 0000-cover-letter.patch\n> > 0001-dt-bindings-interrupt-controller-arm-gic-v3-Add-quir.patch\n> > ...\n> > 0006-arm64-dts-mediatek-mt8195-Add-mediatek-gicr-save-qui.patch\n> > To whom should the emails be sent (if anyone)?\n> > Message-ID to be used as In-Reply-To for the first email (if any)?\n> > (mbox) Adding cc: Douglas Anderson from line\n> > 'From: Douglas Anderson '\n> >\n> > From: Douglas Anderson \n> > To:\n> > Cc: Douglas Anderson \n> > Subject: [PATCH 0/6] irqchip/gic-v3: Disable pseudo NMIs on Mediatek\n> > Chromebooks w/ bad FW\n> > Date: Thu, 11 May 2023 15:25:55 -0700\n> > Message-ID: <20230511151719.6.Ia0b6ebbaa351e3cd67e201355b9ae67783c7d718@changeid>\n> > ```\n> >\n> > If you look at `0000-cover-letter.patch` you can see that it has no\n> > Message-ID, but the above clearly shows that the cover letter is being\n> > sent with a Message-ID (and the one from the last patch).\n>\n> It is correct that Message-ID needs to be assigned by send-email if\n> the outgoing message lacks one. I am not sure what is meant by\n> \"from the last patch\". Do you mean that Message-ID exists in\n> 0006-*.patch but not in 0000-cover-letter.patch [*]?\n\nYes. It exists in all of the patches except 0000-cover-letter.patch.\n...but when the mail gets actually sent the cover letter and last\npatch (0006 in the case I reported) end up sharing the same Change ID.\nWith older versions of git send-email the cover letter would get an\nauto-generated Message-Id.\n\n\n> I suspect that\n> is the root cause of the problem; if 000[1-6]-*.patch already has\n> their own Message-ID: because --thread is used when running\n> git-format-patch, they would also have In-Reply-To: and References:,\n> but there is no way for them to reference 0000-cover-letter.patch\n> (because format-patch did not get a chance to generate Message-ID to\n> it), is there?\n\nThe patches were generated with git-format-patch but the Message-ID\nwas added by patman [1]. The Message-ID encodes the local Change-Id\nwhich can make it easier to associate one version of the same patch\nwith another (same reason gerrit uses Change-Id) [2]. There is no\nChange-Id associated with the cover letter so patman doesn't bother\nadding one there and has always just let it be auto-generated. We\ncould certainly change patman to make up a Message-Id for the cover\nletter, but there is no real need.\n\n[1] https://source.denx.de/u-boot/u-boot/-/blob/master/tools/patman/patman.rst\n[2] https://source.denx.de/u-boot/u-boot/-/commit/833e4192cd791733ddc0106996a4f86f9269ceba\n"},{"id":"477481","messageId":"xmqqfs7upvvw.fsf@gitster.g","threadId":"59760","inReplyTo":"CAD=FV=XnzFrczC1dvsHYgNabZMhC7-K1uG8=MH20qNE25o0CEA@mail.gmail.com","subject":"Re: bug report: cover letter is inheriting last patch's message ID with send-email","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-05-17T20:04:35Z","receivedAt":"2023-05-17T20:04:40Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Doug Anderson writes:\n\n> Yes. It exists in all of the patches except 0000-cover-letter.patch.\n> ...but when the mail gets actually sent the cover letter and last\n> patch (0006 in the case I reported) end up sharing the same Change ID.\n> With older versions of git send-email the cover letter would get an\n> auto-generated Message-Id.\n\nYeah, I think the patch I sent in the thread should help; I'd\nappreciate it if you folks can test and verify.\n\n>> I suspect that\n>> is the root cause of the problem; if 000[1-6]-*.patch already has\n>> their own Message-ID: because --thread is used when running\n>> git-format-patch, they would also have In-Reply-To: and References:,\n>> but there is no way for them to reference 0000-cover-letter.patch\n>> (because format-patch did not get a chance to generate Message-ID to\n>> it), is there?\n>\n> The patches were generated with git-format-patch but the Message-ID\n> was added by patman [1]. The Message-ID encodes the local Change-Id\n> which can make it easier to associate one version of the same patch\n> with another (same reason gerrit uses Change-Id) [2]. There is no\n> Change-Id associated with the cover letter so patman doesn't bother\n> adding one there and has always just let it be auto-generated.\n\n> We\n> could certainly change patman to make up a Message-Id for the cover\n> letter, but there is no real need.\n\nThis is a tangent, as I think the earlier patch should fix the\nregression, but wouldn't a recipient of such a series have a hard\ntime to locate and group the patches in the same series with the\ncover letter, without having In-Reply-To: or References: that links\nthe later message back to the initial message (i.e. cover letter)?\nAssigning a Message-ID to the cover, and referencing it from the\npatches via In-Reply-To:, is what is commonly done, I think, for\nthat kind of threading.\n\nThanks.\n"},{"id":"477482","messageId":"CAD=FV=UkZBQ6SFB7xu8OD3vxtODp6RUq=K3xXzofpJjUZO18+w@mail.gmail.com","threadId":"59760","inReplyTo":"xmqqjzx6pxuu.fsf@gitster.g","subject":"Re: bug report: cover letter is inheriting last patch's message ID with send-email","fromName":"Doug Anderson","fromEmail":"dianders@chromium.org","sentAt":"2023-05-17T20:14:55Z","receivedAt":"2023-05-17T20:15:15Z","isPatch":false,"sender":{"key":"dianders@chromium.org","avatar":null},"body":"Hi,\n\nOn Wed, May 17, 2023 at 12:22 PM Junio C Hamano wrote:\n>\n> Junio C Hamano writes:\n>\n> >> # With the attached patches, where all of the patches have a\n> >> # Message-Id but the cover letter doesn't.\n> >> git send-email *.patch\n>\n> I suspect this is a recent regression with the addition of the\n> pre_process_file step. 56adddaa (send-email: refactor header\n> generation functions, 2023-04-19) makes all messages parsed\n> before the first message is sent out, by calling a sub\n> \"pre_process_file\" before invoking the validate hook. The same sub\n> is called again for each message when it is sent out, as the\n> processing in that step is shared between the time the message gets\n> vetted and the time the message gets sent.\n>\n> Unfortunately, $message_id variable is assigned to in that sub. So\n> it is very much understandable why this happens.\n>\n> I wonder if it is just doing something silly like this?\n>\n> --- >8 ---\n> Subject: [PATCH] send-email: clear the $message_id after validation\n>\n> Recently git-send-email started parsing the same message twice, once\n> to validate _all_ the message before sending even the first one, and\n> then after the validation hook is happy and each message gets sent,\n> to read the contents to find out where to send to etc.\n>\n> Unfortunately, the effect of reading the messages for validation\n> lingered even after the validation is done. Namely $message_id gets\n> assigned if exists in the input files but the variable is global,\n> and it is not cleared before pre_process_file runs. This causes\n> reading a message without a message-id followed by reading a message\n> with a message-id to misbehave---the sub reports as if the message\n> had the same id as the previously written one.\n>\n> Clear the variable before starting to read the headers in\n> pre_process_file\n>\n> Signed-off-by: Junio C Hamano \n> ---\n>\n> * I am not surprised at all if there are similar problems in this\n> function around variables other than $message_id; this patch is\n> merely reacting to the bug report and not systematically hunting\n> and fixing the bugs coming from the same root cause. If the\n> original author of the pre_process_file change is still around,\n> the second sets of eyes from them is very much appreciated.\n>\n> git-send-email.perl | 1 +\n> 1 file changed, 1 insertion(+)\n>\n> diff --git c/git-send-email.perl w/git-send-email.perl\n> index 89d8237e89..889ef388c8 100755\n> --- c/git-send-email.perl\n> +++ w/git-send-email.perl\n> @@ -1771,6 +1771,7 @@ sub send_message {\n> sub pre_process_file {\n> my ($t, $quiet) = @_;\n>\n> + undef $message_id;\n> open my $fh, \"<\", $t or die sprintf(__(\"can't open file %s\"), $t);\n>\n> my $author = undef;\n\nI can confirm this fixes the regression for me. Thus:\n\nTested-by: Douglas Anderson \n"},{"id":"477484","messageId":"CAD=FV=WxB8bqKZSkGQYuPxJ6BeA5fbNWcwfiTyhktn83bzCHfg@mail.gmail.com","threadId":"59760","inReplyTo":"xmqqfs7upvvw.fsf@gitster.g","subject":"Re: bug report: cover letter is inheriting last patch's message ID with send-email","fromName":"Doug Anderson","fromEmail":"dianders@chromium.org","sentAt":"2023-05-17T20:20:55Z","receivedAt":"2023-05-17T20:21:15Z","isPatch":false,"sender":{"key":"dianders@chromium.org","avatar":null},"body":"Hi,\n\nOn Wed, May 17, 2023 at 1:04 PM Junio C Hamano wrote:\n>\n> Doug Anderson writes:\n>\n> > Yes. It exists in all of the patches except 0000-cover-letter.patch.\n> > ...but when the mail gets actually sent the cover letter and last\n> > patch (0006 in the case I reported) end up sharing the same Change ID.\n> > With older versions of git send-email the cover letter would get an\n> > auto-generated Message-Id.\n>\n> Yeah, I think the patch I sent in the thread should help; I'd\n> appreciate it if you folks can test and verify.\n\nYup, tested. It works!\n\n\n> >> I suspect that\n> >> is the root cause of the problem; if 000[1-6]-*.patch already has\n> >> their own Message-ID: because --thread is used when running\n> >> git-format-patch, they would also have In-Reply-To: and References:,\n> >> but there is no way for them to reference 0000-cover-letter.patch\n> >> (because format-patch did not get a chance to generate Message-ID to\n> >> it), is there?\n> >\n> > The patches were generated with git-format-patch but the Message-ID\n> > was added by patman [1]. The Message-ID encodes the local Change-Id\n> > which can make it easier to associate one version of the same patch\n> > with another (same reason gerrit uses Change-Id) [2]. There is no\n> > Change-Id associated with the cover letter so patman doesn't bother\n> > adding one there and has always just let it be auto-generated.\n>\n> > We\n> > could certainly change patman to make up a Message-Id for the cover\n> > letter, but there is no real need.\n>\n> This is a tangent, as I think the earlier patch should fix the\n> regression, but wouldn't a recipient of such a series have a hard\n> time to locate and group the patches in the same series with the\n> cover letter, without having In-Reply-To: or References: that links\n> the later message back to the initial message (i.e. cover letter)?\n> Assigning a Message-ID to the cover, and referencing it from the\n> patches via In-Reply-To:, is what is commonly done, I think, for\n> that kind of threading.\n\nIt has always magically worked.\n\nFor instance, looking at a patch series I sent before the regression.\nYou can see the cover letter here with an automatically-assigned\nMessage-Id:\n\nhttps://lore.kernel.org/linux-arm-kernel/20230504221349.1535669-1-dianders@chromium.org/\n\nYou can then look at patch #1, which had a Message-Id assigned to it\nby patman (by simply adding a \"Message-Id\" line to the patch file\nafter git format-patch but before calling git send-email):\n\nhttps://lore.kernel.org/linux-arm-kernel/20230504151100.v4.1.I8cbb2f4fa740528fcfade4f5439b6cdcdd059251@changeid/\n\nYou can see that it properly references the cover letter. Specifically\nin the raw message you can see:\n\nIn-Reply-To: <20230504221349.1535669-1-dianders@chromium.org>\nReferences: <20230504221349.1535669-1-dianders@chromium.org>\n\n-Doug\n"},{"id":"477485","messageId":"xmqqbkiipv48.fsf@gitster.g","threadId":"59760","inReplyTo":"CAD=FV=UkZBQ6SFB7xu8OD3vxtODp6RUq=K3xXzofpJjUZO18+w@mail.gmail.com","subject":"Re: bug report: cover letter is inheriting last patch's message ID with send-email","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-05-17T20:21:11Z","receivedAt":"2023-05-17T20:21:18Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Doug Anderson writes:\n\n> Hi,\n>\n> On Wed, May 17, 2023 at 12:22 PM Junio C Hamano wrote:\n>>\n>> Junio C Hamano writes:\n>>\n>> >> # With the attached patches, where all of the patches have a\n>> >> # Message-Id but the cover letter doesn't.\n>> >> git send-email *.patch\n>>\n>> I suspect this is a recent regression with the addition of the\n>> pre_process_file step. 56adddaa (send-email: refactor header\n>> generation functions, 2023-04-19) makes all messages parsed\n>> before the first message is sent out, by calling a sub\n>> \"pre_process_file\" before invoking the validate hook. The same sub\n>> is called again for each message when it is sent out, as the\n>> processing in that step is shared between the time the message gets\n>> vetted and the time the message gets sent.\n>>\n>> Unfortunately, $message_id variable is assigned to in that sub. So\n>> it is very much understandable why this happens.\n>>\n>> I wonder if it is just doing something silly like this?\n>>\n>> --- >8 ---\n>> Subject: [PATCH] send-email: clear the $message_id after validation\n>>\n>> Recently git-send-email started parsing the same message twice, once\n>> to validate _all_ the message before sending even the first one, and\n>> then after the validation hook is happy and each message gets sent,\n>> to read the contents to find out where to send to etc.\n>>\n>> Unfortunately, the effect of reading the messages for validation\n>> lingered even after the validation is done. Namely $message_id gets\n>> assigned if exists in the input files but the variable is global,\n>> and it is not cleared before pre_process_file runs. This causes\n>> reading a message without a message-id followed by reading a message\n>> with a message-id to misbehave---the sub reports as if the message\n>> had the same id as the previously written one.\n>>\n>> Clear the variable before starting to read the headers in\n>> pre_process_file\n>>\n>> Signed-off-by: Junio C Hamano \n>> ---\n>>\n>> * I am not surprised at all if there are similar problems in this\n>> function around variables other than $message_id; this patch is\n>> merely reacting to the bug report and not systematically hunting\n>> and fixing the bugs coming from the same root cause. If the\n>> original author of the pre_process_file change is still around,\n>> the second sets of eyes from them is very much appreciated.\n>>\n>> git-send-email.perl | 1 +\n>> 1 file changed, 1 insertion(+)\n>>\n>> diff --git c/git-send-email.perl w/git-send-email.perl\n>> index 89d8237e89..889ef388c8 100755\n>> --- c/git-send-email.perl\n>> +++ w/git-send-email.perl\n>> @@ -1771,6 +1771,7 @@ sub send_message {\n>> sub pre_process_file {\n>> my ($t, $quiet) = @_;\n>>\n>> + undef $message_id;\n>> open my $fh, \"<\", $t or die sprintf(__(\"can't open file %s\"), $t);\n>>\n>> my $author = undef;\n>\n> I can confirm this fixes the regression for me. Thus:\n>\n> Tested-by: Douglas Anderson \n\nThanks.\n\nNow I need to write (or trick somebody into writing) a test for this\n;-)\n"},{"id":"477505","messageId":"05888c33-612e-3c93-55da-53b9f35cfc2a@amd.com","threadId":"59760","inReplyTo":"xmqqjzx6pxuu.fsf@gitster.g","subject":"Re: bug report: cover letter is inheriting last patch's message ID with send-email","fromName":"Michael Strawbridge","fromEmail":"michael.strawbridge@amd.com","sentAt":"2023-05-18T00:51:06Z","receivedAt":"2023-05-18T00:51:17Z","isPatch":false,"sender":{"key":"michael.strawbridge@amd.com","avatar":null},"body":"On 2023-05-17 15:22, Junio C Hamano wrote:\n> Junio C Hamano writes:\n>\n>>> # With the attached patches, where all of the patches have a\n>>> # Message-Id but the cover letter doesn't.\n>>> git send-email *.patch\n> I suspect this is a recent regression with the addition of the\n> pre_process_file step. 56adddaa (send-email: refactor header\n> generation functions, 2023-04-19) makes all messages parsed\n> before the first message is sent out, by calling a sub\n> \"pre_process_file\" before invoking the validate hook. The same sub\n> is called again for each message when it is sent out, as the\n> processing in that step is shared between the time the message gets\n> vetted and the time the message gets sent.\n>\n> Unfortunately, $message_id variable is assigned to in that sub. So\n> it is very much understandable why this happens.\n>\n> I wonder if it is just doing something silly like this?\n>\n> --- >8 ---\n> Subject: [PATCH] send-email: clear the $message_id after validation\n>\n> Recently git-send-email started parsing the same message twice, once\n> to validate _all_ the message before sending even the first one, and\n> then after the validation hook is happy and each message gets sent,\n> to read the contents to find out where to send to etc.\n>\n> Unfortunately, the effect of reading the messages for validation\n> lingered even after the validation is done. Namely $message_id gets\n> assigned if exists in the input files but the variable is global,\n> and it is not cleared before pre_process_file runs. This causes\n> reading a message without a message-id followed by reading a message\n> with a message-id to misbehave---the sub reports as if the message\n> had the same id as the previously written one.\n>\n> Clear the variable before starting to read the headers in\n> pre_process_file\n>\n> Signed-off-by: Junio C Hamano \n> ---\n>\n> * I am not surprised at all if there are similar problems in this\n> function around variables other than $message_id; this patch is\n> merely reacting to the bug report and not systematically hunting\n> and fixing the bugs coming from the same root cause. If the\n> original author of the pre_process_file change is still around,\n> the second sets of eyes from them is very much appreciated.\n>\n> git-send-email.perl | 1 +\n> 1 file changed, 1 insertion(+)\n>\n> diff --git c/git-send-email.perl w/git-send-email.perl\n> index 89d8237e89..889ef388c8 100755\n> --- c/git-send-email.perl\n> +++ w/git-send-email.perl\n> @@ -1771,6 +1771,7 @@ sub send_message {\n> sub pre_process_file {\n> \tmy ($t, $quiet) = @_;\n> \n> +\tundef $message_id;\n> \topen my $fh, \"<\", $t or die sprintf(__(\"can't open file %s\"), $t);\n> \n> \tmy $author = undef;\nSorry I missed clearing $message_id in my initial patch.  After going\nthrough the variables again I believe it is the only one that is not\nreset properly.\n"},{"id":"477506","messageId":"xmqqjzx6v45z.fsf@gitster.g","threadId":"59760","inReplyTo":"05888c33-612e-3c93-55da-53b9f35cfc2a@amd.com","subject":"Re: bug report: cover letter is inheriting last patch's message ID with send-email","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-05-18T01:06:48Z","receivedAt":"2023-05-18T01:06:53Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Strawbridge writes:\n\n> Sorry I missed clearing $message_id in my initial patch.  After going\n> through the variables again I believe it is the only one that is not\n> reset properly.\n\nExcellent. Thank you very much for quickly checking.\n"}]}