Message ID | 20200624195520.2062298-1-aquini@redhat.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | send-email: restore --in-reply-to superseding behavior | expand |
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, $_;
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, $_;
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
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
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
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, $_;
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(-)