[PATCH] send-email: fix bug breaking shallow threading if the first patch is edited

[PATCH] send-email: fix bug breaking shallow threading if the first patch is edited

[Aditya Garg]

There is a bug in send-email that turns off shallow threading if
some special conditions are there. Those conditions are:

1. An --in-reply-to must be specified when sending the patch
2. When asked for confirmation before sending the first patch, the
  user must edit the patch (pressing e and enter).

If these two conditions are fulfilled, the threading will turn off
and all subsequent messages will become as replies to the
Message-ID set in --in-reply-to, rather than becoming replies to
the first patch.

The cause of this bug was very simple. There are many conditions
that determine whether threading should be done or not. The
relevant ones for this case are:

1. --in-reply-to is not defined
2. $message_num is 1

If ANY ONE of these is fulfilled, threading will occur. Now, in
our case, we have defined an --in-reply-to, so condition 1 is
not fulfilled, and thus is omitted out. The only condition that
can enable threading is $message_num being 1. As far as I
understand, this condition was based on the assumption that the
first message being send will have $message_num as 1, since in
case of shallow threads, we just set in-reply-to only for the
Message-ID of the first patch sent. But, in case we edit a patch,
its $message_num increases by one, and thus, our second condition
for threading is also not fulfilled, thus turning off threading.

Luckily, the script also keeps count of the number of messages
actually sent using the $num_sent variable. This was implemented
for people who have set a particular batch size for emails. This
is a more reliable indicator to track the actual first patch.

So, whenever the first patch is sent, $num_sent will become 1.
If we replace the condition to use threading from $message_num
to $num_sent=1, it will always be fulfilled irrespective of
whether the user edits the first patch or not, and thus threading
will turn on.

This bug will not be triggered if --in-reply-to is not set,
because the first condition (not having an --in-reply-to) gets
fulfilled, so the script doesn't care what $message_num is there.

Signed-off-by: Aditya Garg <gargaditya08@live.com>
---
git-send-email.perl | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 55b7e00d29..2cfd61b4b9 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -2041,10 +2041,11 @@ sub process_file {
	}

	# set up for the next message
+	$num_sent++;
	if ($thread) {
		if ($message_was_sent &&
		  ($chain_reply_to || !defined $in_reply_to || length($in_reply_to) == 0 ||
-		  $message_num == 1)) {
+		  $num_sent == 1)) {
			$in_reply_to = $message_id;
			if (length $references > 0) {
				$references .= "\n $message_id";
@@ -2060,7 +2061,6 @@ sub process_file {
		$references = '';
	}
	$message_id = undef;
-	$num_sent++;
	if (defined $batch_size && $num_sent == $batch_size) {
		$num_sent = 0;
		$smtp->quit if defined $smtp;
-- 
2.43.0

Re: [PATCH] send-email: fix bug breaking shallow threading if the first patch is edited

[Aditya Garg]

> On 23 May 2025, at 9:06 PM, Aditya Garg <gargaditya08@live.com> wrote:
> 
> There is a bug in send-email that turns off shallow threading if
> some special conditions are there. Those conditions are:
> 
> 1. An --in-reply-to must be specified when sending the patch
> 2. When asked for confirmation before sending the first patch, the
>  user must edit the patch (pressing e and enter).
> 
> If these two conditions are fulfilled, the threading will turn off
> and all subsequent messages will become as replies to the
> Message-ID set in --in-reply-to, rather than becoming replies to
> the first patch.
> 
> The cause of this bug was very simple. There are many conditions
> that determine whether threading should be done or not. The
> relevant ones for this case are:
> 
> 1. --in-reply-to is not defined
> 2. $message_num is 1
> 
> If ANY ONE of these is fulfilled, threading will occur. Now, in
> our case, we have defined an --in-reply-to, so condition 1 is
> not fulfilled, and thus is omitted out. The only condition that
> can enable threading is $message_num being 1. As far as I
> understand, this condition was based on the assumption that the
> first message being send will have $message_num as 1, since in
> case of shallow threads, we just set in-reply-to only for the
> Message-ID of the first patch sent. But, in case we edit a patch,
> its $message_num increases by one, and thus, our second condition
> for threading is also not fulfilled, thus turning off threading.
> 
> Luckily, the script also keeps count of the number of messages
> actually sent using the $num_sent variable. This was implemented
> for people who have set a particular batch size for emails. This
> is a more reliable indicator to track the actual first patch.
> 
> So, whenever the first patch is sent, $num_sent will become 1.
> If we replace the condition to use threading from $message_num
> to $num_sent=1, it will always be fulfilled irrespective of
> whether the user edits the first patch or not, and thus threading
> will turn on.
> 
> This bug will not be triggered if --in-reply-to is not set,
> because the first condition (not having an --in-reply-to) gets
> fulfilled, so the script doesn't care what $message_num is there.
> 
> Signed-off-by: Aditya Garg <gargaditya08@live.com>

BTW, this patch is also my first successful trial with imap-send
using XOAUTH2 and Apple Mail as the sender ;)

Re: [PATCH] send-email: fix bug breaking shallow threading if the first patch is edited

[Junio C Hamano]

Aditya Garg <gargaditya08@live.com> writes:

> So, whenever the first patch is sent, $num_sent will become 1.

The reverse is not always true, though.

> 	# set up for the next message
> +	$num_sent++;
> 	if ($thread) {
> 		if ($message_was_sent &&
> 		  ($chain_reply_to || !defined $in_reply_to || length($in_reply_to) == 0 ||
> -		  $message_num == 1)) {
> +		  $num_sent == 1)) {

This sais "enter this block if we have sent a message and one of
(num_set is 1, or we are told to chain-reply-to, or we do not have
in-reply-to) holds true".

But is $num_set == 1 really limited to "the first message"?  Given
that ...

> 			$in_reply_to = $message_id;
> 			if (length $references > 0) {
> 				$references .= "\n $message_id";
> @@ -2060,7 +2061,6 @@ sub process_file {
> 		$references = '';
> 	}
> 	$message_id = undef;
> -	$num_sent++;
> 	if (defined $batch_size && $num_sent == $batch_size) {
> 		$num_sent = 0;

... the counter is reset when we send out the batch_size message
(and we sleep in this block, which is outside the post-context of
this hunk).  So when you send the first message of the next batch,
you'd do the same, no?  By that time, we have in_reply_to set, but
that does not prevent from $num_sent, which was reset to 0 at the
batch boundary and then incremented to 1, to reenter the block in
the first hunk, no?

> 		$smtp->quit if defined $smtp;

Re: [PATCH] send-email: fix bug breaking shallow threading if the first patch is edited

[Jacob Keller]

On 5/23/2025 8:36 AM, Aditya Garg wrote:
> There is a bug in send-email that turns off shallow threading if
> some special conditions are there. Those conditions are:
> 
> 1. An --in-reply-to must be specified when sending the patch
> 2. When asked for confirmation before sending the first patch, the
>   user must edit the patch (pressing e and enter).
> 
> If these two conditions are fulfilled, the threading will turn off
> and all subsequent messages will become as replies to the
> Message-ID set in --in-reply-to, rather than becoming replies to
> the first patch.
> 
> The cause of this bug was very simple. There are many conditions
> that determine whether threading should be done or not. The
> relevant ones for this case are:
> 
> 1. --in-reply-to is not defined
> 2. $message_num is 1
> 
> If ANY ONE of these is fulfilled, threading will occur. Now, in
> our case, we have defined an --in-reply-to, so condition 1 is
> not fulfilled, and thus is omitted out. The only condition that
> can enable threading is $message_num being 1. As far as I
> understand, this condition was based on the assumption that the
> first message being send will have $message_num as 1, since in
> case of shallow threads, we just set in-reply-to only for the
> Message-ID of the first patch sent. But, in case we edit a patch,
> its $message_num increases by one, and thus, our second condition
> for threading is also not fulfilled, thus turning off threading.
> 

Why does editing a message change the message_num??? That feels like the
real bug to me..

> Luckily, the script also keeps count of the number of messages
> actually sent using the $num_sent variable. This was implemented
> for people who have set a particular batch size for emails. This
> is a more reliable indicator to track the actual first patch.
> 
> So, whenever the first patch is sent, $num_sent will become 1.
> If we replace the condition to use threading from $message_num
> to $num_sent=1, it will always be fulfilled irrespective of
> whether the user edits the first patch or not, and thus threading
> will turn on.
> 

Just from reading the commit message, this smells like a hack or
workaround to me.

I don't fully understand the logic in place as-is, and I think using
num_sent is a bit of a weird way to resolve this.

[PATCH v2] send-email: fix bug resulting in increased message number if a message is edited

[Aditya Garg]

In case a message is edited before it is sent, its message number gets
increased by 1, and so does its order in the message id. The cause of
this bug was that when a person attempts to edit the message, the whole
sub process_file gets terminated, and the user is asked to edit the message.
After necessary edits are done, the whole sub process_file is executed again.
The way sub process_file is designed, every time is runs, it increases the
$message_num variable by 1. The reason for this was that the function ran
again everytime a next message was sent in a thread, and thus we need to
increase the message number for that message. In case a user edits the message,
there is no check for the same and the new message gets treated as a subsequent
message of a thread, therefore increasing its message number by one. This
breaks the shallow thread logic which relies on $message_num being 1 for the
first message, and it gets changed in case the user edits the first message.

So, upon scanning the whole code, there are two significant variables at play
here. First is $message_num, responsible for the message number and second
is $message_id_serial, responsible for showing the message number in the
Message-ID header. So, whenever we edit a message, lets just decrease them
by 1, so that when the whole process to compose and send the message starts,
these variables increase by 1 again, thus get set to the original values for
that message.

We also are doing the same thing in case the user chooses to not send a message
out of many messages in a thread. By doing so, we will simply decrease these
variables by 1 for further messages, thus ensuring the whole thread doesn't
break.

Signed-off-by: Aditya Garg <gargaditya08@live.com>
---
 git-send-email.perl | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/git-send-email.perl b/git-send-email.perl
index 55b7e00d29..b09251c4fc 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1639,8 +1639,20 @@ sub send_message {
 		         default => $ask_default);
 		die __("Send this email reply required") unless defined $_;
 		if (/^n/i) {
+			# If we are skipping a message, we should make sure that
+			# the next message is treated as the successor to the
+			# previously sent message, and not the skipped message.
+			$message_num--;
+			$message_id_serial--;
 			return 0;
 		} elsif (/^e/i) {
+			# Since the same message will be sent again, we need to
+			# decrement the message number to the previous message.
+			# Otherwise, the edited message will be treated as a
+			# different message sent after the original non-edited
+			# message.
+			$message_num--;
+			$message_id_serial--;
 			return -1;
 		} elsif (/^q/i) {
 			cleanup_compose_files();
-- 
2.49.0.windows.1

[PATCH v3 0/2] send-email: fix threads breaking in case user edits emails and improvements to outlook ID fix.

[Aditya Garg]

Hi all,

This patch series fixes two minor issues with git-send-email.

The first patch fixes a bug that caused the message number to increase
when a user edits an email. As a result of this bug, threads would
break when a user edits an email.

The second patch improves the logging of the new message ID assigned by
Outlook when a user edits an email.

Aditya Garg (2):
  send-email: fix bug resulting in increased message number if a message
    is edited
  send-email: show the new message id assigned by outlook in the logs

 git-send-email.perl | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

-- 
2.43.0

[PATCH v3 1/2] send-email: fix bug resulting in increased message number if a message is edited

[Aditya Garg]

In case a message is edited before it is sent, its message number gets
increased by 1, and so does its order in the message id. The cause of
this bug was that when a person attempts to edit the message, the whole
sub process_file gets terminated, and the user is asked to edit the message.
After necessary edits are done, the whole sub process_file is executed again.
The way sub process_file is designed, every time is runs, it increases the
$message_num variable by 1. The reason for this was that the function ran
again everytime a next message was sent in a thread, and thus we need to
increase the message number for that message. In case a user edits the message,
there is no check for the same and the new message gets treated as a subsequent
message of a thread, therefore increasing its message number by one. This
breaks the shallow thread logic which relies on $message_num being 1 for the
first message, and it gets changed in case the user edits the first message.

So, upon scanning the whole code, there are two significant variables at play
here. First is $message_num, responsible for the message number and second
is $message_id_serial, responsible for showing the message number in the
Message-ID header. So, whenever we edit a message, lets just decrease them
by 1, so that when the whole process to compose and send the message starts,
these variables increase by 1 again, thus get set to the original values for
that message.

We also are doing the same thing in case the user chooses to not send a message
out of many messages in a thread. By doing so, we will simply decrease these
variables by 1 for further messages, thus ensuring the whole thread doesn't
break.

Signed-off-by: Aditya Garg <gargaditya08@live.com>
---
 git-send-email.perl | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/git-send-email.perl b/git-send-email.perl
index 55b7e00d29..b09251c4fc 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1639,8 +1639,20 @@ sub send_message {
 		         default => $ask_default);
 		die __("Send this email reply required") unless defined $_;
 		if (/^n/i) {
+			# If we are skipping a message, we should make sure that
+			# the next message is treated as the successor to the
+			# previously sent message, and not the skipped message.
+			$message_num--;
+			$message_id_serial--;
 			return 0;
 		} elsif (/^e/i) {
+			# Since the same message will be sent again, we need to
+			# decrement the message number to the previous message.
+			# Otherwise, the edited message will be treated as a
+			# different message sent after the original non-edited
+			# message.
+			$message_num--;
+			$message_id_serial--;
 			return -1;
 		} elsif (/^q/i) {
 			cleanup_compose_files();
-- 
2.43.0

Re: [PATCH v3 1/2] send-email: fix bug resulting in increased message number if a message is edited

[Kristoffer Haugsbakk]

Hi

> send-email: fix bug resulting in increased message number if a message is edited

I don’t understand what the bug is from the title.  “Message number”
sounds harmless.  It breaks the threading?  The summary/subject could
say that instead.  Fix threading bug.

On Sun, May 25, 2025, at 19:12, Aditya Garg wrote:
> In case a message is edited before it is sent, its message number gets
> increased by 1, and so does its order in the message id.

It feels like this part about increasing by one and if-editing gets
repeated at least two times in this paragraph.

> The cause of this bug was that when a person attempts to edit the
> message, the whole sub process_file gets terminated, and the user is
> asked to edit the message.

Here’s the repetition.

Also I am not familiar with the code.  Just testing it I get this `6` here:

    Message-ID: <20250525181003.40129-6-kristofferhaugsbakk@fastmail.com>

Which was incremented every time I did an edit with:

    send-email --suppress-cc=all --to=<me> \
        --confirm=always one two

But that turned out to be benign in my simple case since the next email
used the correct In-Reply-To.

So at this point (reading the paragraph) I don’t know what the bug is.

> After necessary edits are done, the whole sub process_file is executed again.
> The way sub process_file is designed, every time is runs, it increases the
> $message_num variable by 1. The reason for this was that the function ran
> again everytime a next message was sent in a thread, and thus we need to
> increase the message number for that message. In case a user edits the message,
> there is no check for the same and the new message gets treated as a subsequent
> message of a thread, therefore increasing its message number by one.

This feels like repetition again.  You say that a variable is
incremented because the message is edited.

> This breaks the shallow thread logic which relies on $message_num
> being 1 for the first message, and it gets changed in case the user
> edits the first message.

If I’m right in my assumption that this number is the `4` here:

    Message-ID: <20250525182426.41076-4-kristofferhaugsbakk@fastmail.com>

This was the first proposed email I got with “shallow thread” (all in
reply to first):

    git send-email --suppress-cc=all --to=<me> \
        --thread --no-chain-reply-to --confirm=always one two three

Then I edit all the messages.  They still all manage to refer to the
first message id in the thread.

I still don’t understand what the bug is.

❦

    $ git diagnose
    Collecting diagnostic info

    git version 2.49.0.780.g892193c3f50
    cpu: x86_64
    built from commit: 892193c3f509fb8a9e4e7a5a19a2e24137befda8
    sizeof-long: 8
    sizeof-size_t: 8
    shell-path: /bin/sh
    libcurl: 7.81.0
    OpenSSL: OpenSSL 3.0.2 15 Mar 2022
    zlib: 1.2.11
    SHA-1: SHA1_DC
    SHA-256: SHA256_BLK
    Repository root: /home/kristoffer/programming/git
    Available space on '/home/kristoffer/programming/git': 200.56 GiB (mount flags 0x1000)

Re: [PATCH v3 1/2] send-email: fix bug resulting in increased message number if a message is edited

[Aditya Garg]

On 26/05/25 12:19 am, Kristoffer Haugsbakk wrote:
> Hi
> 
>> send-email: fix bug resulting in increased message number if a message is edited
> 
> I don’t understand what the bug is from the title.  “Message number”
> sounds harmless.  It breaks the threading?  The summary/subject could
> say that instead.  Fix threading bug.
> 
> On Sun, May 25, 2025, at 19:12, Aditya Garg wrote:
>> In case a message is edited before it is sent, its message number gets
>> increased by 1, and so does its order in the message id.
> 
> It feels like this part about increasing by one and if-editing gets
> repeated at least two times in this paragraph.
> 
>> The cause of this bug was that when a person attempts to edit the
>> message, the whole sub process_file gets terminated, and the user is
>> asked to edit the message.
> 
> Here’s the repetition.
> 
> Also I am not familiar with the code.  Just testing it I get this `6` here:
> 
>     Message-ID: <20250525181003.40129-6-kristofferhaugsbakk@fastmail.com>
> 
> Which was incremented every time I did an edit with:
> 
>     send-email --suppress-cc=all --to=<me> \
>         --confirm=always one two
> 

I have re-written the whole message again, but forgot to --in-reply-to this
thread :(

https://lore.kernel.org/git/cover.1748274404.git.gargaditya08@live.com/T/#t
should have the v4.

[PATCH v3 2/2] send-email: show the new message id assigned by outlook in the logs

[Aditya Garg]

Whenever an email is sent, send-email shows a log at last, which
contains all the headers of the email that was send successfully.
In case outlook changes the Message-ID, a log for the same is
shown to the user, but that change is not reflected when the log
containing all the headers is displayed.

This patch fixes this by modifying the $header variable, which is
responsible for showing the logs at the end. Also, the log which
states that the Message-ID has been changed will now be shown only
when smtp-debug is enabled, since the main log having all of the
headers is anyways displaying the new Message-ID.

Signed-off-by: Aditya Garg <gargaditya08@live.com>
---
 git-send-email.perl | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index b09251c4fc..e8019c40ba 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1776,7 +1776,9 @@ sub send_message {
 		if (is_outlook($smtp_server)) {
 			if ($smtp->message =~ /<([^>]+)>/) {
 				$message_id = "<$1>";
-				printf __("Outlook reassigned Message-ID to: %s\n"), $message_id;
+				# Replace the original Message-ID in $header with the new one
+				$header =~ s/^(Message-ID:\s*).*\n/${1}$message_id\n/m;
+				printf __("Outlook reassigned Message-ID to: %s\n"), $message_id if $smtp->debug;
 			} else {
 				warn __("Warning: Could not retrieve Message-ID from server response.\n");
 			}
-- 
2.43.0

Re: [PATCH] send-email: fix bug breaking shallow threading if the first patch is edited

[Aditya Garg]

> On 24 May 2025, at 3:49 AM, Junio C Hamano <gitster@pobox.com> wrote:
> 
> Aditya Garg <gargaditya08@live.com> writes:
> 
>> So, whenever the first patch is sent, $num_sent will become 1.
> 
> The reverse is not always true, though.
> 
>>    # set up for the next message
>> +    $num_sent++;
>>    if ($thread) {
>>        if ($message_was_sent &&
>>          ($chain_reply_to || !defined $in_reply_to || length($in_reply_to) == 0 ||
>> -          $message_num == 1)) {
>> +          $num_sent == 1)) {
> 
> This sais "enter this block if we have sent a message and one of
> (num_set is 1, or we are told to chain-reply-to, or we do not have
> in-reply-to) holds true".
> 
> But is $num_set == 1 really limited to "the first message"?  Given
> that ...
> 
>>            $in_reply_to = $message_id;
>>            if (length $references > 0) {
>>                $references .= "\n $message_id";
>> @@ -2060,7 +2061,6 @@ sub process_file {
>>        $references = '';
>>    }
>>    $message_id = undef;
>> -    $num_sent++;
>>    if (defined $batch_size && $num_sent == $batch_size) {
>>        $num_sent = 0;
> 
> ... the counter is reset when we send out the batch_size message
> (and we sleep in this block, which is outside the post-context of
> this hunk).  So when you send the first message of the next batch,
> you'd do the same, no?  By that time, we have in_reply_to set, but
> that does not prevent from $num_sent, which was reset to 0 at the
> batch boundary and then incremented to 1, to reenter the block in
> the first hunk, no?
> 

Hmm, you are right, that's a problem. I think we need to fix message
number then.

Re: [PATCH] send-email: fix bug breaking shallow threading if the first patch is edited

[Aditya Garg]

> On 24 May 2025, at 5:08 AM, Jacob Keller <jacob.e.keller@intel.com> wrote:
> 
> 
> 
>> On 5/23/2025 8:36 AM, Aditya Garg wrote:
>> There is a bug in send-email that turns off shallow threading if
>> some special conditions are there. Those conditions are:
>> 
>> 1. An --in-reply-to must be specified when sending the patch
>> 2. When asked for confirmation before sending the first patch, the
>>  user must edit the patch (pressing e and enter).
>> 
>> If these two conditions are fulfilled, the threading will turn off
>> and all subsequent messages will become as replies to the
>> Message-ID set in --in-reply-to, rather than becoming replies to
>> the first patch.
>> 
>> The cause of this bug was very simple. There are many conditions
>> that determine whether threading should be done or not. The
>> relevant ones for this case are:
>> 
>> 1. --in-reply-to is not defined
>> 2. $message_num is 1
>> 
>> If ANY ONE of these is fulfilled, threading will occur. Now, in
>> our case, we have defined an --in-reply-to, so condition 1 is
>> not fulfilled, and thus is omitted out. The only condition that
>> can enable threading is $message_num being 1. As far as I
>> understand, this condition was based on the assumption that the
>> first message being send will have $message_num as 1, since in
>> case of shallow threads, we just set in-reply-to only for the
>> Message-ID of the first patch sent. But, in case we edit a patch,
>> its $message_num increases by one, and thus, our second condition
>> for threading is also not fulfilled, thus turning off threading.
>> 
> 
> Why does editing a message change the message_num??? That feels like the
> real bug to me..

I thought that was intended, not it doesn't seem like that to me.