ResolveMerger: choose OURS on gitlink when ignoreConflicts

Option ignoreConflicts is used when a caller want to create a virtual
commit and use it in a future merge (recursive merge) or show it on
UI (e.g. Gerrit). According to contract in case of ignoreConflicts
ResolveMerger should populate only stage 0 for entries with merge
conflicts as if there is no conflict. Current implementation breaks
this contract for cases when gitlink revision is ambiguous.

Therefore, always select 'ours' when we merge in ignoreConflicts mode.
This will satisfy the contract contract, so recursive merge can
succeed, however it is an arbitrary decision, so it is not guaranteed
to select best GITLINK in all cases.

GITLINK merging is a special case of recursive merge because of
limitations of GITLINK type of entry. It can't contain more than 1 sha-1
so jgit can't write merge conflicts in place like it can with a blob.
Ideally we could signal the conflict with a special value (like
'0000...'), but that must be supported by all tooling (git fsck, c-git)."

Signed-off-by: Demetr Starshov <dstarshov@google.com>
Change-Id: Id4e9bebc8e828f7a1ef9f83259159137df477d89
This commit is contained in:
Demetr Starshov 2020-08-12 14:47:15 -07:00
parent 29e998a0e6
commit c084729f79
2 changed files with 11 additions and 14 deletions

View File

@ -27,7 +27,6 @@
import org.eclipse.jgit.lib.PersonIdent; import org.eclipse.jgit.lib.PersonIdent;
import org.eclipse.jgit.test.resources.SampleDataRepositoryTestCase; import org.eclipse.jgit.test.resources.SampleDataRepositoryTestCase;
import org.eclipse.jgit.treewalk.TreeWalk; import org.eclipse.jgit.treewalk.TreeWalk;
import org.junit.Ignore;
import org.junit.Test; import org.junit.Test;
public class GitlinkMergeTest extends SampleDataRepositoryTestCase { public class GitlinkMergeTest extends SampleDataRepositoryTestCase {
@ -63,7 +62,6 @@ public void testGitLinkMerging_AddNew_ignoreConflicts() throws Exception {
} }
@Test @Test
@Ignore("broken - doesn't ignore conflicts")
public void testGitLinkMerging_UpdateUpdate_ignoreConflicts() public void testGitLinkMerging_UpdateUpdate_ignoreConflicts()
throws Exception { throws Exception {
assertGitLinkValue(testGitLink(LINK_ID1, LINK_ID2, LINK_ID3, assertGitLinkValue(testGitLink(LINK_ID1, LINK_ID2, LINK_ID3,
@ -71,7 +69,6 @@ public void testGitLinkMerging_UpdateUpdate_ignoreConflicts()
} }
@Test @Test
@Ignore("broken - doesn't ignore conflicts")
public void testGitLinkMerging_bothAddedSameLink_ignoreConflicts() public void testGitLinkMerging_bothAddedSameLink_ignoreConflicts()
throws Exception { throws Exception {
assertGitLinkValue(testGitLink(null, LINK_ID2, LINK_ID2, assertGitLinkValue(testGitLink(null, LINK_ID2, LINK_ID2,
@ -79,7 +76,6 @@ public void testGitLinkMerging_bothAddedSameLink_ignoreConflicts()
} }
@Test @Test
@Ignore("broken - doesn't ignore conflicts")
public void testGitLinkMerging_bothAddedDifferentLink_ignoreConflicts() public void testGitLinkMerging_bothAddedDifferentLink_ignoreConflicts()
throws Exception { throws Exception {
assertGitLinkValue(testGitLink(null, LINK_ID2, LINK_ID3, assertGitLinkValue(testGitLink(null, LINK_ID2, LINK_ID3,
@ -222,7 +218,6 @@ public void testGitLinkMerging_linkWithLink() throws Exception {
} }
@Test @Test
@Ignore("broken - try to do content-merge with GITLINK")
public void testGitLinkMerging_blobWithBlobFromLink() throws Exception { public void testGitLinkMerging_blobWithBlobFromLink() throws Exception {
DirCache treeB = db.readDirCache(); DirCache treeB = db.readDirCache();
DirCache treeO = db.readDirCache(); DirCache treeO = db.readDirCache();

View File

@ -588,7 +588,8 @@ protected boolean processEntry(CanonicalTreeParser base,
final int modeO = tw.getRawMode(T_OURS); final int modeO = tw.getRawMode(T_OURS);
final int modeT = tw.getRawMode(T_THEIRS); final int modeT = tw.getRawMode(T_THEIRS);
final int modeB = tw.getRawMode(T_BASE); final int modeB = tw.getRawMode(T_BASE);
boolean gitLinkMerging = isGitLink(modeO) || isGitLink(modeT)
|| isGitLink(modeB);
if (modeO == 0 && modeT == 0 && modeB == 0) if (modeO == 0 && modeT == 0 && modeB == 0)
// File is either untracked or new, staged but uncommitted // File is either untracked or new, staged but uncommitted
return true; return true;
@ -737,20 +738,20 @@ protected boolean processEntry(CanonicalTreeParser base,
return false; return false;
} }
boolean gitlinkConflict = isGitLink(modeO) || isGitLink(modeT); if (gitLinkMerging && ignoreConflicts) {
// Don't attempt to resolve submodule link conflicts // Always select 'ours' in case of GITLINK merge failures so
if (gitlinkConflict) { // a caller can use virtual commit.
add(tw.getRawPath(), ours, DirCacheEntry.STAGE_0, EPOCH, 0);
return true;
} else if (gitLinkMerging) {
add(tw.getRawPath(), base, DirCacheEntry.STAGE_1, EPOCH, 0); add(tw.getRawPath(), base, DirCacheEntry.STAGE_1, EPOCH, 0);
add(tw.getRawPath(), ours, DirCacheEntry.STAGE_2, EPOCH, 0); add(tw.getRawPath(), ours, DirCacheEntry.STAGE_2, EPOCH, 0);
add(tw.getRawPath(), theirs, DirCacheEntry.STAGE_3, EPOCH, 0); add(tw.getRawPath(), theirs, DirCacheEntry.STAGE_3, EPOCH, 0);
MergeResult<SubmoduleConflict> result = createGitLinksMergeResult( MergeResult<SubmoduleConflict> result = createGitLinksMergeResult(
base, ours, theirs); base, ours, theirs);
result.setContainsConflicts(true); result.setContainsConflicts(true);
mergeResults.put(tw.getPathString(), result); mergeResults.put(tw.getPathString(), result);
if (!ignoreConflicts) {
unmergedPaths.add(tw.getPathString()); unmergedPaths.add(tw.getPathString());
}
return true; return true;
} else if (!attributes.canBeContentMerged()) { } else if (!attributes.canBeContentMerged()) {
add(tw.getRawPath(), base, DirCacheEntry.STAGE_1, EPOCH, 0); add(tw.getRawPath(), base, DirCacheEntry.STAGE_1, EPOCH, 0);
@ -806,7 +807,8 @@ protected boolean processEntry(CanonicalTreeParser base,
} }
if (nonTree(modeT)) { if (nonTree(modeT)) {
if (e != null) { if (e != null) {
addToCheckout(tw.getPathString(), e, attributes); addToCheckout(tw.getPathString(), e,
attributes);
} }
} }
} }