Re: [PATCH 7/7] Accelerate rename_dst setup

[Junio C Hamano <gitster@pobox.com>]

Elijah Newren <newren@gmail.com> writes:

> It's identical for runtime correctness, yes.  But the primary point
> isn't what happens at runtime, but what happens when future code
> readers come along.  If I only keep the "if (dst->is_rename)" line
> that comes after without the assert, then someone in the future will
> come along (maybe even myself) and think "the original author wasn't
> being careful here; they should change this to a check on (dst &&
> dst->is_rename)" (because honestly, it is the kind of mistake I'm
> prone to make).  However, if they were to do so, and some bug gets
> introduced so that locate_rename_dst() returns a NULL for a pair of
> interest, then they've masked a bug in the algorithm and made it fail
> in harder-to-detect-and-track-down ways.  I wanted it to be clear that
> dst == NULL is unacceptable.  I guess I could have marked it with
> BUG(), but between an assertion and a NULL-pointer indirection, I
> figured that code aborting under that condition was pretty well
> covered.  :-)

assert() often gets turned into no-op, so it is more like leaving a
comment in the code.  A comment or BUG("text") can describe not just
the fact that reaching this point with dst==NULL is a bug in the
caller but also why it is a bug (e.g. how NULL-dst would screw up
the computation downstream from this point), but an assert() cannot.