From: Junio C Hamano <gitster@pobox.com>
Date: Wed, 10 Jun 2009 05:51:56 GMT
Subject: Re: [PATCH 3/6] send-email: fix threaded mails without chain-reply-to
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:

> 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.