{"thread":{"id":"53751","subject":"[PATCH] send-email: restore --in-reply-to superseding behavior","startedAt":"2020-06-24T19:55:36Z","lastAt":"2020-07-01T22:10:12Z","messageCount":8,"participants":["Rafael Aquini","Junio C Hamano","Carlo Arenas","Carlo Marcelo Arenas Belón"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"400555","messageId":"20200624195520.2062298-1-aquini@redhat.com","threadId":"53751","inReplyTo":null,"subject":"[PATCH] send-email: restore --in-reply-to superseding behavior","fromName":"Rafael Aquini","fromEmail":"aquini@redhat.com","sentAt":"2020-06-24T19:55:20Z","receivedAt":"2020-06-24T19:55:36Z","isPatch":true,"sender":{"key":"aquini@redhat.com","avatar":"https://avatars.githubusercontent.com/u/918150?v=4"},"body":"git send-email --in-reply-to= fails to override the email headers,\nif they're present in the output of format-patch, which breakes the\ncontract of the command line switch. This can cause an email thread to\nbreak, if subsequent replies to a given message need to be sent after\namending fixes to a commit and re-running git format-patch to get the\nincremental patch-fix posted.\n\nFixes: 256be1d3f0 (send-email: avoid duplicate In-Reply-To/References, 2018-04-17)\nSigned-off-by: Rafael Aquini \n---\n git-send-email.perl | 8 ++++++--\n 1 file changed, 6 insertions(+), 2 deletions(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex dc95656f75..342e00b1f3 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -1699,10 +1699,14 @@ sub process_file {\n \t\t\t\t$xfer_encoding = $1 if not defined $xfer_encoding;\n \t\t\t}\n \t\t\telsif (/^In-Reply-To: (.*)/i) {\n-\t\t\t\t$in_reply_to = $1;\n+\t\t\t\tif (!$initial_in_reply_to) {\n+\t\t\t\t\t$in_reply_to = $1;\n+\t\t\t\t}\n \t\t\t}\n \t\t\telsif (/^References: (.*)/i) {\n-\t\t\t\t$references = $1;\n+\t\t\t\tif (!$initial_in_reply_to) {\n+\t\t\t\t\t$references = $1;\n+\t\t\t\t}\n \t\t\t}\n \t\t\telsif (!/^Date:\\s/i && /^[-A-Za-z]+:\\s+\\S/) {\n \t\t\t\tpush @xh, $_;\n-- \n2.23.0\n\n"},{"id":"400558","messageId":"xmqqo8p85eud.fsf@gitster.c.googlers.com","threadId":"53751","inReplyTo":"20200624195520.2062298-1-aquini@redhat.com","subject":"Re: [PATCH] send-email: restore --in-reply-to superseding behavior","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-06-24T21:33:14Z","receivedAt":"2020-06-24T21:33:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rafael Aquini writes:\n\n> git send-email --in-reply-to= fails to override the email headers,\n> if they're present in the output of format-patch, which breakes the\n\nWill do s/breakes/breaks/ while applying.\n\nIt makes me wonder, however, why it is a good idea to have the I-R-T\nin the format patch output in the first place.\n\n> \t\t\telsif (/^In-Reply-To: (.*)/i) {\n> -\t\t\t\t$in_reply_to = $1;\n> +\t\t\t\tif (!$initial_in_reply_to) {\n> +\t\t\t\t\t$in_reply_to = $1;\n> +\t\t\t\t}\n\nI can see how this would work the way it should for the first\nmessage we send out, so it would work well for a single patch.\n\nBut what does this change do to the chaining (either making [PATCH\n1/N] thru [PATCH N/N] as responses to the cover letter [PATCH 0/N],\nor making [PATCH n+1/N] as response to [PATCH n/N] for 1 <= n < N)\nof multiple messages?\n\nWhen you prepare a series whose 1..N/N are all pointing at 0/N with\nthe already prepared In-Reply-To (so you have N+1 files to send\nout), wouldn't you want to make 0/N a reply to a particular message\nyou specify on the command line, while keeping the relationship\namong your messages intact? Doesn't having $initial_in_reply_to\n(i.e. command line override) help above code break the chaning?\n\n\n\n> \t\t\t}\n> \t\t\telsif (/^References: (.*)/i) {\n> -\t\t\t\t$references = $1;\n> +\t\t\t\tif (!$initial_in_reply_to) {\n> +\t\t\t\t\t$references = $1;\n> +\t\t\t\t}\n> \t\t\t}\n> \t\t\telsif (!/^Date:\\s/i && /^[-A-Za-z]+:\\s+\\S/) {\n> \t\t\t\tpush @xh, $_;\n"},{"id":"400566","messageId":"20200624234539.GH1987277@optiplex-lnx","threadId":"53751","inReplyTo":"xmqqo8p85eud.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] send-email: restore --in-reply-to superseding behavior","fromName":"Rafael Aquini","fromEmail":"aquini@redhat.com","sentAt":"2020-06-24T23:45:39Z","receivedAt":"2020-06-24T23:45:57Z","isPatch":true,"sender":{"key":"aquini@redhat.com","avatar":"https://avatars.githubusercontent.com/u/918150?v=4"},"body":"On Wed, Jun 24, 2020 at 02:33:14PM -0700, Junio C Hamano wrote:\n> Rafael Aquini writes:\n> \n> > git send-email --in-reply-to= fails to override the email headers,\n> > if they're present in the output of format-patch, which breakes the\n> \n> Will do s/breakes/breaks/ while applying.\n>\n\nUGH! I've been fat-fingering typos the whole day, today... Sorry about\nthat one.\n\n \n> It makes me wonder, however, why it is a good idea to have the I-R-T\n> in the format patch output in the first place.\n> \n> > \t\t\telsif (/^In-Reply-To: (.*)/i) {\n> > -\t\t\t\t$in_reply_to = $1;\n> > +\t\t\t\tif (!$initial_in_reply_to) {\n> > +\t\t\t\t\t$in_reply_to = $1;\n> > +\t\t\t\t}\n> \n> I can see how this would work the way it should for the first\n> message we send out, so it would work well for a single patch.\n> \n> But what does this change do to the chaining (either making [PATCH\n> 1/N] thru [PATCH N/N] as responses to the cover letter [PATCH 0/N],\n> or making [PATCH n+1/N] as response to [PATCH n/N] for 1 <= n < N)\n> of multiple messages?\n> \n> When you prepare a series whose 1..N/N are all pointing at 0/N with\n> the already prepared In-Reply-To (so you have N+1 files to send\n> out), wouldn't you want to make 0/N a reply to a particular message\n> you specify on the command line, while keeping the relationship\n> among your messages intact? Doesn't having $initial_in_reply_to\n> (i.e. command line override) help above code break the chaning?\n>\n\nThis change will make all emails to appear as a reply to the msgid\nfed to --in-reply-to. I see your point, though, and at its light \nI think now this patch, is actually incomplete. \n\nWith this change we get back the override desired behavior,\nbut it also breaks the contract, according to the man page.\n\n\"\n --in-reply-to=\n Make the first mail (or all the mails with --no-thread) appear as a reply to the given Message-Id, which\n avoids breaking threads to provide a new patch series. The second and subsequent emails will be sent as\n replies according to the --[no-]chain-reply-to setting.\n\"\n\nI drove the change based on my usecase, which is marginal to the\nmulti-part reply case. \n\nI guess we just need the following, for a complete solution:\n\n\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex dc95656f75..768296ea0a 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -1699,10 +1699,14 @@ sub process_file {\n \t\t\t\t$xfer_encoding = $1 if not defined $xfer_encoding;\n \t\t\t}\n \t\t\telsif (/^In-Reply-To: (.*)/i) {\n-\t\t\t\t$in_reply_to = $1;\n+\t\t\t\tif (!$initial_in_reply_to || $thread) {\n+\t\t\t\t\t$in_reply_to = $1;\n+\t\t\t\t}\n \t\t\t}\n \t\t\telsif (/^References: (.*)/i) {\n-\t\t\t\t$references = $1;\n+\t\t\t\tif (!$initial_in_reply_to || $thread) {\n+\t\t\t\t\t$references = $1;\n+\t\t\t\t}\n \t\t\t}\n \t\t\telsif (!/^Date:\\s/i && /^[-A-Za-z]+:\\s+\\S/) {\n \t\t\t\tpush @xh, $_;\n\n"},{"id":"400627","messageId":"20200625184715.GC2117795@optiplex-lnx","threadId":"53751","inReplyTo":"20200624234539.GH1987277@optiplex-lnx","subject":"Re: [PATCH] send-email: restore --in-reply-to superseding behavior","fromName":"Rafael Aquini","fromEmail":"aquini@redhat.com","sentAt":"2020-06-25T18:47:15Z","receivedAt":"2020-06-25T18:47:32Z","isPatch":true,"sender":{"key":"aquini@redhat.com","avatar":"https://avatars.githubusercontent.com/u/918150?v=4"},"body":"On Wed, Jun 24, 2020 at 07:45:43PM -0400, Rafael Aquini wrote:\n> On Wed, Jun 24, 2020 at 02:33:14PM -0700, Junio C Hamano wrote:\n> > Rafael Aquini writes:\n> > \n> > > git send-email --in-reply-to= fails to override the email headers,\n> > > if they're present in the output of format-patch, which breakes the\n> > \n> > Will do s/breakes/breaks/ while applying.\n> >\n> \n> UGH! I've been fat-fingering typos the whole day, today... Sorry about\n> that one.\n> \n> \n> > It makes me wonder, however, why it is a good idea to have the I-R-T\n> > in the format patch output in the first place.\n> > \n> > > \t\t\telsif (/^In-Reply-To: (.*)/i) {\n> > > -\t\t\t\t$in_reply_to = $1;\n> > > +\t\t\t\tif (!$initial_in_reply_to) {\n> > > +\t\t\t\t\t$in_reply_to = $1;\n> > > +\t\t\t\t}\n> > \n> > I can see how this would work the way it should for the first\n> > message we send out, so it would work well for a single patch.\n> > \n> > But what does this change do to the chaining (either making [PATCH\n> > 1/N] thru [PATCH N/N] as responses to the cover letter [PATCH 0/N],\n> > or making [PATCH n+1/N] as response to [PATCH n/N] for 1 <= n < N)\n> > of multiple messages?\n> > \n> > When you prepare a series whose 1..N/N are all pointing at 0/N with\n> > the already prepared In-Reply-To (so you have N+1 files to send\n> > out), wouldn't you want to make 0/N a reply to a particular message\n> > you specify on the command line, while keeping the relationship\n> > among your messages intact? Doesn't having $initial_in_reply_to\n> > (i.e. command line override) help above code break the chaning?\n> >\n> \n> This change will make all emails to appear as a reply to the msgid\n> fed to --in-reply-to. I see your point, though, and at its light \n> I think now this patch, is actually incomplete. \n> \n> With this change we get back the override desired behavior,\n> but it also breaks the contract, according to the man page.\n> \n> \"\n> --in-reply-to=\n> Make the first mail (or all the mails with --no-thread) appear as a reply to the given Message-Id, which\n> avoids breaking threads to provide a new patch series. The second and subsequent emails will be sent as\n> replies according to the --[no-]chain-reply-to setting.\n> \"\n> \n> I drove the change based on my usecase, which is marginal to the\n> multi-part reply case. \n> \n> I guess we just need the following, for a complete solution:\n> \n> \n> \n> diff --git a/git-send-email.perl b/git-send-email.perl\n> index dc95656f75..768296ea0a 100755\n> --- a/git-send-email.perl\n> +++ b/git-send-email.perl\n> @@ -1699,10 +1699,14 @@ sub process_file {\n> \t\t\t\t$xfer_encoding = $1 if not defined $xfer_encoding;\n> \t\t\t}\n> \t\t\telsif (/^In-Reply-To: (.*)/i) {\n> -\t\t\t\t$in_reply_to = $1;\n> +\t\t\t\tif (!$initial_in_reply_to || $thread) {\n> +\t\t\t\t\t$in_reply_to = $1;\n> +\t\t\t\t}\n> \t\t\t}\n> \t\t\telsif (/^References: (.*)/i) {\n> -\t\t\t\t$references = $1;\n> +\t\t\t\tif (!$initial_in_reply_to || $thread) {\n> +\t\t\t\t\t$references = $1;\n> +\t\t\t\t}\n> \t\t\t}\n> \t\t\telsif (!/^Date:\\s/i && /^[-A-Za-z]+:\\s+\\S/) {\n> \t\t\t\tpush @xh, $_;\n\nThis guy worked like a charm, and git send-email, now, follows what the\nman page says wrt the --in-reply-to usage.\n\nI'll reformat the commit log, and repost the patch ASAP, if you are\nOK with it.\n\n-- Rafael\n\n"},{"id":"400650","messageId":"CAPUEspgoDUGUmFqoKkmKDpRzSB5dQNb+PasaPjbPwb04SBz=Ww@mail.gmail.com","threadId":"53751","inReplyTo":"20200625184715.GC2117795@optiplex-lnx","subject":"Re: [PATCH] send-email: restore --in-reply-to superseding behavior","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2020-06-26T01:08:24Z","receivedAt":"2020-06-26T01:08:37Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"a test case in t/t9001-send-email.sh will also help, as I am not sure\nthis might be \"expected behaviour\" as hinted in the man page for\ngit-send-email (under --thread):\n\n\"It is up to the user to ensure that no In-Reply-To header already exists\n exists when git send-email is asked to add it (especially note that\n git format-patch can be configured to do the threading itself).\n Failure to do so may not produce the expected result in the\n recipient's MUA.\"\n\nCarlo\n"},{"id":"400682","messageId":"20200626133947.GB2235017@optiplex-lnx","threadId":"53751","inReplyTo":"CAPUEspgoDUGUmFqoKkmKDpRzSB5dQNb+PasaPjbPwb04SBz=Ww@mail.gmail.com","subject":"Re: [PATCH] send-email: restore --in-reply-to superseding behavior","fromName":"Rafael Aquini","fromEmail":"aquini@redhat.com","sentAt":"2020-06-26T13:39:47Z","receivedAt":"2020-06-26T13:40:00Z","isPatch":true,"sender":{"key":"aquini@redhat.com","avatar":"https://avatars.githubusercontent.com/u/918150?v=4"},"body":"On Thu, Jun 25, 2020 at 06:08:24PM -0700, Carlo Arenas wrote:\n> a test case in t/t9001-send-email.sh will also help, as I am not sure\n> this might be \"expected behaviour\" as hinted in the man page for\n> git-send-email (under --thread):\n> \n> \"It is up to the user to ensure that no In-Reply-To header already exists\n> exists when git send-email is asked to add it (especially note that\n> git format-patch can be configured to do the threading itself).\n> Failure to do so may not produce the expected result in the\n> recipient's MUA.\"\n>\n\nA quick note: this change does not break that assumption, as well.\nThe \"ensure it exists\" part means the header must either be in the\nmessages, as populated by format-patch, or it is provided by the\n--in-reply-to switch option. send-email --thread is not orthogonal, \nbut complementar with --in-reply-to, AFAICS.\n\nThe problem we have, right now, is that \"send-email --in-reply-to\" input gets \ndropped on the floor if you don't explicitly do \"format-patch --no-thread\" (or\nextract a single patch), and this is a behavior regression introduced after v2.17.2\n\n\nI took a glance in the test-cases, an it seems this is already covered:\n\ntest_expect_success $PREREQ 'in-reply-to but no threading' '\n git send-email \\\n --dry-run \\\n --from=\"Example \" \\\n --to=nobody@example.com \\\n --in-reply-to=\"\" \\\n --no-thread \\\n $patches >out &&\n grep \"In-Reply-To: \" out\n'\n\nCheers,\n-- Rafael\n\n"},{"id":"400765","messageId":"20200629141104.2522011-1-aquini@redhat.com","threadId":"53751","inReplyTo":"xmqqo8p85eud.fsf@gitster.c.googlers.com","subject":"[PATCH v2] send-email: restore --in-reply-to superseding behavior","fromName":"Rafael Aquini","fromEmail":"aquini@redhat.com","sentAt":"2020-06-29T14:11:04Z","receivedAt":"2020-06-29T20:55:25Z","isPatch":true,"sender":{"key":"aquini@redhat.com","avatar":"https://avatars.githubusercontent.com/u/918150?v=4"},"body":"git send-email --in-reply-to= fails to override In-Reply-To email headers,\nif they're present in the output of format-patch, even when explicitly\ntold to do so by the option --no-thread, which breaks the contract of the\ncommand line switch option, per its man page.\n\n\"\n --in-reply-to=\n Make the first mail (or all the mails with --no-thread) appear as\n a reply to the given Message-Id, which avoids breaking threads to\n provide a new patch series.\n\"\n\nThis patch fixes the aformentioned issue, by bringing --in-reply-to's old\noverriding behavior back.\n\nFixes: 256be1d3f0 (send-email: avoid duplicate In-Reply-To/References, 2018-04-17)\nSigned-off-by: Rafael Aquini \n---\nv2 changelog:\n* conform to the command manual page, when send-email threading is requested\n\n git-send-email.perl | 8 ++++++--\n 1 file changed, 6 insertions(+), 2 deletions(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex dc95656f75..36c47bae1d 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -1699,10 +1699,14 @@ sub process_file {\n \t\t\t\t$xfer_encoding = $1 if not defined $xfer_encoding;\n \t\t\t}\n \t\t\telsif (/^In-Reply-To: (.*)/i) {\n-\t\t\t\t$in_reply_to = $1;\n+\t\t\t\tif (!$initial_in_reply_to || $thread) {\n+\t\t\t\t\t$in_reply_to = $1;\n+\t\t\t\t}\n \t\t\t}\n \t\t\telsif (/^References: (.*)/i) {\n-\t\t\t\t$references = $1;\n+\t\t\t\tif (!$initial_in_reply_to || $thread) {\n+\t\t\t\t\t$references = $1;\n+\t\t\t\t}\n \t\t\t}\n \t\t\telsif (!/^Date:\\s/i && /^[-A-Za-z]+:\\s+\\S/) {\n \t\t\t\tpush @xh, $_;\n-- \n2.23.0\n\n"},{"id":"400954","messageId":"20200701221007.GA49195@Carlos-MBP","threadId":"53751","inReplyTo":"20200629141104.2522011-1-aquini@redhat.com","subject":"Re: [PATCH v2] send-email: restore --in-reply-to superseding behavior","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2020-07-01T22:10:07Z","receivedAt":"2020-07-01T22:10:12Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Mon, Jun 29, 2020 at 10:11:04AM -0400, Rafael Aquini wrote:\n> \n> This patch fixes the aformentioned issue, by bringing --in-reply-to's old\n> overriding behavior back.\n> \n> Fixes: 256be1d3f0 (send-email: avoid duplicate In-Reply-To/References, 2018-04-17)\n\nthe following test case could be squashed on top to make the regression more\nvisible IMHO (it at least pass when applied on top of v2.17.1 and also\nra/send-email-in-reply-to-from-command-line-wins)\n\n--- 8< ---\n t/t9001-send-email.sh | 14 +++++++++++++-\n 1 file changed, 13 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh\nindex 90f61c3400..ec261085ec 100755\n--- a/t/t9001-send-email.sh\n+++ b/t/t9001-send-email.sh\n@@ -42,7 +42,8 @@ clean_fake_sendmail () {\n }\n \n test_expect_success $PREREQ 'Extract patches' '\n-\tpatches=$(git format-patch -s --cc=\"One \" --cc=two@example.com -n HEAD^1)\n+\tpatches=$(git format-patch -s --cc=\"One \" --cc=two@example.com -n HEAD^1) &&\n+\tthreaded_patches=$(git format-patch -o threaded -s --in-reply-to=\"format\" HEAD^1)\n '\n \n # Test no confirm early to ensure remaining tests will not hang\n@@ -1219,6 +1220,17 @@ test_expect_success $PREREQ 'threading but no chain-reply-to' '\n \tgrep \"In-Reply-To: \" stdout\n '\n \n+test_expect_success $PREREQ 'override in-reply-to if no threading' '\n+\tgit send-email \\\n+\t\t--dry-run \\\n+\t\t--from=\"Example \" \\\n+\t\t--to=nobody@example.com \\\n+\t\t--no-thread \\\n+\t\t--in-reply-to=\"override\" \\\n+\t\t$threaded_patches >stdout &&\n+\tgrep \"In-Reply-To: \" stdout\n+'\n+\n test_expect_success $PREREQ 'sendemail.to works' '\n \tgit config --replace-all sendemail.to \"Somebody \" &&\n \tgit send-email \\\n\nbase-commit: 096547052491426a29e040a5bd94d7f8a4cab8ac\n--- >8 ---\n\n> diff --git a/git-send-email.perl b/git-send-email.perl\n> index dc95656f75..36c47bae1d 100755\n> --- a/git-send-email.perl\n> +++ b/git-send-email.perl\n> @@ -1699,10 +1699,14 @@ sub process_file {\n> \t\t\t\t$xfer_encoding = $1 if not defined $xfer_encoding;\n> \t\t\t}\n> \t\t\telsif (/^In-Reply-To: (.*)/i) {\n> -\t\t\t\t$in_reply_to = $1;\n> +\t\t\t\tif (!$initial_in_reply_to || $thread) {\n> +\t\t\t\t\t$in_reply_to = $1;\n> +\t\t\t\t}\n> \t\t\t}\n> \t\t\telsif (/^References: (.*)/i) {\n> -\t\t\t\t$references = $1;\n> +\t\t\t\tif (!$initial_in_reply_to || $thread) {\n> +\t\t\t\t\t$references = $1;\n> +\t\t\t\t}\n> \t\t\t}\n> \t\t\telsif (!/^Date:\\s/i && /^[-A-Za-z]+:\\s+\\S/) {\n> \t\t\t\tpush @xh, $_;\n\nin both cases, doing `!defined $initial_in_reply_to` might seem like a\nmore consistent option.\n\nCarlo\n"}]}