On Dec 19, 2016, at 13:01, Junio C Hamano wrote:
> Jonathan Tan <jonathantanmy@xxxxxxxxxx> 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@xxxxxxxxxx>
Helped-by: Jeff King <peff@xxxxxxxx>
Acked-by: Johannes Schindelin <Johannes.Schindelin@xxxxxx>
Signed-off-by: Kyle J. McKay <mackyle@xxxxxxxxx>
---
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);
}
---