Re: [BUG] send-email propagates "In-Reply-To"
- From
Jeff King <peff@peff.net>
- Date
- Aug 23, 2021, 18:27 UTC
- Message-ID
- <YSPomC95hxZZTHRe@coredump.intra.peff.net>
- In-Reply-To
- <5cd5a58b-ac9e-4628-a8d3-836b1f795732@posteo.de>
On Mon, Aug 23, 2021 at 05:44:32PM +0000, Marvin Häuser wrote:
Show 9 quoted lines
> > 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 &&