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@xxxxxxxxxxx>
+ Subject: patch with in-reply-to
+ Message-ID: <patch.with.in.reply.to@xxxxxxxxxxx>
+ In-Reply-To: <replied.to@xxxxxxxxxxx>
+ References: <replied.to@xxxxxxxxxxx>
+
+ This is the body.
+ EOF
+ cat >no-reply.patch <<-\EOF
+ From: A U Thor <author@xxxxxxxxxxx>
+ Subject: patch without in-reply-to
+ Message-ID: <patch.without.in.reply.to@xxxxxxxxxxx>
+
+ 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@xxxxxxxxxxx \
+ --smtp-server="$(pwd)/fake.sendmail" \
+ has-reply.patch no-reply.patch &&
+ grep "In-Reply-To: <replied.to@xxxxxxxxxxx>" msgtxt1 &&
+ grep "References: <replied.to@xxxxxxxxxxx>" msgtxt1 &&
+ ! grep replied.to@xxxxxxxxxxx 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@xxxxxxxxxxx>" \
+ --to=nobody@xxxxxxxxxxx \
+ --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@xxxxxxxxxxx>" msgtxt2 &&
+ grep "References: <cmdline.reply@xxxxxxxxxxx>" msgtxt2
+'
+
test_expect_success $PREREQ 'invoke hook' '
mkdir -p .git/hooks &&
