Re: [PR] OAK-10875 - unmerged bc on root can make it appear merged [jackrabbit-oak]

["stefan-egli (via GitHub)" <gi...@apache.org>]

stefan-egli commented on code in PR #1605:
URL: https://github.com/apache/jackrabbit-oak/pull/1605#discussion_r1707015446


##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/NodeDocument.java:
##########
@@ -1943,6 +1943,9 @@ public static void setBranchCommit(@NotNull UpdateOp op,
     public static void removeBranchCommit(@NotNull UpdateOp op,
                                           @NotNull Revision revision) {
         checkNotNull(op).removeMapEntry(BRANCH_COMMITS, revision);
+        if (op.getId().equals(Utils.getIdFromPath("/"))) {
+            checkNotNull(op).removeMapEntry(COMMIT_ROOT, revision);
+        }

Review Comment:
   I think removing the unmerged revision from `_commitRoot` is indeed what we should do. I do however think there is more required than just this. For example the question, what happens to a later resolving of a revision that is then older than sweep but all the info about the revision having been a branch commit is by then gone. That I think requires more testing and dependent on that perhaps another change.
   
   For example what could be done as an alternative to removing `_commitRoot` *and* `_bc` would be to not remove neither - in case of an OAK-10875-type unmerged-bc. That would be somewhat of a step back, but it would clearly mark the revision as unmerged for good. (But I'd prefer a solution where we could remove `_commitRoot` as you did above).
   
   Now one thing we'd need to change though, if we wanted to remove the `_commitRoot` : that shouldn't be done inside `removeBranchCommit` - as these static methos here on NodeDocument are basic operations on the UpdateOp that do only exactly what the method name says, that's a bit the contract of those methods. And also removing a `_commitRoot` as part of removing a `_bc` seems to break that contract.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: oak-dev-unsubscribe@jackrabbit.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org