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

[Junio C Hamano <gitster@pobox.com>]

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();