* [PATCH] send-email: restore --in-reply-to superseding behavior @ 2020-06-24 19:55 Rafael Aquini 2020-06-24 21:33 ` Junio C Hamano 0 siblings, 1 reply; 8+ messages in thread From: Rafael Aquini @ 2020-06-24 19:55 UTC (permalink / raw) To: git git send-email --in-reply-to= fails to override the email headers, if they're present in the output of format-patch, which breakes the contract of the command line switch. This can cause an email thread to break, if subsequent replies to a given message need to be sent after amending fixes to a commit and re-running git format-patch to get the incremental patch-fix posted. Fixes: 256be1d3f0 (send-email: avoid duplicate In-Reply-To/References, 2018-04-17) Signed-off-by: Rafael Aquini <aquini@redhat.com> --- git-send-email.perl | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/git-send-email.perl b/git-send-email.perl index dc95656f75..342e00b1f3 100755 --- a/git-send-email.perl +++ b/git-send-email.perl @@ -1699,10 +1699,14 @@ sub process_file { $xfer_encoding = $1 if not defined $xfer_encoding; } elsif (/^In-Reply-To: (.*)/i) { - $in_reply_to = $1; + if (!$initial_in_reply_to) { + $in_reply_to = $1; + } } elsif (/^References: (.*)/i) { - $references = $1; + if (!$initial_in_reply_to) { + $references = $1; + } } elsif (!/^Date:\s/i && /^[-A-Za-z]+:\s+\S/) { push @xh, $_; -- 2.23.0 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] send-email: restore --in-reply-to superseding behavior 2020-06-24 19:55 [PATCH] send-email: restore --in-reply-to superseding behavior Rafael Aquini @ 2020-06-24 21:33 ` Junio C Hamano 2020-06-24 23:45 ` Rafael Aquini 2020-06-29 14:11 ` [PATCH v2] " Rafael Aquini 0 siblings, 2 replies; 8+ messages in thread From: Junio C Hamano @ 2020-06-24 21:33 UTC (permalink / raw) To: Rafael Aquini; +Cc: git Rafael Aquini <aquini@redhat.com> writes: > git send-email --in-reply-to= fails to override the email headers, > if they're present in the output of format-patch, which breakes the Will do s/breakes/breaks/ while applying. It makes me wonder, however, why it is a good idea to have the I-R-T in the format patch output in the first place. > elsif (/^In-Reply-To: (.*)/i) { > - $in_reply_to = $1; > + if (!$initial_in_reply_to) { > + $in_reply_to = $1; > + } I can see how this would work the way it should for the first message we send out, so it would work well for a single patch. But what does this change do to the chaining (either making [PATCH 1/N] thru [PATCH N/N] as responses to the cover letter [PATCH 0/N], or making [PATCH n+1/N] as response to [PATCH n/N] for 1 <= n < N) of multiple messages? When you prepare a series whose 1..N/N are all pointing at 0/N with the already prepared In-Reply-To (so you have N+1 files to send out), wouldn't you want to make 0/N a reply to a particular message you specify on the command line, while keeping the relationship among your messages intact? Doesn't having $initial_in_reply_to (i.e. command line override) help above code break the chaning? > } > elsif (/^References: (.*)/i) { > - $references = $1; > + if (!$initial_in_reply_to) { > + $references = $1; > + } > } > elsif (!/^Date:\s/i && /^[-A-Za-z]+:\s+\S/) { > push @xh, $_; ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] send-email: restore --in-reply-to superseding behavior 2020-06-24 21:33 ` Junio C Hamano @ 2020-06-24 23:45 ` Rafael Aquini 2020-06-25 18:47 ` Rafael Aquini 2020-06-29 14:11 ` [PATCH v2] " Rafael Aquini 1 sibling, 1 reply; 8+ messages in thread From: Rafael Aquini @ 2020-06-24 23:45 UTC (permalink / raw) To: Junio C Hamano; +Cc: git On Wed, Jun 24, 2020 at 02:33:14PM -0700, Junio C Hamano wrote: > Rafael Aquini <aquini@redhat.com> writes: > > > git send-email --in-reply-to= fails to override the email headers, > > if they're present in the output of format-patch, which breakes the > > Will do s/breakes/breaks/ while applying. > UGH! I've been fat-fingering typos the whole day, today... Sorry about that one. > It makes me wonder, however, why it is a good idea to have the I-R-T > in the format patch output in the first place. > > > elsif (/^In-Reply-To: (.*)/i) { > > - $in_reply_to = $1; > > + if (!$initial_in_reply_to) { > > + $in_reply_to = $1; > > + } > > I can see how this would work the way it should for the first > message we send out, so it would work well for a single patch. > > But what does this change do to the chaining (either making [PATCH > 1/N] thru [PATCH N/N] as responses to the cover letter [PATCH 0/N], > or making [PATCH n+1/N] as response to [PATCH n/N] for 1 <= n < N) > of multiple messages? > > When you prepare a series whose 1..N/N are all pointing at 0/N with > the already prepared In-Reply-To (so you have N+1 files to send > out), wouldn't you want to make 0/N a reply to a particular message > you specify on the command line, while keeping the relationship > among your messages intact? Doesn't having $initial_in_reply_to > (i.e. command line override) help above code break the chaning? > This change will make all emails to appear as a reply to the msgid fed to --in-reply-to. I see your point, though, and at its light I think now this patch, is actually incomplete. With this change we get back the override desired behavior, but it also breaks the contract, according to the man page. " --in-reply-to=<identifier> Make the first mail (or all the mails with --no-thread) appear as a reply to the given Message-Id, which avoids breaking threads to provide a new patch series. The second and subsequent emails will be sent as replies according to the --[no-]chain-reply-to setting. " I drove the change based on my usecase, which is marginal to the multi-part reply case. I guess we just need the following, for a complete solution: diff --git a/git-send-email.perl b/git-send-email.perl index dc95656f75..768296ea0a 100755 --- a/git-send-email.perl +++ b/git-send-email.perl @@ -1699,10 +1699,14 @@ sub process_file { $xfer_encoding = $1 if not defined $xfer_encoding; } elsif (/^In-Reply-To: (.*)/i) { - $in_reply_to = $1; + if (!$initial_in_reply_to || $thread) { + $in_reply_to = $1; + } } elsif (/^References: (.*)/i) { - $references = $1; + if (!$initial_in_reply_to || $thread) { + $references = $1; + } } elsif (!/^Date:\s/i && /^[-A-Za-z]+:\s+\S/) { push @xh, $_; ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] send-email: restore --in-reply-to superseding behavior 2020-06-24 23:45 ` Rafael Aquini @ 2020-06-25 18:47 ` Rafael Aquini 2020-06-26 1:08 ` Carlo Arenas 0 siblings, 1 reply; 8+ messages in thread From: Rafael Aquini @ 2020-06-25 18:47 UTC (permalink / raw) To: Junio C Hamano; +Cc: git On Wed, Jun 24, 2020 at 07:45:43PM -0400, Rafael Aquini wrote: > On Wed, Jun 24, 2020 at 02:33:14PM -0700, Junio C Hamano wrote: > > Rafael Aquini <aquini@redhat.com> writes: > > > > > git send-email --in-reply-to= fails to override the email headers, > > > if they're present in the output of format-patch, which breakes the > > > > Will do s/breakes/breaks/ while applying. > > > > UGH! I've been fat-fingering typos the whole day, today... Sorry about > that one. > > > > It makes me wonder, however, why it is a good idea to have the I-R-T > > in the format patch output in the first place. > > > > > elsif (/^In-Reply-To: (.*)/i) { > > > - $in_reply_to = $1; > > > + if (!$initial_in_reply_to) { > > > + $in_reply_to = $1; > > > + } > > > > I can see how this would work the way it should for the first > > message we send out, so it would work well for a single patch. > > > > But what does this change do to the chaining (either making [PATCH > > 1/N] thru [PATCH N/N] as responses to the cover letter [PATCH 0/N], > > or making [PATCH n+1/N] as response to [PATCH n/N] for 1 <= n < N) > > of multiple messages? > > > > When you prepare a series whose 1..N/N are all pointing at 0/N with > > the already prepared In-Reply-To (so you have N+1 files to send > > out), wouldn't you want to make 0/N a reply to a particular message > > you specify on the command line, while keeping the relationship > > among your messages intact? Doesn't having $initial_in_reply_to > > (i.e. command line override) help above code break the chaning? > > > > This change will make all emails to appear as a reply to the msgid > fed to --in-reply-to. I see your point, though, and at its light > I think now this patch, is actually incomplete. > > With this change we get back the override desired behavior, > but it also breaks the contract, according to the man page. > > " > --in-reply-to=<identifier> > Make the first mail (or all the mails with --no-thread) appear as a reply to the given Message-Id, which > avoids breaking threads to provide a new patch series. The second and subsequent emails will be sent as > replies according to the --[no-]chain-reply-to setting. > " > > I drove the change based on my usecase, which is marginal to the > multi-part reply case. > > I guess we just need the following, for a complete solution: > > > > diff --git a/git-send-email.perl b/git-send-email.perl > index dc95656f75..768296ea0a 100755 > --- a/git-send-email.perl > +++ b/git-send-email.perl > @@ -1699,10 +1699,14 @@ sub process_file { > $xfer_encoding = $1 if not defined $xfer_encoding; > } > elsif (/^In-Reply-To: (.*)/i) { > - $in_reply_to = $1; > + if (!$initial_in_reply_to || $thread) { > + $in_reply_to = $1; > + } > } > elsif (/^References: (.*)/i) { > - $references = $1; > + if (!$initial_in_reply_to || $thread) { > + $references = $1; > + } > } > elsif (!/^Date:\s/i && /^[-A-Za-z]+:\s+\S/) { > push @xh, $_; This guy worked like a charm, and git send-email, now, follows what the man page says wrt the --in-reply-to usage. I'll reformat the commit log, and repost the patch ASAP, if you are OK with it. -- Rafael ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] send-email: restore --in-reply-to superseding behavior 2020-06-25 18:47 ` Rafael Aquini @ 2020-06-26 1:08 ` Carlo Arenas 2020-06-26 13:39 ` Rafael Aquini 0 siblings, 1 reply; 8+ messages in thread From: Carlo Arenas @ 2020-06-26 1:08 UTC (permalink / raw) To: Rafael Aquini; +Cc: Junio C Hamano, git a test case in t/t9001-send-email.sh will also help, as I am not sure this might be "expected behaviour" as hinted in the man page for git-send-email (under --thread): "It is up to the user to ensure that no In-Reply-To header already exists exists when git send-email is asked to add it (especially note that git format-patch can be configured to do the threading itself). Failure to do so may not produce the expected result in the recipient's MUA." Carlo ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] send-email: restore --in-reply-to superseding behavior 2020-06-26 1:08 ` Carlo Arenas @ 2020-06-26 13:39 ` Rafael Aquini 0 siblings, 0 replies; 8+ messages in thread From: Rafael Aquini @ 2020-06-26 13:39 UTC (permalink / raw) To: Carlo Arenas; +Cc: Junio C Hamano, git On Thu, Jun 25, 2020 at 06:08:24PM -0700, Carlo Arenas wrote: > a test case in t/t9001-send-email.sh will also help, as I am not sure > this might be "expected behaviour" as hinted in the man page for > git-send-email (under --thread): > > "It is up to the user to ensure that no In-Reply-To header already exists > exists when git send-email is asked to add it (especially note that > git format-patch can be configured to do the threading itself). > Failure to do so may not produce the expected result in the > recipient's MUA." > A quick note: this change does not break that assumption, as well. The "ensure it exists" part means the header must either be in the messages, as populated by format-patch, or it is provided by the --in-reply-to switch option. send-email --thread is not orthogonal, but complementar with --in-reply-to, AFAICS. The problem we have, right now, is that "send-email --in-reply-to" input gets dropped on the floor if you don't explicitly do "format-patch --no-thread" (or extract a single patch), and this is a behavior regression introduced after v2.17.2 I took a glance in the test-cases, an it seems this is already covered: test_expect_success $PREREQ 'in-reply-to but no threading' ' git send-email \ --dry-run \ --from="Example <nobody@example.com>" \ --to=nobody@example.com \ --in-reply-to="<in-reply-id@example.com>" \ --no-thread \ $patches >out && grep "In-Reply-To: <in-reply-id@example.com>" out ' Cheers, -- Rafael ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2] send-email: restore --in-reply-to superseding behavior 2020-06-24 21:33 ` Junio C Hamano 2020-06-24 23:45 ` Rafael Aquini @ 2020-06-29 14:11 ` Rafael Aquini 2020-07-01 22:10 ` Carlo Marcelo Arenas Belón 1 sibling, 1 reply; 8+ messages in thread From: Rafael Aquini @ 2020-06-29 14:11 UTC (permalink / raw) To: git; +Cc: gitster, carenas git send-email --in-reply-to= fails to override In-Reply-To email headers, if they're present in the output of format-patch, even when explicitly told to do so by the option --no-thread, which breaks the contract of the command line switch option, per its man page. " --in-reply-to=<identifier> Make the first mail (or all the mails with --no-thread) appear as a reply to the given Message-Id, which avoids breaking threads to provide a new patch series. " This patch fixes the aformentioned issue, by bringing --in-reply-to's old overriding behavior back. Fixes: 256be1d3f0 (send-email: avoid duplicate In-Reply-To/References, 2018-04-17) Signed-off-by: Rafael Aquini <aquini@redhat.com> --- v2 changelog: * conform to the command manual page, when send-email threading is requested git-send-email.perl | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/git-send-email.perl b/git-send-email.perl index dc95656f75..36c47bae1d 100755 --- a/git-send-email.perl +++ b/git-send-email.perl @@ -1699,10 +1699,14 @@ sub process_file { $xfer_encoding = $1 if not defined $xfer_encoding; } elsif (/^In-Reply-To: (.*)/i) { - $in_reply_to = $1; + if (!$initial_in_reply_to || $thread) { + $in_reply_to = $1; + } } elsif (/^References: (.*)/i) { - $references = $1; + if (!$initial_in_reply_to || $thread) { + $references = $1; + } } elsif (!/^Date:\s/i && /^[-A-Za-z]+:\s+\S/) { push @xh, $_; -- 2.23.0 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2] send-email: restore --in-reply-to superseding behavior 2020-06-29 14:11 ` [PATCH v2] " Rafael Aquini @ 2020-07-01 22:10 ` Carlo Marcelo Arenas Belón 0 siblings, 0 replies; 8+ messages in thread From: Carlo Marcelo Arenas Belón @ 2020-07-01 22:10 UTC (permalink / raw) To: Rafael Aquini; +Cc: git, gitster On Mon, Jun 29, 2020 at 10:11:04AM -0400, Rafael Aquini wrote: > > This patch fixes the aformentioned issue, by bringing --in-reply-to's old > overriding behavior back. > > Fixes: 256be1d3f0 (send-email: avoid duplicate In-Reply-To/References, 2018-04-17) the following test case could be squashed on top to make the regression more visible IMHO (it at least pass when applied on top of v2.17.1 and also ra/send-email-in-reply-to-from-command-line-wins) --- 8< --- t/t9001-send-email.sh | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh index 90f61c3400..ec261085ec 100755 --- a/t/t9001-send-email.sh +++ b/t/t9001-send-email.sh @@ -42,7 +42,8 @@ clean_fake_sendmail () { } test_expect_success $PREREQ 'Extract patches' ' - patches=$(git format-patch -s --cc="One <one@example.com>" --cc=two@example.com -n HEAD^1) + patches=$(git format-patch -s --cc="One <one@example.com>" --cc=two@example.com -n HEAD^1) && + threaded_patches=$(git format-patch -o threaded -s --in-reply-to="format" HEAD^1) ' # Test no confirm early to ensure remaining tests will not hang @@ -1219,6 +1220,17 @@ test_expect_success $PREREQ 'threading but no chain-reply-to' ' grep "In-Reply-To: " stdout ' +test_expect_success $PREREQ 'override in-reply-to if no threading' ' + git send-email \ + --dry-run \ + --from="Example <nobody@example.com>" \ + --to=nobody@example.com \ + --no-thread \ + --in-reply-to="override" \ + $threaded_patches >stdout && + grep "In-Reply-To: <override>" stdout +' + test_expect_success $PREREQ 'sendemail.to works' ' git config --replace-all sendemail.to "Somebody <somebody@ex.com>" && git send-email \ base-commit: 096547052491426a29e040a5bd94d7f8a4cab8ac --- >8 --- > diff --git a/git-send-email.perl b/git-send-email.perl > index dc95656f75..36c47bae1d 100755 > --- a/git-send-email.perl > +++ b/git-send-email.perl > @@ -1699,10 +1699,14 @@ sub process_file { > $xfer_encoding = $1 if not defined $xfer_encoding; > } > elsif (/^In-Reply-To: (.*)/i) { > - $in_reply_to = $1; > + if (!$initial_in_reply_to || $thread) { > + $in_reply_to = $1; > + } > } > elsif (/^References: (.*)/i) { > - $references = $1; > + if (!$initial_in_reply_to || $thread) { > + $references = $1; > + } > } > elsif (!/^Date:\s/i && /^[-A-Za-z]+:\s+\S/) { > push @xh, $_; in both cases, doing `!defined $initial_in_reply_to` might seem like a more consistent option. Carlo ^ permalink raw reply related [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-07-01 22:10 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-06-24 19:55 [PATCH] send-email: restore --in-reply-to superseding behavior Rafael Aquini 2020-06-24 21:33 ` Junio C Hamano 2020-06-24 23:45 ` Rafael Aquini 2020-06-25 18:47 ` Rafael Aquini 2020-06-26 1:08 ` Carlo Arenas 2020-06-26 13:39 ` Rafael Aquini 2020-06-29 14:11 ` [PATCH v2] " Rafael Aquini 2020-07-01 22:10 ` Carlo Marcelo Arenas Belón
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).