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