On Dec 19, 2016, at 12:03, Jeff King wrote:
On Sat, Dec 17, 2016 at 11:54:18AM -0800, Kyle J. McKay wrote:
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.
Signed-off-by: Kyle J. McKay <mackyle@xxxxxxxxx>
---
Notes:
Please include this PATCH in 2.11.x maint
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
-or-
b) Yeah, it could fail in the real world and it should "die" (and
probably have a test added that triggers such death)
-or-
c) Actually, if check_header does return false we can keep going
without problem
-or-
d) Actually, if check_header does return false we can keep going by
making a minor change that should be in the patch
I assume that since Jonathan added the code he will just know the
answer as to which one it is and I won't have to rely on the results
of my imaginary analysis. ;)
On Dec 19, 2016, at 09:45, Johannes Schindelin wrote:
ACK. I noticed this problem (and fixed it independently as a part of a
huge patch series I did not get around to submit yet) while trying
to get
Git to build correctly with Visual C.
Does this mean that Dscho and I are the only ones who add -DNDEBUG for
release builds? Or are we just the only ones who actually run the
test suite on such builds?
--Kyle