Re: [PATCH 2/2] merge-ort: fix slightly overzealous assertion for rename-to-self

[Taylor Blau <me@ttaylorr.com>]

On Thu, Mar 06, 2025 at 03:30:27PM +0000, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@gmail.com>
>
> 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