diff --git a/api/model/src/main/java/org/projectnessie/api/v1/params/BaseMergeTransplant.java b/api/model/src/main/java/org/projectnessie/api/v1/params/BaseMergeTransplant.java index 40dc64560db..c908f305412 100644 --- a/api/model/src/main/java/org/projectnessie/api/v1/params/BaseMergeTransplant.java +++ b/api/model/src/main/java/org/projectnessie/api/v1/params/BaseMergeTransplant.java @@ -39,6 +39,7 @@ public interface BaseMergeTransplant { @Nullable @jakarta.annotation.Nullable @JsonInclude(Include.NON_NULL) + @Deprecated Boolean keepIndividualCommits(); @Nullable diff --git a/api/model/src/main/java/org/projectnessie/model/CommitMeta.java b/api/model/src/main/java/org/projectnessie/model/CommitMeta.java index 2f3b5f220b9..b635256547d 100644 --- a/api/model/src/main/java/org/projectnessie/model/CommitMeta.java +++ b/api/model/src/main/java/org/projectnessie/model/CommitMeta.java @@ -191,6 +191,9 @@ public static CommitMeta fromMessage(String message) { } public interface Builder { + + Builder message(String message); + default ImmutableCommitMeta.Builder author(String author) { if (author != null) { return addAllAuthors(author); @@ -234,6 +237,8 @@ default ImmutableCommitMeta.Builder putProperties(String key, String value) { ImmutableCommitMeta.Builder putAllProperties(String key, List value); ImmutableCommitMeta.Builder allProperties(Map> entries); + + CommitMeta build(); } /** diff --git a/api/model/src/main/java/org/projectnessie/model/MergeResponse.java b/api/model/src/main/java/org/projectnessie/model/MergeResponse.java index 94e9423dc0f..87143024fb3 100644 --- a/api/model/src/main/java/org/projectnessie/model/MergeResponse.java +++ b/api/model/src/main/java/org/projectnessie/model/MergeResponse.java @@ -55,7 +55,14 @@ default boolean wasSuccessful() { @jakarta.annotation.Nullable String getResultantTargetHash(); - /** Commit-ID of the identified common ancestor, only returned for a merge operation. */ + /** + * Commit-ID of the identified merge base, only returned for a merge operation. + * + *

Note: earlier Nessie versions only supported merging using the common ancestor, so only + * considering the direct commit parents (predecessors). Nessie identifies the "nearest" + * merge-base singe version 0.61.0 (with the new storage model), and allows "incremental merges". + * Since renaming a public API fields is not good practice, this field represents the merge-base. + */ @Nullable @jakarta.annotation.Nullable String getCommonAncestor(); diff --git a/integrations/spark-extensions-basetests/src/main/java/org/projectnessie/spark/extensions/AbstractNessieSparkSqlExtensionTest.java b/integrations/spark-extensions-basetests/src/main/java/org/projectnessie/spark/extensions/AbstractNessieSparkSqlExtensionTest.java index e9a3d619e78..295a2a9ffe8 100644 --- a/integrations/spark-extensions-basetests/src/main/java/org/projectnessie/spark/extensions/AbstractNessieSparkSqlExtensionTest.java +++ b/integrations/spark-extensions-basetests/src/main/java/org/projectnessie/spark/extensions/AbstractNessieSparkSqlExtensionTest.java @@ -15,6 +15,7 @@ */ package org.projectnessie.spark.extensions; +import static java.util.Collections.emptyMap; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; @@ -500,6 +501,8 @@ private void mergeReferencesInto(String query) .map(SparkCommitLogEntry::relevantFromMerge) .get(); + result = ImmutableSparkCommitLogEntry.builder().from(result).properties(emptyMap()).build(); + sql(query, refName); // here we are skipping commit time as its variable diff --git a/servers/rest-services/src/main/java/org/projectnessie/services/rest/RestTreeResource.java b/servers/rest-services/src/main/java/org/projectnessie/services/rest/RestTreeResource.java index cd762ca76a8..9b731cd3308 100644 --- a/servers/rest-services/src/main/java/org/projectnessie/services/rest/RestTreeResource.java +++ b/servers/rest-services/src/main/java/org/projectnessie/services/rest/RestTreeResource.java @@ -15,6 +15,7 @@ */ package org.projectnessie.services.rest; +import static com.google.common.base.Preconditions.checkArgument; import static org.projectnessie.services.impl.RefUtil.toReference; import static org.projectnessie.services.spi.TreeService.MAX_COMMIT_LOG_ENTRIES; @@ -22,6 +23,7 @@ import javax.enterprise.context.RequestScoped; import javax.inject.Inject; import org.projectnessie.api.v1.http.HttpTreeApi; +import org.projectnessie.api.v1.params.BaseMergeTransplant; import org.projectnessie.api.v1.params.CommitLogParams; import org.projectnessie.api.v1.params.EntriesParams; import org.projectnessie.api.v1.params.GetReferenceParams; @@ -220,6 +222,7 @@ public void deleteReference( public MergeResponse transplantCommitsIntoBranch( String branchName, String expectedHash, String message, Transplant transplant) throws NessieNotFoundException, NessieConflictException { + validateKeepIndividual(transplant, true, "Transplant", "unsquashed"); return resource() .transplantCommitsIntoBranch( branchName, @@ -227,7 +230,6 @@ public MergeResponse transplantCommitsIntoBranch( message != null ? CommitMeta.fromMessage(message) : null, transplant.getHashesToTransplant(), transplant.getFromRefName(), - transplant.keepIndividualCommits(), transplant.getKeyMergeModes(), transplant.getDefaultKeyMergeMode(), transplant.isDryRun(), @@ -239,13 +241,13 @@ public MergeResponse transplantCommitsIntoBranch( @Override public MergeResponse mergeRefIntoBranch(String branchName, String expectedHash, Merge merge) throws NessieNotFoundException, NessieConflictException { + validateKeepIndividual(merge, false, "Merge", "squashed"); return resource() .mergeRefIntoBranch( branchName, expectedHash, merge.getFromRefName(), merge.getFromHash(), - merge.keepIndividualCommits(), null, merge.getKeyMergeModes(), merge.getDefaultKeyMergeMode(), @@ -254,6 +256,17 @@ public MergeResponse mergeRefIntoBranch(String branchName, String expectedHash, merge.isReturnConflictAsResult()); } + private void validateKeepIndividual( + BaseMergeTransplant base, boolean goodValue, String opsName, String supported) { + @SuppressWarnings("deprecation") + Boolean keep = base.keepIndividualCommits(); + checkArgument( + keep == null || keep == goodValue, + "The parameter 'keepIndividualCommits' is deprecated. %s operations only support %s now.", + opsName, + supported); + } + @JsonView(Views.V1.class) @Override public Branch commitMultipleOperations( diff --git a/servers/rest-services/src/main/java/org/projectnessie/services/rest/RestV2TreeResource.java b/servers/rest-services/src/main/java/org/projectnessie/services/rest/RestV2TreeResource.java index d58c78c6a3d..9d1edea4478 100644 --- a/servers/rest-services/src/main/java/org/projectnessie/services/rest/RestV2TreeResource.java +++ b/servers/rest-services/src/main/java/org/projectnessie/services/rest/RestV2TreeResource.java @@ -356,7 +356,6 @@ public MergeResponse transplantCommitsIntoBranch(String branch, Transplant trans meta, transplant.getHashesToTransplant(), transplant.getFromRefName(), - true, transplant.getKeyMergeModes(), transplant.getDefaultKeyMergeMode(), transplant.isDryRun(), @@ -390,7 +389,6 @@ public MergeResponse mergeRefIntoBranch(String branch, Merge merge) ref.hashWithRelativeSpec(), merge.getFromRefName(), merge.getFromHash(), - false, meta.build(), merge.getKeyMergeModes(), merge.getDefaultKeyMergeMode(), diff --git a/servers/services/src/main/java/org/projectnessie/services/impl/BaseApiImpl.java b/servers/services/src/main/java/org/projectnessie/services/impl/BaseApiImpl.java index b16ce80a0be..457daf9b65f 100644 --- a/servers/services/src/main/java/org/projectnessie/services/impl/BaseApiImpl.java +++ b/servers/services/src/main/java/org/projectnessie/services/impl/BaseApiImpl.java @@ -48,6 +48,7 @@ import org.projectnessie.services.authz.BatchAccessChecker; import org.projectnessie.services.authz.ServerAccessContext; import org.projectnessie.services.config.ServerConfig; +import org.projectnessie.versioned.DefaultMetadataRewriter; import org.projectnessie.versioned.DetachedRef; import org.projectnessie.versioned.GetNamedRefsParams; import org.projectnessie.versioned.Hash; @@ -201,6 +202,6 @@ protected MetadataRewriter commitMetaUpdate( IntFunction squashMessage) { Principal principal = getPrincipal(); String committer = principal == null ? "" : principal.getName(); - return new CommitMetaUpdater(committer, Instant.now(), commitMeta, squashMessage); + return new DefaultMetadataRewriter(committer, Instant.now(), commitMeta, squashMessage); } } diff --git a/servers/services/src/main/java/org/projectnessie/services/impl/TreeApiImpl.java b/servers/services/src/main/java/org/projectnessie/services/impl/TreeApiImpl.java index 73946898e76..c166842aa6b 100644 --- a/servers/services/src/main/java/org/projectnessie/services/impl/TreeApiImpl.java +++ b/servers/services/src/main/java/org/projectnessie/services/impl/TreeApiImpl.java @@ -566,7 +566,6 @@ public MergeResponse transplantCommitsIntoBranch( @Nullable @jakarta.annotation.Nullable CommitMeta commitMeta, List hashesToTransplant, String fromRefName, - Boolean keepIndividualCommits, Collection keyMergeBehaviors, MergeBehavior defaultMergeBehavior, Boolean dryRun, @@ -592,7 +591,7 @@ public MergeResponse transplantCommitsIntoBranch( transplants = s.collect(Collectors.toList()); } - if (Boolean.TRUE.equals(keepIndividualCommits) && transplants.size() > 1) { + if (transplants.size() > 1) { // Message overrides are not meaningful when transplanting more than one commit. // This matches old behaviour where `message` was ignored in all cases. commitMeta = null; @@ -619,7 +618,6 @@ public MergeResponse transplantCommitsIntoBranch( lastHash, branchName, into.map(h -> " at " + h.asString()).orElse("")))) - .keepIndividualCommits(Boolean.TRUE.equals(keepIndividualCommits)) .mergeKeyBehaviors(keyMergeBehaviors(keyMergeBehaviors)) .defaultMergeBehavior(defaultMergeBehavior(defaultMergeBehavior)) .dryRun(Boolean.TRUE.equals(dryRun)) @@ -647,7 +645,6 @@ public MergeResponse mergeRefIntoBranch( String expectedHash, String fromRefName, String fromHash, - Boolean keepIndividualCommits, @Nullable @jakarta.annotation.Nullable CommitMeta commitMeta, Collection keyMergeBehaviors, MergeBehavior defaultMergeBehavior, @@ -680,14 +677,13 @@ public MergeResponse mergeRefIntoBranch( commitMetaUpdate( commitMeta, numCommits -> + // numCommits is always 1 for merges String.format( - "Merged %d commits from %s at %s into %s%s", - numCommits, + "Merged %s at %s into %s%s", fromRefName, from.asString(), branchName, into.map(h -> " at " + h.asString()).orElse("")))) - .keepIndividualCommits(Boolean.TRUE.equals(keepIndividualCommits)) .mergeKeyBehaviors(keyMergeBehaviors(keyMergeBehaviors)) .defaultMergeBehavior(defaultMergeBehavior(defaultMergeBehavior)) .dryRun(Boolean.TRUE.equals(dryRun)) diff --git a/servers/services/src/main/java/org/projectnessie/services/spi/TreeService.java b/servers/services/src/main/java/org/projectnessie/services/spi/TreeService.java index 0a9eaef0584..15464c6bff1 100644 --- a/servers/services/src/main/java/org/projectnessie/services/spi/TreeService.java +++ b/servers/services/src/main/java/org/projectnessie/services/spi/TreeService.java @@ -214,7 +214,6 @@ MergeResponse transplantCommitsIntoBranch( regexp = REF_NAME_REGEX, message = REF_NAME_MESSAGE) String fromRefName, - Boolean keepIndividualCommits, Collection keyMergeBehaviors, MergeBehavior defaultMergeType, @Nullable @jakarta.annotation.Nullable Boolean dryRun, @@ -259,7 +258,6 @@ MergeResponse mergeRefIntoBranch( @Pattern(regexp = HASH_REGEX, message = HASH_MESSAGE) @jakarta.validation.constraints.Pattern(regexp = HASH_REGEX, message = HASH_MESSAGE) String fromHash, - @Nullable @jakarta.annotation.Nullable Boolean keepIndividualCommits, @Nullable @jakarta.annotation.Nullable CommitMeta commitMeta, Collection keyMergeBehaviors, MergeBehavior defaultMergeType, diff --git a/servers/services/src/testFixtures/java/org/projectnessie/services/impl/AbstractTestAccessChecks.java b/servers/services/src/testFixtures/java/org/projectnessie/services/impl/AbstractTestAccessChecks.java index 770060e5bb8..c14f8c3cf30 100644 --- a/servers/services/src/testFixtures/java/org/projectnessie/services/impl/AbstractTestAccessChecks.java +++ b/servers/services/src/testFixtures/java/org/projectnessie/services/impl/AbstractTestAccessChecks.java @@ -103,7 +103,7 @@ public Map check() { @Test public void commitMergeTransplantAccessChecks() throws BaseNessieClientServerException { - assumeThat(persist).isNotNull(); + assumeThat(databaseAdapter).isNull(); ContentKey keyUnrelated = ContentKey.of("unrelated"); ContentKey keyNamespace1 = ContentKey.of("ns1"); @@ -209,7 +209,6 @@ public void commitMergeTransplantAccessChecks() throws BaseNessieClientServerExc common.getHash(), source.getName(), source.getHash(), - false, null, emptyList(), NORMAL, @@ -237,7 +236,6 @@ public void commitMergeTransplantAccessChecks() throws BaseNessieClientServerExc null, asList(source1.getHash(), source.getHash()), source1.getName(), - true, emptyList(), NORMAL, false, @@ -373,7 +371,6 @@ public Map check() { merge.getHash(), ref.getName(), ref.getHash(), - false, null, emptyList(), NORMAL, @@ -392,7 +389,6 @@ public Map check() { null, singletonList(ref.getHash()), ref.getName(), - false, emptyList(), NORMAL, false, diff --git a/servers/services/src/testFixtures/java/org/projectnessie/services/impl/AbstractTestMergeTransplant.java b/servers/services/src/testFixtures/java/org/projectnessie/services/impl/AbstractTestMergeTransplant.java index f9895960d47..8c37c0d61e1 100644 --- a/servers/services/src/testFixtures/java/org/projectnessie/services/impl/AbstractTestMergeTransplant.java +++ b/servers/services/src/testFixtures/java/org/projectnessie/services/impl/AbstractTestMergeTransplant.java @@ -70,23 +70,13 @@ public abstract class AbstractTestMergeTransplant extends BaseTestServiceImpl { @ParameterizedTest @ValueSource(booleans = {true, false}) - public void transplantKeepCommits(boolean withDetachedCommit) - throws BaseNessieClientServerException { - testTransplant(withDetachedCommit, true); - } - - @ParameterizedTest - @ValueSource(booleans = {true, false}) - public void transplantSquashed(boolean withDetachedCommit) - throws BaseNessieClientServerException { - testTransplant(withDetachedCommit, false); + public void transplant(boolean withDetachedCommit) throws BaseNessieClientServerException { + testTransplant(withDetachedCommit); } - private void testTransplant(boolean withDetachedCommit, boolean keepIndividualCommits) - throws BaseNessieClientServerException { + private void testTransplant(boolean withDetachedCommit) throws BaseNessieClientServerException { mergeTransplant( false, - keepIndividualCommits, (target, source, committed1, committed2, returnConflictAsResult) -> treeApi() .transplantCommitsIntoBranch( @@ -96,33 +86,20 @@ private void testTransplant(boolean withDetachedCommit, boolean keepIndividualCo ImmutableList.of( requireNonNull(committed1.getHash()), requireNonNull(committed2.getHash())), maybeAsDetachedName(withDetachedCommit, source), - keepIndividualCommits, emptyList(), NORMAL, false, false, returnConflictAsResult), withDetachedCommit, - "Transplanted"); - } - - @ParameterizedTest - @EnumSource(names = {"UNCHANGED", "DETACHED"}) // hash is required - public void mergeKeepCommits(ReferenceMode refMode) throws BaseNessieClientServerException { - testMerge(refMode, true); + false); } @ParameterizedTest @EnumSource(names = {"UNCHANGED", "DETACHED"}) // hash is required - public void mergeSquashed(ReferenceMode refMode) throws BaseNessieClientServerException { - testMerge(refMode, false); - } - - private void testMerge(ReferenceMode refMode, boolean keepIndividualCommits) - throws BaseNessieClientServerException { + public void merge(ReferenceMode refMode) throws BaseNessieClientServerException { mergeTransplant( - !keepIndividualCommits, - keepIndividualCommits, + true, (target, source, committed1, committed2, returnConflictAsResult) -> { Reference fromRef = refMode.transform(committed2); return treeApi() @@ -131,7 +108,6 @@ private void testMerge(ReferenceMode refMode, boolean keepIndividualCommits) target.getHash(), fromRef.getName(), fromRef.getHash(), - keepIndividualCommits, null, emptyList(), NORMAL, @@ -140,7 +116,7 @@ private void testMerge(ReferenceMode refMode, boolean keepIndividualCommits) returnConflictAsResult); }, refMode == ReferenceMode.DETACHED, - "Merged"); + true); } @FunctionalInterface @@ -156,10 +132,9 @@ MergeResponse act( private void mergeTransplant( boolean verifyAdditionalParents, - boolean keepIndividualCommits, MergeTransplantActor actor, boolean detached, - String mergedTransplanted) + boolean isMerge) throws BaseNessieClientServerException { Branch root = createBranch("root"); @@ -210,22 +185,40 @@ private void mergeTransplant( // try again --> conflict - soft.assertThatThrownBy(() -> actor.act(target, source, committed1, committed2, false)) - .isInstanceOf(NessieReferenceConflictException.class) - .hasMessageContaining("keys have been changed in conflict") - .asInstanceOf(type(NessieReferenceConflictException.class)) - .extracting(NessieReferenceConflictException::getErrorDetails) - .isNotNull() - .extracting(ReferenceConflicts::conflicts, list(Conflict.class)) - .hasSizeGreaterThan(0); - - // try again --> conflict, but return information - - conflictExceptionReturnedAsMergeResult( - actor, target, source, key1, committed1, committed2, newHead); + if (isNewStorageModel() && isMerge) { + // New storage model allows "merging the same branch again". If nothing changed, it returns a + // successful, but not-applied merge-response. This request is effectively a merge without any + // commits to merge, reported as "successful". + soft.assertThat(actor.act(target, source, committed1, committed2, false)) + .extracting( + MergeResponse::getCommonAncestor, + MergeResponse::getEffectiveTargetHash, + MergeResponse::getResultantTargetHash, + MergeResponse::wasApplied, + MergeResponse::wasSuccessful) + .containsExactly(committed2.getHash(), newHead.getHash(), newHead.getHash(), false, true); + } else if (!isNewStorageModel()) { + // For the new storage model, the following transplant will NOT fail (correct behavior), + // because the eventually + // applied content-value is exactly the same as the currently existing one. + + soft.assertThatThrownBy(() -> actor.act(target, source, committed1, committed2, false)) + .isInstanceOf(NessieReferenceConflictException.class) + .hasMessageContaining("keys have been changed in conflict") + .asInstanceOf(type(NessieReferenceConflictException.class)) + .extracting(NessieReferenceConflictException::getErrorDetails) + .isNotNull() + .extracting(ReferenceConflicts::conflicts, list(Conflict.class)) + .hasSizeGreaterThan(0); + + // try again --> conflict, but return information + + conflictExceptionReturnedAsMergeResult( + actor, target, source, key1, committed1, committed2, newHead); + } List log = commitLog(target.getName(), MINIMAL, target.getHash(), null, null); - if (keepIndividualCommits) { + if (!isMerge) { soft.assertThat(log.stream().map(LogEntry::getCommitMeta).map(CommitMeta::getMessage)) .containsExactly("test-branch2", "test-branch1", "test-main", "root"); } else { @@ -234,8 +227,8 @@ private void mergeTransplant( .first(InstanceOfAssertFactories.STRING) .isEqualTo( format( - "%s 2 commits from %s at %s into %s at %s", - mergedTransplanted, + "%s %s at %s into %s at %s", + isMerge ? "Merged" : "Transplanted 2 commits from", detached ? "DETACHED" : source.getName(), committed2.getHash(), target.getName(), @@ -389,7 +382,6 @@ public void mergeMessage() throws BaseNessieClientServerException { target.getHash(), source.getName(), source.getHash(), - false, CommitMeta.fromMessage("test-message-override-123"), emptyList(), NORMAL, @@ -438,7 +430,6 @@ public void mergeMessageDefault() throws BaseNessieClientServerException { target.getHash(), source.getName(), source.getHash(), - false, null, emptyList(), NORMAL, @@ -451,33 +442,12 @@ public void mergeMessageDefault() throws BaseNessieClientServerException { .extracting(e -> e.getCommitMeta().getMessage()) .containsExactly( format( - "Merged 2 commits from %s at %s into %s at %s", + "Merged %s at %s into %s at %s", source.getName(), source.getHash(), target.getName(), target.getHash())); } @Test - public void transplantMessageSquashed() throws BaseNessieClientServerException { - testMergeTransplantMessage( - (target, source, committed1, committed2, returnConflictAsResult) -> - treeApi() - .transplantCommitsIntoBranch( - target.getName(), - target.getHash(), - CommitMeta.fromMessage("test-message-override-123"), - ImmutableList.of( - requireNonNull(committed1.getHash()), requireNonNull(committed2.getHash())), - source.getName(), - false, - emptyList(), - NORMAL, - false, - false, - returnConflictAsResult), - ImmutableList.of("test-message-override-123")); - } - - @Test - public void transplantMessageSingle() throws BaseNessieClientServerException { + public void transplantMessage() throws BaseNessieClientServerException { testMergeTransplantMessage( (target, source, committed1, committed2, returnConflictAsResult) -> treeApi() @@ -487,7 +457,6 @@ public void transplantMessageSingle() throws BaseNessieClientServerException { CommitMeta.fromMessage("test-message-override-123"), ImmutableList.of(requireNonNull(committed1.getHash())), source.getName(), - false, emptyList(), NORMAL, false, @@ -508,7 +477,6 @@ public void transplantMessageOverrideMultiple() throws BaseNessieClientServerExc ImmutableList.of( requireNonNull(committed1.getHash()), requireNonNull(committed2.getHash())), source.getName(), - true, emptyList(), NORMAL, false, @@ -620,7 +588,6 @@ public void mergeWithNamespaces(ReferenceMode refMode) throws BaseNessieClientSe base.getHash(), fromRef.getName(), fromRef.getHash(), - false, null, emptyList(), NORMAL, @@ -634,7 +601,7 @@ public void mergeWithNamespaces(ReferenceMode refMode) throws BaseNessieClientSe .get() .isEqualTo( format( - "Merged 4 commits from %s at %s into %s at %s", + "Merged %s at %s into %s at %s", fromRef.getName(), fromRef.getHash(), base.getName(), base.getHash())); soft.assertThat( @@ -655,7 +622,6 @@ public void mergeWithCustomModes() throws BaseNessieClientServerException { target.getHash(), source.getName(), source.getHash(), - false, null, asList(MergeKeyBehavior.of(KEY_1, DROP), MergeKeyBehavior.of(KEY_3, NORMAL)), FORCE, @@ -676,7 +642,6 @@ public void transplantWithCustomModes() throws BaseNessieClientServerException { ImmutableList.of( requireNonNull(committed1.getHash()), requireNonNull(committed2.getHash())), source.getName(), - false, asList(MergeKeyBehavior.of(KEY_1, DROP), MergeKeyBehavior.of(KEY_3, NORMAL)), FORCE, false, diff --git a/servers/services/src/testFixtures/java/org/projectnessie/services/impl/AbstractTestMisc.java b/servers/services/src/testFixtures/java/org/projectnessie/services/impl/AbstractTestMisc.java index 34bc1fed842..98666cad2c3 100644 --- a/servers/services/src/testFixtures/java/org/projectnessie/services/impl/AbstractTestMisc.java +++ b/servers/services/src/testFixtures/java/org/projectnessie/services/impl/AbstractTestMisc.java @@ -40,7 +40,7 @@ public void testRepositoryInformation() { soft.assertThat(info.getNoAncestorHash()) .isNotNull() .isEqualTo(versionStore().noAncestorHash().asString()); - if (versionStore().getClass().getName().endsWith("VersionStoreImpl")) { + if (isNewStorageModel()) { soft.assertThat(info.getRepositoryCreationTimestamp()).isNotNull(); soft.assertThat(info.getOldestPossibleCommitTimestamp()).isNotNull(); } diff --git a/servers/services/src/testFixtures/java/org/projectnessie/services/impl/AbstractTestNamespace.java b/servers/services/src/testFixtures/java/org/projectnessie/services/impl/AbstractTestNamespace.java index 6c2f1be62a6..c305e9bc4a5 100644 --- a/servers/services/src/testFixtures/java/org/projectnessie/services/impl/AbstractTestNamespace.java +++ b/servers/services/src/testFixtures/java/org/projectnessie/services/impl/AbstractTestNamespace.java @@ -18,6 +18,7 @@ import static java.util.Collections.emptyList; import static java.util.Collections.emptyMap; import static java.util.Collections.emptySet; +import static org.assertj.core.api.Assumptions.assumeThat; import static org.assertj.core.api.InstanceOfAssertFactories.list; import static org.assertj.core.api.InstanceOfAssertFactories.type; import static org.projectnessie.model.CommitMeta.fromMessage; @@ -200,6 +201,8 @@ public void testNamespaceDeletion() throws BaseNessieClientServerException { @Test public void testNamespaceMerge() throws BaseNessieClientServerException { + assumeThat(databaseAdapter).isNotNull(); + Branch base = createBranch("merge-base"); base = commit( @@ -222,7 +225,6 @@ public void testNamespaceMerge() throws BaseNessieClientServerException { base.getHash(), branch.getName(), branch.getHash(), - true, null, emptyList(), NORMAL, @@ -230,11 +232,6 @@ public void testNamespaceMerge() throws BaseNessieClientServerException { false, false); - List log = commitLog(base.getName(), MINIMAL, base.getHash(), null, null); - String expectedCommitMsg = "create namespace a.b.c"; - soft.assertThat(log.stream().map(LogEntry::getCommitMeta).map(CommitMeta::getMessage)) - .containsExactly(expectedCommitMsg, "create namespaces", "root"); - soft.assertThat(entries(base.getName(), null).stream().map(EntriesResponse.Entry::getName)) .contains(ns.toContentKey()); @@ -275,7 +272,6 @@ public void testNamespaceMergeWithConflict() throws BaseNessieClientServerExcept finalBase.getHash(), finalBranch.getName(), finalBranch.getHash(), - false, CommitMeta.fromMessage("foo"), emptyList(), NORMAL, diff --git a/servers/services/src/testFixtures/java/org/projectnessie/services/impl/AbstractTestPrincipals.java b/servers/services/src/testFixtures/java/org/projectnessie/services/impl/AbstractTestPrincipals.java index 0e1cd78932d..43790270940 100644 --- a/servers/services/src/testFixtures/java/org/projectnessie/services/impl/AbstractTestPrincipals.java +++ b/servers/services/src/testFixtures/java/org/projectnessie/services/impl/AbstractTestPrincipals.java @@ -91,7 +91,6 @@ public void committerAndAuthorMerge() throws Exception { merge.getHash(), main.getName(), main.getHash(), - false, null, emptyList(), NORMAL, @@ -111,53 +110,13 @@ public void committerAndAuthorMerge() throws Exception { CommitMeta::getHash) .containsExactly( "NessieHerself", - "ThatNessieGuy", + isNewStorageModel() ? "NessieHerself" : "ThatNessieGuy", format( - "Merged 2 commits from %s at %s into %s at %s", + "Merged %s at %s into %s at %s", main.getName(), main.getHash(), merge.getName(), merge.getHash()), merged.getHash()); } - @Test - public void committerAndAuthorMergeUnsquashed() throws Exception { - Branch root = createBranch("root"); - - root = - commit( - root, - CommitMeta.fromMessage("root"), - Put.of(ContentKey.of("other"), IcebergTable.of("/dev/null", 42, 42, 42, 42))) - .getTargetBranch(); - - Branch main = makeCommits(createBranch("committerAndAuthorMergeUnsquashed_main", root)); - Branch merge = createBranch("committerAndAuthorMergeUnsquashed_target", root); - - setPrincipal(() -> "NessieHerself"); - - treeApi() - .mergeRefIntoBranch( - merge.getName(), - merge.getHash(), - main.getName(), - main.getHash(), - true, - null, - emptyList(), - NORMAL, - false, - false, - false); - - merge = (Branch) getReference(merge.getName()); - - assertThat(commitLog(merge.getName()).stream().limit(2).collect(Collectors.toList())) - .extracting(LogEntry::getCommitMeta) - .extracting(CommitMeta::getCommitter, CommitMeta::getAuthor, CommitMeta::getMessage) - .containsExactly( - tuple("NessieHerself", "ThatNessieGuy", "with security"), - tuple("NessieHerself", "NessieHerself", "no security context")); - } - @Test public void committerAndAuthorTransplant() throws Exception { Branch main = makeCommits(createBranch("committerAndAuthorTransplant_main")); @@ -179,7 +138,6 @@ public void committerAndAuthorTransplant() throws Exception { null, hashesToTransplant, main.getName(), - true, emptyList(), NORMAL, false, diff --git a/servers/services/src/testFixtures/java/org/projectnessie/services/impl/AbstractTestReferences.java b/servers/services/src/testFixtures/java/org/projectnessie/services/impl/AbstractTestReferences.java index d21b6af5b61..e95e1a2e9a3 100644 --- a/servers/services/src/testFixtures/java/org/projectnessie/services/impl/AbstractTestReferences.java +++ b/servers/services/src/testFixtures/java/org/projectnessie/services/impl/AbstractTestReferences.java @@ -287,7 +287,6 @@ public void referenceNames(String refNamePart) throws BaseNessieClientServerExce branchHash2, branchName, newHash, - false, null, Collections.emptyList(), MergeBehavior.NORMAL, diff --git a/servers/services/src/testFixtures/java/org/projectnessie/services/impl/BaseTestServiceImpl.java b/servers/services/src/testFixtures/java/org/projectnessie/services/impl/BaseTestServiceImpl.java index 89245074758..59a218878f5 100644 --- a/servers/services/src/testFixtures/java/org/projectnessie/services/impl/BaseTestServiceImpl.java +++ b/servers/services/src/testFixtures/java/org/projectnessie/services/impl/BaseTestServiceImpl.java @@ -162,6 +162,10 @@ protected VersionStore versionStore() { throw new IllegalStateException("Neither Persist nor DatabaseAdapter instance present"); } + protected boolean isNewStorageModel() { + return versionStore().getClass().getName().endsWith("VersionStoreImpl"); + } + @BeforeEach protected void setup() { if (persist != null) { 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 d4e6aa339dc..87b47e61a80 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 @@ -2129,7 +2129,7 @@ protected CommitLogEntry squashCommits( return null; } - ByteString newCommitMeta = rewriteMetadata.squash(commitMeta); + ByteString newCommitMeta = rewriteMetadata.squash(commitMeta, commitMeta.size()); CommitLogEntry targetHeadCommit = fetchFromCommitLog(ctx, toHead); diff --git a/versioned/persist/store/src/main/java/org/projectnessie/versioned/persist/store/PersistVersionStore.java b/versioned/persist/store/src/main/java/org/projectnessie/versioned/persist/store/PersistVersionStore.java index 7139e1e596d..80e42028293 100644 --- a/versioned/persist/store/src/main/java/org/projectnessie/versioned/persist/store/PersistVersionStore.java +++ b/versioned/persist/store/src/main/java/org/projectnessie/versioned/persist/store/PersistVersionStore.java @@ -229,7 +229,7 @@ public MergeResult transplant(TransplantOp transplantOp) .sequenceToTransplant(transplantOp.sequenceToTransplant()) .updateCommitMetadata( updateCommitMetadataFunction(transplantOp.updateCommitMetadata())) - .keepIndividualCommits(transplantOp.keepIndividualCommits()) + .keepIndividualCommits(true) .mergeTypes(mergeTypes) .defaultMergeType(MergeType.valueOf(transplantOp.defaultMergeBehavior().name())) .isDryRun(transplantOp.dryRun()) @@ -259,7 +259,7 @@ public MergeResult merge(MergeOp mergeOp) .mergeFromHash(mergeOp.fromHash()) .updateCommitMetadata( updateCommitMetadataFunction(mergeOp.updateCommitMetadata())) - .keepIndividualCommits(mergeOp.keepIndividualCommits()) + .keepIndividualCommits(false) .mergeTypes(mergeTypes) .defaultMergeType(MergeType.valueOf(mergeOp.defaultMergeBehavior().name())) .isDryRun(mergeOp.dryRun()) @@ -369,12 +369,13 @@ public ByteString rewriteSingle(ByteString metadata) { } @Override - public ByteString squash(List metadata) { + public ByteString squash(List metadata, int numCommits) { return serializeMetadata( updateCommitMetadata.squash( metadata.stream() .map(PersistVersionStore.this::deserializeMetadata) - .collect(Collectors.toList()))); + .collect(Collectors.toList()), + numCommits)); } }; } diff --git a/versioned/persist/tests/src/main/java/org/projectnessie/versioned/persist/tests/AbstractEvents.java b/versioned/persist/tests/src/main/java/org/projectnessie/versioned/persist/tests/AbstractEvents.java index 9ad841999c2..72100b26d99 100644 --- a/versioned/persist/tests/src/main/java/org/projectnessie/versioned/persist/tests/AbstractEvents.java +++ b/versioned/persist/tests/src/main/java/org/projectnessie/versioned/persist/tests/AbstractEvents.java @@ -424,7 +424,7 @@ public ByteString rewriteSingle(ByteString metadata) { } @Override - public ByteString squash(List metadata) { + public ByteString squash(List metadata, int numCommits) { return rewriteSingle(metadata.get(0)); } }; diff --git a/versioned/persist/tests/src/main/java/org/projectnessie/versioned/persist/tests/AbstractMergeTransplant.java b/versioned/persist/tests/src/main/java/org/projectnessie/versioned/persist/tests/AbstractMergeTransplant.java index 13e613ade89..51587dd224e 100644 --- a/versioned/persist/tests/src/main/java/org/projectnessie/versioned/persist/tests/AbstractMergeTransplant.java +++ b/versioned/persist/tests/src/main/java/org/projectnessie/versioned/persist/tests/AbstractMergeTransplant.java @@ -124,7 +124,7 @@ public ByteString rewriteSingle(ByteString metadata) { } @Override - public ByteString squash(List metadata) { + public ByteString squash(List metadata, int numCommits) { return ByteString.copyFromUtf8( metadata.stream().map(ByteString::toStringUtf8).collect(Collectors.joining(";")) + " " diff --git a/servers/services/src/main/java/org/projectnessie/services/impl/CommitMetaUpdater.java b/versioned/spi/src/main/java/org/projectnessie/versioned/DefaultMetadataRewriter.java similarity index 87% rename from servers/services/src/main/java/org/projectnessie/services/impl/CommitMetaUpdater.java rename to versioned/spi/src/main/java/org/projectnessie/versioned/DefaultMetadataRewriter.java index e925481a320..10a70ce1575 100644 --- a/servers/services/src/main/java/org/projectnessie/services/impl/CommitMetaUpdater.java +++ b/versioned/spi/src/main/java/org/projectnessie/versioned/DefaultMetadataRewriter.java @@ -13,7 +13,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -package org.projectnessie.services.impl; +package org.projectnessie.versioned; import static java.util.Collections.emptyList; import static java.util.Collections.singletonList; @@ -30,15 +30,17 @@ import java.util.function.Supplier; import org.projectnessie.model.CommitMeta; import org.projectnessie.model.ImmutableCommitMeta; -import org.projectnessie.versioned.MetadataRewriter; -public class CommitMetaUpdater implements MetadataRewriter { +public class DefaultMetadataRewriter implements MetadataRewriter { + public static final MetadataRewriter DEFAULT_METADATA_REWRITER = + new DefaultMetadataRewriter(null, null, null, numCommits -> null); + private final String committer; private final Instant now; private final CommitMeta commitMetaOverride; private final IntFunction squashMessage; - public CommitMetaUpdater( + public DefaultMetadataRewriter( String committer, Instant now, CommitMeta commitMetaOverride, @@ -57,8 +59,8 @@ private CommitMeta buildCommitMeta( if (hasAuthors(commitMetaOverride)) { metaBuilder.allAuthors(emptyList()); copyAuthors(commitMetaOverride, metaBuilder::addAllAuthors); - } else if (!hasAuthors(pre)) { - metaBuilder.allAuthors(singletonList(committer != null ? committer : "")); + } else if (!hasAuthors(pre) && committer != null) { + metaBuilder.allAuthors(singletonList(committer)); } if (commitMetaOverride != null && !commitMetaOverride.getAllSignedOffBy().isEmpty()) { @@ -90,14 +92,16 @@ private CommitMeta buildCommitMeta( @Override public CommitMeta rewriteSingle(CommitMeta metadata) { - return buildCommitMeta(CommitMeta.builder().from(metadata), metadata::getMessage); + return buildCommitMeta( + CommitMeta.builder().from(metadata).hash(null).parentCommitHashes(emptyList()), + metadata::getMessage); } @Override - public CommitMeta squash(List metadata) { - Optional msg = Optional.ofNullable(squashMessage.apply(metadata.size())); + public CommitMeta squash(List metadata, int numCommits) { + Optional msg = Optional.ofNullable(squashMessage.apply(numCommits)); - if (metadata.size() == 1 && !msg.isPresent()) { + if (numCommits == 1 && !msg.isPresent()) { return rewriteSingle(metadata.get(0)); } diff --git a/versioned/spi/src/main/java/org/projectnessie/versioned/MetadataRewriter.java b/versioned/spi/src/main/java/org/projectnessie/versioned/MetadataRewriter.java index d902bd3c919..3fc3c0e831c 100644 --- a/versioned/spi/src/main/java/org/projectnessie/versioned/MetadataRewriter.java +++ b/versioned/spi/src/main/java/org/projectnessie/versioned/MetadataRewriter.java @@ -16,23 +16,9 @@ package org.projectnessie.versioned; import java.util.List; -import org.projectnessie.model.CommitMeta; public interface MetadataRewriter { T rewriteSingle(T metadata); - T squash(List metadata); - - MetadataRewriter NOOP_REWRITER = - new MetadataRewriter() { - @Override - public CommitMeta rewriteSingle(CommitMeta metadata) { - return metadata; - } - - @Override - public CommitMeta squash(List metadata) { - return metadata.get(0); - } - }; + T squash(List metadata, int numCommits); } diff --git a/versioned/spi/src/main/java/org/projectnessie/versioned/VersionStore.java b/versioned/spi/src/main/java/org/projectnessie/versioned/VersionStore.java index 8a3dec37073..cfa3c28e5ac 100644 --- a/versioned/spi/src/main/java/org/projectnessie/versioned/VersionStore.java +++ b/versioned/spi/src/main/java/org/projectnessie/versioned/VersionStore.java @@ -15,7 +15,7 @@ */ package org.projectnessie.versioned; -import static org.projectnessie.versioned.MetadataRewriter.NOOP_REWRITER; +import static org.projectnessie.versioned.DefaultMetadataRewriter.DEFAULT_METADATA_REWRITER; import com.google.errorprone.annotations.MustBeClosed; import java.util.Collection; @@ -135,18 +135,12 @@ default Optional expectedHash() { return Optional.empty(); } - /** - * Function that rewrites the commit metadata, gets a multiple commit metadata if {@link - * #keepIndividualCommits()} is {@code false}. - */ + /** Function that rewrites the commit metadata. */ @Value.Default default MetadataRewriter updateCommitMetadata() { - return NOOP_REWRITER; + return DEFAULT_METADATA_REWRITER; } - /** Whether to keep the individual commits and do not squash the commits to merge. */ - boolean keepIndividualCommits(); - /** Merge behaviors per content key. */ Map mergeKeyBehaviors(); @@ -180,12 +174,6 @@ interface MergeOp extends MergeTransplantOpBase { /** The hash we are using to get additional commits. */ Hash fromHash(); - @Override - @Value.Default - default boolean keepIndividualCommits() { - return false; - } - static ImmutableMergeOp.Builder builder() { return ImmutableMergeOp.builder(); } @@ -196,12 +184,6 @@ interface TransplantOp extends MergeTransplantOpBase { /** The sequence of hashes to transplant. */ List sequenceToTransplant(); - @Override - @Value.Default - default boolean keepIndividualCommits() { - return true; - } - static ImmutableTransplantOp.Builder builder() { return ImmutableTransplantOp.builder(); } diff --git a/servers/services/src/test/java/org/projectnessie/services/impl/TestCommitMetaUpdater.java b/versioned/spi/src/test/java/org/projectnessie/versioned/TestDefaultMetadataRewriter.java similarity index 80% rename from servers/services/src/test/java/org/projectnessie/services/impl/TestCommitMetaUpdater.java rename to versioned/spi/src/test/java/org/projectnessie/versioned/TestDefaultMetadataRewriter.java index dffbe74d2c8..171cad049a4 100644 --- a/servers/services/src/test/java/org/projectnessie/services/impl/TestCommitMetaUpdater.java +++ b/versioned/spi/src/test/java/org/projectnessie/versioned/TestDefaultMetadataRewriter.java @@ -13,7 +13,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -package org.projectnessie.services.impl; +package org.projectnessie.versioned; import static java.time.Instant.now; import static java.time.temporal.ChronoUnit.DAYS; @@ -35,7 +35,7 @@ import org.projectnessie.model.CommitMeta; @ExtendWith(SoftAssertionsExtension.class) -class TestCommitMetaUpdater { +class TestDefaultMetadataRewriter { @InjectSoftAssertions protected SoftAssertions soft; static Instant NOW = now(); @@ -46,55 +46,59 @@ class TestCommitMetaUpdater { @MethodSource("rewriteSingle") void rewriteSingle(String committer, CommitMeta inNessie, CommitMeta expected) { CommitMeta parameter = null; - CommitMetaUpdater updater = new CommitMetaUpdater(committer, NOW, parameter, p -> null); + DefaultMetadataRewriter updater = + new DefaultMetadataRewriter(committer, NOW, parameter, p -> null); soft.assertThat(updater.rewriteSingle(inNessie)).isEqualTo(expected); - soft.assertThat(updater.squash(singletonList(inNessie))).isEqualTo(expected); + soft.assertThat(updater.squash(singletonList(inNessie), 1)).isEqualTo(expected); parameter = fromMessage("forced message"); - updater = new CommitMetaUpdater(committer, NOW, parameter, p -> null); + updater = new DefaultMetadataRewriter(committer, NOW, parameter, p -> null); CommitMeta expectedUpdated = CommitMeta.builder().from(expected).message("forced message").build(); soft.assertThat(updater.rewriteSingle(inNessie)).isEqualTo(expectedUpdated); - soft.assertThat(updater.squash(singletonList(inNessie))).isEqualTo(expectedUpdated); + soft.assertThat(updater.squash(singletonList(inNessie), 1)).isEqualTo(expectedUpdated); parameter = CommitMeta.builder().message("").authorTime(AUTHOR_TIME_OLDER).build(); - updater = new CommitMetaUpdater(committer, NOW, parameter, p -> null); + updater = new DefaultMetadataRewriter(committer, NOW, parameter, p -> null); expectedUpdated = CommitMeta.builder().from(expected).authorTime(AUTHOR_TIME_OLDER).build(); soft.assertThat(updater.rewriteSingle(inNessie)).isEqualTo(expectedUpdated); - soft.assertThat(updater.squash(singletonList(inNessie))).isEqualTo(expectedUpdated); + soft.assertThat(updater.squash(singletonList(inNessie), 1)).isEqualTo(expectedUpdated); parameter = CommitMeta.builder().message("").addAllAuthors("myself").build(); - updater = new CommitMetaUpdater(committer, NOW, parameter, p -> null); + updater = new DefaultMetadataRewriter(committer, NOW, parameter, p -> null); expectedUpdated = CommitMeta.builder().from(expected).allAuthors(singletonList("myself")).build(); soft.assertThat(updater.rewriteSingle(inNessie)).isEqualTo(expectedUpdated); - soft.assertThat(updater.squash(singletonList(inNessie))).isEqualTo(expectedUpdated); + soft.assertThat(updater.squash(singletonList(inNessie), 1)).isEqualTo(expectedUpdated); } @ParameterizedTest @MethodSource("squash") void squash(String committer, List inNessie, CommitMeta expected) { CommitMeta parameter = null; - CommitMetaUpdater updater = - new CommitMetaUpdater(committer, NOW, parameter, p -> "Default message for " + p); - soft.assertThat(updater.squash(inNessie)).isEqualTo(expected); + DefaultMetadataRewriter updater = + new DefaultMetadataRewriter(committer, NOW, parameter, p -> "Default message for " + p); + soft.assertThat(updater.squash(inNessie, inNessie.size())).isEqualTo(expected); parameter = fromMessage("forced message"); - updater = new CommitMetaUpdater(committer, NOW, parameter, p -> "Default message for " + p); + updater = + new DefaultMetadataRewriter(committer, NOW, parameter, p -> "Default message for " + p); CommitMeta expectedUpdated = CommitMeta.builder().from(expected).message("forced message").build(); - soft.assertThat(updater.squash(inNessie)).isEqualTo(expectedUpdated); + soft.assertThat(updater.squash(inNessie, inNessie.size())).isEqualTo(expectedUpdated); parameter = CommitMeta.builder().message("").authorTime(AUTHOR_TIME_OLDER).build(); - updater = new CommitMetaUpdater(committer, NOW, parameter, p -> "Default message for " + p); + updater = + new DefaultMetadataRewriter(committer, NOW, parameter, p -> "Default message for " + p); expectedUpdated = CommitMeta.builder().from(expected).authorTime(AUTHOR_TIME_OLDER).build(); - soft.assertThat(updater.squash(inNessie)).isEqualTo(expectedUpdated); + soft.assertThat(updater.squash(inNessie, inNessie.size())).isEqualTo(expectedUpdated); parameter = CommitMeta.builder().message("").addAllAuthors("myself").build(); - updater = new CommitMetaUpdater(committer, NOW, parameter, p -> "Default message for " + p); + updater = + new DefaultMetadataRewriter(committer, NOW, parameter, p -> "Default message for " + p); expectedUpdated = CommitMeta.builder().from(expected).allAuthors(singletonList("myself")).build(); - soft.assertThat(updater.squash(inNessie)).isEqualTo(expectedUpdated); + soft.assertThat(updater.squash(inNessie, inNessie.size())).isEqualTo(expectedUpdated); } static Stream rewriteSingle() { diff --git a/versioned/storage/common-tests/build.gradle.kts b/versioned/storage/common-tests/build.gradle.kts index 9987659e78d..b931e65cf8f 100644 --- a/versioned/storage/common-tests/build.gradle.kts +++ b/versioned/storage/common-tests/build.gradle.kts @@ -31,6 +31,7 @@ dependencies { implementation(project(":nessie-versioned-tests")) implementation(project(":nessie-versioned-spi")) implementation(project(":nessie-server-store")) + implementation(project(":nessie-model")) // javax/jakarta compileOnly(libs.jakarta.validation.api) @@ -47,6 +48,10 @@ dependencies { compileOnly(libs.immutables.value.annotations) annotationProcessor(libs.immutables.value.processor) + compileOnly(libs.microprofile.openapi) + compileOnly(platform(libs.jackson.bom)) + compileOnly("com.fasterxml.jackson.core:jackson-annotations") + implementation(platform(libs.junit.bom)) implementation(libs.bundles.junit.testing) diff --git a/versioned/storage/common-tests/src/main/java/org/projectnessie/versioned/storage/commontests/AbstractMergeScenarios.java b/versioned/storage/common-tests/src/main/java/org/projectnessie/versioned/storage/commontests/AbstractMergeScenarios.java new file mode 100644 index 00000000000..a394a3e3c06 --- /dev/null +++ b/versioned/storage/common-tests/src/main/java/org/projectnessie/versioned/storage/commontests/AbstractMergeScenarios.java @@ -0,0 +1,436 @@ +/* + * Copyright (C) 2023 Dremio + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.projectnessie.versioned.storage.commontests; + +import static java.util.Objects.requireNonNull; + +import com.google.errorprone.annotations.CanIgnoreReturnValue; +import java.util.ArrayList; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.Optional; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicLong; +import org.assertj.core.api.SoftAssertions; +import org.assertj.core.api.junit.jupiter.InjectSoftAssertions; +import org.assertj.core.api.junit.jupiter.SoftAssertionsExtension; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.projectnessie.model.CommitMeta; +import org.projectnessie.model.ContentKey; +import org.projectnessie.model.IcebergTable; +import org.projectnessie.versioned.BranchName; +import org.projectnessie.versioned.Commit; +import org.projectnessie.versioned.CommitResult; +import org.projectnessie.versioned.Delete; +import org.projectnessie.versioned.Hash; +import org.projectnessie.versioned.ImmutableMergeOp; +import org.projectnessie.versioned.MergeResult; +import org.projectnessie.versioned.Operation; +import org.projectnessie.versioned.Put; +import org.projectnessie.versioned.ReferenceNotFoundException; +import org.projectnessie.versioned.VersionStore; +import org.projectnessie.versioned.VersionStoreException; +import org.projectnessie.versioned.tests.AbstractNestedVersionStore; + +@SuppressWarnings("unused") +@ExtendWith(SoftAssertionsExtension.class) +public abstract class AbstractMergeScenarios extends AbstractNestedVersionStore { + @InjectSoftAssertions protected SoftAssertions soft; + + protected AbstractMergeScenarios(VersionStore store) { + super(store); + } + + /** + * Unrelated branches case. 

+   * ----B-----D      b2
+   *
+   * ----A-----C      b1
+   *
+ */ + @Test + void noMergeBase() throws VersionStoreException { + Commit a = buildCommit().commitToNewBranch("b1"); + Commit b = buildCommit().commitToNewBranch("b2"); + Commit c = buildCommit().commitTo("b1"); + Commit d = buildCommit().commitTo("b2"); + + soft.assertThatThrownBy(() -> buildMerge().from("b1").merge("b2")) + .isInstanceOf(ReferenceNotFoundException.class) + .hasMessageStartingWith("No common ancestor"); + } + + /** + * Test a simple merge base case. 
+   *       ----B      b2
+   *      /
+   * ----A-----C      b1
+   *
+ * + *

Best merge-base of {@code B} onto {@code C} is {@code A}. + */ + @Test + void simpleCase() throws VersionStoreException { + Commit a = buildCommit().commitToNewBranch("b1"); + Commit b = buildCommit().commitToNewBranch("b2", "b1", a); + Commit c = buildCommit().commitTo("b1"); + + buildMerge().from("b2").assertMergeBase("b1", a); + } + + /** + * Merge-again case. 

+   *       ----B---------E      b2
+   *      /          \
+   * ----A------C-----D         b1
+   *
+ * + *

Best merge-base of {@code E} onto {@code D} is {@code B}. + */ + @Test + void doubleMerge() throws VersionStoreException { + Commit a = buildCommit().commitToNewBranch("b1"); + Commit b = buildCommit().commitToNewBranch("b2", "b1", a); + Commit c = buildCommit().commitTo("b1"); + Commit d = buildMerge().from("b2").merge("b1"); + Commit e = buildCommit().commitTo("b2"); + + buildMerge().from("b1").assertMergeBase("b2", b); + } + + /** + * Merge-again case. 

+   *       ----B----D-------F      b2
+   *      /             \
+   * ----A------C--------E         b1
+   *
+ * + *

Best merge-base of {@code F} onto {@code E} is {@code D}. + */ + @Test + void doubleMerge2() throws VersionStoreException { + Commit a = buildCommit().commitToNewBranch("b1"); + Commit b = buildCommit().commitToNewBranch("b2", "b1", a); + Commit c = buildCommit().commitTo("b1"); + Commit d = buildCommit().commitTo("b2"); + Commit e = buildMerge().from("b2").merge("b1"); + Commit f = buildCommit().commitTo("b2"); + + buildMerge().from("b1").assertMergeBase("b2", d); + } + + /** + * Merge-again case. 

+   *              ----E----G---------I      b3
+   *            /       /           /
+   *       ----B----D----------H----        b2
+   *      /               /
+   * ----A------C--------F                  b1
+   *
+ * + *

Best merge-base of {@code I} onto {@code F} is {@code F}. + */ + @Test + void multiMerge1() throws VersionStoreException { + Commit a = buildCommit().commitToNewBranch("b1"); + Commit b = buildCommit().commitToNewBranch("b2", "b1", a); + Commit c = buildCommit().commitTo("b1"); + Commit d = buildCommit().commitTo("b2"); + Commit e = buildCommit().commitToNewBranch("b3", "b2", b); + Commit f = buildCommit().commitTo("b1"); + Commit g = buildMerge().from("b2").merge("b3"); + Commit h = buildMerge().from("b1").merge("b2"); + Commit i = buildMerge().from("b1").merge("b3"); + + buildMerge().from("b3").assertMergeBase("b1", f); + } + + /** + * Merge-again case. 

+   * A ----- C - E - F - G               b1
+   *  \- B -/           /                b2
+   *   \    \--------- / ---\
+   *    \- D ---------/------ H - J      b3
+   *
+ * + *

Best merge-base of {@code J} onto {@code G} is {@code D}. + */ + @Test + void multiMerge2() throws VersionStoreException { + Commit a = buildCommit().commitToNewBranch("b1"); + Commit b = buildCommit().commitToNewBranch("b2", "b1", a); + Commit c = buildMerge().from("b2").merge("b1"); + Commit d = buildCommit().commitToNewBranch("b3", "b1", a); + Commit e = buildCommit().commitTo("b1"); + Commit f = buildCommit().commitTo("b1"); + Commit g = buildMerge().from("b3").merge("b1"); + Commit h = buildMerge().from("b2").merge("b3"); + Commit j = buildCommit().commitTo("b3"); + + buildMerge().from("b3").assertMergeBase("b1", d); + } + + /** + * Cross-merge case. 

+   *        ----B--------D----F      b2
+   *       /       \ /
+   *      /        / \
+   * ----A------C--------E----G      b1
+   *
+ * + *

Merge-base outcome for {@code F} onto {@code G} is either {@code B} or {@code C}. + */ + @Test + void afterCrossMerge() throws VersionStoreException { + Commit a = buildCommit().commitToNewBranch("b1"); + Commit b = buildCommit().commitToNewBranch("b2", "b1", a); + Commit c = buildCommit().commitTo("b1"); + Commit d = buildMerge().from("b1", c).merge("b2"); + Commit e = buildMerge().from("b2", b).merge("b1"); + Commit f = buildCommit().commitTo("b2"); + Commit g = buildCommit().commitTo("b1"); + + buildMerge().dryRun().from("b2").assertMergeBase("b1", b); + buildMerge().dryRun().from("b1").assertMergeBase("b2", c); + } + + /** + * Merge two previous merges. 

+   *        B-----------------I      b3
+   *       /                 /
+   *      / F-----------G   /        b2
+   *     | /           /   /
+   *     |/           /   /
+   * ----A---C---D---E---H           b1
+   *
+ * + *

Best merge-base of {@code I} onto {@code G} is {@code E}. + */ + @Test + void nestedBranches() throws VersionStoreException { + Commit a = buildCommit().commitToNewBranch("b1"); + Commit b = buildCommit().commitToNewBranch("b3", "b1", a); + Commit c = buildCommit().commitTo("b1"); + Commit d = buildCommit().commitTo("b1"); + Commit e = buildCommit().commitTo("b1"); + Commit f = buildCommit().commitToNewBranch("b2", "b1", a); + Commit g = buildMerge().from("b1", e).merge("b2"); + Commit h = buildCommit().commitTo("b1"); + Commit i = buildMerge().from("b1").merge("b3"); + + buildMerge().dryRun().from("b3").assertMergeBase("b2", e); + } + + /** + * Merge two previous merges. 

+   *      ---- B ---- E ---- G ---- I ---- K      b2
+   *     /      /             /
+   *    /      /             /
+   *   A ---- C ---- D ---- F ---- H ---- J       b1
+   *
+ * + *

Best merge-base of {@code K} onto {@code J} is {@code F}. + */ + @Test + void featureBranch() throws VersionStoreException { + Commit a = buildCommit().commitToNewBranch("b1"); + Commit b = buildCommit().commitToNewBranch("b2", "b1", a); + Commit c = buildCommit().commitTo("b1"); + Commit d = buildCommit().commitTo("b1"); + Commit e = buildMerge().from("b1", c).merge("b2"); + Commit f = buildCommit().commitTo("b1"); + Commit g = buildCommit().commitTo("b2"); + Commit h = buildCommit().commitTo("b1"); + Commit i = buildMerge().from("b1", f).merge("b2"); + Commit j = buildCommit().commitTo("b1"); + Commit k = buildCommit().commitTo("b2"); + + buildMerge().from("b2").assertMergeBase("b1", f); + } + + private MergeBuilder buildMerge() { + return new MergeBuilder(); + } + + private CommitBuilder buildCommit() { + return new CommitBuilder(); + } + + private String commitMessage() { + char c = (char) ('a' + commitCount.getAndIncrement()); + return Character.toString(c); + } + + private final class MergeBuilder { + final ImmutableMergeOp.Builder merge = VersionStore.MergeOp.builder(); + + @CanIgnoreReturnValue + MergeBuilder from(String from) { + Hash head = requireNonNull(branches.get(from), "Branch " + from + " not created"); + merge.fromRef(BranchName.of(from)).fromHash(head); + return this; + } + + @CanIgnoreReturnValue + MergeBuilder from(String from, Commit fromCommit) { + merge.fromRef(BranchName.of(from)).fromHash(fromCommit.getHash()); + return this; + } + + @CanIgnoreReturnValue + MergeBuilder dryRun() { + merge.dryRun(true); + return this; + } + + Commit merge(String target) throws VersionStoreException { + return doMerge(target).getCreatedCommits().get(0); + } + + Hash mergeReturnMergeBase(String target) throws VersionStoreException { + return doMerge(target).getCommonAncestor(); + } + + void assertMergeBase(String target, Commit expected) throws VersionStoreException { + Hash mergeBase = mergeReturnMergeBase(target); + String expectedCommitMessage = commits.get(expected.getHash()); + String receivedCommitMessage = commits.get(mergeBase); + soft.assertThat(mergeBase) + .describedAs( + "Expected commit '%s', but got commit '%s' for merge from %s onto %s (expected hash %s, got hash %s)", + expectedCommitMessage, + receivedCommitMessage, + merge.build().fromRef().getName(), + target, + expected.getHash(), + mergeBase) + .isEqualTo(expected.getHash()); + } + + MergeResult doMerge(String target) throws VersionStoreException { + Hash head = requireNonNull(branches.get(target), "Branch " + target + " not created"); + VersionStore.MergeOp op = + merge.toBranch(BranchName.of(target)).expectedHash(Optional.of(head)).build(); + MergeResult result = store().merge(op); + if (!op.dryRun()) { + head = result.getCreatedCommits().get(0).getHash(); + branches.put(target, head); + commits.put(head, commitMessage()); + } + return result; + } + } + + final Map branches = new HashMap<>(); + final Map commits = new HashMap<>(); + final Map tableIds = new HashMap<>(); + final AtomicInteger commitCount = new AtomicInteger(); + final AtomicLong unique = new AtomicLong(); + + @BeforeEach + void setup() { + tableIds.clear(); + commits.clear(); + commitCount.set(0); + unique.set(0); + } + + private final class CommitBuilder { + final CommitMeta.Builder meta; + final List operations = new ArrayList<>(); + + CommitBuilder() { + meta = CommitMeta.builder(); + } + + @CanIgnoreReturnValue + CommitBuilder createTable(String name) { + operations.add( + Put.of( + ContentKey.fromPathString(name), + IcebergTable.of(name, unique.incrementAndGet(), 1, 2, 3))); + return this; + } + + @CanIgnoreReturnValue + CommitBuilder update(String name) { + ContentKey key = ContentKey.fromPathString(name); + String cid = requireNonNull(tableIds.get(key), "Table " + name + " not yet created"); + operations.add(Put.of(key, IcebergTable.of(name, unique.incrementAndGet(), 1, 2, 3, cid))); + return this; + } + + @CanIgnoreReturnValue + CommitBuilder delete(String name) { + ContentKey key = ContentKey.fromPathString(name); + requireNonNull(tableIds.get(key), "Table " + name + " not yet created"); + operations.add(Delete.of(key)); + return this; + } + + Commit commitToNewBranch(String branch) throws VersionStoreException { + return commitToNewBranch(branch, null, null); + } + + Commit commitToNewBranch(String branch, String from, Commit fromCommit) + throws VersionStoreException { + if (branches.containsKey(branch)) { + throw new IllegalStateException("Branch " + branch + " already created"); + } + + Hash origin = null; + if (from != null) { + origin = fromCommit.getHash(); + requireNonNull(branches.get(from), "Branch " + from + " not yet created"); + } + + Hash head = store().create(BranchName.of(branch), Optional.ofNullable(origin)).getHash(); + branches.put(branch, head); + + return commitTo(branch); + } + + Commit commitTo(String branch) throws VersionStoreException { + String msg = commitMessage(); + meta.message(msg); + + if (operations.isEmpty()) { + createTable("table created on " + msg); + } + + Hash head = branches.get(branch); + requireNonNull(head, "Branch " + branch + " not created"); + CommitResult result = + store() + .commit( + BranchName.of(branch), + Optional.of(head), + meta.build(), + operations, + v -> {}, + tableIds::put); + + head = result.getCommitHash(); + commits.put(head, msg); + branches.put(branch, head); + + return result.getCommit(); + } + } +} diff --git a/versioned/storage/common-tests/src/main/java/org/projectnessie/versioned/storage/commontests/AbstractVersionStoreTests.java b/versioned/storage/common-tests/src/main/java/org/projectnessie/versioned/storage/commontests/AbstractVersionStoreTests.java index 289d217c1dd..88ede7e56e1 100644 --- a/versioned/storage/common-tests/src/main/java/org/projectnessie/versioned/storage/commontests/AbstractVersionStoreTests.java +++ b/versioned/storage/common-tests/src/main/java/org/projectnessie/versioned/storage/commontests/AbstractVersionStoreTests.java @@ -16,6 +16,7 @@ package org.projectnessie.versioned.storage.commontests; import org.assertj.core.api.junit.jupiter.SoftAssertionsExtension; +import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.extension.ExtendWith; import org.projectnessie.versioned.VersionStore; import org.projectnessie.versioned.storage.common.persist.Persist; @@ -33,4 +34,11 @@ public class AbstractVersionStoreTests extends AbstractVersionStoreTestBase { protected VersionStore store() { return new VersionStoreImpl(persist); } + + @Nested + public class MergeScenarios extends AbstractMergeScenarios { + public MergeScenarios() { + super(AbstractVersionStoreTests.this.store()); + } + } } diff --git a/versioned/storage/common/src/main/java/org/projectnessie/versioned/storage/common/logic/CommitLogic.java b/versioned/storage/common/src/main/java/org/projectnessie/versioned/storage/common/logic/CommitLogic.java index 1a2cb286c74..1a0e763e341 100644 --- a/versioned/storage/common/src/main/java/org/projectnessie/versioned/storage/common/logic/CommitLogic.java +++ b/versioned/storage/common/src/main/java/org/projectnessie/versioned/storage/common/logic/CommitLogic.java @@ -151,8 +151,8 @@ boolean storeCommit( * @param conflictHandler Callback that decides how a particular {@link CommitConflict} shall be * handled. *

The callback can decide among the simple resolutions {@link ConflictResolution#CONFLICT} - * to propagate the conflict, {@link ConflictResolution#IGNORE} to commit the conflict and - * {@link ConflictResolution#DROP} to not commit the conflict. + * to propagate the conflict, {@link ConflictResolution#ADD} to commit the conflict and {@link + * ConflictResolution#DROP} to not commit the conflict. *

Advanced conflict resolutions can be implemented via the {@code * expectedValueReplacement} and {@code committedValueReplacement} callbacks, which are * evaluated before conflict detection happens.. @@ -194,8 +194,15 @@ interface ValueReplacement { @Nonnull @jakarta.annotation.Nonnull ObjId findCommonAncestor( - @Nonnull @jakarta.annotation.Nonnull ObjId headId, - @Nonnull @jakarta.annotation.Nonnull ObjId otherId) + @Nonnull @jakarta.annotation.Nonnull ObjId targetId, + @Nonnull @jakarta.annotation.Nonnull ObjId sourceId) + throws NoSuchElementException; + + @Nonnull + @jakarta.annotation.Nonnull + ObjId findMergeBase( + @Nonnull @jakarta.annotation.Nonnull ObjId targetId, + @Nonnull @jakarta.annotation.Nonnull ObjId sourceId) throws NoSuchElementException; /** Retrieves the {@link CommitObj commit object} referenced by {@code commitId}. */ diff --git a/versioned/storage/common/src/main/java/org/projectnessie/versioned/storage/common/logic/CommitLogicImpl.java b/versioned/storage/common/src/main/java/org/projectnessie/versioned/storage/common/logic/CommitLogicImpl.java index 8fef3fedc25..e12f1f04a7f 100644 --- a/versioned/storage/common/src/main/java/org/projectnessie/versioned/storage/common/logic/CommitLogicImpl.java +++ b/versioned/storage/common/src/main/java/org/projectnessie/versioned/storage/common/logic/CommitLogicImpl.java @@ -35,7 +35,6 @@ import static org.projectnessie.versioned.storage.common.logic.CommitConflict.ConflictType.PAYLOAD_DIFFERS; import static org.projectnessie.versioned.storage.common.logic.CommitConflict.ConflictType.VALUE_DIFFERS; import static org.projectnessie.versioned.storage.common.logic.CommitConflict.commitConflict; -import static org.projectnessie.versioned.storage.common.logic.CommitLogQuery.commitLogQuery; import static org.projectnessie.versioned.storage.common.logic.CommitLogic.ValueReplacement.NO_VALUE_REPLACEMENT; import static org.projectnessie.versioned.storage.common.logic.ConflictHandler.ConflictResolution.CONFLICT; import static org.projectnessie.versioned.storage.common.logic.CreateCommit.Add.commitAdd; @@ -74,7 +73,6 @@ import java.util.stream.Collectors; import javax.annotation.Nonnull; import javax.annotation.Nullable; -import org.agrona.collections.ObjectHashSet; import org.projectnessie.versioned.storage.common.config.StoreConfig; import org.projectnessie.versioned.storage.common.exceptions.CommitConflictException; import org.projectnessie.versioned.storage.common.exceptions.ObjNotFoundException; @@ -160,10 +158,13 @@ protected CommitObj computeNext() { try { b = batch = Arrays.asList(persist.fetchObjs(n.toArray(new ObjId[0]))).iterator(); } catch (ObjNotFoundException e) { + List ids = e.objIds(); throw new NoSuchElementException( - "Commit(s) " - + e.objIds().stream().map(ObjId::toString).collect(Collectors.joining(", ")) - + " not found"); + ids.size() == 1 + ? "Commit '" + ids.get(0) + "' not found" + : "Commit(s) " + + ids.stream().map(ObjId::toString).collect(Collectors.joining(", ")) + + " not found"); } } @@ -824,58 +825,37 @@ private static StoreIndexElement existingFromIndex( @jakarta.annotation.Nonnull @Override public ObjId findCommonAncestor( - @Nonnull @jakarta.annotation.Nonnull ObjId headId, - @Nonnull @jakarta.annotation.Nonnull ObjId otherId) + @Nonnull @jakarta.annotation.Nonnull ObjId targetId, + @Nonnull @jakarta.annotation.Nonnull ObjId sourceId) throws NoSuchElementException { - PagedResult log1 = commitIdLog(commitLogQuery(headId)); - PagedResult log2 = commitIdLog(commitLogQuery(otherId)); - - ObjectHashSet commits1 = new ObjectHashSet<>(); - ObjectHashSet commits2 = new ObjectHashSet<>(); - - if (!log2.hasNext() && !EMPTY_OBJ_ID.equals(otherId)) { - // this is a race, commit deleted in the meantime - throw commonAncestorCommitNotFound(otherId); - } - if (!log1.hasNext() && !EMPTY_OBJ_ID.equals(headId)) { - // this is a race, commit deleted in the meantime - throw commonAncestorCommitNotFound(headId); - } - - while (true) { - ObjId current1 = log1.hasNext() ? log1.next() : EMPTY_OBJ_ID; - ObjId current2 = log2.hasNext() ? log2.next() : EMPTY_OBJ_ID; - - boolean eol1 = current1.equals(EMPTY_OBJ_ID); - boolean eol2 = current2.equals(EMPTY_OBJ_ID); - - if (eol1 && eol2) { - throw noCommonAncestor(headId, otherId); - } - - if (!eol1) { - if (commits2.contains(current1)) { - return current1; - } - commits1.add(current1); - } - - if (!eol2) { - if (commits1.contains(current2)) { - return current2; - } - commits2.add(current2); - } - } + return identifyMergeBase(targetId, sourceId, false); } - private static NoSuchElementException commonAncestorCommitNotFound(ObjId id) { - return new NoSuchElementException("Commit '" + id + "' not found"); + @Nonnull + @jakarta.annotation.Nonnull + @Override + public ObjId findMergeBase( + @Nonnull @jakarta.annotation.Nonnull ObjId targetId, + @Nonnull @jakarta.annotation.Nonnull ObjId sourceId) + throws NoSuchElementException { + return identifyMergeBase(targetId, sourceId, true); } - private static NoSuchElementException noCommonAncestor(ObjId headId, ObjId otherId) { - return new NoSuchElementException( - NO_COMMON_ANCESTOR_IN_PARENTS_OF + headId + " and " + otherId); + private ObjId identifyMergeBase(ObjId targetId, ObjId sourceId, boolean respectMergeParents) { + return MergeBase.builder() + .loadCommit( + commitId -> { + try { + return fetchCommit(commitId); + } catch (ObjNotFoundException e) { + return null; + } + }) + .targetCommitId(targetId) + .fromCommitId(sourceId) + .respectMergeParents(respectMergeParents) + .build() + .identifyMergeBase(); } @Nullable diff --git a/versioned/storage/common/src/main/java/org/projectnessie/versioned/storage/common/logic/MergeBase.java b/versioned/storage/common/src/main/java/org/projectnessie/versioned/storage/common/logic/MergeBase.java new file mode 100644 index 00000000000..f7fe0f465e6 --- /dev/null +++ b/versioned/storage/common/src/main/java/org/projectnessie/versioned/storage/common/logic/MergeBase.java @@ -0,0 +1,334 @@ +/* + * Copyright (C) 2023 Dremio + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.projectnessie.versioned.storage.common.logic; + +import static com.google.common.collect.Lists.newArrayList; +import static java.util.Collections.singletonList; +import static java.util.Comparator.comparing; +import static java.util.Objects.requireNonNull; +import static org.projectnessie.versioned.storage.common.logic.CommitLogicImpl.NO_COMMON_ANCESTOR_IN_PARENTS_OF; +import static org.projectnessie.versioned.storage.common.logic.ShallowCommit.BOTH_COMMITS; +import static org.projectnessie.versioned.storage.common.logic.ShallowCommit.CANDIDATE; +import static org.projectnessie.versioned.storage.common.persist.ObjId.EMPTY_OBJ_ID; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; +import java.util.NoSuchElementException; +import java.util.Objects; +import java.util.PriorityQueue; +import java.util.function.Function; +import java.util.stream.Stream; +import org.agrona.collections.Object2ObjectHashMap; +import org.immutables.value.Value; +import org.projectnessie.versioned.storage.common.objtypes.CommitObj; +import org.projectnessie.versioned.storage.common.persist.ObjId; + +/** + * Identifies the nearest commit that shall be used as the base commit for a merge of the given + * {@link #fromCommitId() from-commit} onto the given {@link #targetCommitId() target commit}. + * + *

This class also supports finding the base commit for N-way merges, although N-way merges are + * not implemented for Nessie (yet). + */ +@Value.Immutable +public abstract class MergeBase { + public abstract Function loadCommit(); + + public abstract ObjId targetCommitId(); + + public abstract ObjId fromCommitId(); + + /** + * Whether merge-parents shall be respected, defaults to {@code true}. Setting this to {@code + * false} changes the behavior to return the common ancestor instead of the nearest + * merge-base. + */ + @Value.Default + public boolean respectMergeParents() { + return true; + } + + public static ImmutableMergeBase.Builder builder() { + return ImmutableMergeBase.builder(); + } + + @Value.NonAttribute + public ObjId identifyMergeBase() { + List mergeBases = identifyAllMergeBases(); + if (mergeBases == null || mergeBases.isEmpty()) { + throw noCommonAncestor(); + } + return mergeBases.get(0).id(); + } + + private List identifyAllMergeBases() { + ShallowCommit targetCommit = shallowCommit(targetCommitId()); + if (targetCommit == null) { + // This is rather a UX hack, to raise a "not found" exception if any of the from-commit-IDs + // does not exist + shallowCommit(fromCommitId()); + return null; + } + + ShallowCommit fromCommit = shallowCommit(fromCommitId()); + if (fromCommit == null) { + return singletonList(targetCommit); + } + + return findMergeBases(fromCommit, targetCommit); + } + + private List findMergeBases(ShallowCommit commitA, ShallowCommit commitB) { + if (commitB.id().equals(commitA.id())) { + return newArrayList(commitA); + } + + // Theoretically it should be correct to always return `reachableCommits` here. However, it + // might be necessary to re-incarnate `removeRedundant`, if there is a situation in which it + // would yield the "better" result, so keep the code. + return flagReachableCommits(commitA, commitB); + + // List reachableCommits = flagReachableCommits(commitA, commitB); + // + // int numReachable = reachableCommits.size(); + // if (numReachable < 2) { + // // Short-cut, if there is only one reachable commit that is already the result for both + // // commits. Also if there are no reachable commits at all. + // return reachableCommits; + // } + // + // clearFlags(newArrayList(commitA, commitB), ALL_FLAGS); + // + // return removeRedundant(reachableCommits); + } + + private List flagReachableCommits(ShallowCommit commitA, ShallowCommit commitB) { + PriorityQueue queue = + new PriorityQueue<>(comparing(ShallowCommit::seq).reversed()); + List result = new ArrayList<>(); + + commitA.setCommitA(); + queue.add(commitA); + + commitB.setCommitB(); + queue.add(commitB); + + while (queue.stream().anyMatch(ShallowCommit::isNotCandidate)) { + ShallowCommit commit = requireNonNull(queue.poll()); + + int reachabilityFlags = commit.reachabilityFlags(); + if (reachabilityFlags == BOTH_COMMITS) { + if (commit.setResult()) { + // A new result commit + result.add(commit); + } + // Populate the CANDIDATE flag "down". + reachabilityFlags |= CANDIDATE; + } + + // Propagate the relevant COMMIT_A, COMMIT_B, CANDIDATE flags down to the parent commits, + // enqueue those, if the relevant flags were not already set. + int reachabilityFlagsFinal = reachabilityFlags; + parentCommits(commit) + .filter(parent -> parent.setAllFlagsIfAnyMissing(reachabilityFlagsFinal)) + .forEach(queue::add); + } + + return result; + } + + // private List removeRedundant(List reachableCommits) { + // // Note: all commits in 'reachableCommits' have the RESULT flag set. + // + // // Sort the given commits by 'seq'. This allows us to increase the 'min seq' limit when we + // // discover the commit with the lowest 'seq' that is CANDIDATE. + // reachableCommits.sort(comparing(ShallowCommit::seq)); + // // 'commitMinSeq.seq' allows us to "ignore" commits that are _not_ nearer. (Want to find the + // // "nearest" or "closest" merge-base.) + // // The index 'minSeqIndex' points to the current index of 'commitMinSeq' within + // // 'reachableCommits' that is not yet known to be CANDIDATE. + // int minSeqIndex = 0; + // ShallowCommit commitMinSeq = reachableCommits.get(minSeqIndex); + // + // // For all reachable commits that were not yet flagged as RESULT, flag all their direct + // parents + // // as CANDIDATE and return the new candidates. 'newCandidates' holds the commits, "newest" + // ones + // // (by 'seq') first. + // List newCandidates = + // reachableCommits.stream() + // .peek(ShallowCommit::setResult) + // .flatMap(this::parentCommits) + // .filter(ShallowCommit::setCandidate) + // .sorted(comparing(ShallowCommit::seq).reversed()) + // .collect(Collectors.toList()); + // + // // Remove CANDIDATE flag for now to allow walking through parents. + // newCandidates.forEach(ShallowCommit::clearCandidate); + // + // // Iterate, start with the highest 'seq'. It should find all other commits during + // parents-walk, + // // allowing us to terminate early. + // int reachableCommitCount = reachableCommits.size(); + // // Number of commits that are flagged as a RESULT. + // int remainingResults = reachableCommitCount; + // Deque deque = new ArrayDeque<>(); + // for (int i = 0; i < newCandidates.size() && remainingResults > 1; i++) { + // // note: 'deque' is always empty here + // + // ShallowCommit candidate = newCandidates.get(i); + // candidate.setCandidate(); + // deque.add(candidate); + // + // while (!deque.isEmpty()) { + // ShallowCommit c = deque.peek(); + // + // if (c.clearResult()) { + // // 'c' had the RESULT flag set + // if (--remainingResults == 0) { + // // All RESULTs processed - done with all candidates (will exit inner and outer + // loop). + // break; + // } + // + // // If 'c' is the commit "min-seq commit", push minSeq to the first non-CANDIDATE + // commit. + // if (c.id().equals(commitMinSeq.id())) { + // while (minSeqIndex < reachableCommitCount - 1 && commitMinSeq.isCandidate()) { + // minSeqIndex++; + // commitMinSeq = reachableCommits.get(minSeqIndex); + // } + // } + // } + // + // // Find the first "new" candidate commit with a higher 'seq', enqueue it and start over. + // if (c.seq() >= commitMinSeq.seq()) { + // // Get the first non-CANDIDATE flagged parent commit (then flagged as CANDIDATE). + // Optional firstNonCandidate = + // parentCommits(c).filter(ShallowCommit::setCandidate).findFirst(); + // if (firstNonCandidate.isPresent()) { + // // Found a commit that is "nearer" _and_ was not flagged as CANDIDATE. + // deque.addFirst(firstNonCandidate.get()); + // continue; + // } + // } + // + // // All candidates have been visited, remove 'c' from the deque. + // deque.remove(); + // } + // } + // + // // Clear RESULT flag and build the 'result' list ordered by seq. Need to '.collect()' here, + // // because we need the CANDIDATE flag as a filter condition, but have to clear it before + // // returning. + // List result = + // reachableCommits.stream() + // .peek(ShallowCommit::clearResult) + // .filter(ShallowCommit::isNotCandidate) + // .sorted(comparing(ShallowCommit::seq)) + // .collect(Collectors.toList()); + // + // // Clear CANDIDATE flag + // clearFlags(newCandidates, CANDIDATE); + // + // return result; + // } + + // /** Clears the given flags from all given commits recursively. */ + // private void clearFlags(List commits, int flags) { + // Deque remainingParents = new ArrayDeque<>(); + // + // for (ShallowCommit commit : commits) { + // // Note: 'remainingParents' is mutated + // clearFlagsInner(remainingParents, commit, flags); + // } + // + // while (!remainingParents.isEmpty()) { + // ShallowCommit commit = remainingParents.removeFirst(); + // // Note: 'remainingParents' is mutated + // clearFlagsInner(remainingParents, commit, flags); + // } + // } + + // private void clearFlagsInner( + // Deque remainingParents, ShallowCommit commit, int flags) { + // // Clear commit flags of the given commit and its _direct_ parents (predecessors), repeat + // until + // // the first "untouched" commit is reached (matching the logic elsewhere in this class). + // while (commit.clearFlagsIfAnySet(flags)) { + // + // Iterator parents = parentCommits(commit).iterator(); + // if (!parents.hasNext()) { + // // no parents, at "beginning of time", nothing to do anymore + // return; + // } + // + // // If 'commit' or any of its parents has any flag to be cleared, add it to + // 'remainingParents' + // // so it is handled in the next iteration in 'clearFlags()'. + // // Start with the direct parent of the commit passed into 'clearFlagsInner()'. + // commit = parents.next(); + // if (commit.isAnyFlagSet(flags)) { + // remainingParents.addLast(commit); + // } + // while (parents.hasNext()) { + // ShallowCommit parent = parents.next(); + // if (parent.isAnyFlagSet(flags)) { + // remainingParents.addLast(parent); + // } + // } + // } + // } + + private NoSuchElementException noCommonAncestor() { + return new NoSuchElementException( + NO_COMMON_ANCESTOR_IN_PARENTS_OF + targetCommitId() + " and " + fromCommitId()); + } + + private Stream parentCommits(ShallowCommit commit) { + return Arrays.stream(commit.parents()).map(this::shallowCommit).filter(Objects::nonNull); + } + + private ShallowCommit shallowCommit(ObjId objId) { + if (EMPTY_OBJ_ID.equals(objId)) { + return null; + } + return commits.computeIfAbsent( + objId, + id -> { + CommitObj commit = loadCommit().apply(id); + if (commit == null) { + throw new NoSuchElementException("Commit '" + id + "' not found"); + } + ObjId[] parents; + if (respectMergeParents()) { + List secondary = commit.secondaryParents(); + parents = new ObjId[1 + secondary.size()]; + int end = parents.length - 1; + for (int i = 0; i < end; i++) { + parents[i] = secondary.get(i); + } + parents[end] = commit.directParent(); + } else { + parents = new ObjId[] {commit.directParent()}; + } + return new ShallowCommit(commit.id(), parents, commit.seq()); + }); + } + + private final Object2ObjectHashMap commits = new Object2ObjectHashMap<>(); +} diff --git a/versioned/storage/common/src/main/java/org/projectnessie/versioned/storage/common/logic/ShallowCommit.java b/versioned/storage/common/src/main/java/org/projectnessie/versioned/storage/common/logic/ShallowCommit.java new file mode 100644 index 00000000000..b65824445b1 --- /dev/null +++ b/versioned/storage/common/src/main/java/org/projectnessie/versioned/storage/common/logic/ShallowCommit.java @@ -0,0 +1,181 @@ +/* + * Copyright (C) 2023 Dremio + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.projectnessie.versioned.storage.common.logic; + +import java.util.Arrays; +import org.projectnessie.versioned.storage.common.objtypes.CommitObj; +import org.projectnessie.versioned.storage.common.persist.ObjId; + +/** + * For {@link MergeBase}, holds only some properties from {@link CommitObj} to reduce heap pressure. + * + *

Identifying the merge-base may require holding a lot of commits, so keeping only the needed + * data should help reducing the heap pressure (think: {@link CommitObj#incrementalIndex()}. + */ +final class ShallowCommit { + /** Whether a commit is reachable via 'commit A'. */ + static final int COMMIT_A = 1; + + /** Whether a commit is reachable via 'commit B'. */ + static final int COMMIT_B = 2; + + static final int CANDIDATE = 4; + static final int RESULT = 8; + + static final int BOTH_COMMITS = COMMIT_A | COMMIT_B; + static final int REACHABILITY_FLAGS = BOTH_COMMITS | CANDIDATE; + static final int ALL_FLAGS = COMMIT_A | COMMIT_B | CANDIDATE | RESULT; + + private final ObjId id; + private final ObjId[] parents; + private final long seq; + private int flags; + + ShallowCommit(ObjId id, ObjId[] parents, long seq) { + this.id = id; + this.parents = parents; + this.seq = seq; + } + + ObjId id() { + return id; + } + + ObjId[] parents() { + return parents; + } + + long seq() { + return seq; + } + + int flags() { + return flags; + } + + int reachabilityFlags() { + return flags & REACHABILITY_FLAGS; + } + + boolean isCandidate() { + return (flags & CANDIDATE) != 0; + } + + boolean isNotCandidate() { + return (flags & CANDIDATE) == 0; + } + + boolean isAnyFlagSet(int flags) { + return (this.flags & flags) != 0; + } + + void setCommitA() { + this.flags |= COMMIT_A; + } + + void setCommitB() { + this.flags |= COMMIT_B; + } + + void clearCandidate() { + this.flags &= ~CANDIDATE; + } + + boolean clearFlagsIfAnySet(int flags) { + int ex = this.flags; + if ((ex & flags) != 0) { + this.flags = ex & ~flags; + return true; + } + return false; + } + + boolean setAllFlagsIfAnyMissing(int flags) { + int ex = this.flags; + if ((ex & flags) != flags) { + this.flags = ex | flags; + return true; + } + return false; + } + + boolean setCandidate() { + int ex = this.flags; + if ((ex & CANDIDATE) == 0) { + this.flags = ex | CANDIDATE; + return true; + } + return false; + } + + boolean setResult() { + int ex = this.flags; + if ((ex & RESULT) == 0) { + this.flags = ex | RESULT; + return true; + } + return false; + } + + boolean clearResult() { + int ex = this.flags; + if ((ex & RESULT) != 0) { + this.flags = ex & ~RESULT; + return true; + } + return false; + } + + @Override + public String toString() { + StringBuilder f = new StringBuilder(); + if ((flags & BOTH_COMMITS) == BOTH_COMMITS) { + f.append("BOTH_COMMITS"); + } else { + if ((flags & COMMIT_A) != 0) { + f.append("COMMIT_A"); + } + if ((flags & COMMIT_B) != 0) { + if (f.length() > 0) { + f.append(", "); + } + f.append("COMMIT_B"); + } + } + if ((flags & CANDIDATE) != 0) { + if (f.length() > 0) { + f.append(", "); + } + f.append("CANDIDATE"); + } + if ((flags & RESULT) != 0) { + if (f.length() > 0) { + f.append(", "); + } + f.append("RESULT"); + } + return "ShallowCommit{" + + "id=" + + id + + ", parents=" + + Arrays.toString(parents) + + ", seq=" + + seq + + ", flags=" + + f + + '}'; + } +} diff --git a/versioned/storage/common/src/test/java/org/projectnessie/versioned/storage/common/logic/TestMergeBase.java b/versioned/storage/common/src/test/java/org/projectnessie/versioned/storage/common/logic/TestMergeBase.java new file mode 100644 index 00000000000..ca47d16d5c7 --- /dev/null +++ b/versioned/storage/common/src/test/java/org/projectnessie/versioned/storage/common/logic/TestMergeBase.java @@ -0,0 +1,476 @@ +/* + * Copyright (C) 2023 Dremio + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.projectnessie.versioned.storage.common.logic; + +import static org.projectnessie.versioned.storage.common.logic.CommitLogicImpl.NO_COMMON_ANCESTOR_IN_PARENTS_OF; +import static org.projectnessie.versioned.storage.common.logic.ShallowCommit.ALL_FLAGS; +import static org.projectnessie.versioned.storage.common.logic.ShallowCommit.CANDIDATE; +import static org.projectnessie.versioned.storage.common.logic.ShallowCommit.COMMIT_A; +import static org.projectnessie.versioned.storage.common.logic.ShallowCommit.COMMIT_B; +import static org.projectnessie.versioned.storage.common.objtypes.CommitHeaders.newCommitHeaders; +import static org.projectnessie.versioned.storage.common.objtypes.CommitObj.commitBuilder; +import static org.projectnessie.versioned.storage.common.persist.ObjId.EMPTY_OBJ_ID; +import static org.projectnessie.versioned.storage.common.persist.ObjId.randomObjId; + +import java.nio.ByteBuffer; +import java.util.HashMap; +import java.util.LinkedHashSet; +import java.util.Map; +import java.util.NoSuchElementException; +import java.util.Set; +import org.assertj.core.api.SoftAssertions; +import org.assertj.core.api.junit.jupiter.InjectSoftAssertions; +import org.assertj.core.api.junit.jupiter.SoftAssertionsExtension; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.CsvSource; +import org.junit.jupiter.params.provider.ValueSource; +import org.projectnessie.nessie.relocated.protobuf.ByteString; +import org.projectnessie.versioned.storage.common.objtypes.CommitObj; +import org.projectnessie.versioned.storage.common.persist.ObjId; + +@ExtendWith(SoftAssertionsExtension.class) +public class TestMergeBase { + @InjectSoftAssertions protected SoftAssertions soft; + + private MockRepo repo; + + @Test + void noCommits() { + soft.assertThatThrownBy( + () -> + MergeBase.builder() + .targetCommitId(EMPTY_OBJ_ID) + .fromCommitId(EMPTY_OBJ_ID) + .loadCommit(x -> null) + .build() + .identifyMergeBase()) + .isInstanceOf(NoSuchElementException.class) + .hasMessageStartingWith(NO_COMMON_ANCESTOR_IN_PARENTS_OF); + } + + @Test + void sameCommits() { + CommitObj a = repo.add(repo.initialCommit()); + soft.assertThat( + MergeBase.builder() + .loadCommit(repo::loadCommit) + .targetCommitId(a.id()) + .fromCommitId(a.id()) + .build() + .identifyMergeBase()) + .isEqualTo(a.id()); + } + + /** + * Unrelated branches case. 

+   * ----B-----D
+   *
+   * ----A-----C
+   *
+ */ + @ParameterizedTest + @ValueSource(booleans = {true, false}) + void noMergeBase(boolean respectMergeParents) { + CommitObj a = repo.add(repo.buildCommit("initial A", null)); + CommitObj b = repo.add(repo.buildCommit("initial B", null)); + CommitObj c = repo.add(repo.buildCommit("c", a)); + CommitObj d = repo.add(repo.buildCommit("d", b)); + + soft.assertThatThrownBy( + () -> + MergeBase.builder() + .loadCommit(repo::loadCommit) + .respectMergeParents(respectMergeParents) + .targetCommitId(c.id()) + .fromCommitId(d.id()) + .build() + .identifyMergeBase()) + .isInstanceOf(NoSuchElementException.class) + .hasMessageStartingWith(NO_COMMON_ANCESTOR_IN_PARENTS_OF); + + soft.assertThat(repo.loaded).doesNotContain(repo.root).contains(a, b); + } + + /** + * Test a simple merge base case. 
+   *       ----B
+   *      /
+   * ----A-----C
+   *
+ * + *

Best merge-base of {@code B} onto {@code C} is {@code A}. + */ + @ParameterizedTest + @ValueSource(booleans = {true, false}) + void simpleCase(boolean respectMergeParents) { + CommitObj a = repo.add(repo.initialCommit()); + CommitObj b = repo.add(repo.buildCommit("b", a)); + CommitObj c = repo.add(repo.buildCommit("c", a)); + + soft.assertThat( + MergeBase.builder() + .loadCommit(repo::loadCommit) + .respectMergeParents(respectMergeParents) + .targetCommitId(c.id()) + .fromCommitId(b.id()) + .build() + .identifyMergeBase()) + .isEqualTo(a.id()); + + soft.assertThat(repo.loaded).doesNotContain(repo.root); + } + + /** + * Merge-again case. 

+   *       ----B---------E
+   *      /          \
+   * ----A------C-----D
+   *
+ * + *

Best merge-base of {@code E} onto {@code D} is {@code B}. + */ + @ParameterizedTest + @ValueSource(booleans = {true, false}) + void doubleMerge(boolean respectMergeParents) { + CommitObj a = repo.add(repo.initialCommit()); + CommitObj b = repo.add(repo.buildCommit("b", a)); + CommitObj c = repo.add(repo.buildCommit("c", a)); + CommitObj d = repo.add(repo.buildCommit("d", c).addSecondaryParents(b.id())); + CommitObj e = repo.add(repo.buildCommit("e", b)); + + soft.assertThat( + MergeBase.builder() + .loadCommit(repo::loadCommit) + .respectMergeParents(respectMergeParents) + .targetCommitId(d.id()) + .fromCommitId(e.id()) + .build() + .identifyMergeBase()) + .isEqualTo(respectMergeParents ? b.id() : a.id()); + + soft.assertThat(repo.loaded).doesNotContain(repo.root); + } + + /** + * Merge-again case. 

+   *       ----B----D-------F
+   *      /             \
+   * ----A------C--------E
+   *
+ * + *

Best merge-base of {@code F} onto {@code E} is {@code D}. + */ + @ParameterizedTest + @ValueSource(booleans = {true, false}) + void doubleMerge2(boolean respectMergeParents) { + CommitObj a = repo.add(repo.initialCommit()); + CommitObj b = repo.add(repo.buildCommit("b", a)); + CommitObj c = repo.add(repo.buildCommit("c", a)); + CommitObj d = repo.add(repo.buildCommit("d", b)); + CommitObj e = repo.add(repo.buildCommit("e", c).addSecondaryParents(d.id())); + CommitObj f = repo.add(repo.buildCommit("f", d)); + + soft.assertThat( + MergeBase.builder() + .loadCommit(repo::loadCommit) + .respectMergeParents(respectMergeParents) + .targetCommitId(e.id()) + .fromCommitId(f.id()) + .build() + .identifyMergeBase()) + .isEqualTo(respectMergeParents ? d.id() : a.id()); + + soft.assertThat(repo.loaded).doesNotContain(repo.root); + } + + /** + * Merge-again case. 

+   *              ----E----G---------I
+   *            /       /           /
+   *       ----B----D----------H----
+   *      /               /
+   * ----A------C--------F
+   *
+ * + *

Best merge-base of {@code I} onto {@code F} is {@code F}. + */ + @ParameterizedTest + @ValueSource(booleans = {true, false}) + void multiMerge1(boolean respectMergeParents) { + CommitObj a = repo.add(repo.initialCommit()); + CommitObj b = repo.add(repo.buildCommit("b", a)); + CommitObj c = repo.add(repo.buildCommit("c", a)); + CommitObj d = repo.add(repo.buildCommit("d", b)); + CommitObj e = repo.add(repo.buildCommit("e", b)); + CommitObj f = repo.add(repo.buildCommit("f", c)); + CommitObj g = repo.add(repo.buildCommit("g", e).addSecondaryParents(d.id())); + CommitObj h = repo.add(repo.buildCommit("h", d).addSecondaryParents(f.id())); + CommitObj i = repo.add(repo.buildCommit("i", g).addSecondaryParents(h.id())); + + soft.assertThat( + MergeBase.builder() + .loadCommit(repo::loadCommit) + .respectMergeParents(respectMergeParents) + .targetCommitId(f.id()) + .fromCommitId(i.id()) + .build() + .identifyMergeBase()) + .isEqualTo(respectMergeParents ? f.id() : a.id()); + + soft.assertThat(repo.loaded).doesNotContain(repo.root); + } + + /** + * Merge-again case. 

+   * A ----- C - E - F - G
+   *  \- B -/           /
+   *   \    \--------- / ---\
+   *    \- D ---------/------ H - J
+   *
+ * + *

Best merge-base of {@code J} onto {@code G} is {@code D}. + */ + @ParameterizedTest + @ValueSource(booleans = {true, false}) + void multiMerge2(boolean respectMergeParents) { + CommitObj a = repo.add(repo.initialCommit()); + CommitObj b = repo.add(repo.buildCommit("b", a)); + CommitObj c = repo.add(repo.buildCommit("c", a).addSecondaryParents(b.id())); + CommitObj d = repo.add(repo.buildCommit("d", a)); + CommitObj e = repo.add(repo.buildCommit("e", c)); + CommitObj f = repo.add(repo.buildCommit("f", e)); + CommitObj g = repo.add(repo.buildCommit("g", f).addSecondaryParents(d.id())); + CommitObj h = repo.add(repo.buildCommit("h", d).addSecondaryParents(b.id())); + CommitObj j = repo.add(repo.buildCommit("j", h)); + + soft.assertThat( + MergeBase.builder() + .loadCommit(repo::loadCommit) + .respectMergeParents(respectMergeParents) + .targetCommitId(g.id()) + .fromCommitId(j.id()) + .build() + .identifyMergeBase()) + .isEqualTo(respectMergeParents ? d.id() : a.id()); + + soft.assertThat(repo.loaded).doesNotContain(repo.root); + } + + /** + * Cross-merge case. 

+   *        ----B--------D----F
+   *       /       \ /
+   *      /        / \
+   * ----A------C--------E----G
+   *
+ * + *

Merge-base outcome for {@code F} onto {@code G} is either {@code B} or {@code C}. + */ + @ParameterizedTest + @CsvSource(value = {"true,false", "true,true", "false,false", "false,true"}) + void afterCrossMerge(boolean respectMergeParents, boolean reverse) { + CommitObj a = repo.add(repo.initialCommit()); + CommitObj b = repo.add(repo.buildCommit("b", a)); + CommitObj c = repo.add(repo.buildCommit("c", a)); + CommitObj d = repo.add(repo.buildCommit("d", b).addSecondaryParents(c.id())); + CommitObj e = repo.add(repo.buildCommit("e", c).addSecondaryParents(b.id())); + CommitObj f = repo.add(repo.buildCommit("f", d)); + CommitObj g = repo.add(repo.buildCommit("g", e)); + + soft.assertThat( + MergeBase.builder() + .loadCommit(repo::loadCommit) + .respectMergeParents(respectMergeParents) + .targetCommitId(reverse ? f.id() : g.id()) + .fromCommitId(reverse ? g.id() : f.id()) + .build() + .identifyMergeBase()) + .isEqualTo(respectMergeParents ? (reverse ? c.id() : b.id()) : a.id()); + + soft.assertThat(repo.loaded).doesNotContain(repo.root); + } + + /** + * Merge two previous merges. 

+   *        B-----------------H
+   *       /                 /
+   *      / ------------F   /
+   *     | /           /   /
+   *     |/           /   /
+   * ----A---C---D---E---G
+   *
+ * + *

Best merge-base of {@code H} onto {@code F} is {@code E}. + */ + @ParameterizedTest + @ValueSource(booleans = {true, false}) + void nestedBranches(boolean respectMergeParents) { + CommitObj a = repo.add(repo.initialCommit("a")); + CommitObj c = repo.add(repo.buildCommit("c", a)); + CommitObj b = repo.add(repo.buildCommit("b", a)); + CommitObj d = repo.add(repo.buildCommit("d", c)); + CommitObj e = repo.add(repo.buildCommit("e", d)); + CommitObj g = repo.add(repo.buildCommit("g", e)); + CommitObj f = repo.add(repo.buildCommit("f", a).addSecondaryParents(e.id())); + CommitObj h = repo.add(repo.buildCommit("h", b).addSecondaryParents(g.id())); + + soft.assertThat( + MergeBase.builder() + .loadCommit(repo::loadCommit) + .respectMergeParents(respectMergeParents) + .targetCommitId(f.id()) + .fromCommitId(h.id()) + .build() + .identifyMergeBase()) + .isEqualTo(respectMergeParents ? e.id() : a.id()); + + soft.assertThat(repo.loaded).doesNotContain(repo.root); + } + + /** + * Merge two previous merges. 

+   *      ---- B ---- E ---- G ---- I ---- K
+   *     /      /             /
+   *    /      /             /
+   *   A ---- C ---- D ---- F ---- H ---- J
+   *
+ * + *

Best merge-base of {@code K} onto {@code J} is {@code F}. + */ + @ParameterizedTest + @ValueSource(booleans = {true, false}) + void featureBranch(boolean respectMergeParents) { + CommitObj a = repo.add(repo.initialCommit("a")); + CommitObj b = repo.add(repo.buildCommit("b", a)); + CommitObj c = repo.add(repo.buildCommit("c", a)); + CommitObj d = repo.add(repo.buildCommit("d", c)); + CommitObj e = repo.add(repo.buildCommit("e", b).addSecondaryParents(c.id())); + CommitObj f = repo.add(repo.buildCommit("f", d)); + CommitObj g = repo.add(repo.buildCommit("g", e)); + CommitObj h = repo.add(repo.buildCommit("h", f)); + CommitObj i = repo.add(repo.buildCommit("i", g).addSecondaryParents(f.id())); + CommitObj j = repo.add(repo.buildCommit("j", h)); + CommitObj k = repo.add(repo.buildCommit("k", i)); + + soft.assertThat( + MergeBase.builder() + .loadCommit(repo::loadCommit) + .respectMergeParents(respectMergeParents) + .targetCommitId(j.id()) + .fromCommitId(k.id()) + .build() + .identifyMergeBase()) + .isEqualTo(respectMergeParents ? f.id() : a.id()); + + soft.assertThat(repo.loaded).doesNotContain(repo.root); + } + + @Test + void shallowCommitFlags() { + ShallowCommit commit = new ShallowCommit(randomObjId(), new ObjId[] {randomObjId()}, 1L); + + soft.assertThat(commit.isAnyFlagSet(ALL_FLAGS)).isFalse(); + soft.assertThat(commit.isAnyFlagSet(CANDIDATE)).isFalse(); + + commit.setCandidate(); + + soft.assertThat(commit.isAnyFlagSet(ALL_FLAGS)).isTrue(); + soft.assertThat(commit.isAnyFlagSet(CANDIDATE)).isTrue(); + + commit.clearCandidate(); + + soft.assertThat(commit.flags()).isEqualTo(0); + + int multiple = CANDIDATE | COMMIT_A | COMMIT_B; + commit.setCandidate(); + commit.setCommitA(); + commit.setCommitB(); + + soft.assertThat(commit.isAnyFlagSet(multiple)).isTrue(); + soft.assertThat(commit.isAnyFlagSet(CANDIDATE)).isTrue(); + soft.assertThat(commit.isAnyFlagSet(CANDIDATE | COMMIT_A)).isTrue(); + + commit.clearCandidate(); + + soft.assertThat(commit.isAnyFlagSet(multiple)).isTrue(); + soft.assertThat(commit.isAnyFlagSet(COMMIT_A)).isTrue(); + soft.assertThat(commit.isAnyFlagSet(COMMIT_A | COMMIT_B)).isTrue(); + } + + @BeforeEach + void setupRepo() { + repo = new MockRepo(); + } + + static class MockRepo { + final Map commits = new HashMap<>(); + final Set loaded = new LinkedHashSet<>(); + final CommitObj root; + final CommitObj testRoot; + + MockRepo() { + root = add(buildCommit("root", null)); + CommitObj parent = null; + for (int i = 1; i <= 16; i++) { + parent = add(buildCommit("unrelated " + i, parent)); + } + testRoot = parent; + } + + CommitObj add(CommitObj.Builder c) { + int num = commits.size(); + ByteBuffer bb = ByteBuffer.allocate(4); + bb.putInt(num); + bb.flip(); + + c.created(num).id(ObjId.objIdFromByteBuffer(bb)); + CommitObj commit = c.build(); + commits.put(commit.id(), commit); + return commit; + } + + public CommitObj loadCommit(ObjId id) { + CommitObj c = commits.get(id); + loaded.add(c); + return c; + } + + CommitObj.Builder initialCommit() { + return initialCommit("initial"); + } + + CommitObj.Builder initialCommit(String name) { + return buildCommit(name, testRoot); + } + + CommitObj.Builder buildCommit(String msg, CommitObj parent) { + CommitObj.Builder commit = + commitBuilder() + .headers(newCommitHeaders().build()) + .message(msg) + .incrementalIndex(ByteString.empty()); + if (parent != null) { + commit.seq(parent.seq() + 1).addTail(parent.id()); + } else { + commit.seq(0).addTail(EMPTY_OBJ_ID); + } + return commit; + } + } +} diff --git a/versioned/storage/common/src/test/java/org/projectnessie/versioned/storage/common/logic/TestShallowCommit.java b/versioned/storage/common/src/test/java/org/projectnessie/versioned/storage/common/logic/TestShallowCommit.java new file mode 100644 index 00000000000..20cff12fa26 --- /dev/null +++ b/versioned/storage/common/src/test/java/org/projectnessie/versioned/storage/common/logic/TestShallowCommit.java @@ -0,0 +1,137 @@ +/* + * Copyright (C) 2023 Dremio + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.projectnessie.versioned.storage.common.logic; + +import static org.projectnessie.versioned.storage.common.logic.ShallowCommit.ALL_FLAGS; +import static org.projectnessie.versioned.storage.common.logic.ShallowCommit.CANDIDATE; +import static org.projectnessie.versioned.storage.common.logic.ShallowCommit.COMMIT_A; +import static org.projectnessie.versioned.storage.common.logic.ShallowCommit.COMMIT_B; +import static org.projectnessie.versioned.storage.common.logic.ShallowCommit.RESULT; +import static org.projectnessie.versioned.storage.common.persist.ObjId.randomObjId; + +import org.assertj.core.api.SoftAssertions; +import org.assertj.core.api.junit.jupiter.InjectSoftAssertions; +import org.assertj.core.api.junit.jupiter.SoftAssertionsExtension; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.projectnessie.versioned.storage.common.persist.ObjId; + +@ExtendWith(SoftAssertionsExtension.class) +public class TestShallowCommit { + @InjectSoftAssertions protected SoftAssertions soft; + + @Test + void setAllFlagsIfAnyMissing() { + ShallowCommit commit = commit(); + soft.assertThat(commit.setAllFlagsIfAnyMissing(CANDIDATE | RESULT)).isTrue(); + soft.assertThat(commit.flags()).isEqualTo(CANDIDATE | RESULT); + soft.assertThat(commit.setAllFlagsIfAnyMissing(CANDIDATE)).isFalse(); + soft.assertThat(commit.setAllFlagsIfAnyMissing(RESULT)).isFalse(); + soft.assertThat(commit.setAllFlagsIfAnyMissing(CANDIDATE | RESULT)).isFalse(); + + commit = commit(); + soft.assertThat(commit.setAllFlagsIfAnyMissing(CANDIDATE)).isTrue(); + soft.assertThat(commit.flags()).isEqualTo(CANDIDATE); + soft.assertThat(commit.setAllFlagsIfAnyMissing(CANDIDATE)).isFalse(); + soft.assertThat(commit.setAllFlagsIfAnyMissing(RESULT)).isTrue(); + soft.assertThat(commit.flags()).isEqualTo(CANDIDATE | RESULT); + soft.assertThat(commit.setAllFlagsIfAnyMissing(RESULT)).isFalse(); + } + + @Test + void clearFlagsIfAnySet() { + ShallowCommit commit = commit(); + soft.assertThat(commit.clearFlagsIfAnySet(CANDIDATE | RESULT)).isFalse(); + soft.assertThat(commit.clearFlagsIfAnySet(CANDIDATE)).isFalse(); + soft.assertThat(commit.clearFlagsIfAnySet(RESULT)).isFalse(); + soft.assertThat(commit.clearFlagsIfAnySet(CANDIDATE | RESULT)).isFalse(); + + commit = commit(); + soft.assertThat(commit.setAllFlagsIfAnyMissing(CANDIDATE | RESULT)).isTrue(); + soft.assertThat(commit.flags()).isEqualTo(CANDIDATE | RESULT); + soft.assertThat(commit.clearFlagsIfAnySet(CANDIDATE | RESULT)).isTrue(); + soft.assertThat(commit.clearFlagsIfAnySet(CANDIDATE | RESULT)).isFalse(); + + commit = commit(); + soft.assertThat(commit.setAllFlagsIfAnyMissing(CANDIDATE)).isTrue(); + soft.assertThat(commit.flags()).isEqualTo(CANDIDATE); + soft.assertThat(commit.clearFlagsIfAnySet(CANDIDATE | RESULT)).isTrue(); + soft.assertThat(commit.clearFlagsIfAnySet(CANDIDATE | RESULT)).isFalse(); + + commit = commit(); + soft.assertThat(commit.setAllFlagsIfAnyMissing(CANDIDATE)).isTrue(); + soft.assertThat(commit.clearFlagsIfAnySet(CANDIDATE)).isTrue(); + soft.assertThat(commit.clearFlagsIfAnySet(CANDIDATE)).isFalse(); + } + + @Test + void setClearResult() { + ShallowCommit commit = commit(); + soft.assertThat(commit.clearResult()).isFalse(); + soft.assertThat(commit.setResult()).isTrue(); + soft.assertThat(commit.flags()).isEqualTo(RESULT); + soft.assertThat(commit.setResult()).isFalse(); + soft.assertThat(commit.clearResult()).isTrue(); + soft.assertThat(commit.flags()).isEqualTo(0); + soft.assertThat(commit.clearResult()).isFalse(); + } + + @Test + void setStale() { + ShallowCommit commit = commit(); + soft.assertThat(commit.setCandidate()).isTrue(); + soft.assertThat(commit.flags()).isEqualTo(CANDIDATE); + } + + @Test + void setClearFlags() { + ShallowCommit commit = commit(); + + soft.assertThat(commit.isAnyFlagSet(ALL_FLAGS)).isFalse(); + soft.assertThat(commit.isAnyFlagSet(CANDIDATE)).isFalse(); + soft.assertThat(commit.flags()).isEqualTo(0); + + commit.setCandidate(); + soft.assertThat(commit.flags()).isEqualTo(CANDIDATE); + + soft.assertThat(commit.isAnyFlagSet(ALL_FLAGS)).isTrue(); + soft.assertThat(commit.isAnyFlagSet(CANDIDATE)).isTrue(); + + commit.clearCandidate(); + soft.assertThat(commit.flags()).isEqualTo(0); + + int multiple = CANDIDATE | COMMIT_A | COMMIT_B; + commit.setCandidate(); + commit.setCommitA(); + commit.setCommitB(); + soft.assertThat(commit.flags()).isEqualTo(multiple); + + soft.assertThat(commit.isAnyFlagSet(multiple)).isTrue(); + soft.assertThat(commit.isAnyFlagSet(CANDIDATE)).isTrue(); + soft.assertThat(commit.isAnyFlagSet(CANDIDATE | COMMIT_A)).isTrue(); + + commit.clearCandidate(); + soft.assertThat(commit.flags()).isEqualTo(COMMIT_A | COMMIT_B); + + soft.assertThat(commit.isAnyFlagSet(multiple)).isTrue(); + soft.assertThat(commit.isAnyFlagSet(COMMIT_A)).isTrue(); + soft.assertThat(commit.isAnyFlagSet(COMMIT_A | COMMIT_B)).isTrue(); + } + + private ShallowCommit commit() { + return new ShallowCommit(randomObjId(), new ObjId[] {randomObjId()}, 1L); + } +} 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 d6d04729761..789eabb0a12 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 @@ -15,7 +15,6 @@ */ package org.projectnessie.versioned.storage.versionstore; -import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Preconditions.checkState; import static com.google.common.collect.Maps.newHashMapWithExpectedSize; import static java.lang.String.format; @@ -30,7 +29,6 @@ import static org.projectnessie.versioned.CommitValidation.CommitOperationType.UPDATE; import static org.projectnessie.versioned.MergeResult.KeyDetails.keyDetails; import static org.projectnessie.versioned.storage.common.logic.CommitConflict.ConflictType.KEY_EXISTS; -import static org.projectnessie.versioned.storage.common.logic.CommitLogQuery.commitLogQuery; import static org.projectnessie.versioned.storage.common.logic.CommitRetry.commitRetry; import static org.projectnessie.versioned.storage.common.logic.Logics.commitLogic; import static org.projectnessie.versioned.storage.common.logic.Logics.indexesLogic; @@ -46,13 +44,12 @@ import static org.projectnessie.versioned.store.DefaultStoreWorker.payloadForContent; import java.util.ArrayList; -import java.util.Collections; import java.util.HashMap; import java.util.Iterator; import java.util.List; import java.util.Map; import java.util.Map.Entry; -import java.util.NoSuchElementException; +import java.util.Objects; import java.util.Optional; import java.util.Set; import java.util.UUID; @@ -80,7 +77,6 @@ import org.projectnessie.versioned.ReferenceConflictException; import org.projectnessie.versioned.ReferenceNotFoundException; import org.projectnessie.versioned.ReferenceRetryFailureException; -import org.projectnessie.versioned.VersionStore; import org.projectnessie.versioned.VersionStore.CommitValidator; import org.projectnessie.versioned.VersionStoreException; import org.projectnessie.versioned.storage.batching.BatchingPersist; @@ -100,7 +96,6 @@ import org.projectnessie.versioned.storage.common.logic.ConflictHandler.ConflictResolution; import org.projectnessie.versioned.storage.common.logic.CreateCommit; import org.projectnessie.versioned.storage.common.logic.IndexesLogic; -import org.projectnessie.versioned.storage.common.logic.PagedResult; import org.projectnessie.versioned.storage.common.objtypes.CommitObj; import org.projectnessie.versioned.storage.common.objtypes.CommitOp; import org.projectnessie.versioned.storage.common.objtypes.ContentValueObj; @@ -434,128 +429,6 @@ void verifyMergeTransplantCommitPolicies( validateNamespaces(checkContents, deletedKeysAndPayload, headIndex); } - /** Source commits for merge and transplant operations. */ - static final class SourceCommitsAndParent { - - /** Source commits in chronological order, most recent commit last. */ - final List sourceCommits; - - /** Parent of the oldest commit. */ - final CommitObj sourceParent; - - SourceCommitsAndParent(List sourceCommits, CommitObj sourceParent) { - this.sourceCommits = sourceCommits; - this.sourceParent = sourceParent; - } - - CommitObj mostRecent() { - return sourceCommits.get(sourceCommits.size() - 1); - } - } - - SourceCommitsAndParent loadSourceCommitsForTransplant(List commitHashes) - throws ReferenceNotFoundException { - checkArgument( - !commitHashes.isEmpty(), - "No hashes to transplant onto %s @ %s, expected commit ID from request was %s.", - head != null ? head.id() : EMPTY_OBJ_ID, - branch.getName(), - referenceHash.map(Hash::asString).orElse("not specified")); - - Obj[] objs; - try { - objs = - persist.fetchObjs( - commitHashes.stream().map(TypeMapping::hashToObjId).toArray(ObjId[]::new)); - } catch (ObjNotFoundException e) { - throw referenceNotFound(e); - } - List commits = new ArrayList<>(commitHashes.size()); - CommitObj parent = null; - CommitLogic commitLogic = commitLogic(persist); - for (int i = 0; i < objs.length; i++) { - Obj o = objs[i]; - if (o == null) { - throw RefMapping.hashNotFound(commitHashes.get(i)); - } - CommitObj commit = (CommitObj) o; - if (i > 0) { - if (!commit.directParent().equals(commits.get(i - 1).id())) { - throw new IllegalArgumentException("Sequence of hashes is not contiguous."); - } - } else { - try { - parent = commitLogic.fetchCommit(commit.directParent()); - } catch (ObjNotFoundException e) { - throw referenceNotFound(e); - } - } - commits.add(commit); - } - - return new SourceCommitsAndParent(commits, parent); - } - - SourceCommitsAndParent loadSourceCommitsForMerge( - @Nonnull @jakarta.annotation.Nonnull ObjId startCommitId, - @Nonnull @jakarta.annotation.Nonnull ObjId endCommitId) { - CommitLogic commitLogic = commitLogic(persist); - List commits = new ArrayList<>(); - CommitObj parent = null; - for (PagedResult commitLog = - commitLogic.commitLog(commitLogQuery(null, startCommitId, endCommitId)); - commitLog.hasNext(); ) { - CommitObj commit = commitLog.next(); - if (commit.id().equals(endCommitId)) { - parent = commit; - break; - } - commits.add(commit); - } - - checkArgument( - !commits.isEmpty(), - "No hashes to merge from %s onto %s @ %s using common ancestor %s, expected commit ID from request was %s.", - startCommitId, - head != null ? head.id() : EMPTY_OBJ_ID, - branch.getName(), - endCommitId, - referenceHash.map(Hash::asString).orElse("not specified")); - - // Ends here, if 'endCommitId' is NO_ANCESTOR (parent == null) - Collections.reverse(commits); - return new SourceCommitsAndParent(commits, parent); - } - - ImmutableMergeResult mergeSquashFastForward( - VersionStore.MergeOp mergeOp, - ObjId fromId, - CommitObj source, - ImmutableMergeResult.Builder result) - throws RetryException { - result.wasSuccessful(true); - - MergeBehaviors mergeBehaviors = new MergeBehaviors(mergeOp); - - IndexesLogic indexesLogic = indexesLogic(persist); - for (StoreIndexElement el : indexesLogic.commitOperations(source)) { - StoreKey k = el.key(); - ContentKey key = storeKeyToKey(k); - // Note: key==null, if not the "main universe" or not a "content" discriminator - if (key != null) { - result.putDetails(key, keyDetails(mergeBehaviors.mergeBehavior(key), null)); - } - } - - // Only need bump the reference pointer - if (!mergeOp.dryRun()) { - bumpReferencePointer(fromId, Optional.empty()); - result.wasApplied(true).resultantTargetHash(objIdToHash(fromId)); - } - - return result.build(); - } - ImmutableMergeResult.Builder prepareMergeResult() { ImmutableMergeResult.Builder mergeResult = MergeResult.builder() @@ -566,17 +439,6 @@ ImmutableMergeResult.Builder prepareMergeResult() { return mergeResult; } - ObjId identifyCommonAncestor(ObjId fromId) throws ReferenceNotFoundException { - CommitLogic commitLogic = commitLogic(persist); - ObjId commonAncestorId; - try { - commonAncestorId = commitLogic.findCommonAncestor(headId(), fromId); - } catch (NoSuchElementException notFound) { - throw new ReferenceNotFoundException(notFound.getMessage()); - } - return commonAncestorId; - } - CommitObj createMergeTransplantCommit( MergeBehaviors mergeBehaviors, Map keyDetailsMap, @@ -604,11 +466,15 @@ CommitObj createMergeTransplantCommit( // change). CommitOp op = conflict.op(); CommitOp ex = conflict.existing(); - if (op != null - && ex != null - && op.payload() == ex.payload() - && contentTypeForPayload(op.payload()) == NAMESPACE) { - return ConflictResolution.ADD; + if (op != null && ex != null && op.payload() == ex.payload()) { + if (Objects.equals(op.value(), ex.value())) { + // Got another add for the exact same content that is already on the target, so + // drop the conflicting operation on the floor. + return ConflictResolution.DROP; + } + if (contentTypeForPayload(op.payload()) == NAMESPACE) { + return ConflictResolution.ADD; + } } } 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 deleted file mode 100644 index 3da90735e6b..00000000000 --- a/versioned/storage/store/src/main/java/org/projectnessie/versioned/storage/versionstore/BaseMergeTransplantIndividual.java +++ /dev/null @@ -1,164 +0,0 @@ -/* - * Copyright (C) 2022 Dremio - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ -package org.projectnessie.versioned.storage.versionstore; - -import static java.util.Objects.requireNonNull; -import static org.projectnessie.versioned.storage.common.logic.CreateCommit.Add.commitAdd; -import static org.projectnessie.versioned.storage.common.logic.CreateCommit.Remove.commitRemove; -import static org.projectnessie.versioned.storage.common.logic.CreateCommit.newCommitBuilder; -import static org.projectnessie.versioned.storage.common.logic.Logics.commitLogic; -import static org.projectnessie.versioned.storage.common.logic.Logics.indexesLogic; -import static org.projectnessie.versioned.storage.versionstore.TypeMapping.fromCommitMeta; -import static org.projectnessie.versioned.storage.versionstore.TypeMapping.toCommitMeta; - -import java.util.ArrayList; -import java.util.HashMap; -import java.util.List; -import java.util.Map; -import java.util.Optional; -import javax.annotation.Nonnull; -import javax.annotation.Nullable; -import org.projectnessie.model.CommitMeta; -import org.projectnessie.model.ContentKey; -import org.projectnessie.versioned.BranchName; -import org.projectnessie.versioned.Commit; -import org.projectnessie.versioned.Hash; -import org.projectnessie.versioned.ImmutableMergeResult; -import org.projectnessie.versioned.MergeResult; -import org.projectnessie.versioned.MergeResult.KeyDetails; -import org.projectnessie.versioned.MetadataRewriter; -import org.projectnessie.versioned.ReferenceConflictException; -import org.projectnessie.versioned.ReferenceNotFoundException; -import org.projectnessie.versioned.VersionStore.MergeTransplantOpBase; -import org.projectnessie.versioned.storage.common.indexes.StoreIndex; -import org.projectnessie.versioned.storage.common.indexes.StoreIndexElement; -import org.projectnessie.versioned.storage.common.logic.CommitLogic; -import org.projectnessie.versioned.storage.common.logic.CommitRetry.RetryException; -import org.projectnessie.versioned.storage.common.logic.CreateCommit; -import org.projectnessie.versioned.storage.common.logic.IndexesLogic; -import org.projectnessie.versioned.storage.common.objtypes.CommitObj; -import org.projectnessie.versioned.storage.common.objtypes.CommitOp; -import org.projectnessie.versioned.storage.common.persist.Obj; -import org.projectnessie.versioned.storage.common.persist.ObjId; -import org.projectnessie.versioned.storage.common.persist.Persist; -import org.projectnessie.versioned.storage.common.persist.Reference; - -class BaseMergeTransplantIndividual extends BaseCommitHelper { - - BaseMergeTransplantIndividual( - @Nonnull @jakarta.annotation.Nonnull BranchName branch, - @Nonnull @jakarta.annotation.Nonnull Optional referenceHash, - @Nonnull @jakarta.annotation.Nonnull Persist persist, - @Nonnull @jakarta.annotation.Nonnull Reference reference, - @Nullable @jakarta.annotation.Nullable CommitObj head) - throws ReferenceNotFoundException { - super(branch, referenceHash, persist, reference, head); - } - - MergeResult individualCommits( - MergeTransplantOpBase mergeTransplantOpBase, - ImmutableMergeResult.Builder mergeResult, - SourceCommitsAndParent sourceCommits) - throws RetryException, ReferenceNotFoundException, ReferenceConflictException { - IndexesLogic indexesLogic = indexesLogic(persist); - StoreIndex sourceParentIndex = - indexesLogic.buildCompleteIndexOrEmpty(sourceCommits.sourceParent); - StoreIndex targetParentIndex = indexesLogic.buildCompleteIndexOrEmpty(head); - - MergeBehaviors mergeBehaviors = new MergeBehaviors(mergeTransplantOpBase); - - CommitLogic commitLogic = commitLogic(persist); - ObjId newHead = headId(); - boolean empty = true; - Map keyDetailsMap = new HashMap<>(); - for (CommitObj sourceCommit : sourceCommits.sourceCommits) { - CreateCommit createCommit = - cloneCommit( - mergeTransplantOpBase.updateCommitMetadata(), - sourceCommit, - sourceParentIndex, - newHead); - - validateMergeTransplantCommit( - createCommit, mergeTransplantOpBase.validator(), targetParentIndex); - - verifyMergeTransplantCommitPolicies(targetParentIndex, sourceCommit); - - List objsToStore = new ArrayList<>(); - CommitObj newCommit = - createMergeTransplantCommit( - mergeBehaviors, keyDetailsMap, createCommit, objsToStore::add); - - if (!indexesLogic.commitOperations(newCommit).iterator().hasNext()) { - // No operations in this commit, skip it. - continue; - } - - empty = false; - if (!mergeTransplantOpBase.dryRun()) { - newHead = newCommit.id(); - boolean committed = commitLogic.storeCommit(newCommit, objsToStore); - if (committed) { - mergeResult.addCreatedCommits(commitObjToCommit(newCommit)); - } - } - - sourceParentIndex = indexesLogic.buildCompleteIndex(sourceCommit, Optional.empty()); - targetParentIndex = indexesLogic.buildCompleteIndex(newCommit, Optional.empty()); - } - - boolean hasConflicts = recordKeyDetailsAndCheckConflicts(mergeResult, keyDetailsMap); - - return finishMergeTransplant( - empty, mergeResult, newHead, mergeTransplantOpBase.dryRun(), hasConflicts); - } - - private CreateCommit cloneCommit( - MetadataRewriter updateCommitMetadata, - CommitObj sourceCommit, - StoreIndex sourceParentIndex, - ObjId newHead) { - CreateCommit.Builder createCommitBuilder = newCommitBuilder().parentCommitId(newHead); - - CommitMeta commitMeta = toCommitMeta(sourceCommit); - CommitMeta updatedMeta = updateCommitMetadata.rewriteSingle(commitMeta); - fromCommitMeta(updatedMeta, createCommitBuilder); - - IndexesLogic indexesLogic = indexesLogic(persist); - for (StoreIndexElement el : indexesLogic.commitOperations(sourceCommit)) { - StoreIndexElement expected = sourceParentIndex.get(el.key()); - ObjId expectedId = null; - if (expected != null) { - CommitOp expectedContent = expected.content(); - if (expectedContent.action().exists()) { - expectedId = expectedContent.value(); - } - } - - CommitOp op = el.content(); - if (op.action().exists()) { - createCommitBuilder.addAdds( - commitAdd( - el.key(), op.payload(), requireNonNull(op.value()), expectedId, op.contentId())); - } else { - createCommitBuilder.addRemoves( - commitRemove(el.key(), op.payload(), requireNonNull(expectedId), op.contentId())); - } - } - - return createCommitBuilder.build(); - } -} 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 c0589d371e2..fd21ef0691a 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 @@ -21,9 +21,9 @@ import static org.projectnessie.versioned.storage.common.logic.Logics.indexesLogic; import static org.projectnessie.versioned.storage.versionstore.TypeMapping.fromCommitMeta; import static org.projectnessie.versioned.storage.versionstore.TypeMapping.storeKeyToKey; -import static org.projectnessie.versioned.storage.versionstore.TypeMapping.toCommitMeta; import java.util.ArrayList; +import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -74,7 +74,7 @@ class BaseMergeTransplantSquash extends BaseCommitHelper { MergeResult squash( MergeTransplantOpBase mergeTransplantOpBase, ImmutableMergeResult.Builder mergeResult, - SourceCommitsAndParent sourceCommits, + MergeTransplantContext mergeTransplantContext, @Nullable @jakarta.annotation.Nullable ObjId mergeFromId) throws RetryException, ReferenceNotFoundException, ReferenceConflictException { @@ -87,7 +87,7 @@ MergeResult squash( mergeBehaviors, keyDetailsMap, mergeTransplantOpBase.updateCommitMetadata(), - sourceCommits, + mergeTransplantContext, mergeFromId); List objsToStore = new ArrayList<>(); @@ -133,17 +133,15 @@ private CreateCommit createSquashCommit( MergeBehaviors mergeBehaviors, Map keyDetailsMap, MetadataRewriter updateCommitMetadata, - SourceCommitsAndParent sourceCommits, + MergeTransplantContext mergeTransplantContext, @Nullable @jakarta.annotation.Nullable ObjId mergeFromId) { CreateCommit.Builder commitBuilder = newCommitBuilder().parentCommitId(headId()); - List commitsMetadata = new ArrayList<>(sourceCommits.sourceCommits.size()); - for (CommitObj sourceCommit : sourceCommits.sourceCommits) { - commitsMetadata.add(toCommitMeta(sourceCommit)); - } - - CommitMeta metadata = updateCommitMetadata.squash(commitsMetadata); - fromCommitMeta(metadata, commitBuilder); + fromCommitMeta( + updateCommitMetadata.squash( + Collections.singletonList(mergeTransplantContext.metadata()), + mergeTransplantContext.numCommits()), + commitBuilder); if (mergeFromId != null) { commitBuilder.addSecondaryParents(mergeFromId); @@ -164,7 +162,11 @@ private CreateCommit createSquashCommit( CommitLogic commitLogic = commitLogic(persist); PagedResult diff = commitLogic.diff( - diffQuery(sourceCommits.sourceParent, sourceCommits.mostRecent(), true, filter)); + diffQuery( + mergeTransplantContext.baseCommit(), + mergeTransplantContext.headCommit(), + true, + filter)); return commitLogic.diffToCreateCommit(diff, commitBuilder).build(); } diff --git a/versioned/storage/store/src/main/java/org/projectnessie/versioned/storage/versionstore/MergeBehaviors.java b/versioned/storage/store/src/main/java/org/projectnessie/versioned/storage/versionstore/MergeBehaviors.java index 494670d4405..2476266ac9d 100644 --- a/versioned/storage/store/src/main/java/org/projectnessie/versioned/storage/versionstore/MergeBehaviors.java +++ b/versioned/storage/store/src/main/java/org/projectnessie/versioned/storage/versionstore/MergeBehaviors.java @@ -80,12 +80,6 @@ private void validate() { .mergeKeyBehaviors() .forEach( (key, mergeKeyBehavior) -> { - checkArgument( - !mergeTransplantOpBase.keepIndividualCommits() - || (mergeKeyBehavior.getExpectedTargetContent() == null - && mergeKeyBehavior.getResolvedContent() == null), - "MergeKeyBehavior.expectedTargetContent and MergeKeyBehavior.resolvedContent are only supported for squashing merge/transplant operations."); - switch (mergeKeyBehavior.getMergeBehavior()) { case NORMAL: if (mergeKeyBehavior.getResolvedContent() != null) { diff --git a/versioned/storage/store/src/main/java/org/projectnessie/versioned/storage/versionstore/MergeIndividualImpl.java b/versioned/storage/store/src/main/java/org/projectnessie/versioned/storage/versionstore/MergeIndividualImpl.java deleted file mode 100644 index 4cbea8c0712..00000000000 --- a/versioned/storage/store/src/main/java/org/projectnessie/versioned/storage/versionstore/MergeIndividualImpl.java +++ /dev/null @@ -1,86 +0,0 @@ -/* - * Copyright (C) 2022 Dremio - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ -package org.projectnessie.versioned.storage.versionstore; - -import static org.projectnessie.versioned.storage.common.persist.ObjType.COMMIT; -import static org.projectnessie.versioned.storage.versionstore.RefMapping.referenceNotFound; -import static org.projectnessie.versioned.storage.versionstore.TypeMapping.hashToObjId; -import static org.projectnessie.versioned.storage.versionstore.TypeMapping.objIdToHash; - -import java.util.Optional; -import javax.annotation.Nonnull; -import javax.annotation.Nullable; -import org.projectnessie.versioned.BranchName; -import org.projectnessie.versioned.Commit; -import org.projectnessie.versioned.Hash; -import org.projectnessie.versioned.ImmutableMergeResult; -import org.projectnessie.versioned.MergeResult; -import org.projectnessie.versioned.ReferenceConflictException; -import org.projectnessie.versioned.ReferenceNotFoundException; -import org.projectnessie.versioned.ResultType; -import org.projectnessie.versioned.VersionStore.MergeOp; -import org.projectnessie.versioned.storage.common.exceptions.ObjNotFoundException; -import org.projectnessie.versioned.storage.common.logic.CommitRetry.RetryException; -import org.projectnessie.versioned.storage.common.objtypes.CommitObj; -import org.projectnessie.versioned.storage.common.persist.ObjId; -import org.projectnessie.versioned.storage.common.persist.Persist; -import org.projectnessie.versioned.storage.common.persist.Reference; - -final class MergeIndividualImpl extends BaseMergeTransplantIndividual implements Merge { - - MergeIndividualImpl( - @Nonnull @jakarta.annotation.Nonnull BranchName branch, - @Nonnull @jakarta.annotation.Nonnull Optional referenceHash, - @Nonnull @jakarta.annotation.Nonnull Persist persist, - @Nonnull @jakarta.annotation.Nonnull Reference reference, - @Nullable @jakarta.annotation.Nullable CommitObj head) - throws ReferenceNotFoundException { - super(branch, referenceHash, persist, reference, head); - } - - @Override - public MergeResult merge(Optional retryState, MergeOp mergeOp) - throws ReferenceNotFoundException, RetryException, ReferenceConflictException { - ObjId fromId = hashToObjId(mergeOp.fromHash()); - ObjId commonAncestorId = identifyCommonAncestor(fromId); - - CommitObj source; - try { - source = persist.fetchTypedObj(fromId, COMMIT, CommitObj.class); - } catch (ObjNotFoundException e) { - throw referenceNotFound(e); - } - - ImmutableMergeResult.Builder mergeResult = - prepareMergeResult() - .resultType(ResultType.MERGE) - .sourceRef(mergeOp.fromRef()) - .commonAncestor(objIdToHash(commonAncestorId)); - - // Fast-forward, if possible - if (commonAncestorId.equals(headId()) - // TODO the following is a ROUGH port of the existing fast-forward restriction. - // Need to check whether the commit-metadata has changed as well. - && source.directParent().equals(commonAncestorId)) { - - return mergeSquashFastForward(mergeOp, fromId, source, mergeResult); - } - - SourceCommitsAndParent sourceCommits = loadSourceCommitsForMerge(fromId, commonAncestorId); - - return individualCommits(mergeOp, mergeResult, sourceCommits); - } -} diff --git a/versioned/storage/store/src/main/java/org/projectnessie/versioned/storage/versionstore/MergeSquashImpl.java b/versioned/storage/store/src/main/java/org/projectnessie/versioned/storage/versionstore/MergeSquashImpl.java index 33d4648b0b8..45264d3e378 100644 --- a/versioned/storage/store/src/main/java/org/projectnessie/versioned/storage/versionstore/MergeSquashImpl.java +++ b/versioned/storage/store/src/main/java/org/projectnessie/versioned/storage/versionstore/MergeSquashImpl.java @@ -15,12 +15,16 @@ */ package org.projectnessie.versioned.storage.versionstore; +import static org.projectnessie.model.CommitMeta.fromMessage; +import static org.projectnessie.versioned.storage.common.logic.Logics.commitLogic; import static org.projectnessie.versioned.storage.versionstore.TypeMapping.hashToObjId; import static org.projectnessie.versioned.storage.versionstore.TypeMapping.objIdToHash; +import java.util.NoSuchElementException; import java.util.Optional; import javax.annotation.Nonnull; import javax.annotation.Nullable; +import org.projectnessie.model.CommitMeta; import org.projectnessie.versioned.BranchName; import org.projectnessie.versioned.Commit; import org.projectnessie.versioned.Hash; @@ -30,6 +34,8 @@ import org.projectnessie.versioned.ReferenceNotFoundException; import org.projectnessie.versioned.ResultType; import org.projectnessie.versioned.VersionStore.MergeOp; +import org.projectnessie.versioned.storage.common.exceptions.ObjNotFoundException; +import org.projectnessie.versioned.storage.common.logic.CommitLogic; import org.projectnessie.versioned.storage.common.logic.CommitRetry.RetryException; import org.projectnessie.versioned.storage.common.objtypes.CommitObj; import org.projectnessie.versioned.storage.common.persist.ObjId; @@ -52,9 +58,10 @@ final class MergeSquashImpl extends BaseMergeTransplantSquash implements Merge { public MergeResult merge(Optional retryState, MergeOp mergeOp) throws ReferenceNotFoundException, RetryException, ReferenceConflictException { ObjId fromId = hashToObjId(mergeOp.fromHash()); - ObjId commonAncestorId = identifyCommonAncestor(fromId); + ObjId commonAncestorId = identifyMergeBase(fromId); - SourceCommitsAndParent sourceCommits = loadSourceCommitsForMerge(fromId, commonAncestorId); + MergeTransplantContext mergeTransplantContext = + loadSourceCommitsForMerge(fromId, commonAncestorId, mergeOp); ImmutableMergeResult.Builder mergeResult = prepareMergeResult() @@ -62,6 +69,39 @@ public MergeResult merge(Optional retryState, MergeOp mergeOp) .sourceRef(mergeOp.fromRef()) .commonAncestor(objIdToHash(commonAncestorId)); - return squash(mergeOp, mergeResult, sourceCommits, fromId); + if (fromId.equals(commonAncestorId)) { + return mergeResult + .wasSuccessful(true) + .wasApplied(false) + .resultantTargetHash(objIdToHash(headId())) + .build(); + } + + return squash(mergeOp, mergeResult, mergeTransplantContext, fromId); + } + + private ObjId identifyMergeBase(ObjId fromId) throws ReferenceNotFoundException { + CommitLogic commitLogic = commitLogic(persist); + try { + return commitLogic.findMergeBase(headId(), fromId); + } catch (NoSuchElementException notFound) { + throw new ReferenceNotFoundException(notFound.getMessage()); + } + } + + private MergeTransplantContext loadSourceCommitsForMerge( + @Nonnull @jakarta.annotation.Nonnull ObjId startCommitId, + @Nonnull @jakarta.annotation.Nonnull ObjId endCommitId, + @Nonnull @jakarta.annotation.Nonnull MergeOp mergeOp) { + CommitObj[] startEndCommits; + try { + startEndCommits = commitLogic(persist).fetchCommits(startCommitId, endCommitId); + } catch (ObjNotFoundException e) { + throw new RuntimeException(e); + } + + CommitMeta metadata = mergeOp.updateCommitMetadata().rewriteSingle(fromMessage("")); + + return new MergeTransplantContext(startEndCommits[0], startEndCommits[1], metadata); } } diff --git a/versioned/storage/store/src/main/java/org/projectnessie/versioned/storage/versionstore/MergeTransplantContext.java b/versioned/storage/store/src/main/java/org/projectnessie/versioned/storage/versionstore/MergeTransplantContext.java new file mode 100644 index 00000000000..89e8593cb0e --- /dev/null +++ b/versioned/storage/store/src/main/java/org/projectnessie/versioned/storage/versionstore/MergeTransplantContext.java @@ -0,0 +1,71 @@ +/* + * Copyright (C) 2023 Dremio + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.projectnessie.versioned.storage.versionstore; + +import java.util.List; +import org.projectnessie.model.CommitMeta; +import org.projectnessie.versioned.storage.common.objtypes.CommitObj; + +/** Source commits for merge and transplant operations. */ +final class MergeTransplantContext { + + /** Source commits in chronological order, most recent commit last. */ + private final List sourceCommits; + + /** Parent of the oldest commit. */ + private final CommitObj baseCommit; + + private final CommitObj headCommit; + private final CommitMeta metadata; + private final int numCommits; + + MergeTransplantContext(List sourceCommits, CommitObj baseCommit, CommitMeta metadata) { + this.sourceCommits = sourceCommits; + this.baseCommit = baseCommit; + int sourceCommitCount = sourceCommits.size(); + this.headCommit = sourceCommitCount > 0 ? sourceCommits.get(sourceCommitCount - 1) : null; + this.metadata = metadata; + this.numCommits = sourceCommits.size(); + } + + MergeTransplantContext(CommitObj headCommit, CommitObj baseCommit, CommitMeta metadata) { + this.sourceCommits = null; + this.baseCommit = baseCommit; + this.headCommit = headCommit; + this.metadata = metadata; + this.numCommits = 0; + } + + List sourceCommits() { + return sourceCommits; + } + + CommitObj baseCommit() { + return baseCommit; + } + + CommitObj headCommit() { + return headCommit; + } + + CommitMeta metadata() { + return metadata; + } + + int numCommits() { + return numCommits; + } +} diff --git a/versioned/storage/store/src/main/java/org/projectnessie/versioned/storage/versionstore/TransplantIndividualImpl.java b/versioned/storage/store/src/main/java/org/projectnessie/versioned/storage/versionstore/TransplantIndividualImpl.java index 9c246f939fe..0a6a8961775 100644 --- a/versioned/storage/store/src/main/java/org/projectnessie/versioned/storage/versionstore/TransplantIndividualImpl.java +++ b/versioned/storage/store/src/main/java/org/projectnessie/versioned/storage/versionstore/TransplantIndividualImpl.java @@ -15,24 +15,53 @@ */ package org.projectnessie.versioned.storage.versionstore; +import static com.google.common.base.Preconditions.checkArgument; +import static java.util.Objects.requireNonNull; +import static org.projectnessie.versioned.storage.common.logic.CreateCommit.Add.commitAdd; +import static org.projectnessie.versioned.storage.common.logic.CreateCommit.Remove.commitRemove; +import static org.projectnessie.versioned.storage.common.logic.CreateCommit.newCommitBuilder; +import static org.projectnessie.versioned.storage.common.logic.Logics.commitLogic; +import static org.projectnessie.versioned.storage.common.logic.Logics.indexesLogic; +import static org.projectnessie.versioned.storage.common.persist.ObjId.EMPTY_OBJ_ID; +import static org.projectnessie.versioned.storage.versionstore.RefMapping.referenceNotFound; +import static org.projectnessie.versioned.storage.versionstore.TypeMapping.fromCommitMeta; +import static org.projectnessie.versioned.storage.versionstore.TypeMapping.toCommitMeta; + +import java.util.ArrayList; +import java.util.HashMap; +import java.util.List; +import java.util.Map; import java.util.Optional; import javax.annotation.Nonnull; import javax.annotation.Nullable; +import org.projectnessie.model.CommitMeta; +import org.projectnessie.model.ContentKey; import org.projectnessie.versioned.BranchName; import org.projectnessie.versioned.Commit; import org.projectnessie.versioned.Hash; import org.projectnessie.versioned.ImmutableMergeResult; import org.projectnessie.versioned.MergeResult; +import org.projectnessie.versioned.MetadataRewriter; import org.projectnessie.versioned.ReferenceConflictException; import org.projectnessie.versioned.ReferenceNotFoundException; import org.projectnessie.versioned.ResultType; +import org.projectnessie.versioned.VersionStore; import org.projectnessie.versioned.VersionStore.TransplantOp; +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; import org.projectnessie.versioned.storage.common.logic.CommitRetry.RetryException; +import org.projectnessie.versioned.storage.common.logic.CreateCommit; +import org.projectnessie.versioned.storage.common.logic.IndexesLogic; import org.projectnessie.versioned.storage.common.objtypes.CommitObj; +import org.projectnessie.versioned.storage.common.objtypes.CommitOp; +import org.projectnessie.versioned.storage.common.persist.Obj; +import org.projectnessie.versioned.storage.common.persist.ObjId; import org.projectnessie.versioned.storage.common.persist.Persist; import org.projectnessie.versioned.storage.common.persist.Reference; -final class TransplantIndividualImpl extends BaseMergeTransplantIndividual implements Transplant { +final class TransplantIndividualImpl extends BaseCommitHelper implements Transplant { TransplantIndividualImpl( @Nonnull @jakarta.annotation.Nonnull BranchName branch, @@ -47,12 +76,144 @@ final class TransplantIndividualImpl extends BaseMergeTransplantIndividual imple @Override public MergeResult transplant(Optional retryState, TransplantOp transplantOp) throws ReferenceNotFoundException, RetryException, ReferenceConflictException { - SourceCommitsAndParent sourceCommits = - loadSourceCommitsForTransplant(transplantOp.sequenceToTransplant()); + MergeTransplantContext mergeTransplantContext = loadSourceCommitsForTransplant(transplantOp); ImmutableMergeResult.Builder mergeResult = prepareMergeResult().resultType(ResultType.TRANSPLANT).sourceRef(transplantOp.fromRef()); - return individualCommits(transplantOp, mergeResult, sourceCommits); + IndexesLogic indexesLogic = indexesLogic(persist); + StoreIndex sourceParentIndex = + indexesLogic.buildCompleteIndexOrEmpty(mergeTransplantContext.baseCommit()); + StoreIndex targetParentIndex = indexesLogic.buildCompleteIndexOrEmpty(head); + + MergeBehaviors mergeBehaviors = new MergeBehaviors(transplantOp); + + CommitLogic commitLogic = commitLogic(persist); + ObjId newHead = headId(); + boolean empty = true; + Map keyDetailsMap = new HashMap<>(); + for (CommitObj sourceCommit : mergeTransplantContext.sourceCommits()) { + CreateCommit createCommit = + cloneCommit( + transplantOp.updateCommitMetadata(), sourceCommit, sourceParentIndex, newHead); + + validateMergeTransplantCommit(createCommit, transplantOp.validator(), targetParentIndex); + + verifyMergeTransplantCommitPolicies(targetParentIndex, sourceCommit); + + List objsToStore = new ArrayList<>(); + CommitObj newCommit = + createMergeTransplantCommit( + mergeBehaviors, keyDetailsMap, createCommit, objsToStore::add); + + if (!indexesLogic.commitOperations(newCommit).iterator().hasNext()) { + // No operations in this commit, skip it. + continue; + } + + empty = false; + if (!transplantOp.dryRun()) { + newHead = newCommit.id(); + boolean committed = commitLogic.storeCommit(newCommit, objsToStore); + if (committed) { + mergeResult.addCreatedCommits(commitObjToCommit(newCommit)); + } + } + + sourceParentIndex = indexesLogic.buildCompleteIndex(sourceCommit, Optional.empty()); + targetParentIndex = indexesLogic.buildCompleteIndex(newCommit, Optional.empty()); + } + + boolean hasConflicts = recordKeyDetailsAndCheckConflicts(mergeResult, keyDetailsMap); + + return finishMergeTransplant(empty, mergeResult, newHead, transplantOp.dryRun(), hasConflicts); + } + + private CreateCommit cloneCommit( + MetadataRewriter updateCommitMetadata, + CommitObj sourceCommit, + StoreIndex sourceParentIndex, + ObjId newHead) { + CreateCommit.Builder createCommitBuilder = newCommitBuilder().parentCommitId(newHead); + + CommitMeta commitMeta = toCommitMeta(sourceCommit); + CommitMeta updatedMeta = updateCommitMetadata.rewriteSingle(commitMeta); + fromCommitMeta(updatedMeta, createCommitBuilder); + + IndexesLogic indexesLogic = indexesLogic(persist); + for (StoreIndexElement el : indexesLogic.commitOperations(sourceCommit)) { + StoreIndexElement expected = sourceParentIndex.get(el.key()); + ObjId expectedId = null; + if (expected != null) { + CommitOp expectedContent = expected.content(); + if (expectedContent.action().exists()) { + expectedId = expectedContent.value(); + } + } + + CommitOp op = el.content(); + if (op.action().exists()) { + createCommitBuilder.addAdds( + commitAdd( + el.key(), op.payload(), requireNonNull(op.value()), expectedId, op.contentId())); + } else { + createCommitBuilder.addRemoves( + commitRemove(el.key(), op.payload(), requireNonNull(expectedId), op.contentId())); + } + } + + return createCommitBuilder.build(); + } + + MergeTransplantContext loadSourceCommitsForTransplant(VersionStore.TransplantOp transplantOp) + throws ReferenceNotFoundException { + List commitHashes = transplantOp.sequenceToTransplant(); + + checkArgument( + !commitHashes.isEmpty(), + "No hashes to transplant onto %s @ %s, expected commit ID from request was %s.", + head != null ? head.id() : EMPTY_OBJ_ID, + branch.getName(), + referenceHash.map(Hash::asString).orElse("not specified")); + + Obj[] objs; + try { + objs = + persist.fetchObjs( + commitHashes.stream().map(TypeMapping::hashToObjId).toArray(ObjId[]::new)); + } catch (ObjNotFoundException e) { + throw referenceNotFound(e); + } + List commits = new ArrayList<>(commitHashes.size()); + CommitObj parent = null; + CommitLogic commitLogic = commitLogic(persist); + for (int i = 0; i < objs.length; i++) { + Obj o = objs[i]; + if (o == null) { + throw RefMapping.hashNotFound(commitHashes.get(i)); + } + CommitObj commit = (CommitObj) o; + if (i > 0) { + if (!commit.directParent().equals(commits.get(i - 1).id())) { + throw new IllegalArgumentException("Sequence of hashes is not contiguous."); + } + } else { + try { + parent = commitLogic.fetchCommit(commit.directParent()); + } catch (ObjNotFoundException e) { + throw referenceNotFound(e); + } + } + commits.add(commit); + } + + List commitsMetadata = new ArrayList<>(commits.size()); + for (CommitObj sourceCommit : commits) { + commitsMetadata.add(toCommitMeta(sourceCommit)); + } + CommitMeta metadata = + transplantOp.updateCommitMetadata().squash(commitsMetadata, commits.size()); + + return new MergeTransplantContext(commits, parent, metadata); } } diff --git a/versioned/storage/store/src/main/java/org/projectnessie/versioned/storage/versionstore/TransplantSquashImpl.java b/versioned/storage/store/src/main/java/org/projectnessie/versioned/storage/versionstore/TransplantSquashImpl.java deleted file mode 100644 index d197f87f137..00000000000 --- a/versioned/storage/store/src/main/java/org/projectnessie/versioned/storage/versionstore/TransplantSquashImpl.java +++ /dev/null @@ -1,58 +0,0 @@ -/* - * Copyright (C) 2022 Dremio - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ -package org.projectnessie.versioned.storage.versionstore; - -import java.util.Optional; -import javax.annotation.Nonnull; -import javax.annotation.Nullable; -import org.projectnessie.versioned.BranchName; -import org.projectnessie.versioned.Commit; -import org.projectnessie.versioned.Hash; -import org.projectnessie.versioned.ImmutableMergeResult; -import org.projectnessie.versioned.MergeResult; -import org.projectnessie.versioned.ReferenceConflictException; -import org.projectnessie.versioned.ReferenceNotFoundException; -import org.projectnessie.versioned.ResultType; -import org.projectnessie.versioned.VersionStore.TransplantOp; -import org.projectnessie.versioned.storage.common.logic.CommitRetry.RetryException; -import org.projectnessie.versioned.storage.common.objtypes.CommitObj; -import org.projectnessie.versioned.storage.common.persist.Persist; -import org.projectnessie.versioned.storage.common.persist.Reference; - -final class TransplantSquashImpl extends BaseMergeTransplantSquash implements Transplant { - - TransplantSquashImpl( - @Nonnull @jakarta.annotation.Nonnull BranchName branch, - @Nonnull @jakarta.annotation.Nonnull Optional referenceHash, - @Nonnull @jakarta.annotation.Nonnull Persist persist, - @Nonnull @jakarta.annotation.Nonnull Reference reference, - @Nullable @jakarta.annotation.Nullable CommitObj head) - throws ReferenceNotFoundException { - super(branch, referenceHash, persist, reference, head); - } - - @Override - public MergeResult transplant(Optional retryState, TransplantOp transplantOp) - throws ReferenceNotFoundException, RetryException, ReferenceConflictException { - SourceCommitsAndParent sourceCommits = - loadSourceCommitsForTransplant(transplantOp.sequenceToTransplant()); - - ImmutableMergeResult.Builder mergeResult = - prepareMergeResult().resultType(ResultType.TRANSPLANT).sourceRef(transplantOp.fromRef()); - - return squash(transplantOp, mergeResult, sourceCommits, null); - } -} diff --git a/versioned/storage/store/src/main/java/org/projectnessie/versioned/storage/versionstore/VersionStoreImpl.java b/versioned/storage/store/src/main/java/org/projectnessie/versioned/storage/versionstore/VersionStoreImpl.java index 28f4c407f12..062feeb0608 100644 --- a/versioned/storage/store/src/main/java/org/projectnessie/versioned/storage/versionstore/VersionStoreImpl.java +++ b/versioned/storage/store/src/main/java/org/projectnessie/versioned/storage/versionstore/VersionStoreImpl.java @@ -435,8 +435,7 @@ private ReferenceInfo buildReferenceInfo( refInfo.commonAncestor(objIdToHash(commonAncestorId)); if (opts.isComputeAheadBehind()) { - CommitObj commonAncestor = - persist.fetchTypedObj(commonAncestorId, COMMIT, CommitObj.class); + CommitObj commonAncestor = commitLogic.fetchCommit(commonAncestorId); long commonAncestorSeq = commonAncestor.seq(); refInfo.aheadBehind( CommitsAheadBehind.of( @@ -774,9 +773,7 @@ public CommitResult commit( @Override public MergeResult merge(MergeOp mergeOp) throws ReferenceNotFoundException, ReferenceConflictException { - - CommitterSupplier supplier = - mergeOp.keepIndividualCommits() ? MergeIndividualImpl::new : MergeSquashImpl::new; + CommitterSupplier supplier = MergeSquashImpl::new; if (mergeOp.dryRun()) { supplier = dryRunCommitterSupplier(supplier); @@ -798,10 +795,7 @@ public MergeResult merge(MergeOp mergeOp) public MergeResult transplant(TransplantOp transplantOp) throws ReferenceNotFoundException, ReferenceConflictException { - CommitterSupplier supplier = - transplantOp.keepIndividualCommits() - ? TransplantIndividualImpl::new - : TransplantSquashImpl::new; + CommitterSupplier supplier = TransplantIndividualImpl::new; if (transplantOp.dryRun()) { supplier = dryRunCommitterSupplier(supplier); diff --git a/versioned/tests/src/main/java/org/projectnessie/versioned/tests/AbstractDiff.java b/versioned/tests/src/main/java/org/projectnessie/versioned/tests/AbstractDiff.java index aa6e9d878d7..80e19e21743 100644 --- a/versioned/tests/src/main/java/org/projectnessie/versioned/tests/AbstractDiff.java +++ b/versioned/tests/src/main/java/org/projectnessie/versioned/tests/AbstractDiff.java @@ -155,7 +155,7 @@ protected void checkDiff() throws VersionStoreException { soft.assertThat(diffAsList(firstCommit, firstCommit)).isEmpty(); // Key restrictions - if (store().getClass().getName().endsWith("VersionStoreImpl")) { + if (isNewStorageModel()) { soft.assertThat( diffAsList( initial, 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 811ab42170d..f56bc59a9a9 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 @@ -15,6 +15,7 @@ */ package org.projectnessie.versioned.tests; +import static java.util.Arrays.asList; import static java.util.Collections.emptyList; import static java.util.Collections.singletonList; import static org.assertj.core.api.Assertions.assertThat; @@ -30,11 +31,11 @@ import java.util.List; import java.util.Optional; import java.util.function.Supplier; -import java.util.stream.Collectors; import org.assertj.core.api.SoftAssertions; import org.assertj.core.api.junit.jupiter.InjectSoftAssertions; import org.assertj.core.api.junit.jupiter.SoftAssertionsExtension; import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.CsvSource; @@ -56,7 +57,6 @@ import org.projectnessie.versioned.MergeConflictException; import org.projectnessie.versioned.MergeResult; import org.projectnessie.versioned.MergeResult.KeyDetails; -import org.projectnessie.versioned.MetadataRewriter; import org.projectnessie.versioned.Put; import org.projectnessie.versioned.ReferenceConflictException; import org.projectnessie.versioned.ReferenceInfo; @@ -131,23 +131,6 @@ protected void setupCommits() throws VersionStoreException { commits = commitsList(MAIN_BRANCH, false).subList(0, 3); } - private MetadataRewriter createMetadataRewriter(String suffix) { - return new MetadataRewriter() { - @Override - public CommitMeta rewriteSingle(CommitMeta metadata) { - return CommitMeta.fromMessage(metadata.getMessage() + suffix); - } - - @Override - public CommitMeta squash(List metadata) { - return CommitMeta.fromMessage( - metadata.stream() - .map(cm -> cm.getMessage() + suffix) - .collect(Collectors.joining("\n-----------------------------------\n"))); - } - }; - } - @ParameterizedTest @ValueSource(booleans = {false, true}) protected void mergeKeyBehaviorValidation(boolean dryRun) throws Exception { @@ -234,9 +217,8 @@ protected void mergeKeyBehaviorValidation(boolean dryRun) throws Exception { } } - @ParameterizedTest - @ValueSource(booleans = {false, true}) - protected void mergeResolveConflict(boolean individualCommits) throws VersionStoreException { + @Test + protected void mergeResolveConflict() throws VersionStoreException { BranchName sourceBranch = BranchName.of("mergeResolveConflict"); store().create(sourceBranch, Optional.of(thirdCommit)); @@ -261,7 +243,6 @@ protected void mergeResolveConflict(boolean individualCommits) throws VersionSto .fromRef(sourceBranch) .fromHash(sourceHead) .toBranch(MAIN_BRANCH) - .keepIndividualCommits(individualCommits) .build())) .isInstanceOf(MergeConflictException.class); @@ -278,7 +259,6 @@ protected void mergeResolveConflict(boolean individualCommits) throws VersionSto .fromRef(sourceBranch) .fromHash(sourceHead) .toBranch(MAIN_BRANCH) - .keepIndividualCommits(individualCommits) .putMergeKeyBehaviors( key2, MergeKeyBehavior.of( @@ -292,30 +272,6 @@ protected void mergeResolveConflict(boolean individualCommits) throws VersionSto return; } - if (individualCommits) { - soft.assertThatIllegalArgumentException() - .isThrownBy( - () -> - store() - .merge( - MergeOp.builder() - .fromRef(sourceBranch) - .fromHash(sourceHead) - .toBranch(MAIN_BRANCH) - .keepIndividualCommits(individualCommits) - .putMergeKeyBehaviors( - key2, - MergeKeyBehavior.of( - key2, - MergeBehavior.NORMAL, - wrongExpectedContent, - resolvedContent)) - .build())) - .withMessage( - "MergeKeyBehavior.expectedTargetContent and MergeKeyBehavior.resolvedContent are only supported for squashing merge/transplant operations."); - return; - } - soft.assertThatThrownBy( () -> store() @@ -324,7 +280,6 @@ protected void mergeResolveConflict(boolean individualCommits) throws VersionSto .fromRef(sourceBranch) .fromHash(sourceHead) .toBranch(MAIN_BRANCH) - .keepIndividualCommits(individualCommits) .putMergeKeyBehaviors( key2, MergeKeyBehavior.of( @@ -352,7 +307,6 @@ protected void mergeResolveConflict(boolean individualCommits) throws VersionSto .fromRef(sourceBranch) .fromHash(sourceHead) .toBranch(MAIN_BRANCH) - .keepIndividualCommits(individualCommits) .putMergeKeyBehaviors( key2, MergeKeyBehavior.of( @@ -387,47 +341,35 @@ protected void mergeResolveConflict(boolean individualCommits) throws VersionSto soft.assertThat(mergedContent).isEqualTo(resolvedContent); } - @ParameterizedTest - @ValueSource(booleans = {false, true}) - protected void mergeIntoEmptyBranch3Commits(boolean individualCommits) - throws VersionStoreException { + @Test + protected void mergeIntoEmptyBranch3Commits() throws VersionStoreException { + assumeThat(isNewStorageModel()).isFalse(); + final BranchName newBranch = BranchName.of("mergeIntoEmptyBranch3Commits"); store().create(newBranch, Optional.of(initialHash)); - MetadataRewriter metadataRewriter = createMetadataRewriter(""); - - doMergeIntoEmpty(individualCommits, newBranch, metadataRewriter, true); + doMergeIntoEmpty(newBranch); - if (individualCommits) { - // not modifying commit meta, will just "fast forward" - soft.assertThat(store().hashOnReference(newBranch, Optional.empty(), emptyList())) - .isEqualTo(thirdCommit); + // must modify commit meta, it's more than one commit + assertThat(store().hashOnReference(newBranch, Optional.empty(), emptyList())) + .isNotEqualTo(thirdCommit); - assertCommitMeta( - soft, commitsList(newBranch, false).subList(0, 3), commits, metadataRewriter); - } else { - // must modify commit meta, it's more than one commit - assertThat(store().hashOnReference(newBranch, Optional.empty(), emptyList())) - .isNotEqualTo(thirdCommit); - - soft.assertThat(commitsList(newBranch, false)) - .first() - .extracting(Commit::getCommitMeta) - .extracting(CommitMeta::getMessage) - .asString() - .contains( - commits.stream() - .map(Commit::getCommitMeta) - .map(CommitMeta::getMessage) - .toArray(String[]::new)); - } + String[] expectContains = + isNewStorageModel() + ? new String[] {""} + : commits.stream() + .map(Commit::getCommitMeta) + .map(CommitMeta::getMessage) + .toArray(String[]::new); + soft.assertThat(commitsList(newBranch, false)) + .first() + .extracting(Commit::getCommitMeta) + .extracting(CommitMeta::getMessage) + .asString() + .contains(expectContains); } - private void doMergeIntoEmpty( - boolean individualCommits, - BranchName newBranch, - MetadataRewriter metadataRewriter, - boolean expectFastForward) + private void doMergeIntoEmpty(BranchName newBranch) throws ReferenceNotFoundException, ReferenceConflictException { MergeResult result = store() @@ -437,18 +379,59 @@ private void doMergeIntoEmpty( .fromHash(thirdCommit) .toBranch(newBranch) .expectedHash(Optional.of(initialHash)) - .updateCommitMetadata(metadataRewriter) - .keepIndividualCommits(individualCommits) .build()); - checkAddedCommits(individualCommits, initialHash, result, expectFastForward); + soft.assertThat(result.getCreatedCommits()) + .singleElement() + .satisfies( + c -> { + soft.assertThat(c.getParentHash()).isEqualTo(initialHash); + + if (isNewStorageModel()) { + soft.assertThat(c.getCommitMeta().getMessage()).isEmpty(); + } else { + soft.assertThat(c.getCommitMeta().getMessage()) + .contains("First Commit") + .contains("Second Commit") + .contains("Third Commit"); + } + soft.assertThat(c.getOperations()) + // old storage model keeps the Delete operation when a key is Put then Deleted, + // see https://github.com/projectnessie/nessie/pull/6472 + .hasSizeBetween(3, 4) + .anySatisfy( + o -> { + if (c.getOperations() != null && c.getOperations().size() == 4) { + soft.assertThat(o).isInstanceOf(Delete.class); + soft.assertThat(o.getKey()).isEqualTo(ContentKey.of("t3")); + } + }) + .anySatisfy( + 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); + }) + .anySatisfy( + 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); + }) + .anySatisfy( + 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); + }); + }); soft.assertThat( contentsWithoutId( store() .getValues( newBranch, - Arrays.asList( + asList( ContentKey.of("t1"), ContentKey.of("t2"), ContentKey.of("t3"), @@ -460,137 +443,11 @@ private void doMergeIntoEmpty( ContentKey.of("t4"), V_4_1)); } - private void checkAddedCommits( - boolean individualCommits, - Hash targetHead, - MergeResult result, - boolean expectFastForward) { - if (individualCommits) { - if (expectFastForward) { - soft.assertThat(result.getCreatedCommits()).isEmpty(); - } else { - soft.assertThat(result.getCreatedCommits()) - .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.getCreatedCommits().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.getCreatedCommits().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 - ); - }); - } - } else { - soft.assertThat(result.getCreatedCommits()) - .singleElement() - .satisfies( - c -> { - soft.assertThat(c.getParentHash()).isEqualTo(targetHead); - soft.assertThat(c.getCommitMeta().getMessage()) - .contains("First Commit") - .contains("Second Commit") - .contains("Third Commit"); - soft.assertThat(c.getOperations()) - // old storage model keeps the Delete operation when a key is Put then Deleted, - // see https://github.com/projectnessie/nessie/pull/6472 - .hasSizeBetween(3, 4) - .anySatisfy( - o -> { - if (c.getOperations() != null && c.getOperations().size() == 4) { - soft.assertThat(o).isInstanceOf(Delete.class); - soft.assertThat(o.getKey()).isEqualTo(ContentKey.of("t3")); - } - }) - .anySatisfy( - 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); - }) - .anySatisfy( - 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); - }) - .anySatisfy( - 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); - }); - }); - } - } - @SuppressWarnings("deprecation") - @ParameterizedTest - @ValueSource(booleans = {false, true}) - void compareDryAndEffectiveMergeResults(boolean individualCommits) throws VersionStoreException { + @Test + void compareDryAndEffectiveMergeResults() throws VersionStoreException { + assumeThat(isNewStorageModel()).isFalse(); + final BranchName newBranch = BranchName.of("compareDryAndEffectiveMergeResults"); store().create(newBranch, Optional.of(initialHash)); @@ -604,7 +461,6 @@ void compareDryAndEffectiveMergeResults(boolean individualCommits) throws Versio .fromHash(firstCommit) .toBranch(newBranch) .expectedHash(Optional.of(initialHash)) - .keepIndividualCommits(individualCommits) .dryRun(true) .fetchAdditionalInfo(true) .build()); @@ -631,7 +487,6 @@ void compareDryAndEffectiveMergeResults(boolean individualCommits) throws Versio .fromHash(firstCommit) .toBranch(newBranch) .expectedHash(Optional.of(initialHash)) - .keepIndividualCommits(individualCommits) .fetchAdditionalInfo(true) .build()); @@ -653,13 +508,15 @@ void compareDryAndEffectiveMergeResults(boolean individualCommits) throws Versio tuple( firstCommit, "First Commit", - Arrays.asList( + asList( Put.of(ContentKey.of("t1"), V_1_1), 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 no commits added - soft.assertThat(mergeResult.getCreatedCommits()).isEmpty(); + if (!isNewStorageModel()) { + // The branch was fast-forwarded to firstCommit, so no commits added + soft.assertThat(mergeResult.getCreatedCommits()).isEmpty(); + } soft.assertThat(mergeResult.getDetails()) .containsKeys(ContentKey.of("t1"), ContentKey.of("t2"), ContentKey.of("t3")); soft.assertThat(mergeResult) @@ -684,19 +541,19 @@ void compareDryAndEffectiveMergeResults(boolean individualCommits) throws Versio MergeResult.builder() .from(dryMergeResult) .wasApplied(true) + .createdCommits(mergeResult.getCreatedCommits()) .resultantTargetHash(mergeResult.getResultantTargetHash()) .build()); } - @ParameterizedTest - @ValueSource(booleans = {false, true}) - protected void mergeIntoEmptyBranch1Commit(boolean individualCommits) - throws VersionStoreException { + @Test + protected void mergeIntoEmptyBranch1Commit() throws VersionStoreException { + // This test is a "fast-forward-merge" test, no longer supported since #7035 + assumeThat(isNewStorageModel()).isFalse(); + final BranchName newBranch = BranchName.of("mergeIntoEmptyBranch1Commit"); store().create(newBranch, Optional.of(initialHash)); - MetadataRewriter metadataRewriter = createMetadataRewriter(""); - MergeResult result = store() .merge( @@ -704,16 +561,14 @@ protected void mergeIntoEmptyBranch1Commit(boolean individualCommits) .fromRef(MAIN_BRANCH) .fromHash(firstCommit) .toBranch(newBranch) - .updateCommitMetadata(metadataRewriter) .expectedHash(Optional.of(initialHash)) - .keepIndividualCommits(individualCommits) .build()); soft.assertThat( contentsWithoutId( store() .getValues( newBranch, - Arrays.asList( + asList( ContentKey.of("t1"), ContentKey.of("t2"), ContentKey.of("t3"), @@ -730,7 +585,7 @@ protected void mergeIntoEmptyBranch1Commit(boolean individualCommits) soft.assertThat(result.getCreatedCommits()).isEmpty(); // no new commits List mergedCommit = commitsList(newBranch, false).subList(0, 1); - assertCommitMeta(soft, mergedCommit, commits.subList(2, 3), metadataRewriter); + assertCommitMeta(soft, mergedCommit, commits.subList(2, 3)); // fast-forward merge, additional parents not necessary (and not set) soft.assertThat(mergedCommit.get(0)) @@ -740,58 +595,48 @@ protected void mergeIntoEmptyBranch1Commit(boolean individualCommits) .hasSize(1); } - @ParameterizedTest - @ValueSource(booleans = {false, true}) - protected void mergeIntoEmptyBranchModifying(boolean individualCommits) - throws VersionStoreException { + @Test + protected void mergeIntoEmptyBranchModifying() throws VersionStoreException { + assumeThat(isNewStorageModel()).isFalse(); + final BranchName newBranch = BranchName.of("mergeIntoEmptyBranchModifying"); store().create(newBranch, Optional.of(initialHash)); - MetadataRewriter metadataRewriter = createMetadataRewriter(", merged"); - - doMergeIntoEmpty( - individualCommits, newBranch, metadataRewriter, false /* because of metadata rewrite */); - - // modify the commit meta, will generate new commits and therefore new commit hashes - soft.assertThat(store().hashOnReference(newBranch, Optional.empty(), emptyList())) - .isNotEqualTo(thirdCommit); + doMergeIntoEmpty(newBranch); List mergedCommits = commitsList(newBranch, false); - if (individualCommits) { - assertCommitMeta(soft, mergedCommits.subList(0, 3), commits, metadataRewriter); - } else { - soft.assertThat(mergedCommits) - .first() - .extracting(Commit::getCommitMeta) - .extracting(CommitMeta::getMessage) - .asString() - .contains( - commits.stream() - .map(Commit::getCommitMeta) - .map(CommitMeta::getMessage) - .toArray(String[]::new)); - - soft.assertThat(mergedCommits) - .first() - .extracting(Commit::getCommitMeta) - .extracting(CommitMeta::getParentCommitHashes) - .asInstanceOf(list(String.class)) - .containsExactly( - mergedCommits.get(1).getHash().asString(), commits.get(0).getHash().asString()); - } + String[] expectContains = + isNewStorageModel() + ? new String[] {""} + : commits.stream() + .map(Commit::getCommitMeta) + .map(CommitMeta::getMessage) + .toArray(String[]::new); + soft.assertThat(mergedCommits) + .first() + .extracting(Commit::getCommitMeta) + .extracting(CommitMeta::getMessage) + .asString() + .contains(expectContains); + + soft.assertThat(mergedCommits) + .first() + .extracting(Commit::getCommitMeta) + .extracting(CommitMeta::getParentCommitHashes) + .asInstanceOf(list(String.class)) + .containsExactly( + mergedCommits.get(1).getHash().asString(), commits.get(0).getHash().asString()); } @SuppressWarnings("deprecation") - @ParameterizedTest - @ValueSource(booleans = {false, true}) - protected void mergeIntoNonConflictingBranch(boolean individualCommits) - throws VersionStoreException { + @Test + protected void mergeIntoNonConflictingBranch() throws VersionStoreException { + assumeThat(isNewStorageModel()).isFalse(); + final BranchName newBranch = BranchName.of("bar_2"); store().create(newBranch, Optional.of(initialHash)); final Hash newCommit = commit("Unrelated commit").put("t5", V_5_1).toBranch(newBranch); - MetadataRewriter metadataRewriter = createMetadataRewriter(""); - MergeResult result = store() .merge( @@ -799,8 +644,6 @@ protected void mergeIntoNonConflictingBranch(boolean individualCommits) .fromRef(MAIN_BRANCH) .fromHash(thirdCommit) .toBranch(newBranch) - .updateCommitMetadata(metadataRewriter) - .keepIndividualCommits(individualCommits) .build()); soft.assertThat(result.getResultantTargetHash()).isNotEqualTo(thirdCommit); @@ -820,7 +663,7 @@ protected void mergeIntoNonConflictingBranch(boolean individualCommits) store() .getValues( newBranch, - Arrays.asList( + asList( ContentKey.of("t1"), ContentKey.of("t2"), ContentKey.of("t3"), @@ -834,67 +677,31 @@ protected void mergeIntoNonConflictingBranch(boolean individualCommits) ContentKey.of("t5"), V_5_1)); final List commits = commitsList(newBranch, false); - if (individualCommits) { - soft.assertThat(commits) - .satisfiesExactly( - c0 -> - assertThat(c0) - .extracting(Commit::getCommitMeta) - .extracting(CommitMeta::getMessage) - .isEqualTo("Third Commit"), - c1 -> - assertThat(c1) - .extracting(Commit::getCommitMeta) - .extracting(CommitMeta::getMessage) - .isEqualTo("Second Commit"), - c2 -> - assertThat(c2) - .extracting(Commit::getCommitMeta) - .extracting(CommitMeta::getMessage) - .isEqualTo("First Commit"), - c3 -> assertThat(c3).extracting(Commit::getHash).isEqualTo(newCommit), - c4 -> assertThat(c4).extracting(Commit::getHash).isEqualTo(initialHash)); - } else { - soft.assertThat(commits) - .satisfiesExactly( - c0 -> - assertThat(c0) - .extracting(Commit::getCommitMeta) - .extracting(CommitMeta::getMessage) - .asString() - .contains("Third Commit", "Second Commit", "First Commit"), - c3 -> assertThat(c3).extracting(Commit::getHash).isEqualTo(newCommit), - c4 -> assertThat(c4).extracting(Commit::getHash).isEqualTo(initialHash)); - } - if (individualCommits) { - soft.assertThat(result.getCreatedCommits()) - .hasSize(3) - .satisfiesExactly( - commit -> { - soft.assertThat(commit.getParentHash()).isEqualTo(newCommit); - soft.assertThat(commit.getCommitMeta().getMessage()).isEqualTo("First Commit"); - }, - commit -> { - soft.assertThat(commit.getParentHash()) - .isEqualTo(result.getCreatedCommits().get(0).getHash()); - soft.assertThat(commit.getCommitMeta().getMessage()).isEqualTo("Second Commit"); - }, - commit -> { - soft.assertThat(commit.getParentHash()) - .isEqualTo(result.getCreatedCommits().get(1).getHash()); - soft.assertThat(commit.getCommitMeta().getMessage()).isEqualTo("Third Commit"); - }); - } else { - soft.assertThat(result.getCreatedCommits()) - .singleElement() - .extracting(Commit::getParentHash) - .isEqualTo(newCommit); - } + soft.assertThat(commits) + .satisfiesExactly( + c0 -> { + String[] expectContains = + isNewStorageModel() + ? new String[] {""} + : new String[] {"Third Commit", "Second Commit", "First Commit"}; + assertThat(c0) + .extracting(Commit::getCommitMeta) + .extracting(CommitMeta::getMessage) + .asString() + .contains(expectContains); + }, + c3 -> assertThat(c3).extracting(Commit::getHash).isEqualTo(newCommit), + c4 -> assertThat(c4).extracting(Commit::getHash).isEqualTo(initialHash)); + soft.assertThat(result.getCreatedCommits()) + .singleElement() + .extracting(Commit::getParentHash) + .isEqualTo(newCommit); } - @ParameterizedTest - @ValueSource(booleans = {false, true}) - protected void nonEmptyFastForwardMerge(boolean individualCommits) throws VersionStoreException { + @Test + protected void nonEmptyFastForwardMerge() throws VersionStoreException { + assumeThat(isNewStorageModel()).isFalse(); + final ContentKey key = ContentKey.of("t1"); final BranchName etl = BranchName.of("etl"); final BranchName review = BranchName.of("review"); @@ -916,7 +723,6 @@ protected void nonEmptyFastForwardMerge(boolean individualCommits) throws Versio .fromRef(etl) .fromHash(store().hashOnReference(etl, Optional.empty(), emptyList())) .toBranch(review) - .keepIndividualCommits(individualCommits) .build()); soft.assertThat(mergeResult1.getResultantTargetHash()).isEqualTo(etl1.getCommitHash()); soft.assertThat(mergeResult1.getCreatedCommits()).isEmpty(); // fast-forward, so nothing added @@ -935,7 +741,6 @@ protected void nonEmptyFastForwardMerge(boolean individualCommits) throws Versio .fromRef(etl) .fromHash(store().hashOnReference(etl, Optional.empty(), emptyList())) .toBranch(review) - .keepIndividualCommits(individualCommits) .build()); soft.assertThat(mergeResult2.getResultantTargetHash()).isEqualTo(etl2.getCommitHash()); soft.assertThat(mergeResult2.getCreatedCommits()).isEmpty(); // fast-forward, so nothing added @@ -943,16 +748,15 @@ protected void nonEmptyFastForwardMerge(boolean individualCommits) throws Versio soft.assertThat(contentWithoutId(store().getValue(review, key))).isEqualTo(VALUE_2); } - @ParameterizedTest - @ValueSource(booleans = {false, true}) - protected void mergeWithCommonAncestor(boolean individualCommits) throws VersionStoreException { + @Test + protected void mergeWithCommonAncestor() throws VersionStoreException { + assumeThat(isNewStorageModel()).isFalse(); + final BranchName newBranch = BranchName.of("bar_2"); store().create(newBranch, Optional.of(firstCommit)); final Hash newCommit = commit("Unrelated commit").put("t5", V_5_1).toBranch(newBranch); - MetadataRewriter metadataRewriter = createMetadataRewriter(""); - MergeResult result = store() .merge( @@ -960,15 +764,13 @@ protected void mergeWithCommonAncestor(boolean individualCommits) throws Version .fromRef(MAIN_BRANCH) .fromHash(thirdCommit) .toBranch(newBranch) - .updateCommitMetadata(metadataRewriter) - .keepIndividualCommits(individualCommits) .build()); soft.assertThat( contentsWithoutId( store() .getValues( newBranch, - Arrays.asList( + asList( ContentKey.of("t1"), ContentKey.of("t2"), ContentKey.of("t3"), @@ -982,52 +784,42 @@ protected void mergeWithCommonAncestor(boolean individualCommits) throws Version ContentKey.of("t5"), V_5_1)); final List commits = commitsList(newBranch, true); - if (individualCommits) { - soft.assertThat(commits) - .hasSize(5) - .satisfiesExactly( - c -> assertThat(c.getCommitMeta().getMessage()).isEqualTo("Third Commit"), - c -> assertThat(c.getCommitMeta().getMessage()).isEqualTo("Second Commit"), - c -> assertThat(c.getHash()).isEqualTo(newCommit), - c -> assertThat(c.getHash()).isEqualTo(firstCommit), - c -> assertThat(c.getHash()).isEqualTo(initialHash)); - soft.assertThat(result.getCreatedCommits()) - .hasSize(2) - .satisfiesExactly( - c -> assertThat(c).isEqualTo(commits.get(1)), - c -> assertThat(c).isEqualTo(commits.get(0))); + soft.assertThat(commits) + .hasSize(4) + .satisfiesExactly( + c -> { + if (isNewStorageModel()) { + assertThat(c.getCommitMeta().getMessage()).isEmpty(); + } else { + assertThat(c.getCommitMeta().getMessage()) + .contains("Second Commit", "Third Commit"); + } + }, + c -> assertThat(c.getHash()).isEqualTo(newCommit), + c -> assertThat(c.getHash()).isEqualTo(firstCommit), + c -> assertThat(c.getHash()).isEqualTo(initialHash)); - } else { - soft.assertThat(commits) - .hasSize(4) - .satisfiesExactly( - c -> - assertThat(c.getCommitMeta().getMessage()) - .contains("Second Commit", "Third Commit"), - c -> assertThat(c.getHash()).isEqualTo(newCommit), - c -> assertThat(c.getHash()).isEqualTo(firstCommit), - c -> assertThat(c.getHash()).isEqualTo(initialHash)); - - soft.assertThat(result.getCreatedCommits()).singleElement().isEqualTo(commits.get(0)); - - soft.assertThat(commits) - .first() - .extracting(Commit::getCommitMeta) - .extracting(CommitMeta::getParentCommitHashes) - .asInstanceOf(list(String.class)) - .containsExactly(newCommit.asString(), thirdCommit.asString()); - } + soft.assertThat(result.getCreatedCommits()).singleElement().isEqualTo(commits.get(0)); + + soft.assertThat(commits) + .first() + .extracting(Commit::getCommitMeta) + .extracting(CommitMeta::getParentCommitHashes) + .asInstanceOf(list(String.class)) + .containsExactly(newCommit.asString(), thirdCommit.asString()); } @ParameterizedTest @CsvSource({ "false,false", - "false,true", "true,false", + "false,true", "true,true", }) - protected void mergeWithConflictingKeys(boolean individualCommits, boolean dryRun) + protected void mergeWithConflictingKeys(boolean dryRun, boolean allForce) throws VersionStoreException { + assumeThat(isNewStorageModel()).isFalse(); + final BranchName mergeInto = BranchName.of("foofoo"); final BranchName mergeFrom = BranchName.of("barbar"); store().create(mergeInto, Optional.of(this.initialHash)); @@ -1051,14 +843,14 @@ protected void mergeWithConflictingKeys(boolean individualCommits, boolean dryRu mergeFrom, Optional.empty(), CommitMeta.fromMessage("commit 2"), - Arrays.asList(Put.of(conflictingKey1, VALUE_2), Put.of(key3, VALUE_5))); + asList(Put.of(conflictingKey1, VALUE_2), Put.of(key3, VALUE_5))); Hash mergeIntoHead = store() .commit( mergeInto, Optional.empty(), CommitMeta.fromMessage("commit 3"), - Arrays.asList(Put.of(conflictingKey2, VALUE_3), Put.of(key4, VALUE_6))) + asList(Put.of(conflictingKey2, VALUE_3), Put.of(key4, VALUE_6))) .getCommitHash(); Hash mergeFromHash = store() @@ -1079,7 +871,6 @@ protected void mergeWithConflictingKeys(boolean individualCommits, boolean dryRu .fromRef(mergeFrom) .fromHash(mergeFromHash) .toBranch(mergeInto) - .keepIndividualCommits(individualCommits) .dryRun(dryRun) .build())) .isInstanceOf(ReferenceConflictException.class) @@ -1099,7 +890,6 @@ protected void mergeWithConflictingKeys(boolean individualCommits, boolean dryRu .fromRef(mergeFrom) .fromHash(mergeFromHash) .toBranch(mergeInto) - .keepIndividualCommits(individualCommits) .putMergeKeyBehaviors( conflictingKey2, MergeKeyBehavior.of(conflictingKey2, MergeBehavior.DROP)) @@ -1122,7 +912,6 @@ protected void mergeWithConflictingKeys(boolean individualCommits, boolean dryRu .fromRef(mergeFrom) .fromHash(mergeFromHash) .toBranch(mergeInto) - .keepIndividualCommits(individualCommits) .putMergeKeyBehaviors( conflictingKey1, MergeKeyBehavior.of(conflictingKey1, MergeBehavior.NORMAL)) @@ -1146,7 +935,6 @@ protected void mergeWithConflictingKeys(boolean individualCommits, boolean dryRu .fromRef(mergeFrom) .fromHash(mergeFromHash) .toBranch(mergeInto) - .keepIndividualCommits(individualCommits) .putMergeKeyBehaviors( conflictingKey1, MergeKeyBehavior.of(conflictingKey1, MergeBehavior.FORCE)) @@ -1171,89 +959,44 @@ protected void mergeWithConflictingKeys(boolean individualCommits, boolean dryRu soft.assertThat(mergeIntoHeadSupplier.get()).isEqualTo(mergeIntoHead); - // Merge with force-merge of conflictingKey1 + drop of conflictingKey2 - MergeResult result = - store() - .merge( - MergeOp.builder() - .fromRef(mergeFrom) - .fromHash(mergeFromHash) - .toBranch(mergeInto) - .updateCommitMetadata(createMetadataRewriter(", merge-force-1")) - .keepIndividualCommits(individualCommits) - .putMergeKeyBehaviors( - conflictingKey1, MergeKeyBehavior.of(conflictingKey1, MergeBehavior.FORCE)) - .putMergeKeyBehaviors( - conflictingKey2, MergeKeyBehavior.of(conflictingKey2, MergeBehavior.DROP)) - .build()); - soft.assertThat( - contentsWithoutId( - store.getValues( - mergeIntoHeadSupplier.get(), - Arrays.asList(conflictingKey1, conflictingKey2, key3, key4)))) - .containsEntry(conflictingKey1, VALUE_2) // value as in "mergeFrom" - .containsEntry(conflictingKey2, VALUE_3) // value as in "mergeInto" - .containsEntry(key3, VALUE_5) - .containsEntry(key4, VALUE_6); - - soft.assertThat(result.getCreatedCommits()) - .singleElement() - .satisfies( - commit -> { - if (individualCommits) { - soft.assertThat(commit.getCommitMeta().getMessage()) - .isEqualTo("commit 2, merge-force-1"); - // commit 4 was skipped because empty after drop of conflictingKey2 - } else { - soft.assertThat(commit.getCommitMeta().getMessage()) - .contains("commit 2, merge-force-1") - .contains("commit 4, merge-force-1"); - } - soft.assertThat(commit.getParentHash()).isEqualTo(mergeIntoHead); - soft.assertThat(commit.getOperations()) - .satisfiesExactlyInAnyOrder( - op -> { - soft.assertThat(op.getKey()).isEqualTo(conflictingKey1); - soft.assertThat(contentWithoutId(((Put) op).getValue())).isEqualTo(VALUE_2); - }, - op -> { - soft.assertThat(op.getKey()).isEqualTo(key3); - soft.assertThat(contentWithoutId(((Put) op).getValue())).isEqualTo(VALUE_5); - }); - }); + // Need to be distinct tests, because the two "cases" (in the if-else branches) generate the + // same commits with the same hashes, since we've removed most of the custom MetadataRewriters. + if (!allForce) { + // Merge with force-merge of conflictingKey1 + drop of conflictingKey2 + MergeResult result = + store() + .merge( + MergeOp.builder() + .fromRef(mergeFrom) + .fromHash(mergeFromHash) + .toBranch(mergeInto) + .putMergeKeyBehaviors( + conflictingKey1, + MergeKeyBehavior.of(conflictingKey1, MergeBehavior.FORCE)) + .putMergeKeyBehaviors( + conflictingKey2, MergeKeyBehavior.of(conflictingKey2, MergeBehavior.DROP)) + .build()); + soft.assertThat( + contentsWithoutId( + store.getValues( + mergeIntoHeadSupplier.get(), + Arrays.asList(conflictingKey1, conflictingKey2, key3, key4)))) + .containsEntry(conflictingKey1, VALUE_2) // value as in "mergeFrom" + .containsEntry(conflictingKey2, VALUE_3) // value as in "mergeInto" + .containsEntry(key3, VALUE_5) + .containsEntry(key4, VALUE_6); - // reset merge-into branch - store().assign(mergeInto, Optional.empty(), mergeIntoHead); - - // Merge with force-merge of conflictingKey1 + drop of conflictingKey2 - MergeResult result2 = - store() - .merge( - MergeOp.builder() - .fromRef(mergeFrom) - .fromHash(mergeFromHash) - .toBranch(mergeInto) - .updateCommitMetadata(createMetadataRewriter(", merge-all-force")) - .keepIndividualCommits(individualCommits) - .defaultMergeBehavior(MergeBehavior.FORCE) - .build()); - soft.assertThat( - contentsWithoutId( - store.getValues( - mergeIntoHeadSupplier.get(), - Arrays.asList(conflictingKey1, conflictingKey2, key3, key4)))) - .containsEntry(conflictingKey1, VALUE_2) // value as in "mergeFrom" - .containsEntry(conflictingKey2, VALUE_4) // value as in "mergeFrom" - .containsEntry(key3, VALUE_5) - .containsEntry(key4, VALUE_6); - - if (individualCommits) { - soft.assertThat(result2.getCreatedCommits()) - .hasSize(2) - .satisfiesExactly( + soft.assertThat(result.getCreatedCommits()) + .singleElement() + .satisfies( commit -> { - soft.assertThat(commit.getCommitMeta().getMessage()) - .isEqualTo("commit 2, merge-all-force"); + if (isNewStorageModel()) { + soft.assertThat(commit.getCommitMeta().getMessage()).isEmpty(); + } else { + soft.assertThat(commit.getCommitMeta().getMessage()) + .contains("commit 2") + .contains("commit 4"); + } soft.assertThat(commit.getParentHash()).isEqualTo(mergeIntoHead); soft.assertThat(commit.getOperations()) .satisfiesExactlyInAnyOrder( @@ -1267,29 +1010,39 @@ protected void mergeWithConflictingKeys(boolean individualCommits, boolean dryRu soft.assertThat(contentWithoutId(((Put) op).getValue())) .isEqualTo(VALUE_5); }); - }, - commit -> { - soft.assertThat(commit.getCommitMeta().getMessage()) - .isEqualTo("commit 4, merge-all-force"); - soft.assertThat(commit.getParentHash()) - .isEqualTo(result2.getCreatedCommits().get(0).getHash()); - soft.assertThat(commit.getOperations()) - .singleElement() - .satisfies( - op -> { - soft.assertThat(op.getKey()).isEqualTo(conflictingKey2); - soft.assertThat(contentWithoutId(((Put) op).getValue())) - .isEqualTo(VALUE_4); - }); }); } else { + // Merge with force-merge of conflictingKey1, no drop of conflictingKey2 + MergeResult result2 = + store() + .merge( + MergeOp.builder() + .fromRef(mergeFrom) + .fromHash(mergeFromHash) + .toBranch(mergeInto) + .defaultMergeBehavior(MergeBehavior.FORCE) + .build()); + soft.assertThat( + contentsWithoutId( + store.getValues( + mergeIntoHeadSupplier.get(), + Arrays.asList(conflictingKey1, conflictingKey2, key3, key4)))) + .containsEntry(conflictingKey1, VALUE_2) // value as in "mergeFrom" + .containsEntry(conflictingKey2, VALUE_4) // value as in "mergeFrom" + .containsEntry(key3, VALUE_5) + .containsEntry(key4, VALUE_6); + soft.assertThat(result2.getCreatedCommits()) .singleElement() .satisfies( commit -> { - soft.assertThat(commit.getCommitMeta().getMessage()) - .contains("commit 2, merge-all-force") - .contains("commit 4, merge-all-force"); + if (isNewStorageModel()) { + soft.assertThat(commit.getCommitMeta().getMessage()).isEmpty(); + } else { + soft.assertThat(commit.getCommitMeta().getMessage()) + .contains("commit 2") + .contains("commit 4"); + } soft.assertThat(commit.getParentHash()).isEqualTo(mergeIntoHead); soft.assertThat(commit.getOperations()) .satisfiesExactlyInAnyOrder( @@ -1313,14 +1066,8 @@ protected void mergeWithConflictingKeys(boolean individualCommits, boolean dryRu } @ParameterizedTest - @CsvSource({ - "false,false", - "false,true", - "true,false", - "true,true", - }) - protected void mergeIntoConflictingBranch(boolean individualCommits, boolean dryRun) - throws VersionStoreException { + @ValueSource(booleans = {false, true}) + protected void mergeIntoConflictingBranch(boolean dryRun) throws VersionStoreException { final BranchName newBranch = BranchName.of("bar_3"); store().create(newBranch, Optional.of(initialHash)); commit("Another commit").put("t1", V_1_4).toBranch(newBranch); @@ -1335,7 +1082,6 @@ protected void mergeIntoConflictingBranch(boolean individualCommits, boolean dry .fromHash(thirdCommit) .toBranch(newBranch) .expectedHash(Optional.of(initialHash)) - .keepIndividualCommits(individualCommits) .dryRun(dryRun) .build())) .isInstanceOf(ReferenceConflictException.class); @@ -1345,13 +1091,8 @@ protected void mergeIntoConflictingBranch(boolean individualCommits, boolean dry } @ParameterizedTest - @CsvSource({ - "false,false", - "false,true", - "true,false", - "true,true", - }) - protected void mergeIntoNonExistingBranch(boolean individualCommits, boolean dryRun) { + @ValueSource(booleans = {false, true}) + protected void mergeIntoNonExistingBranch(boolean dryRun) { final BranchName newBranch = BranchName.of("bar_5"); StorageAssertions checkpoint = storageCheckpoint(); soft.assertThatThrownBy( @@ -1363,7 +1104,6 @@ protected void mergeIntoNonExistingBranch(boolean individualCommits, boolean dry .fromHash(thirdCommit) .toBranch(newBranch) .expectedHash(Optional.of(initialHash)) - .keepIndividualCommits(individualCommits) .dryRun(dryRun) .build())) .isInstanceOf(ReferenceNotFoundException.class); @@ -1371,14 +1111,8 @@ protected void mergeIntoNonExistingBranch(boolean individualCommits, boolean dry } @ParameterizedTest - @CsvSource({ - "false,false", - "false,true", - "true,false", - "true,true", - }) - protected void mergeIntoNonExistingReference(boolean individualCommits, boolean dryRun) - throws VersionStoreException { + @ValueSource(booleans = {false, true}) + protected void mergeIntoNonExistingReference(boolean dryRun) throws VersionStoreException { final BranchName newBranch = BranchName.of("bar_6"); store().create(newBranch, Optional.of(initialHash)); StorageAssertions checkpoint = storageCheckpoint(); @@ -1391,7 +1125,6 @@ protected void mergeIntoNonExistingReference(boolean individualCommits, boolean .fromHash(Hash.of("1234567890abcdef")) .toBranch(newBranch) .expectedHash(Optional.of(initialHash)) - .keepIndividualCommits(individualCommits) .dryRun(dryRun) .build())) .isInstanceOf(ReferenceNotFoundException.class); @@ -1399,14 +1132,8 @@ protected void mergeIntoNonExistingReference(boolean individualCommits, boolean } @ParameterizedTest - @CsvSource({ - "false,false", - "false,true", - "true,false", - "true,true", - }) - protected void mergeEmptyCommit(boolean individualCommits, boolean dryRun) - throws VersionStoreException { + @ValueSource(booleans = {false, true}) + protected void mergeEmptyCommit(boolean dryRun) throws VersionStoreException { BranchName source = BranchName.of("source"); BranchName target = BranchName.of("target"); store().create(source, Optional.of(this.initialHash)); @@ -1464,8 +1191,6 @@ protected void mergeEmptyCommit(boolean individualCommits, boolean dryRun) .fromHash(sourceHead) .toBranch(target) .expectedHash(Optional.ofNullable(targetHead)) - .updateCommitMetadata(createMetadataRewriter(", merge-drop")) - .keepIndividualCommits(individualCommits) .putMergeKeyBehaviors(key1, MergeKeyBehavior.of(key1, MergeBehavior.DROP)) .putMergeKeyBehaviors(key2, MergeKeyBehavior.of(key2, MergeBehavior.DROP)) .dryRun(dryRun) @@ -1505,30 +1230,53 @@ public void mergeFromAndIntoHead(boolean dryRun) throws Exception { singletonList(Put.of(key2, VALUE_2))) .getCommitHash(); - soft.assertThatIllegalArgumentException() - .isThrownBy( - () -> - store() - .merge( - MergeOp.builder() - .fromRef(branch) - .fromHash(commit2) - .toBranch(branch) - .expectedHash(Optional.of(commit1)) - .dryRun(dryRun) - .build())); + if (isNewStorageModel()) { + // New storage model allows "merging the same branch again". If nothing changed, it returns a + // successful, but not-applied merge-response. This request is a merge without any commits to + // merge, reported as "successful". + soft.assertThat( + store() + .merge( + MergeOp.builder() + .fromRef(branch) + .fromHash(commit2) + .toBranch(branch) + .expectedHash(Optional.of(commit1)) + .dryRun(dryRun) + .build())) + .extracting( + MergeResult::wasApplied, + MergeResult::wasSuccessful, + MergeResult::getResultantTargetHash, + MergeResult::getCommonAncestor, + MergeResult::getEffectiveTargetHash) + .containsExactly(false, true, commit2, commit2, commit2); + } else { + soft.assertThatIllegalArgumentException() + .isThrownBy( + () -> + store() + .merge( + MergeOp.builder() + .fromRef(branch) + .fromHash(commit2) + .toBranch(branch) + .expectedHash(Optional.of(commit1)) + .dryRun(dryRun) + .build())); - soft.assertThatIllegalArgumentException() - .isThrownBy( - () -> - store() - .merge( - MergeOp.builder() - .fromRef(branch) - .fromHash(commit1) - .toBranch(branch) - .expectedHash(Optional.of(commit2)) - .dryRun(dryRun) - .build())); + soft.assertThatIllegalArgumentException() + .isThrownBy( + () -> + store() + .merge( + MergeOp.builder() + .fromRef(branch) + .fromHash(commit1) + .toBranch(branch) + .expectedHash(Optional.of(commit2)) + .dryRun(dryRun) + .build())); + } } } diff --git a/versioned/tests/src/main/java/org/projectnessie/versioned/tests/AbstractNamespaceValidation.java b/versioned/tests/src/main/java/org/projectnessie/versioned/tests/AbstractNamespaceValidation.java index d165f2c7d12..4b3625a8775 100644 --- a/versioned/tests/src/main/java/org/projectnessie/versioned/tests/AbstractNamespaceValidation.java +++ b/versioned/tests/src/main/java/org/projectnessie/versioned/tests/AbstractNamespaceValidation.java @@ -204,18 +204,12 @@ void preventNamespaceDeletionWithChildren(boolean childNamespace) throws Excepti } enum NamespaceValidationMergeTransplant { - MERGE_SQUASH(true, false, false, false, false), - MERGE_INDIVIDUAL(true, false, false, true, false), - MERGE_CREATE_SQUASH(true, true, false, false, false), - MERGE_CREATE_INDIVIDUAL(true, true, false, true, false), - MERGE_DELETE_SQUASH(true, false, true, false, true), - MERGE_DELETE_INDIVIDUAL(true, false, true, true, true), - TRANSPLANT_SQUASH(false, false, false, false, false), - TRANSPLANT_INDIVIDUAL(false, false, false, true, false), - TRANSPLANT_CREATE_SQUASH(true, true, false, false, false), - TRANSPLANT_CREATE_INDIVIDUAL(true, true, false, true, false), - TRANSPLANT_DELETE_SQUASH(false, false, true, false, true), - TRANSPLANT_DELETE_INDIVIDUAL(false, false, true, true, true), + MERGE(true, false, false, false), + MERGE_CREATE(true, true, false, false), + MERGE_DELETE(true, false, true, true), + TRANSPLANT(false, false, false, false), + TRANSPLANT_CREATE(true, true, false, false), + TRANSPLANT_DELETE(false, false, true, true), ; /** Whether to merge (or transplant, if false). */ @@ -230,21 +224,16 @@ enum NamespaceValidationMergeTransplant { */ final boolean deleteNamespaceOnTarget; - /** Whether merge/transplant shall keep individual commits or "squash" those. */ - final boolean individualCommits; - final boolean error; NamespaceValidationMergeTransplant( boolean merge, boolean createNamespaceOnTarget, boolean deleteNamespaceOnTarget, - boolean individualCommits, boolean error) { this.merge = merge; this.createNamespaceOnTarget = createNamespaceOnTarget; this.deleteNamespaceOnTarget = deleteNamespaceOnTarget; - this.individualCommits = individualCommits; this.error = error; } } @@ -320,7 +309,6 @@ void mergeTransplantWithCommonButRemovedNamespace(NamespaceValidationMergeTransp .fromHash( store().hashOnReference(branch, Optional.empty(), emptyList())) .toBranch(root) - .keepIndividualCommits(mode.individualCommits) .build()) : () -> store() @@ -330,7 +318,6 @@ void mergeTransplantWithCommonButRemovedNamespace(NamespaceValidationMergeTransp .toBranch(root) .addSequenceToTransplant( commit1.getCommitHash(), commit2.getCommitHash()) - .keepIndividualCommits(mode.individualCommits) .build()); if (mode.deleteNamespaceOnTarget) { diff --git a/versioned/tests/src/main/java/org/projectnessie/versioned/tests/AbstractNestedVersionStore.java b/versioned/tests/src/main/java/org/projectnessie/versioned/tests/AbstractNestedVersionStore.java index 11fedffc41c..011700866e5 100644 --- a/versioned/tests/src/main/java/org/projectnessie/versioned/tests/AbstractNestedVersionStore.java +++ b/versioned/tests/src/main/java/org/projectnessie/versioned/tests/AbstractNestedVersionStore.java @@ -17,6 +17,7 @@ import static com.google.common.collect.Lists.newArrayList; import static java.util.Collections.emptyList; +import static org.projectnessie.versioned.DefaultMetadataRewriter.DEFAULT_METADATA_REWRITER; import java.util.ArrayList; import java.util.List; @@ -34,7 +35,6 @@ import org.projectnessie.versioned.Diff; import org.projectnessie.versioned.Hash; import org.projectnessie.versioned.ImmutableCommit; -import org.projectnessie.versioned.MetadataRewriter; import org.projectnessie.versioned.Put; import org.projectnessie.versioned.Ref; import org.projectnessie.versioned.ReferenceInfo; @@ -132,10 +132,7 @@ protected boolean filterMainBranch(ReferenceInfo r) { } protected static void assertCommitMeta( - SoftAssertions soft, - List current, - List expected, - MetadataRewriter commitMetaModifier) { + SoftAssertions soft, List current, List expected) { soft.assertThat(current) .map(Commit::getCommitMeta) .map( @@ -145,7 +142,7 @@ protected static void assertCommitMeta( .containsExactlyElementsOf( expected.stream() .map(Commit::getCommitMeta) - .map(commitMetaModifier::rewriteSingle) + .map(DEFAULT_METADATA_REWRITER::rewriteSingle) .collect(Collectors.toList())); } diff --git a/versioned/tests/src/main/java/org/projectnessie/versioned/tests/AbstractNoNamespaceValidation.java b/versioned/tests/src/main/java/org/projectnessie/versioned/tests/AbstractNoNamespaceValidation.java index 89ae9125e8a..6196fe5082d 100644 --- a/versioned/tests/src/main/java/org/projectnessie/versioned/tests/AbstractNoNamespaceValidation.java +++ b/versioned/tests/src/main/java/org/projectnessie/versioned/tests/AbstractNoNamespaceValidation.java @@ -63,7 +63,7 @@ void commit() throws Exception { } @ParameterizedTest - @CsvSource({"false,false", "false,true", "true,false", "true,true"}) + @CsvSource({"false,false", "false,true", "true,true"}) void mergeTransplant(boolean merge, boolean individual) throws Exception { BranchName root = BranchName.of("root"); BranchName branch = BranchName.of("branch"); @@ -109,12 +109,7 @@ void mergeTransplant(boolean merge, boolean individual) throws Exception { if (merge) { store() .merge( - MergeOp.builder() - .fromRef(branch) - .fromHash(commit2) - .toBranch(root) - .keepIndividualCommits(individual) - .build()); + MergeOp.builder().fromRef(branch).fromHash(commit2).toBranch(root).build()); } else { store() .transplant( @@ -122,7 +117,6 @@ void mergeTransplant(boolean merge, boolean individual) throws Exception { .fromRef(branch) .toBranch(root) .addSequenceToTransplant(commit1, commit2) - .keepIndividualCommits(individual) .build()); } }) diff --git a/versioned/tests/src/main/java/org/projectnessie/versioned/tests/AbstractReferenceNotFound.java b/versioned/tests/src/main/java/org/projectnessie/versioned/tests/AbstractReferenceNotFound.java index 11833cac3e9..5b0e9535e28 100644 --- a/versioned/tests/src/main/java/org/projectnessie/versioned/tests/AbstractReferenceNotFound.java +++ b/versioned/tests/src/main/java/org/projectnessie/versioned/tests/AbstractReferenceNotFound.java @@ -30,7 +30,6 @@ import org.projectnessie.versioned.BranchName; import org.projectnessie.versioned.Delete; import org.projectnessie.versioned.Hash; -import org.projectnessie.versioned.MetadataRewriter; import org.projectnessie.versioned.ReferenceNotFoundException; import org.projectnessie.versioned.TagName; import org.projectnessie.versioned.VersionStore; @@ -76,18 +75,6 @@ interface ThrowingFunction { @SuppressWarnings("MustBeClosedChecker") static List referenceNotFoundFunctions() { - MetadataRewriter metadataRewriter = - new MetadataRewriter() { - @Override - public CommitMeta rewriteSingle(CommitMeta metadata) { - return null; - } - - @Override - public CommitMeta squash(List metadata) { - return null; - } - }; return Arrays.asList( // getCommits() new ReferenceNotFoundFunction("getCommits/branch") @@ -227,7 +214,6 @@ public CommitMeta squash(List metadata) { .addSequenceToTransplant( s.hashOnReference( BranchName.of("main"), Optional.empty(), emptyList())) - .updateCommitMetadata(metadataRewriter) .build())), new ReferenceNotFoundFunction("transplant/hash/empty") .msg( @@ -243,8 +229,6 @@ public CommitMeta squash(List metadata) { Hash.of("12341234123412341234123412341234123412341234"))) .addSequenceToTransplant( Hash.of("12341234123412341234123412341234123412341234")) - .updateCommitMetadata(metadataRewriter) - .keepIndividualCommits(true) .build())), new ReferenceNotFoundFunction("transplant/empty/hash") .msg("Commit '12341234123412341234123412341234123412341234' not found") @@ -256,7 +240,6 @@ public CommitMeta squash(List metadata) { .toBranch(BranchName.of("main")) .addSequenceToTransplant( Hash.of("12341234123412341234123412341234123412341234")) - .updateCommitMetadata(metadataRewriter) .build())), // diff() new ReferenceNotFoundFunction("diff/from-hash") @@ -305,7 +288,6 @@ public CommitMeta squash(List metadata) { .fromRef(BranchName.of("source")) .fromHash(Hash.of("12341234123412341234123412341234123412341234")) .toBranch(BranchName.of("main")) - .keepIndividualCommits(true) .build())), new ReferenceNotFoundFunction("merge/empty/hash") .msg( @@ -320,7 +302,6 @@ public CommitMeta squash(List metadata) { .expectedHash( Optional.of( Hash.of("12341234123412341234123412341234123412341234"))) - .keepIndividualCommits(true) .build())), new ReferenceNotFoundFunction("merge/hash/empty") .msg("Commit '12341234123412341234123412341234123412341234' not found") diff --git a/versioned/tests/src/main/java/org/projectnessie/versioned/tests/AbstractTransplant.java b/versioned/tests/src/main/java/org/projectnessie/versioned/tests/AbstractTransplant.java index 06168fc175d..ed9abde9f64 100644 --- a/versioned/tests/src/main/java/org/projectnessie/versioned/tests/AbstractTransplant.java +++ b/versioned/tests/src/main/java/org/projectnessie/versioned/tests/AbstractTransplant.java @@ -25,14 +25,13 @@ import java.util.Arrays; import java.util.List; import java.util.Optional; -import java.util.stream.Collectors; import org.assertj.core.api.SoftAssertions; import org.assertj.core.api.junit.jupiter.InjectSoftAssertions; import org.assertj.core.api.junit.jupiter.SoftAssertionsExtension; import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; import org.junit.jupiter.params.ParameterizedTest; -import org.junit.jupiter.params.provider.CsvSource; import org.junit.jupiter.params.provider.ValueSource; import org.projectnessie.model.CommitMeta; import org.projectnessie.model.Content; @@ -44,7 +43,6 @@ import org.projectnessie.versioned.Delete; import org.projectnessie.versioned.Hash; import org.projectnessie.versioned.MergeResult; -import org.projectnessie.versioned.MetadataRewriter; import org.projectnessie.versioned.Put; import org.projectnessie.versioned.ReferenceConflictException; import org.projectnessie.versioned.ReferenceNotFoundException; @@ -86,23 +84,6 @@ protected AbstractTransplant(VersionStore store) { private Hash thirdCommit; private List commits; - private MetadataRewriter createMetadataRewriter(String suffix) { - return new MetadataRewriter() { - @Override - public CommitMeta rewriteSingle(CommitMeta metadata) { - return CommitMeta.fromMessage(metadata.getMessage()); - } - - @Override - public CommitMeta squash(List metadata) { - return CommitMeta.fromMessage( - metadata.stream() - .map(cm -> cm.getMessage() + suffix) - .collect(Collectors.joining("\n-----------------------------------\n"))); - } - }; - } - @BeforeEach protected void setupCommits() throws VersionStoreException { store().create(sourceBranch, Optional.empty()); @@ -131,39 +112,10 @@ protected void setupCommits() throws VersionStoreException { commits = commitsList(sourceBranch, false); } - @ParameterizedTest - @ValueSource(booleans = {false, true}) - protected void checkTransplantOnEmptyBranch(boolean individualCommits) - throws VersionStoreException { - checkTransplantOnEmptyBranch(createMetadataRewriter(""), individualCommits); - } - - @ParameterizedTest - @ValueSource(booleans = {false, true}) - protected void checkTransplantOnEmptyBranchModify(boolean individualCommits) - throws VersionStoreException { - BranchName newBranch = - checkTransplantOnEmptyBranch(createMetadataRewriter(""), individualCommits); - - if (!individualCommits) { - assertThat(commitsList(newBranch, false)) - .first() - .extracting(Commit::getCommitMeta) - .extracting(CommitMeta::getMessage) - .asString() - .contains( - commits.stream() - .map(Commit::getCommitMeta) - .map(CommitMeta::getMessage) - .toArray(String[]::new)); - } - } - - private BranchName checkTransplantOnEmptyBranch( - MetadataRewriter commitMetaModify, boolean individualCommits) - throws VersionStoreException { + @Test + protected void checkTransplantOnEmptyBranch() throws VersionStoreException { final BranchName newBranch = BranchName.of("bar_1"); - Hash targetHead = store().create(newBranch, Optional.empty()).getHash(); + store().create(newBranch, Optional.empty()).getHash(); MergeResult result = store() @@ -173,15 +125,9 @@ private BranchName checkTransplantOnEmptyBranch( .toBranch(newBranch) .expectedHash(Optional.of(initialHash)) .addSequenceToTransplant(firstCommit, secondCommit, thirdCommit) - .updateCommitMetadata(commitMetaModify) - .keepIndividualCommits(individualCommits) .build()); - if (individualCommits) { - soft.assertThat(result.getCreatedCommits()).isEmpty(); // fast-forward - } else { - checkSquashedCommit(targetHead, result); - } + soft.assertThat(result.getCreatedCommits()).isEmpty(); // fast-forward soft.assertThat( contentsWithoutId( @@ -199,18 +145,11 @@ private BranchName checkTransplantOnEmptyBranch( ContentKey.of(T_2), V_2_2, ContentKey.of(T_4), V_4_1)); - if (individualCommits) { - assertCommitMeta( - soft, commitsList(newBranch, false).subList(0, 3), commits, commitMetaModify); - } - - return newBranch; + assertCommitMeta(soft, commitsList(newBranch, false).subList(0, 3), commits); } - @ParameterizedTest - @ValueSource(booleans = {false, true}) - protected void checkTransplantWithPreviousCommit(boolean individualCommits) - throws VersionStoreException { + @Test + protected void checkTransplantWithPreviousCommit() throws VersionStoreException { final BranchName newBranch = BranchName.of("bar_2"); store().create(newBranch, Optional.empty()); Hash targetHead = commit("Unrelated commit").put(T_5, V_5_1).toBranch(newBranch); @@ -223,77 +162,8 @@ protected void checkTransplantWithPreviousCommit(boolean individualCommits) .toBranch(newBranch) .expectedHash(Optional.of(initialHash)) .addSequenceToTransplant(firstCommit, secondCommit, thirdCommit) - .updateCommitMetadata(createMetadataRewriter("")) - .keepIndividualCommits(individualCommits) .build()); - if (individualCommits) { - checkRebasedCommits(targetHead, result); // no fast-forward - } else { - checkSquashedCommit(targetHead, result); - } - assertThat( - contentsWithoutId( - store() - .getValues( - newBranch, - Arrays.asList( - ContentKey.of(T_1), - ContentKey.of(T_2), - ContentKey.of(T_3), - ContentKey.of(T_4), - ContentKey.of(T_5))))) - .containsExactlyInAnyOrderEntriesOf( - ImmutableMap.of( - ContentKey.of(T_1), V_1_2, - ContentKey.of(T_2), V_2_2, - ContentKey.of(T_4), V_4_1, - ContentKey.of(T_5), V_5_1)); - } - - private void checkSquashedCommit(Hash targetHead, MergeResult result) { - soft.assertThat(result.getCreatedCommits()) - .singleElement() - .satisfies( - c -> { - soft.assertThat(c.getParentHash()).isEqualTo(targetHead); - soft.assertThat(c.getCommitMeta().getMessage()) - .contains("Initial Commit") - .contains("Second Commit") - .contains("Third Commit"); - soft.assertThat(c.getOperations()) - // old storage model keeps the Delete operation when a key is Put then Deleted, - // see https://github.com/projectnessie/nessie/pull/6472 - .hasSizeBetween(3, 4) - .anySatisfy( - o -> { - if (c.getOperations() != null && c.getOperations().size() == 4) { - soft.assertThat(o).isInstanceOf(Delete.class); - soft.assertThat(o.getKey()).isEqualTo(ContentKey.of("t3")); - } - }) - .anySatisfy( - o -> { - soft.assertThat(o).isInstanceOf(Put.class); - soft.assertThat(o.getKey()).isEqualTo(ContentKey.of(T_1)); - soft.assertThat(contentWithoutId(((Put) o).getValue())).isEqualTo(V_1_2); - }) - .anySatisfy( - o -> { - soft.assertThat(o).isInstanceOf(Put.class); - soft.assertThat(o.getKey()).isEqualTo(ContentKey.of(T_2)); - soft.assertThat(contentWithoutId(((Put) o).getValue())).isEqualTo(V_2_2); - }) - .anySatisfy( - o -> { - soft.assertThat(o).isInstanceOf(Put.class); - soft.assertThat(o.getKey()).isEqualTo(ContentKey.of(T_4)); - soft.assertThat(contentWithoutId(((Put) o).getValue())).isEqualTo(V_4_1); - }); - }); - } - - private void checkRebasedCommits(Hash targetHead, MergeResult result) { soft.assertThat(result.getCreatedCommits()) .hasSize(3) .satisfiesExactly( @@ -362,17 +232,29 @@ private void checkRebasedCommits(Hash targetHead, MergeResult result) { // Unchanged operation not retained ); }); + + assertThat( + contentsWithoutId( + store() + .getValues( + newBranch, + Arrays.asList( + ContentKey.of(T_1), + ContentKey.of(T_2), + ContentKey.of(T_3), + ContentKey.of(T_4), + ContentKey.of(T_5))))) + .containsExactlyInAnyOrderEntriesOf( + ImmutableMap.of( + ContentKey.of(T_1), V_1_2, + ContentKey.of(T_2), V_2_2, + ContentKey.of(T_4), V_4_1, + ContentKey.of(T_5), V_5_1)); } @ParameterizedTest - @CsvSource({ - "false,false", - "false,true", - "true,false", - "true,true", - }) - protected void checkTransplantWithConflictingCommit(boolean individualCommits, boolean dryRun) - throws VersionStoreException { + @ValueSource(booleans = {false, true}) + protected void checkTransplantWithConflictingCommit(boolean dryRun) throws VersionStoreException { final BranchName newBranch = BranchName.of("bar_3"); store().create(newBranch, Optional.empty()); commit("Another commit").put(T_1, V_1_4).toBranch(newBranch); @@ -387,8 +269,6 @@ protected void checkTransplantWithConflictingCommit(boolean individualCommits, b .toBranch(newBranch) .expectedHash(Optional.of(initialHash)) .addSequenceToTransplant(firstCommit, secondCommit, thirdCommit) - .updateCommitMetadata(createMetadataRewriter("")) - .keepIndividualCommits(individualCommits) .dryRun(dryRun) .build())) .isInstanceOf(ReferenceConflictException.class); @@ -397,9 +277,8 @@ protected void checkTransplantWithConflictingCommit(boolean individualCommits, b } } - @ParameterizedTest - @ValueSource(booleans = {false, true}) - protected void checkTransplantWithDelete(boolean individualCommits) throws VersionStoreException { + @Test + protected void checkTransplantWithDelete() throws VersionStoreException { final BranchName newBranch = BranchName.of("bar_4"); store().create(newBranch, Optional.empty()); commit("Another commit").put(T_1, V_1_4).toBranch(newBranch); @@ -412,8 +291,6 @@ protected void checkTransplantWithDelete(boolean individualCommits) throws Versi .toBranch(newBranch) .expectedHash(Optional.of(initialHash)) .addSequenceToTransplant(firstCommit, secondCommit, thirdCommit) - .updateCommitMetadata(createMetadataRewriter("")) - .keepIndividualCommits(individualCommits) .build()); soft.assertThat( contentsWithoutId( @@ -433,13 +310,8 @@ protected void checkTransplantWithDelete(boolean individualCommits) throws Versi } @ParameterizedTest - @CsvSource({ - "false,false", - "false,true", - "true,false", - "true,true", - }) - protected void checkTransplantOnNonExistingBranch(boolean individualCommits, boolean dryRun) { + @ValueSource(booleans = {false, true}) + protected void checkTransplantOnNonExistingBranch(boolean dryRun) { final BranchName newBranch = BranchName.of("bar_5"); StorageAssertions checkpoint = storageCheckpoint(); soft.assertThatThrownBy( @@ -451,8 +323,6 @@ protected void checkTransplantOnNonExistingBranch(boolean individualCommits, boo .toBranch(newBranch) .expectedHash(Optional.of(initialHash)) .addSequenceToTransplant(firstCommit, secondCommit, thirdCommit) - .updateCommitMetadata(createMetadataRewriter("")) - .keepIndividualCommits(individualCommits) .dryRun(dryRun) .build())) .isInstanceOf(ReferenceNotFoundException.class); @@ -460,14 +330,8 @@ protected void checkTransplantOnNonExistingBranch(boolean individualCommits, boo } @ParameterizedTest - @CsvSource({ - "false,false", - "false,true", - "true,false", - "true,true", - }) - protected void checkTransplantWithNonExistingCommit(boolean individualCommits, boolean dryRun) - throws VersionStoreException { + @ValueSource(booleans = {false, true}) + protected void checkTransplantWithNonExistingCommit(boolean dryRun) throws VersionStoreException { final BranchName newBranch = BranchName.of("bar_6"); store().create(newBranch, Optional.empty()); StorageAssertions checkpoint = storageCheckpoint(); @@ -480,18 +344,14 @@ protected void checkTransplantWithNonExistingCommit(boolean individualCommits, b .toBranch(newBranch) .expectedHash(Optional.of(initialHash)) .addSequenceToTransplant(Hash.of("1234567890abcdef")) - .updateCommitMetadata(createMetadataRewriter("")) - .keepIndividualCommits(individualCommits) .dryRun(dryRun) .build())) .isInstanceOf(ReferenceNotFoundException.class); checkpoint.assertNoWrites(); } - @ParameterizedTest - @ValueSource(booleans = {false, true}) - protected void checkTransplantWithNoExpectedHash(boolean individualCommits) - throws VersionStoreException { + @Test + protected void checkTransplantWithNoExpectedHash() throws VersionStoreException { final BranchName newBranch = BranchName.of("bar_7"); store().create(newBranch, Optional.empty()); commit("Another commit").put(T_5, V_5_1).toBranch(newBranch); @@ -504,8 +364,6 @@ protected void checkTransplantWithNoExpectedHash(boolean individualCommits) .fromRef(sourceBranch) .toBranch(newBranch) .addSequenceToTransplant(firstCommit, secondCommit, thirdCommit) - .updateCommitMetadata(createMetadataRewriter("")) - .keepIndividualCommits(individualCommits) .build()); soft.assertThat( contentsWithoutId( @@ -527,13 +385,8 @@ protected void checkTransplantWithNoExpectedHash(boolean individualCommits) } @ParameterizedTest - @CsvSource({ - "false,false", - "false,true", - "true,false", - "true,true", - }) - protected void checkTransplantWithCommitsInWrongOrder(boolean individualCommits, boolean dryRun) + @ValueSource(booleans = {false, true}) + protected void checkTransplantWithCommitsInWrongOrder(boolean dryRun) throws VersionStoreException { final BranchName newBranch = BranchName.of("bar_8"); store().create(newBranch, Optional.empty()); @@ -547,21 +400,13 @@ protected void checkTransplantWithCommitsInWrongOrder(boolean individualCommits, .fromRef(sourceBranch) .toBranch(newBranch) .addSequenceToTransplant(secondCommit, firstCommit, thirdCommit) - .updateCommitMetadata(createMetadataRewriter("")) - .keepIndividualCommits(individualCommits) .dryRun(dryRun) .build())); } @ParameterizedTest - @CsvSource({ - "false,false", - "false,true", - "true,false", - "true,true", - }) - protected void checkInvalidBranchHash(boolean individualCommits, boolean dryRun) - throws VersionStoreException { + @ValueSource(booleans = {false, true}) + protected void checkInvalidBranchHash(boolean dryRun) throws VersionStoreException { final BranchName anotherBranch = BranchName.of("bar"); store().create(anotherBranch, Optional.empty()); final Hash unrelatedCommit = @@ -584,17 +429,14 @@ protected void checkInvalidBranchHash(boolean individualCommits, boolean dryRun) .toBranch(newBranch) .expectedHash(Optional.of(unrelatedCommit)) .addSequenceToTransplant(firstCommit, secondCommit, thirdCommit) - .updateCommitMetadata(createMetadataRewriter("")) - .keepIndividualCommits(individualCommits) .dryRun(dryRun) .build())) .isInstanceOf(ReferenceNotFoundException.class); checkpoint.assertNoWrites(); } - @ParameterizedTest - @ValueSource(booleans = {false, true}) - protected void transplantBasic(boolean individualCommits) throws VersionStoreException { + @Test + protected void transplantBasic() throws VersionStoreException { final BranchName newBranch = BranchName.of("bar_2"); store().create(newBranch, Optional.empty()); Hash targetHead = commit("Unrelated commit").put(T_5, V_5_1).toBranch(newBranch); @@ -607,34 +449,20 @@ protected void transplantBasic(boolean individualCommits) throws VersionStoreExc .toBranch(newBranch) .expectedHash(Optional.of(initialHash)) .addSequenceToTransplant(firstCommit, secondCommit) - .updateCommitMetadata(createMetadataRewriter("")) - .keepIndividualCommits(individualCommits) .build()); - if (individualCommits) { - soft.assertThat(result.getCreatedCommits()) - .hasSize(2) - .satisfiesExactly( - c -> { - soft.assertThat(c.getParentHash()).isEqualTo(targetHead); - soft.assertThat(c.getCommitMeta().getMessage()).isEqualTo("Initial Commit"); - }, - c -> { - soft.assertThat(c.getParentHash()) - .isEqualTo(result.getCreatedCommits().get(0).getHash()); - soft.assertThat(c.getCommitMeta().getMessage()).isEqualTo("Second Commit"); - }); - } else { - soft.assertThat(result.getCreatedCommits()) - .singleElement() - .satisfies( - c -> { - soft.assertThat(c.getParentHash()).isEqualTo(targetHead); - soft.assertThat(c.getCommitMeta().getMessage()) - .contains("Initial Commit") - .contains("Second Commit"); - }); - } + soft.assertThat(result.getCreatedCommits()) + .hasSize(2) + .satisfiesExactly( + c -> { + soft.assertThat(c.getParentHash()).isEqualTo(targetHead); + soft.assertThat(c.getCommitMeta().getMessage()).isEqualTo("Initial Commit"); + }, + c -> { + soft.assertThat(c.getParentHash()) + .isEqualTo(result.getCreatedCommits().get(0).getHash()); + soft.assertThat(c.getCommitMeta().getMessage()).isEqualTo("Second Commit"); + }); soft.assertThat( contentsWithoutId( store() @@ -648,9 +476,8 @@ protected void transplantBasic(boolean individualCommits) throws VersionStoreExc ContentKey.of(T_5), V_5_1)); } - @ParameterizedTest - @ValueSource(booleans = {false, true}) - protected void transplantEmptyCommit(boolean individualCommits) throws VersionStoreException { + @Test + protected void transplantEmptyCommit() throws VersionStoreException { BranchName source = BranchName.of("source"); BranchName target = BranchName.of("target"); store().create(source, Optional.of(this.initialHash)); @@ -708,8 +535,6 @@ protected void transplantEmptyCommit(boolean individualCommits) throws VersionSt .toBranch(target) .expectedHash(Optional.of(targetHead)) .addSequenceToTransplant(source1, source2) - .updateCommitMetadata(createMetadataRewriter(", merge-drop")) - .keepIndividualCommits(individualCommits) .putMergeKeyBehaviors(key1, MergeKeyBehavior.of(key1, MergeBehavior.DROP)) .putMergeKeyBehaviors(key2, MergeKeyBehavior.of(key2, MergeBehavior.DROP)) .build());