[BUG] send-email propagates "In-Reply-To"

[BUG] send-email propagates "In-Reply-To"

[Marvin Häuser]

Good day everyone,

"git send-email" propagates the "In-Reply-To" header of the last prior 
patch with such defined to subsequent patches which do not define such 
explicitly. I suspect this behaviour is incorrect as I could not find 
any documentation on this. I'm sorry if this behaviour is actually 
expected, and would be happy if someone could point me to the 
appropriate documentation. This was reproduced on Fedora 34 with git 
2.33.0 and "--no-thread".

Steps to reproduce:
1. Create two patches, one of which has an "In-Reply-To" field 
("patch-in-reply.patch") and one of which does not 
("patch-no-in-reply.patch").
2. Run "git send-email --dry-run --no-thread patch-in-reply.patch 
patch-no-in-reply.patch"
2.1. Observe the emission of an "In-Reply-To" header for 
"patch-no-in-reply.patch" with no such header.
3. Run "git send-email --dry-run --no-thread patch-no-in-reply.patch 
patch-in-reply.patch"
3.1. Observe the omission of an "In-Reply-To" header for 
"patch-no-in-reply.patch" with no such header.

Expected behaviour:
With no threading and no other sorts of explicitly defining the 
"In-Reply-To" header, I expect to always observe the behaviour of 3.1., 
and to not observe the behaviour of 2.1.

The "issue" is "in_reply_to" is overwritten here [1], which is the main 
loop worker to process all files passed to send-email [2], but it is not 
restored for subsequent patches. Unless required otherwise (e.g. 
send-email threading), it should be restored to its default value for 
each patch I believe.

I wrote a quick patch to adjust 2.1. to 3.1. [3]. I have no time right 
now to review the submission guidelines (and thus did not submit the 
patch "properly" yet), but I will try to get to that tonight or some 
time next week. If in the mean time you could provide any feedback on 
the behaviour or the patch, so that I can get things right the first 
time, that would be great!

Thank you for your time, I am looking forward to your feedback.

Best regards,
Marvin


[1] 
https://github.com/git/git/blob/225bc32a989d7a22fa6addafd4ce7dcd04675dbf/git-send-email.perl#L1800

[2] 
https://github.com/git/git/blob/225bc32a989d7a22fa6addafd4ce7dcd04675dbf/git-send-email.perl#L1952-L1956

[3] 
https://github.com/mhaeuser/git/commit/d87f49a02c0efa3084ae6c70bbf583b865744e43

Re: [BUG] send-email propagates "In-Reply-To"

[Bagas Sanjaya]

On 22/08/21 16.24, Marvin Häuser wrote:
> I wrote a quick patch to adjust 2.1. to 3.1. [3]. I have no time right 
> now to review the submission guidelines (and thus did not submit the 
> patch "properly" yet), but I will try to get to that tonight or some 
> time next week. If in the mean time you could provide any feedback on 
> the behaviour or the patch, so that I can get things right the first 
> time, that would be great!
> 
> [3] 
> https://github.com/mhaeuser/git/commit/d87f49a02c0efa3084ae6c70bbf583b865744e43 
> 

Since you have your desired commits queued on your fork (and thus 
already use GitHub flow), you can open a PR targeting [1] and let 
GitGitGadget prepare and send corresponding patch to git@vger.kernel.org 
(this ML).

Or better learn to use `git format-patch` and `git send-email` - some 
projects (including Git and Linux kernel) prefers contributions to be 
submitted through mailing lists (email-style approach). Git for Windows 
have nice guide to submit patches email-style at [2].

[1]: https://github.com/gitgitgadget/git
[2]: 
https://github.com/git-for-windows/git/blob/main/CONTRIBUTING.md#submit-your-patch

-- 
An old man doll... just what I always wanted! - Clara

Re: [BUG] send-email propagates "In-Reply-To"

[Jeff King]

On Sun, Aug 22, 2021 at 09:24:12AM +0000, Marvin Häuser wrote:

> "git send-email" propagates the "In-Reply-To" header of the last prior patch
> with such defined to subsequent patches which do not define such explicitly.
> I suspect this behaviour is incorrect as I could not find any documentation
> on this. I'm sorry if this behaviour is actually expected, and would be
> happy if someone could point me to the appropriate documentation. This was
> reproduced on Fedora 34 with git 2.33.0 and "--no-thread".
> 
> Steps to reproduce:
> 1. Create two patches, one of which has an "In-Reply-To" field
> ("patch-in-reply.patch") and one of which does not
> ("patch-no-in-reply.patch").
> 2. Run "git send-email --dry-run --no-thread patch-in-reply.patch
> patch-no-in-reply.patch"
> 2.1. Observe the emission of an "In-Reply-To" header for
> "patch-no-in-reply.patch" with no such header.
> 3. Run "git send-email --dry-run --no-thread patch-no-in-reply.patch
> patch-in-reply.patch"
> 3.1. Observe the omission of an "In-Reply-To" header for
> "patch-no-in-reply.patch" with no such header.
> 
> Expected behaviour:
> With no threading and no other sorts of explicitly defining the
> "In-Reply-To" header, I expect to always observe the behaviour of 3.1., and
> to not observe the behaviour of 2.1.

Yes, I think this is just a bug. If the user requested --no-thread, then
we should be sending each patch as it appears on disk, with no
modification to the in-reply-to header.

> The "issue" is "in_reply_to" is overwritten here [1], which is the main loop
> worker to process all files passed to send-email [2], but it is not restored
> for subsequent patches. Unless required otherwise (e.g. send-email
> threading), it should be restored to its default value for each patch I
> believe.

Right. Most of the values we parse are declared inside the process_file
function, and so start empty. $in_reply_to is special in that we need to
carry its value across multiple files, so we need to always handle it in
each loop iteration, whether we are setting it to a new value or
resetting it to null.

> I wrote a quick patch to adjust 2.1. to 3.1. [3]. I have no time right now
> to review the submission guidelines (and thus did not submit the patch
> "properly" yet), but I will try to get to that tonight or some time next
> week. If in the mean time you could provide any feedback on the behaviour or
> the patch, so that I can get things right the first time, that would be
> great!

Your patch looks like the right direction. Handling $references at the
same time is the right thing to do. The extra setting of $subject seems
weird, though.

We'd want to add a test to t/t9001-send-email.sh, too, I'd think.

-Peff

Re: [BUG] send-email propagates "In-Reply-To"

[Marvin Häuser]

23.08.2021 18:35:14 Jeff King <peff@peff.net>:

> On Sun, Aug 22, 2021 at 09:24:12AM +0000, Marvin Häuser wrote:
>
>> "git send-email" propagates the "In-Reply-To" header of the last prior patch
>> with such defined to subsequent patches which do not define such explicitly.
>> I suspect this behaviour is incorrect as I could not find any documentation
>> on this. I'm sorry if this behaviour is actually expected, and would be
>> happy if someone could point me to the appropriate documentation. This was
>> reproduced on Fedora 34 with git 2.33.0 and "--no-thread".
>>
>> Steps to reproduce:
>> 1. Create two patches, one of which has an "In-Reply-To" field
>> ("patch-in-reply.patch") and one of which does not
>> ("patch-no-in-reply.patch").
>> 2. Run "git send-email --dry-run --no-thread patch-in-reply.patch
>> patch-no-in-reply.patch"
>> 2.1. Observe the emission of an "In-Reply-To" header for
>> "patch-no-in-reply.patch" with no such header.
>> 3. Run "git send-email --dry-run --no-thread patch-no-in-reply.patch
>> patch-in-reply.patch"
>> 3.1. Observe the omission of an "In-Reply-To" header for
>> "patch-no-in-reply.patch" with no such header.
>>
>> Expected behaviour:
>> With no threading and no other sorts of explicitly defining the
>> "In-Reply-To" header, I expect to always observe the behaviour of 3.1., and
>> to not observe the behaviour of 2.1.
>
> Yes, I think this is just a bug. If the user requested --no-thread, then
> we should be sending each patch as it appears on disk, with no
> modification to the in-reply-to header.

OK, thank you.

>
>> The "issue" is "in_reply_to" is overwritten here [1], which is the main loop
>> worker to process all files passed to send-email [2], but it is not restored
>> for subsequent patches. Unless required otherwise (e.g. send-email
>> threading), it should be restored to its default value for each patch I
>> believe.
>
> Right. Most of the values we parse are declared inside the process_file
> function, and so start empty. $in_reply_to is special in that we need to
> carry its value across multiple files, so we need to always handle it in
> each loop iteration, whether we are setting it to a new value or
> resetting it to null.
>
>> I wrote a quick patch to adjust 2.1. to 3.1. [3]. I have no time right now
>> to review the submission guidelines (and thus did not submit the patch
>> "properly" yet), but I will try to get to that tonight or some time next
>> week. If in the mean time you could provide any feedback on the behaviour or
>> the patch, so that I can get things right the first time, that would be
>> great!
>
> Your patch looks like the right direction. Handling $references at the
> same time is the right thing to do. The extra setting of $subject seems
> weird, though.

Basically the idea is to either 1) advance the values (for threading) or 2) reset them to their defaults, i.e. never just preserve them. I neither know Perl nor the git design well, so I just applied the pattern to the subject as well. Should I just drop it? What would be the potential issues with enforcing the pattern?

>
> We'd want to add a test to t/t9001-send-email.sh, too, I'd think.

I'm not home and I might need a few days to look into this. Any helping hand is greatly appreciated. :)

Thanks!

Best regards,
Marvin

>
> -Peff

Re: [BUG] send-email propagates "In-Reply-To"

[Jeff King]

On Mon, Aug 23, 2021 at 05:44:32PM +0000, Marvin Häuser wrote:

> > Your patch looks like the right direction. Handling $references at the
> > same time is the right thing to do. The extra setting of $subject seems
> > weird, though.
> 
> Basically the idea is to either 1) advance the values (for threading)
> or 2) reset them to their defaults, i.e. never just preserve them. I
> neither know Perl nor the git design well, so I just applied the
> pattern to the subject as well. Should I just drop it? What would be
> the potential issues with enforcing the pattern?

I'm not all that familiar with the innards of git-send-email.perl
either. Looking closer, I see $subject is indeed declared outside of the
regular loop, so it would need to be reset (just like $message_id is). I
guess nobody noticed because patches almost always have their own
"Subject:" header, which would override it.

But either that should go into its own patch, or the commit message
should be modified to explain that it is covering not just
in-reply-to/references, but we think this fixes all similar variables.

> > We'd want to add a test to t/t9001-send-email.sh, too, I'd think.
> 
> I'm not home and I might need a few days to look into this. Any
> helping hand is greatly appreciated. :)

You'd want something like the patch below (and possibly something
similar for the $subject handling).

Both of the new tests fail without your patch and pass with it, but:

  - note the weird behavior I found with --in-reply-to; this is
    something we might want to address at the same time

  - applying your patch fails the earlier t9001.52 ("In-Reply-To without
    --chain-reply-to"). I didn't dig into what's going on there.

---
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 57fc10e7f8..747872d5be 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -2227,6 +2227,56 @@ test_expect_success $PREREQ 'test shell expression with --sendmail-cmd' '
 	test_path_is_file commandline1
 '
 
+test_expect_success $PREREQ 'set up in-reply-to/references patches' '
+	cat >has-reply.patch <<-\EOF &&
+	From: A U Thor <author@example.com>
+	Subject: patch with in-reply-to
+	Message-ID: <patch.with.in.reply.to@example.com>
+	In-Reply-To: <replied.to@example.com>
+	References: <replied.to@example.com>
+
+	This is the body.
+	EOF
+	cat >no-reply.patch <<-\EOF
+	From: A U Thor <author@example.com>
+	Subject: patch without in-reply-to
+	Message-ID: <patch.without.in.reply.to@example.com>
+
+	This is the body.
+	EOF
+'
+
+test_expect_success $PREREQ 'patch reply headers not carried over with --no-thread' '
+	clean_fake_sendmail &&
+	git send-email \
+		--no-thread \
+		--to=nobody@example.com \
+		--smtp-server="$(pwd)/fake.sendmail" \
+		has-reply.patch no-reply.patch &&
+	grep "In-Reply-To: <replied.to@example.com>" msgtxt1 &&
+	grep "References: <replied.to@example.com>" msgtxt1 &&
+	! grep replied.to@example.com msgtxt2
+'
+
+test_expect_success $PREREQ 'cmdline in-reply-to used with --no-thread' '
+	clean_fake_sendmail &&
+	git send-email \
+		--no-thread \
+		--in-reply-to="<cmdline.reply@example.com>" \
+		--to=nobody@example.com \
+		--smtp-server="$(pwd)/fake.sendmail" \
+		has-reply.patch no-reply.patch &&
+	# what should happen with has-reply.patch here? The
+	# current behavior seems to throw away its in-file
+	# in-reply-to entirely in favor of the command line
+	# one. That seems like a bug?
+	#
+	# But definitely the second message should get the cmdline
+	# value, and not be empty.
+	grep "In-Reply-To: <cmdline.reply@example.com>" msgtxt2 &&
+	grep "References: <cmdline.reply@example.com>" msgtxt2
+'
+
 test_expect_success $PREREQ 'invoke hook' '
 	mkdir -p .git/hooks &&
 

Re: [BUG] send-email propagates "In-Reply-To"

[Marvin Häuser]

On 23/08/2021 20:27, Jeff King wrote:
> But either that should go into its own patch, or the commit message
> should be modified to explain that it is covering not just
> in-reply-to/references, but we think this fixes all similar variables.

Fixed, opted for latter [1].

> You'd want something like the patch below (and possibly something
> similar for the $subject handling).

Thanks a lot! my last question for the patch would now be, how do I use 
your snippet? Do I add you to S-o-b of the single patch, do I split the 
patches with the second S-o-b being yours, or do I submit only the first 
and you will submit the second?

> Both of the new tests fail without your patch and pass with it, but:
>
>    - note the weird behavior I found with --in-reply-to; this is
>      something we might want to address at the same time

I think this case must error? The definition of the "--in-reply-to" does 
not declare it as a default, so it must be enforced (and it is), but 
it's also very unintuitive the file value is discarded. Who would decide 
the behaviour spec?

>    - applying your patch fails the earlier t9001.52 ("In-Reply-To without
>      --chain-reply-to"). I didn't dig into what's going on there.

Fixed [1].

Best regards,
Marvin


[1] 
https://github.com/mhaeuser/git/commit/5f2ff790cc0d0d779bc252b08f9c9c632c4ff01c

Re: [BUG] send-email propagates "In-Reply-To"

[Carlo Marcelo Arenas Belón]

On Mon, Aug 23, 2021 at 09:47:28PM +0000, Marvin Häuser wrote:
> On 23/08/2021 20:27, Jeff King wrote:
> > But either that should go into its own patch, or the commit message
> > should be modified to explain that it is covering not just
> > in-reply-to/references, but we think this fixes all similar variables.
> 
> Fixed, opted for latter [1].

Left a comment in github; dropping the "Subject" move would be also
needed to fully do that (the fix doesn't need it)

> >    - note the weird behavior I found with --in-reply-to; this is
> >      something we might want to address at the same time
> 
> I think this case must error? The definition of the "--in-reply-to" does not
> declare it as a default, so it must be enforced (and it is), but it's also
> very unintuitive the file value is discarded. Who would decide the behaviour
> spec?

AFAIK, this is what the cryptic warning in the documentation[1] talks about.

the "argument" being that most of the times that header is incorrect
(because it was incorrectly set by format-patch) or it is meant to be wiped out
to keep this patch together with the rest in the reroll.

the warning was added with f693b7e9a5 (Improve doc for format-patch threading
options., 2009-07-22), so throwing an error now might not be wise, but maybe
a documentation update.

Carlo

[1] https://git-scm.com/docs/git-send-email#Documentation/git-send-email.txt---no-thread

-------- >8 ----------
diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index 3db4eab4ba..76687d0574 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -385,10 +385,10 @@ default to --thread.
 It is up to the user to ensure that no In-Reply-To header already
 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
+Failure to do so will result in that header being reset, accordingly
+to the options used and may not produce the expected result in the
 recipient's MUA.
 
-
 Administering
 ~~~~~~~~~~~~~
 

Re: [BUG] send-email propagates "In-Reply-To"

[Jeff King]

On Mon, Aug 23, 2021 at 09:47:28PM +0000, Marvin Häuser wrote:

> > You'd want something like the patch below (and possibly something
> > similar for the $subject handling).
> 
> Thanks a lot! my last question for the patch would now be, how do I use your
> snippet? Do I add you to S-o-b of the single patch, do I split the patches
> with the second S-o-b being yours, or do I submit only the first and you
> will submit the second?

The usual thing is to just squash it into your patch. You can say
something like "Test cases from Jeff King." in the commit message to
give credit (though I am happy either way). And then ideally you'd have
my sign-off to make it clear the code matches the DCO. So here it is
(and you can just stick this before your sign-off at the end of the
commit message):

Signed-off-by: Jeff King <peff@peff.net>

And then for the rest, you can follow the guide in
Documentation/SubmittingPatches. Presumably you're comfortable using
send-email, given the topic of the patch. :) But since you're got the
patch on GitHub already, you can also use https://gitgitgadget.github.io.

> > Both of the new tests fail without your patch and pass with it, but:
> > 
> >    - note the weird behavior I found with --in-reply-to; this is
> >      something we might want to address at the same time
> 
> I think this case must error? The definition of the "--in-reply-to" does not
> declare it as a default, so it must be enforced (and it is), but it's also
> very unintuitive the file value is discarded. Who would decide the behaviour
> spec?

Given the text Carlo found in the documentation, I think this is the
intended behavior. So I think we can make it part of the expected
behavior in the test. I do still think that second test is worth having.
An obvious-but-wrong thing to do here would be to set $in_reply_to to
undef after the first iteration through the loop, rather than the value
given on the command-line. The test makes sure we get that right.

-Peff