Skip to content

Commit

Permalink
Don't feed added commits for fast-forwards
Browse files Browse the repository at this point in the history
  • Loading branch information
@adutra
adutra committed May 3, 2023
1 parent 7177891 commit ba74459
Show file tree
Hide file tree
Showing 5 changed files with 20 additions and 106 deletions.
10 changes: 6 additions & 4 deletions ...rc/main/java/org/projectnessie/versioned/persist/adapter/spi/AbstractDatabaseAdapter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -626,10 +626,10 @@ protected Hash mergeTransplantCommon(
newKeyLists,
params.getUpdateCommitMetadata(),
mergePredicate,
additionalParents);
additionalParents,
addedCommits);

if (squashed != null) {
addedCommits.accept(squashed);
if (!params.isDryRun()) {
writtenCommits.accept(squashed);
toHead = squashed.getHash();
Expand Down Expand Up @@ -2109,7 +2109,8 @@ protected CommitLogEntry squashCommits(
Consumer<Hash> newKeyLists,
MetadataRewriter<ByteString> rewriteMetadata,
Predicate<ContentKey> includeKeyPredicate,
Iterable<Hash> additionalParents)
Iterable<Hash> additionalParents,
Consumer<CommitLogEntry> addedCommits)
throws ReferenceConflictException, ReferenceNotFoundException {

List<ByteString> commitMeta = new ArrayList<>();
Expand Down Expand Up @@ -2183,6 +2184,7 @@ protected CommitLogEntry squashCommits(
}

writeIndividualCommit(ctx, squashedCommit);
addedCommits.accept(squashedCommit);

return squashedCommit;
}
Expand Down Expand Up @@ -2263,13 +2265,13 @@ protected Hash copyCommits(

if (!newEntry.getHash().equals(sourceCommit.getHash())) {
commitsChronological.set(i, newEntry);
addedCommits.accept(newEntry);
} else {
// Newly built CommitLogEntry is equal to the CommitLogEntry to transplant.
// This can happen, if the commit to transplant has NO_ANCESTOR as its parent.
commitsChronological.remove(i);
}

addedCommits.accept(newEntry);
targetHead = newEntry.getHash();
}
return targetHead;
Expand Down
2 changes: 0 additions & 2 deletions ...tore/src/main/java/org/projectnessie/versioned/storage/versionstore/BaseCommitHelper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -519,8 +519,6 @@ ImmutableMergeResult<Commit> mergeSquashFastForward(
result.wasApplied(true).resultantTargetHash(objIdToHash(fromId));
}

result.addAddedCommits(commitObjToCommit(source));

return result.build();
}

Expand Down
12 changes: 3 additions & 9 deletions .../java/org/projectnessie/versioned/storage/versionstore/BaseMergeTransplantIndividual.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,6 @@
import org.projectnessie.versioned.ReferenceNotFoundException;
import org.projectnessie.versioned.storage.batching.BatchingPersist;
import org.projectnessie.versioned.storage.batching.WriteBatching;
import org.projectnessie.versioned.storage.common.exceptions.ObjNotFoundException;
import org.projectnessie.versioned.storage.common.indexes.StoreIndex;
import org.projectnessie.versioned.storage.common.indexes.StoreIndexElement;
import org.projectnessie.versioned.storage.common.logic.CommitLogic;
Expand Down Expand Up @@ -121,18 +120,13 @@ MergeResult<Commit> individualCommits(

empty = false;
if (!dryRun) {
commitLogic.storeCommit(newCommit, objsToStore);
newHead = newCommit.id();
try {
// We need to fetch the existing commit in order to return the correct object.
// The existing commit may have a different set of parents than the new commit.
newCommit = commitLogic.fetchCommit(newCommit.id());
} catch (ObjNotFoundException ignored) {
// cannot happen
boolean committed = commitLogic.storeCommit(newCommit, objsToStore);
if (committed) {
mergeResult.addAddedCommits(commitObjToCommit(newCommit));
}
}

mergeResult.addAddedCommits(commitObjToCommit(newCommit));
sourceParentIndex = indexesLogic.buildCompleteIndex(sourceCommit, Optional.empty());
targetParentIndex = indexesLogic.buildCompleteIndex(newCommit, Optional.empty());
}
Expand Down
12 changes: 3 additions & 9 deletions ...main/java/org/projectnessie/versioned/storage/versionstore/BaseMergeTransplantSquash.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@
import org.projectnessie.versioned.MetadataRewriter;
import org.projectnessie.versioned.ReferenceConflictException;
import org.projectnessie.versioned.ReferenceNotFoundException;
import org.projectnessie.versioned.storage.common.exceptions.ObjNotFoundException;
import org.projectnessie.versioned.storage.common.indexes.StoreIndex;
import org.projectnessie.versioned.storage.common.indexes.StoreKey;
import org.projectnessie.versioned.storage.common.logic.CommitLogic;
Expand Down Expand Up @@ -105,18 +104,13 @@ MergeResult<Commit> squash(
newHead = headId();
} else {
CommitLogic commitLogic = commitLogic(persist);
commitLogic.storeCommit(mergeCommit, objsToStore);
newHead = mergeCommit.id();
try {
// We need to fetch the existing commit in order to return the correct object.
// The existing commit may have a different set of parents than the new commit.
mergeCommit = commitLogic.fetchCommit(mergeCommit.id());
} catch (ObjNotFoundException ignored) {
// cannot happen
boolean committed = commitLogic.storeCommit(mergeCommit, objsToStore);
if (committed) {
mergeResult.addAddedCommits(commitObjToCommit(mergeCommit));
}
}

mergeResult.addAddedCommits(commitObjToCommit(mergeCommit));
return finishMergeTransplant(false, mergeResult, newHead, dryRun, hasConflicts);
}

Expand Down
90 changes: 8 additions & 82 deletions versioned/tests/src/main/java/org/projectnessie/versioned/tests/AbstractMerge.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -473,7 +473,7 @@ private void doMergeIntoEmpty(
false,
false);

checkTargetCommits(individualCommits, initialHash, result);
checkAddedCommits(individualCommits, initialHash, result);
soft.assertThat(
contentsWithoutId(
store()
Expand All @@ -491,77 +491,10 @@ private void doMergeIntoEmpty(
ContentKey.of("t4"), V_4_1));
}

private void checkTargetCommits(
private void checkAddedCommits(
boolean individualCommits, Hash targetHead, MergeResult<Commit> result) {
if (individualCommits) {
soft.assertThat(result.getAddedCommits())
.hasSize(3)
.satisfiesExactly(
c -> {
soft.assertThat(c.getParentHash()).isEqualTo(targetHead);
soft.assertThat(c.getCommitMeta().getMessage()).contains("First Commit");
soft.assertThat(c.getOperations())
.hasSize(3)
.satisfiesExactlyInAnyOrder(
o -> {
soft.assertThat(o).isInstanceOf(Put.class);
soft.assertThat(o.getKey()).isEqualTo(ContentKey.of("t1"));
soft.assertThat(contentWithoutId(((Put) o).getValue())).isEqualTo(V_1_1);
},
o -> {
soft.assertThat(o).isInstanceOf(Put.class);
soft.assertThat(o.getKey()).isEqualTo(ContentKey.of("t2"));
soft.assertThat(contentWithoutId(((Put) o).getValue())).isEqualTo(V_2_1);
},
o -> {
soft.assertThat(o).isInstanceOf(Put.class);
soft.assertThat(o.getKey()).isEqualTo(ContentKey.of("t3"));
soft.assertThat(contentWithoutId(((Put) o).getValue())).isEqualTo(V_3_1);
});
},
c -> {
soft.assertThat(c.getParentHash())
.isEqualTo(result.getAddedCommits().get(0).getHash());
soft.assertThat(c.getCommitMeta().getMessage()).contains("Second Commit");
soft.assertThat(c.getOperations())
.hasSize(4)
.satisfiesExactlyInAnyOrder(
o -> {
soft.assertThat(o).isInstanceOf(Put.class);
soft.assertThat(o.getKey()).isEqualTo(ContentKey.of("t1"));
soft.assertThat(contentWithoutId(((Put) o).getValue())).isEqualTo(V_1_2);
},
o ->
soft.assertThat(o)
.asInstanceOf(type(Delete.class))
.extracting(Delete::getKey)
.isEqualTo(ContentKey.of("t2")),
o ->
soft.assertThat(o)
.asInstanceOf(type(Delete.class))
.extracting(Delete::getKey)
.isEqualTo(ContentKey.of("t3")),
o -> {
soft.assertThat(o).isInstanceOf(Put.class);
soft.assertThat(o.getKey()).isEqualTo(ContentKey.of("t4"));
soft.assertThat(contentWithoutId(((Put) o).getValue())).isEqualTo(V_4_1);
});
},
c -> {
soft.assertThat(c.getParentHash())
.isEqualTo(result.getAddedCommits().get(1).getHash());
soft.assertThat(c.getCommitMeta().getMessage()).contains("Third Commit");
soft.assertThat(c.getOperations())
.hasSize(1)
.satisfiesExactlyInAnyOrder(
o -> {
soft.assertThat(o).isInstanceOf(Put.class);
soft.assertThat(o.getKey()).isEqualTo(ContentKey.of("t2"));
soft.assertThat(contentWithoutId(((Put) o).getValue())).isEqualTo(V_2_2);
}
// Unchanged operation not retained
);
});
soft.assertThat(result.getAddedCommits()).isEmpty(); // fast-forward
} else {
soft.assertThat(result.getAddedCommits())
.singleElement()
Expand Down Expand Up @@ -680,12 +613,8 @@ void compareDryAndEffectiveMergeResults(boolean individualCommits) throws Versio
Put.of(ContentKey.of("t2"), V_2_1),
Put.of(ContentKey.of("t3"), V_3_1)))));
soft.assertThat(mergeResult.getTargetCommits()).isNull();
// The branch was fast-forwarded to firstCommit, so the only added commit is firstCommit
// itself
soft.assertThat(mergeResult.getAddedCommits())
.singleElement()
.extracting("hash")
.isEqualTo(firstCommit);
// The branch was fast-forwarded to firstCommit, so no commits added
soft.assertThat(mergeResult.getAddedCommits()).isEmpty();
soft.assertThat(mergeResult.getDetails())
.containsKeys(ContentKey.of("t1"), ContentKey.of("t2"), ContentKey.of("t3"));
soft.assertThat(mergeResult)
Expand Down Expand Up @@ -754,10 +683,7 @@ protected void mergeIntoEmptyBranch1Commit(boolean individualCommits)

// not modifying commit meta, will just "fast forward"
soft.assertThat(store().hashOnReference(newBranch, Optional.empty())).isEqualTo(firstCommit);
soft.assertThat(result.getAddedCommits())
.singleElement()
.extracting(Commit::getParentHash, Commit::getHash)
.containsExactly(initialHash, firstCommit);
soft.assertThat(result.getAddedCommits()).isEmpty(); // no new commits

List<Commit> mergedCommit = commitsList(newBranch, false).subList(0, 1);
assertCommitMeta(soft, mergedCommit, commits.subList(2, 3), metadataRewriter);
Expand Down Expand Up @@ -955,7 +881,7 @@ protected void nonEmptyFastForwardMerge(boolean individualCommits) throws Versio
false,
false);
soft.assertThat(mergeResult1.getResultantTargetHash()).isEqualTo(etl1.getCommitHash());
soft.assertThat(mergeResult1.getAddedCommits()).containsExactly(etl1.getCommit());
soft.assertThat(mergeResult1.getAddedCommits()).isEmpty(); // fast-forward, so nothing added

CommitResult<Commit> etl2 =
store()
Expand All @@ -978,7 +904,7 @@ protected void nonEmptyFastForwardMerge(boolean individualCommits) throws Versio
false,
false);
soft.assertThat(mergeResult2.getResultantTargetHash()).isEqualTo(etl2.getCommitHash());
soft.assertThat(mergeResult2.getAddedCommits()).containsExactly(etl2.getCommit());
soft.assertThat(mergeResult2.getAddedCommits()).isEmpty(); // fast-forward, so nothing added

soft.assertThat(contentWithoutId(store().getValue(review, key))).isEqualTo(VALUE_2);
}
Expand Down

0 comments on commit ba74459

Please sign in to comment.