send-email: fix bug resulting in broken threads if a message is edited

Whenever we send a thread of emails using send-email, a message number
is internally assigned to each email. This number is used to track the
order of the emails in the thread. Whenever a new message is processed
in a thread, the current script logic increments the message number by
one, which is intended.

But, if a message is edited and then resent, its message number again
gets incremented. This is because the script uses the same logic to
process the edited message, which it uses to send the next message.

This minor bug is usually harmless, unless a special situations arises.
That situation is when the first message in a thread is edited and
resent, and an `--in-reply-to` argument is also passed to send-email.
In this case, if the user has chosen shallow threading, the threading
does not work as expected, and all messages become replies to the
Message-ID specified in the `--in-reply-to` argument.

The reason for this bug is hidden in the code for threading itself.

if ($thread) {
	if ($message_was_sent &&
	  ($chain_reply_to || !defined $in_reply_to || length($in_reply_to) == 0 ||
	  $message_num == 1)) {
		$in_reply_to = $message_id;
		if (length $references > 0) {
			$references .= "\n $message_id";
		} else {
			$references = "$message_id";
		}
	}
}

Here `$message_num` is the current message number, and `$in_reply_to` is
the Message-ID of the message to which the current message is a reply.
In case `--in-reply-to` is specified, the `$in_reply_to` variable
is set to the value of the `--in-reply-to` argument.

Whenever this whole set of conditions is true, the script sets the
`$in_reply_to` variable to the current message's ID. This is done to
ensure that the next message in the thread is a reply to this message.

In case we specify an `--in-reply-to` argument, and have shallow
threading, the only condition that can make this true is
`$message_num == 1`, which is true for the first message in a thread.
Thus, the `$in_reply_to` variable gets set to the first message's ID.
For subsequent messages, the `$message_num` variable is always
greater than 1, and the whole set of conditions is false. Therefore, the
`$in_reply_to` variable remains as the first message's ID. This is what
we expect in shallow threading. But if the user edits the first message
and resends it, the `$message_num` variable gets incremented by 1, and
thus the condition `$message_num == 1` becomes false. This means that
the `$in_reply_to` variable is not set to the first message's ID. As a
result the next message in the thread is not a reply to the first
message, but to the `--in-reply-to` argument, effectively breaking the
threading.

In case the user does not specify an `--in-reply-to` argument, the
`!defined $in_reply_to` condition is true, and thus the `$in_reply_to`
variable is set to the first message's ID, and the threading works
as expected, regardless of the message number.

To fix this bug, we need to ensure that the `$message_num` variable is
not incremented by 1 when a message is edited and resent. We do this by
decreasing the `$message_num` variable by 1 whenever the request to edit
a message is received. This way, the next message in the thread will
have the same message number as the edited message. Therefore the
threading will work as expected.

The same logic has also been applied in case the user drops a single
message from the thread by choosing the "[n]o" option during
confirmation. By doing this, the next message in the thread is assigned
the message number of the dropped message, and thus the threading
works as expected.

Signed-off-by: Aditya Garg <gargaditya08@live.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/git-send-email.perl b/git-send-email.perl
index 659e6c588bcf8792363385643ba17f08e4885d06..5bf4307442b57cf8d653891147e7cfbd6d028a8a 100755 (executable)
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1653,8 +1653,18 @@ EOF
                         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--;
                        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--;
                        return -1;
                } elsif (/^q/i) {
                        cleanup_compose_files();