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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
