Re: [PATCH 3/6] send-email: fix threaded mails without chain-reply-to

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 10, 2009, 05:51 UTC
Message-ID: <7vhbyo1vr7.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1244410857-920-4-git-send-email-markus.heidelberg@web.de>

Markus Heidelberg <markus.heidelberg@web.de> writes:

Show 21 quoted lines

> These commands didn't send threaded mails anymore:
>
>     $ git format-patch <revision range>
>     $ git send-email --thread --no-chain-reply-to <files>
>
> This regression was introduced in commit 15da108 ("send-email:
> 'References:' should only reference what is sent", 2009-04-13) by a
> hidden code style change:
>
>     ! defined $reply_to || length($reply_to) == 0
> was changed to
>     not defined $reply_to || length($reply_to) == 0
> which is
>     not (defined $reply_to || length($reply_to) == 0)
> instead of
>     (not defined $reply_to) || (length($reply_to) == 0)
>
> Signed-off-by: Markus Heidelberg <markus.heidelberg@web.de>
> ...
> -	if ($message_was_sent and $chain_reply_to || not defined $reply_to || length($reply_to) == 0) {
> +	if ($message_was_sent and $chain_reply_to || !defined $reply_to || length($reply_to) == 0) {

You were too polite to say "by a hidden code style change", instead of saying something stronger, like "without understanding the operator precedence rules", but I actually thing that is what is going on. Not just the original author of 15da108 but no reviewer noticed when the patch was posted on the list.

The problem description strongly suggests to me that we should probably fix the use of "and" here as well in order to avoid confusing ourselves.

You have another patch that touches this area that adds "and $thread", but I think it is much less error prone if we stick to && and || with explicit parentheses to clarify the precedence we want to see.