diff --git a/versioned/persist/adapter/src/main/java/org/projectnessie/versioned/persist/adapter/spi/AbstractDatabaseAdapter.java b/versioned/persist/adapter/src/main/java/org/projectnessie/versioned/persist/adapter/spi/AbstractDatabaseAdapter.java index e8336b92c98..2ca85f8082a 100644 --- a/versioned/persist/adapter/src/main/java/org/projectnessie/versioned/persist/adapter/spi/AbstractDatabaseAdapter.java +++ b/versioned/persist/adapter/src/main/java/org/projectnessie/versioned/persist/adapter/spi/AbstractDatabaseAdapter.java @@ -626,10 +626,10 @@ protected Hash mergeTransplantCommon( newKeyLists, params.getUpdateCommitMetadata(), mergePredicate, - additionalParents); + additionalParents, + addedCommits); if (squashed != null) { - addedCommits.accept(squashed); if (!params.isDryRun()) { writtenCommits.accept(squashed); toHead = squashed.getHash(); @@ -2109,7 +2109,8 @@ protected CommitLogEntry squashCommits( Consumer newKeyLists, MetadataRewriter rewriteMetadata, Predicate includeKeyPredicate, - Iterable additionalParents) + Iterable additionalParents, + Consumer addedCommits) throws ReferenceConflictException, ReferenceNotFoundException { List commitMeta = new ArrayList<>(); @@ -2183,6 +2184,7 @@ protected CommitLogEntry squashCommits( } writeIndividualCommit(ctx, squashedCommit); + addedCommits.accept(squashedCommit); return squashedCommit; } @@ -2263,13 +2265,13 @@ protected Hash copyCommits( if (!newEntry.getHash().equals(sourceCommit.getHash())) { commitsChronological.set(i, newEntry); + addedCommits.accept(newEntry); } else { // Newly built CommitLogEntry is equal to the CommitLogEntry to transplant. // This can happen, if the commit to transplant has NO_ANCESTOR as its parent. commitsChronological.remove(i); } - addedCommits.accept(newEntry); targetHead = newEntry.getHash(); } return targetHead; diff --git a/versioned/storage/store/src/main/java/org/projectnessie/versioned/storage/versionstore/BaseCommitHelper.java b/versioned/storage/store/src/main/java/org/projectnessie/versioned/storage/versionstore/BaseCommitHelper.java index 505fca3d828..a939ce055d6 100644 --- a/versioned/storage/store/src/main/java/org/projectnessie/versioned/storage/versionstore/BaseCommitHelper.java +++ b/versioned/storage/store/src/main/java/org/projectnessie/versioned/storage/versionstore/BaseCommitHelper.java @@ -519,8 +519,6 @@ ImmutableMergeResult mergeSquashFastForward( result.wasApplied(true).resultantTargetHash(objIdToHash(fromId)); } - result.addAddedCommits(commitObjToCommit(source)); - return result.build(); } diff --git a/versioned/storage/store/src/main/java/org/projectnessie/versioned/storage/versionstore/BaseMergeTransplantIndividual.java b/versioned/storage/store/src/main/java/org/projectnessie/versioned/storage/versionstore/BaseMergeTransplantIndividual.java index 883ef40b4ed..19c55114f21 100644 --- a/versioned/storage/store/src/main/java/org/projectnessie/versioned/storage/versionstore/BaseMergeTransplantIndividual.java +++ b/versioned/storage/store/src/main/java/org/projectnessie/versioned/storage/versionstore/BaseMergeTransplantIndividual.java @@ -44,7 +44,6 @@ import org.projectnessie.versioned.ReferenceNotFoundException; import org.projectnessie.versioned.storage.batching.BatchingPersist; import org.projectnessie.versioned.storage.batching.WriteBatching; -import org.projectnessie.versioned.storage.common.exceptions.ObjNotFoundException; import org.projectnessie.versioned.storage.common.indexes.StoreIndex; import org.projectnessie.versioned.storage.common.indexes.StoreIndexElement; import org.projectnessie.versioned.storage.common.logic.CommitLogic; @@ -121,18 +120,13 @@ MergeResult individualCommits( empty = false; if (!dryRun) { - commitLogic.storeCommit(newCommit, objsToStore); newHead = newCommit.id(); - try { - // We need to fetch the existing commit in order to return the correct object. - // The existing commit may have a different set of parents than the new commit. - newCommit = commitLogic.fetchCommit(newCommit.id()); - } catch (ObjNotFoundException ignored) { - // cannot happen + boolean committed = commitLogic.storeCommit(newCommit, objsToStore); + if (committed) { + mergeResult.addAddedCommits(commitObjToCommit(newCommit)); } } - mergeResult.addAddedCommits(commitObjToCommit(newCommit)); sourceParentIndex = indexesLogic.buildCompleteIndex(sourceCommit, Optional.empty()); targetParentIndex = indexesLogic.buildCompleteIndex(newCommit, Optional.empty()); } diff --git a/versioned/storage/store/src/main/java/org/projectnessie/versioned/storage/versionstore/BaseMergeTransplantSquash.java b/versioned/storage/store/src/main/java/org/projectnessie/versioned/storage/versionstore/BaseMergeTransplantSquash.java index ee1e50e627d..68fbf1ceccf 100644 --- a/versioned/storage/store/src/main/java/org/projectnessie/versioned/storage/versionstore/BaseMergeTransplantSquash.java +++ b/versioned/storage/store/src/main/java/org/projectnessie/versioned/storage/versionstore/BaseMergeTransplantSquash.java @@ -40,7 +40,6 @@ import org.projectnessie.versioned.MetadataRewriter; import org.projectnessie.versioned.ReferenceConflictException; import org.projectnessie.versioned.ReferenceNotFoundException; -import org.projectnessie.versioned.storage.common.exceptions.ObjNotFoundException; import org.projectnessie.versioned.storage.common.indexes.StoreIndex; import org.projectnessie.versioned.storage.common.indexes.StoreKey; import org.projectnessie.versioned.storage.common.logic.CommitLogic; @@ -105,18 +104,13 @@ MergeResult squash( newHead = headId(); } else { CommitLogic commitLogic = commitLogic(persist); - commitLogic.storeCommit(mergeCommit, objsToStore); newHead = mergeCommit.id(); - try { - // We need to fetch the existing commit in order to return the correct object. - // The existing commit may have a different set of parents than the new commit. - mergeCommit = commitLogic.fetchCommit(mergeCommit.id()); - } catch (ObjNotFoundException ignored) { - // cannot happen + boolean committed = commitLogic.storeCommit(mergeCommit, objsToStore); + if (committed) { + mergeResult.addAddedCommits(commitObjToCommit(mergeCommit)); } } - mergeResult.addAddedCommits(commitObjToCommit(mergeCommit)); return finishMergeTransplant(false, mergeResult, newHead, dryRun, hasConflicts); } diff --git a/versioned/tests/src/main/java/org/projectnessie/versioned/tests/AbstractMerge.java b/versioned/tests/src/main/java/org/projectnessie/versioned/tests/AbstractMerge.java index dbf4d8e4e0c..7873fba8d88 100644 --- a/versioned/tests/src/main/java/org/projectnessie/versioned/tests/AbstractMerge.java +++ b/versioned/tests/src/main/java/org/projectnessie/versioned/tests/AbstractMerge.java @@ -473,7 +473,7 @@ private void doMergeIntoEmpty( false, false); - checkTargetCommits(individualCommits, initialHash, result); + checkAddedCommits(individualCommits, initialHash, result); soft.assertThat( contentsWithoutId( store() @@ -491,77 +491,10 @@ private void doMergeIntoEmpty( ContentKey.of("t4"), V_4_1)); } - private void checkTargetCommits( + private void checkAddedCommits( boolean individualCommits, Hash targetHead, MergeResult result) { if (individualCommits) { - soft.assertThat(result.getAddedCommits()) - .hasSize(3) - .satisfiesExactly( - c -> { - soft.assertThat(c.getParentHash()).isEqualTo(targetHead); - soft.assertThat(c.getCommitMeta().getMessage()).contains("First Commit"); - soft.assertThat(c.getOperations()) - .hasSize(3) - .satisfiesExactlyInAnyOrder( - o -> { - soft.assertThat(o).isInstanceOf(Put.class); - soft.assertThat(o.getKey()).isEqualTo(ContentKey.of("t1")); - soft.assertThat(contentWithoutId(((Put) o).getValue())).isEqualTo(V_1_1); - }, - o -> { - soft.assertThat(o).isInstanceOf(Put.class); - soft.assertThat(o.getKey()).isEqualTo(ContentKey.of("t2")); - soft.assertThat(contentWithoutId(((Put) o).getValue())).isEqualTo(V_2_1); - }, - o -> { - soft.assertThat(o).isInstanceOf(Put.class); - soft.assertThat(o.getKey()).isEqualTo(ContentKey.of("t3")); - soft.assertThat(contentWithoutId(((Put) o).getValue())).isEqualTo(V_3_1); - }); - }, - c -> { - soft.assertThat(c.getParentHash()) - .isEqualTo(result.getAddedCommits().get(0).getHash()); - soft.assertThat(c.getCommitMeta().getMessage()).contains("Second Commit"); - soft.assertThat(c.getOperations()) - .hasSize(4) - .satisfiesExactlyInAnyOrder( - o -> { - soft.assertThat(o).isInstanceOf(Put.class); - soft.assertThat(o.getKey()).isEqualTo(ContentKey.of("t1")); - soft.assertThat(contentWithoutId(((Put) o).getValue())).isEqualTo(V_1_2); - }, - o -> - soft.assertThat(o) - .asInstanceOf(type(Delete.class)) - .extracting(Delete::getKey) - .isEqualTo(ContentKey.of("t2")), - o -> - soft.assertThat(o) - .asInstanceOf(type(Delete.class)) - .extracting(Delete::getKey) - .isEqualTo(ContentKey.of("t3")), - o -> { - soft.assertThat(o).isInstanceOf(Put.class); - soft.assertThat(o.getKey()).isEqualTo(ContentKey.of("t4")); - soft.assertThat(contentWithoutId(((Put) o).getValue())).isEqualTo(V_4_1); - }); - }, - c -> { - soft.assertThat(c.getParentHash()) - .isEqualTo(result.getAddedCommits().get(1).getHash()); - soft.assertThat(c.getCommitMeta().getMessage()).contains("Third Commit"); - soft.assertThat(c.getOperations()) - .hasSize(1) - .satisfiesExactlyInAnyOrder( - o -> { - soft.assertThat(o).isInstanceOf(Put.class); - soft.assertThat(o.getKey()).isEqualTo(ContentKey.of("t2")); - soft.assertThat(contentWithoutId(((Put) o).getValue())).isEqualTo(V_2_2); - } - // Unchanged operation not retained - ); - }); + soft.assertThat(result.getAddedCommits()).isEmpty(); // fast-forward } else { soft.assertThat(result.getAddedCommits()) .singleElement() @@ -680,12 +613,8 @@ void compareDryAndEffectiveMergeResults(boolean individualCommits) throws Versio Put.of(ContentKey.of("t2"), V_2_1), Put.of(ContentKey.of("t3"), V_3_1))))); soft.assertThat(mergeResult.getTargetCommits()).isNull(); - // The branch was fast-forwarded to firstCommit, so the only added commit is firstCommit - // itself - soft.assertThat(mergeResult.getAddedCommits()) - .singleElement() - .extracting("hash") - .isEqualTo(firstCommit); + // The branch was fast-forwarded to firstCommit, so no commits added + soft.assertThat(mergeResult.getAddedCommits()).isEmpty(); soft.assertThat(mergeResult.getDetails()) .containsKeys(ContentKey.of("t1"), ContentKey.of("t2"), ContentKey.of("t3")); soft.assertThat(mergeResult) @@ -754,10 +683,7 @@ protected void mergeIntoEmptyBranch1Commit(boolean individualCommits) // not modifying commit meta, will just "fast forward" soft.assertThat(store().hashOnReference(newBranch, Optional.empty())).isEqualTo(firstCommit); - soft.assertThat(result.getAddedCommits()) - .singleElement() - .extracting(Commit::getParentHash, Commit::getHash) - .containsExactly(initialHash, firstCommit); + soft.assertThat(result.getAddedCommits()).isEmpty(); // no new commits List mergedCommit = commitsList(newBranch, false).subList(0, 1); assertCommitMeta(soft, mergedCommit, commits.subList(2, 3), metadataRewriter); @@ -955,7 +881,7 @@ protected void nonEmptyFastForwardMerge(boolean individualCommits) throws Versio false, false); soft.assertThat(mergeResult1.getResultantTargetHash()).isEqualTo(etl1.getCommitHash()); - soft.assertThat(mergeResult1.getAddedCommits()).containsExactly(etl1.getCommit()); + soft.assertThat(mergeResult1.getAddedCommits()).isEmpty(); // fast-forward, so nothing added CommitResult etl2 = store() @@ -978,7 +904,7 @@ protected void nonEmptyFastForwardMerge(boolean individualCommits) throws Versio false, false); soft.assertThat(mergeResult2.getResultantTargetHash()).isEqualTo(etl2.getCommitHash()); - soft.assertThat(mergeResult2.getAddedCommits()).containsExactly(etl2.getCommit()); + soft.assertThat(mergeResult2.getAddedCommits()).isEmpty(); // fast-forward, so nothing added soft.assertThat(contentWithoutId(store().getValue(review, key))).isEqualTo(VALUE_2); }