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

From: Taylor Blau <me@ttaylorr.com>
Date: Mar 12, 2025, 22:00 UTC
Message-ID: <Z9ID/2zx25qesuJs@nand.local>
In-Reply-To: <f48b3310d4ae8d05780fd25e467083c4dc9852cc.1741275027.git.gitgitgadget@gmail.com>

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

Show 19 quoted lines

> 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