On Thu, Mar 06, 2025 at 03:30:27PM +0000, Elijah Newren via GitGitGadget wrote:

From: Elijah Newren <redacted>

merge-ort has a number of sanity checks on the file it is processing in
process_renames().  One of these sanity checks was slightly overzealous
because it indirectly assumed that a renamed file always ended up at a
different path than where it started.  That is normally an entirely fair
assumption, but directory rename detection can make things interesting.

As a quick refresher, if one side of history renames directory A/ -> B/,
and the other side of history adds new files to A/, then directory
rename detection notices and suggests moving those new files to B/.  A
similar thing is done for paths renamed into A/, causing them to be
transitively renamed into B/.  But, if the file originally came from B/,
then this can end up causing a file to be renamed back to itself.

It turns out the rest of the code following this assertion handled the
case fine; the assertion was just an extra sanity check, not a rigid
precondition.  Therefore, simply adjust the assertion to pass under this
special case as well.

Wow, that is a very interesting edge case from Dmitry. The explanation
and fix makes sense, and yeah, looks like just an over-zealous
assertion.

As a meta-comment, I vaguely remember past discussions on the list about
when to assert() versus when to BUG(). I recall that where we landed was
that:

    if (foo)
        BUG("something");

was preferable to:

    assert(foo);

I know that we don't usually define NDEBUG, but I think there are a
couple of cases where we do, like for nedmalloc stuff when building with
USE_NED_ALLOCATOR, and in Windows builds if DEBUG is undefined.

So I don't think it makes a huge practical difference, but it might be
nice to put in CodingGuidelines or similar.

Thanks,
Taylor