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

23.08.2021 18:35:14 Jeff King [off-list ref]:

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

quoted

"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.

quoted

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.

quoted

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