Re: git-name-rev off-by-one bug

Daniel Barkalow <barkalow@iabervon.org> writes:

It describes the multi-base extention while the old one was done before the multi-base, so content-wise it may be more up to date.

One thing I have most trouble with is that it is not obvious if the table is covering all the cases. You have to read from top to bottom and consider the first match as its fate [*1*]. I was about to write "with no match resulting in no merge", but it is not even obvious if there are cases that would fall off at the end from the table by just looking at it. Even worse, if we add "no match results in no merge" at the end, by definition it covers all the cases, but it is not obvious what those fall-off cases are (IOW, what kinds of conflict they are and why they are not handled).

Another thing, perhaps more important, is taht it does not seem to talk about index and up-to-dateness requirements much; it says something about what happens when "no merge" result is taken, but it is not clear about other cases. The table in t1000 test marks the case with "must match X" when index and tree X must agree at the path, and with "must match X and be up-to-date" when in addition the file in the working tree must match what is recorded in the index at the path (i.e. the former can have local modification in the working tree as long as index entry and tree match).

This is vital in making sure that read-tree 3-way merge does not lose information from the working tree. I am sure your updated *code* is doing the right thing, but the documentation is not clear about it. E.g. case 3ALT in the table says "take our branch if the path does not exist in one or more of common ancestors and the other branch does not have it" without saying anything about index nor up-to-dateness requirements. In this case, the index must match HEAD but the working tree file is allowed to have local modification (t1000 table says "must match A"). If somebody wants to audit if the current read-tree.c does the right thing for this case, he needs the documentation to tell him what should happen. There may be thinko in the design (IOW, the index requirements the documentation places may not make sense) that can be found during such an audit. There may be implementation error that the code does not match what the documentation says should happen. Not having that information in the case table makes these verification difficult.

I agree 100% that having them scattered all over is bad and the trivial-merge.txt is the logical place to consolidate them, but I do not think simply removing others and pointing at trivial-merge.txt without updating it is good enough.

[Footnote]

*1* That is OK from an implementation point of view (i.e. we can look at the table, and then go to C implementation and follow its if-elif chain to see if the same checks are done in the same order as specified in the document), but for somebody who wants to understand the semantics, i.e. what the thing it does means, by looking at the documentation it is harder to read.