[PATCH v2] mailinfo.c: move side-effects outside of assert

From: Kyle J. McKay <mackyle@gmail.com>
Date: Dec 19, 2016, 23:13 UTC
Message-ID: <ebaf4c892a78bc3ae614a23d87f9c0f@58437222ff6db9ee7cbe9d1a5a1ad4e>
In-Reply-To: <xmqqbmw7ocoz.fsf@gitster.mtv.corp.google.com>

On Dec 19, 2016, at 13:01, Junio C Hamano wrote:

Show 38 quoted lines

> Jonathan Tan <jonathantanmy@google.com> writes:
>
>>>> This is obviously an improvement, but it makes me wonder if we  
>>>> should be
>>>> doing:
>>>>
>>>> if (!check_header(mi, &mi->inbody_header_accum, mi->s_hdr_data))
>>>>    die("BUG: some explanation of why this can never happen");
>>>>
>>>> which perhaps documents the intended assumptions more clearly. A  
>>>> comment
>>>> regarding the side effects might also be helpful.
>>>
>>> I wondered exactly the same thing myself.  I was hoping Jonathan  
>>> would
>>> pipe in here with some analysis about whether this is:
>>>
>>>  a) a super paranoid, just-in-case, can't really ever fail because  
>>> by
>>> the time we get to this code we've already effectively validated
>>> everything that could cause check_header to return false in this  
>>> case
>>> ...
>> The answer is "a". The only time that mi->inbody_header_accum is
>> appended to is in check_inbody_header, and appending onto a blank
>> mi->inbody_header_accum always happens when is_inbody_header is true
>> (which guarantees a prefix that causes check_header to always return
>> true).
>>
>> Peff's suggestion sounds reasonable to me, maybe with an error  
>> message
>> like "BUG: inbody_header_accum, if not empty, must always contain a
>> valid in-body header".
>
> OK.  So we do not expect it to fail, but we still do want the side
> effect of that function (i.e. accmulation into the field).
>
> Somebody care to send a final "agreed-upon" version?

Yup, here it is:

-- 8< --

Since 6b4b013f18 (mailinfo: handle in-body header continuations, 2016-09-20, v2.11.0) mailinfo.c has contained new code with an assert of the form:

	assert(call_a_function(...))

The function in question, check_header, has side effects. This means that when NDEBUG is defined during a release build the function call is omitted entirely, the side effects do not take place and tests (fortunately) start failing.

Move the function call outside of the assert and assert on the result of the function call instead so that the code still works properly in a release build and passes the tests.

Since the only time that mi->inbody_header_accum is appended to is in check_inbody_header, and appending onto a blank mi->inbody_header_accum always happens when is_inbody_header is true, this guarantees a prefix that causes check_header to always return true.

Therefore replace the assert with an if !check_header + DIE combination to reflect this.

Helped-by: Jonathan Tan <jonathantanmy@google.com>
Helped-by: Jeff King <peff@peff.net>
Acked-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Kyle J. McKay <mackyle@gmail.com>
---

Notes:
    Please include this PATCH in 2.11.x maint

 mailinfo.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/mailinfo.c b/mailinfo.c
index 2fb3877e..a489d9d0 100644
--- a/mailinfo.c
+++ b/mailinfo.c
@@ -710,7 +710,8 @@ static void flush_inbody_header_accum(struct mailinfo *mi)
 {
 	if (!mi->inbody_header_accum.len)
 		return;
-	assert(check_header(mi, &mi->inbody_header_accum, mi->s_hdr_data, 0));
+	if (!check_header(mi, &mi->inbody_header_accum, mi->s_hdr_data, 0))
+		die("BUG: inbody_header_accum, if not empty, must always contain a valid in-body header");
 	strbuf_reset(&mi->inbody_header_accum);
 }
 
---