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

["Marvin Häuser" <mhaeuser@posteo.de>]

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