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

On Dec 19, 2016, at 12:03, Jeff King wrote:

On Sat, Dec 17, 2016 at 11:54:18AM -0800, Kyle J. McKay wrote:

quoted

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

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