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

> On 29 May 2025, at 4:22 AM, Junio C Hamano <gitster@pobox.com> wrote:
> 
> Aditya Garg <gargaditya08@live.com> writes:
> 
>> Subject: Re: [PATCH v5 1/2] send-email: fix bug resulting in increased message number if a message is edited
> 
> Is this the same title Kristoffer said that it does not give much
> meaningful information, to which you said you "have re-written the
> whole message"?

> 
> cf. <PN3PR01MB95973C8CEC731B43816AB38CB865A@PN3PR01MB9597.INDPRD01.PROD.OUTLOOK.COM>
> 
>> 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 increases the message number by
>> one, which is intended.
>> 
>> But, if a message is edited and then resent, its message number again
>> gets increased. This is because the script uses the same logic to
>> process the edited message, which it uses to send the next message.
> 
> "increase" -> "increment" is more common to count up by one.

> 
>> This minor bug is usually harmless, unless some special situations arise.
>> One such situation is when the first message in a thread is edited
> 
> Which makes it sound like you know of more than one bug but only
> telling us about "one such situation" here.  If so, what are others?
> If not, the phrasing in this paragraph is somewhat misleading.

>> 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.
> 
> OK.
> 
>> To fix this bug, we need to ensure that the `$message_num` variable is
>> not increased by 1 when a message is edited and resent. We do this by
>> decreasing both the `$message_num` and `$message_id_serial` variables
>> 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.
> 
> Hmph, isn't it more like "after editing Nth message, we re-process
> that edited message, and we used to call that edited message N+1th,
> which was wrong.  We now keep the same numbering and call the edited
> message Nth (and the version before editing we didn't send, so there
> is no risk of sending two Nth messages)"?  

> 
>> 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.
> 
> OK.
> 
> The above explains why the patch needs to touch message_num.  It
> would be evfen better if it described what the variable is used for,
> exactly.

> 
>    Side note: during the initial round of this change, I explained
>    that $num_sent is the counter in the batch we are sending out
>    (hence it is reset to 0 when a batch fills and the next batch
>    starts).  If there is a similar concise and clear explanation of
>    what $message_num?  "The number, counting from 1, of the message
>    in the set of messages we are sending", or something, perhaps?
> 
> And none of the above justifies why this patch mucks with
> message_id_serial.  Should it always be the same as message_num (in
> which case the natural question is "why do we need both?")?

> If we can prove that message_num and message_id_serial must be
> incremented in sync, it is OK to have a separate topic that unifies
> these two variables into just a single message_num, but I'd prefer
> not to see message_id_serial mentioned above and touched below at
> all in this patch to fix the in_reply_to issue.
> 
>> 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();