Re: [PATCH 02/11] merge-ort: add initial outline for basic rename detection

On Thu, Dec 10, 2020 at 6:39 PM Derrick Stolee <stolee@gmail.com> wrote:

Yeah, there'll be a lot of these...

I can see where you're coming from...but that's a monumentally huge shift. What about all my "loops over the sides"? Sure, the ones that are only two lines long could be just turned into code without a loop, but when the loop is 140 lines long, that doesn't make much sense. Loop over enum ranges?

Are all the variables that track an index still okay? Do I need to also introduce an enum for all the filemask/dirmask/match_mask/etc. variables:

enum merge_mask {
     MERGE_JUST_BASE = 1,
     MERGE_JUST_SIDE1 = 2,
     MERGE_BASE_AND_SIDE1 = 3,
     MERGE_JUST_SIDE2 = 4,
     MERGE_BASE_AND_SIDE2 = 5,
     MERGE_SIDE1_AND_SIDE2 = 6,
     MERGE_BASE_AND_SIDE1_AND_SIDE2 = 7
}

?  That seems like a pretty big pain to use.

Also, what about the code that uses a side index to get the other side index? Or my conversions from side indices to masks (or vice versa)? I tend to put comments by these, but there's a _lot_ of them.

I suspect this one would take a week to change, and I'd miss several locations, and....some cases would certainly look cleaner but I suspect some would be far uglier and end up being unchanged and then leave us with a mess of trying to understand both.

What if I added a big comment near the top of the file that we've got
dozens of variables that are arrays of size 3 which are meant to be
indexed by the "side" of the three-way merge that it is tracking
information for:
    0: merge_base
    1: side1
    2: side2
(though several of the variables might have index 0 unused since it
doesn't track anything specifically for the merge base), and further
that masks are used in certain variables which try to track which
sides are present or match, with 2<<SIDE being the bit to track that a
given side is present/relevant.

I mean, this stuff is all over the place throughout the 4500 line merge-ort.c file. And it is in lots of diffcore-rename.c (which will grow by about 1K lines as well).

As mentioned in the previous patch, the shift of the indices would cause me at least a large amount of mental confusion and I suspect it would for others too. Both conflict_info.stages[] and conflict_info.pathnames[] are meant to be indexed by the side of the merge (or the base), but using conflict_info.stages[MERGE_SIDE1] or conflict_info.pathnames[MERGE_SIDE2] as you have them defined here would provide the wrong answer. Since there's a conflict_info per file and it is used all over the code, this would just be ripe for off-by-one errors.

Since both conflict_info.stages[] and renames->pairs[] are meant to be indexed by the merge side, this kind of conflict is inevitable. The only clean solution is making both be arrays of size three, and just skipping index 0 in the variables that don't need to track something for the merge_base.

Heh, when I used an unsigned for a boolean, Jonathan Tan asked why I didn't use an int. When I use an int for a boolean, you ask why I don't use an unsigned. I think my stock answer should just be that the other reviewer suggested it. ;-)

Yeah, this came from unpack-trees.c and merge-recursive.c, where the value is usually 1 (clean) or 0 (not clean), but the special value of -1 signals something went wrong enough that we need to stop further processing and return up the call chain for any necessary cleanup (e.g. removal of lock files). The value of -1 is only used for things like "disk-is-full, can't write any more files to the working directory", or "failed to read one of the trees involved in the merge from the git object store".

-1, though, is the return value from unpack_trees(), traverse_trees(), and perhaps other places in the code, so I'd be worried about attempting to use a different special value for fear that I'd miss converting the return value I got from one of those to the new special value.

merge-ort has far fewer locations where -1 appears (in part because the checkout() code is an external function rather than being sprinkled everywhere), and it tends to cause the code to return immediately, so most all call sites can assume a simple boolean value of either 0 or 1.

If this were the only one, then I'd absolutely agree. There's 7 such loops in the version of merge-ort.c in the 'ort' branch. I can't get rid of all of them, because even though some are short, some of those are very long for-loops. (The long ones use "side" for the loop counter instead of "s" -- maybe I should use "side" even on the short ones?)

There's also another 6 that loop over the sides including the merge-base (thus including index 0). If those count, it's closer to 13.

I used to do that kind of thing, but the unreferenced static method problem is really annoying. It means the code doesn't even compile, which is a bad state to submit patches in. I can work around that by adding "(void)unused_funcname;" expressions somewhere in the code, but reviewers tend to be even more surprised by those.