Check for external merge tools during merge

Co-authored-by: Thomas Zerr<thomas.zerr@cloudogu.com>
Co-authored-by: René Pfeuffer<rene.pfeuffer@cloudogu.com>
This commit is contained in:
Eduard Heimbuch
2024-06-24 15:42:15 +02:00
committed by René Pfeuffer
parent 11a2dcda41
commit 29a6b42fce
11 changed files with 465 additions and 25 deletions

View File

@@ -0,0 +1,119 @@
/*
* MIT License
*
* Copyright (c) 2020-present Cloudogu GmbH and Contributors
*
* Permission is hereby granted, free of charge, to any person obtaining a copy
* of this software and associated documentation files (the "Software"), to deal
* in the Software without restriction, including without limitation the rights
* to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
* copies of the Software, and to permit persons to whom the Software is
* furnished to do so, subject to the following conditions:
*
* The above copyright notice and this permission notice shall be included in all
* copies or substantial portions of the Software.
*
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
* FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
* AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
* LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
* OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
* SOFTWARE.
*/
package sonia.scm.repository.spi;
import com.google.inject.assistedinject.Assisted;
import jakarta.inject.Inject;
import lombok.extern.slf4j.Slf4j;
import org.eclipse.jgit.attributes.Attributes;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.revwalk.RevWalk;
import org.eclipse.jgit.util.LfsFactory;
import sonia.scm.NotFoundException;
import sonia.scm.repository.GitUtil;
import sonia.scm.repository.Modified;
import java.io.IOException;
import java.util.Collection;
import java.util.HashSet;
import java.util.List;
import java.util.Optional;
import java.util.stream.Collectors;
@Slf4j
class AttributeAnalyzer {
private static final String MERGE_TOOL_ATTRIBUTE_KEY = "merge";
private final GitContext context;
private final GitModificationsCommand modificationsCommand;
@Inject
AttributeAnalyzer(@Assisted GitContext context, GitModificationsCommand modificationsCommand) {
this.context = context;
this.modificationsCommand = modificationsCommand;
}
Optional<Attributes> getAttributes(RevCommit commit, String path) throws NotFoundException {
try (Repository repository = context.open()) {
Attributes attributesForPath = LfsFactory.getAttributesForPath(repository, path, commit);
if (attributesForPath.isEmpty()) {
return Optional.empty();
}
return Optional.of(attributesForPath);
} catch (IOException e) {
log.debug("Failed to get attributes", e);
return Optional.empty();
}
}
boolean hasExternalMergeToolConflicts(String source, String target) {
try (Repository repo = context.open()) {
String commonAncestorRevision = GitUtil.computeCommonAncestor(repo, GitUtil.getRevisionId(repo, source), GitUtil.getRevisionId(repo, target)).name();
return findExternalMergeToolConflicts(source, target, commonAncestorRevision);
} catch (IOException | NotFoundException e) {
log.debug("Failed to read/parse '.gitattributes' files", e);
return false;
}
}
boolean findExternalMergeToolConflicts(String source, String target, String commonAncestor) throws IOException {
List<String> changesInBoth = getPossiblyAffectedPaths(source, target, commonAncestor);
RevCommit targetCommit = getTargetCommit(target);
for (String path : changesInBoth) {
Optional<Attributes> attributes = getAttributes(targetCommit, path);
if (attributes.isPresent() && attributes.get().get(MERGE_TOOL_ATTRIBUTE_KEY) != null) {
return true;
}
}
return false;
}
private List<String> getPossiblyAffectedPaths(String source, String target, String commonAncestor) {
Collection<String> fromSourceToAncestor = findModifiedPaths(source, commonAncestor);
Collection<String> fromTargetToAncestor = findModifiedPaths(target, commonAncestor);
return fromSourceToAncestor.stream().filter(fromTargetToAncestor::contains).toList();
}
private Collection<String> findModifiedPaths(String baseRevision, String targetRevision) {
return modificationsCommand.getModifications(baseRevision, targetRevision).getModified()
.stream().map(Modified::getPath).collect(Collectors.toSet());
}
RevCommit getTargetCommit(String target) throws IOException {
try (Repository repository = context.open()) {
RevWalk rw = new org.eclipse.jgit.revwalk.RevWalk(repository);
return GitUtil.getCommit(repository, rw, repository.findRef(target));
} catch (IOException e) {
log.debug("Failed to get target commit", e);
throw e;
}
}
public interface Factory {
AttributeAnalyzer create(GitContext context);
}
}

View File

@@ -35,7 +35,6 @@ import org.eclipse.jgit.lib.IndexDiff;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.ObjectReader;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.merge.ResolveMerger;
import org.eclipse.jgit.treewalk.CanonicalTreeParser;
import org.eclipse.jgit.treewalk.filter.PathFilter;
import sonia.scm.repository.GitRepositoryHandler;
@@ -43,11 +42,15 @@ import sonia.scm.repository.GitWorkingCopyFactory;
import sonia.scm.repository.InternalRepositoryException;
import sonia.scm.repository.api.MergeCommandResult;
import sonia.scm.repository.api.MergeDryRunCommandResult;
import sonia.scm.repository.api.MergePreventReason;
import sonia.scm.repository.api.MergePreventReasonType;
import sonia.scm.repository.api.MergeStrategy;
import sonia.scm.repository.api.MergeStrategyNotSupportedException;
import java.io.ByteArrayOutputStream;
import java.io.IOException;
import java.util.ArrayList;
import java.util.List;
import java.util.Set;
import static org.eclipse.jgit.merge.MergeStrategy.RECURSIVE;
@@ -57,7 +60,7 @@ import static sonia.scm.NotFoundException.notFound;
public class GitMergeCommand extends AbstractGitCommand implements MergeCommand {
private final GitWorkingCopyFactory workingCopyFactory;
private final AttributeAnalyzer attributeAnalyzer;
private static final Set<MergeStrategy> STRATEGIES = ImmutableSet.of(
MergeStrategy.MERGE_COMMIT,
MergeStrategy.FAST_FORWARD_IF_POSSIBLE,
@@ -66,13 +69,14 @@ public class GitMergeCommand extends AbstractGitCommand implements MergeCommand
);
@Inject
GitMergeCommand(@Assisted GitContext context, GitRepositoryHandler handler) {
this(context, handler.getWorkingCopyFactory());
GitMergeCommand(@Assisted GitContext context, GitRepositoryHandler handler, AttributeAnalyzer attributeAnalyzer) {
this(context, handler.getWorkingCopyFactory(), attributeAnalyzer);
}
GitMergeCommand(@Assisted GitContext context, GitWorkingCopyFactory workingCopyFactory) {
GitMergeCommand(@Assisted GitContext context, GitWorkingCopyFactory workingCopyFactory, AttributeAnalyzer attributeAnalyzer) {
super(context);
this.workingCopyFactory = workingCopyFactory;
this.attributeAnalyzer = attributeAnalyzer;
}
@Override
@@ -86,7 +90,7 @@ public class GitMergeCommand extends AbstractGitCommand implements MergeCommand
}
private MergeCommandResult mergeWithStrategy(MergeCommandRequest request) {
switch(request.getMergeStrategy()) {
switch (request.getMergeStrategy()) {
case SQUASH:
return inClone(clone -> new GitMergeWithSquash(clone, request, context, repository), workingCopyFactory, request.getTargetBranch());
@@ -103,21 +107,32 @@ public class GitMergeCommand extends AbstractGitCommand implements MergeCommand
throw new MergeStrategyNotSupportedException(repository, request.getMergeStrategy());
}
}
@Override
public MergeDryRunCommandResult dryRun(MergeCommandRequest request) {
try {
Repository repository = context.open();
ResolveMerger merger = (ResolveMerger) RECURSIVE.newMerger(repository, true);
return new MergeDryRunCommandResult(
merger.merge(
resolveRevisionOrThrowNotFound(repository, request.getBranchToMerge()),
resolveRevisionOrThrowNotFound(repository, request.getTargetBranch())));
try (Repository repository = context.open()) {
List<MergePreventReason> mergePreventReasons = new ArrayList<>(2);
if (attributeAnalyzer.hasExternalMergeToolConflicts(request.getBranchToMerge(), request.getTargetBranch())) {
mergePreventReasons.add(new MergePreventReason(MergePreventReasonType.EXTERNAL_MERGE_TOOL));
}
if (!isMergeableWithoutFileConflicts(repository, request.getBranchToMerge(), request.getTargetBranch())) {
mergePreventReasons.add(new MergePreventReason(MergePreventReasonType.FILE_CONFLICTS));
}
return new MergeDryRunCommandResult(mergePreventReasons.isEmpty(), mergePreventReasons);
} catch (IOException e) {
throw new InternalRepositoryException(context.getRepository(), "could not clone repository for merge", e);
}
}
private boolean isMergeableWithoutFileConflicts(Repository repository, String sourceRevision, String targetRevision) throws IOException {
return RECURSIVE.newMerger(repository, true).merge(
resolveRevisionOrThrowNotFound(repository,sourceRevision),
resolveRevisionOrThrowNotFound(repository, targetRevision)
);
}
@Override
public boolean isSupported(MergeStrategy strategy) {
return STRATEGIES.contains(strategy);

View File

@@ -0,0 +1,125 @@
/*
* MIT License
*
* Copyright (c) 2020-present Cloudogu GmbH and Contributors
*
* Permission is hereby granted, free of charge, to any person obtaining a copy
* of this software and associated documentation files (the "Software"), to deal
* in the Software without restriction, including without limitation the rights
* to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
* copies of the Software, and to permit persons to whom the Software is
* furnished to do so, subject to the following conditions:
*
* The above copyright notice and this permission notice shall be included in all
* copies or substantial portions of the Software.
*
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
* FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
* AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
* LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
* OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
* SOFTWARE.
*/
package sonia.scm.repository.spi;
import org.eclipse.jgit.attributes.Attribute;
import org.eclipse.jgit.attributes.Attributes;
import org.eclipse.jgit.revwalk.RevCommit;
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.mockito.junit.MockitoJUnitRunner;
import java.io.IOException;
import java.util.Collection;
import java.util.Optional;
import static org.assertj.core.api.Assertions.assertThat;
@RunWith(MockitoJUnitRunner.class)
public class AttributeAnalyzerTest extends AbstractGitCommandTestBase {
private AttributeAnalyzer attributeAnalyzer;
@Before
public void initAnalyzer() {
attributeAnalyzer = new AttributeAnalyzer(createContext(), new GitModificationsCommand(createContext()));
}
@Test
public void shouldNotGetAttributesIfFileDoesNotExist() throws IOException {
RevCommit commit = attributeAnalyzer.getTargetCommit("main");
Optional<Attributes> attributes = attributeAnalyzer.getAttributes(commit,"text1234.txt");
assertThat(attributes).isNotPresent();
}
@Test
public void shouldNotGetAttributesIfDoesNotExistForFilePattern() throws IOException {
RevCommit commit = attributeAnalyzer.getTargetCommit("main");
Optional<Attributes> attributes = attributeAnalyzer.getAttributes(commit,"text.txt");
assertThat(attributes).isNotPresent();
}
@Test
public void shouldGetAttributes() throws IOException {
RevCommit commit = attributeAnalyzer.getTargetCommit("main");
Optional<Attributes> attributes = attributeAnalyzer.getAttributes(commit,"text.ipr");
assertThat(attributes).isPresent();
Collection<Attribute> attributeCollection = attributes.get().getAll();
Attribute firstAttribute = attributeCollection.iterator().next();
assertThat(firstAttribute.getKey()).isEqualTo("merge");
assertThat(firstAttribute.getValue()).isEqualTo("mps");
}
@Test
public void shouldCheckIfMergeIsPreventedByExternalMergeTools_WithNoGitAttributes() {
String source = "change_possibly_needing_merge_tool";
String target = "removed_git_attributes";
assertThat(attributeAnalyzer.hasExternalMergeToolConflicts(source, target)).isFalse();
}
@Test
public void shouldCheckIfMergeIsPreventedByExternalMergeTools_WithEmptyGitAttributes() {
String source = "change_possibly_needing_merge_tool";
String target = "empty_git_attributes";
assertThat(attributeAnalyzer.hasExternalMergeToolConflicts(source, target)).isFalse();
}
@Test
public void shouldCheckIfMergeIsPreventedByExternalMergeTools_PatternFoundButNoMergeToolConfigured() {
String source = "change_possibly_needing_merge_tool";
String target = "removed_merge_tool_from_attributes";
assertThat(attributeAnalyzer.hasExternalMergeToolConflicts(source, target)).isFalse();
}
@Test
public void shouldCheckIfMergeIsPreventedByExternalMergeTools_WithoutMergeToolRelevantChange() {
String source = "change_not_needing_merge_tool";
String target = "conflicting_change_not_needing_merge_tool";
assertThat(attributeAnalyzer.hasExternalMergeToolConflicts(source, target)).isFalse();
}
@Test
public void shouldCheckIfMergeIsPreventedByExternalMergeTools_WithMergeToolRelevantChangeButFastForwardable() {
String source = "change_possibly_needing_merge_tool";
String target = "main";
assertThat(attributeAnalyzer.hasExternalMergeToolConflicts(source, target)).isFalse();
}
@Test
public void shouldCheckIfMergeIsPreventedByExternalMergeTools_WithMergeToolRelevantChangeAndPossibleConflict() {
String source = "change_possibly_needing_merge_tool";
String target = "conflicting_change_possibly_needing_merge_tool";
assertThat(attributeAnalyzer.hasExternalMergeToolConflicts(source, target)).isTrue();
}
@Override
protected String getZippedRepositoryResource() {
return "sonia/scm/repository/spi/scm-git-attributes-spi-test.zip";
}
}

View File

@@ -34,11 +34,14 @@ import sonia.scm.repository.work.WorkdirProvider;
import java.io.IOException;
import static org.assertj.core.api.Assertions.assertThat;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;
import static sonia.scm.repository.spi.MergeConflictResult.ConflictTypes.BOTH_MODIFIED;
import static sonia.scm.repository.spi.MergeConflictResult.ConflictTypes.DELETED_BY_THEM;
import static sonia.scm.repository.spi.MergeConflictResult.ConflictTypes.DELETED_BY_US;
public class GitMergeCommand_Conflict_Test extends AbstractGitCommandTestBase {
public class GitMergeCommandConflictTest extends AbstractGitCommandTestBase {
static final String DIFF_HEADER = "diff --git a/Main.java b/Main.java";
static final String DIFF_FILE_CONFLICT = "--- a/Main.java\n" +
@@ -93,7 +96,9 @@ public class GitMergeCommand_Conflict_Test extends AbstractGitCommandTestBase {
}
private MergeConflictResult computeMergeConflictResult(String branchToMerge, String targetBranch) {
GitMergeCommand gitMergeCommand = new GitMergeCommand(createContext(), new SimpleGitWorkingCopyFactory(new NoneCachingWorkingCopyPool(new WorkdirProvider(null, repositoryLocationResolver)), new SimpleMeterRegistry()));
AttributeAnalyzer attributeAnalyzer = mock(AttributeAnalyzer.class);
when(attributeAnalyzer.hasExternalMergeToolConflicts(any(), any())).thenReturn(false);
GitMergeCommand gitMergeCommand = new GitMergeCommand(createContext(), new SimpleGitWorkingCopyFactory(new NoneCachingWorkingCopyPool(new WorkdirProvider(null, repositoryLocationResolver)), new SimpleMeterRegistry()), attributeAnalyzer);
MergeCommandRequest mergeCommandRequest = new MergeCommandRequest();
mergeCommandRequest.setBranchToMerge(branchToMerge);
mergeCommandRequest.setTargetBranch(targetBranch);

View File

@@ -40,6 +40,9 @@ import org.junit.BeforeClass;
import org.junit.Rule;
import org.junit.Test;
import org.junit.jupiter.api.Assertions;
import org.junit.runner.RunWith;
import org.mockito.Mock;
import org.mockito.junit.MockitoJUnitRunner;
import sonia.scm.NoChangesMadeException;
import sonia.scm.NotFoundException;
import sonia.scm.repository.Added;
@@ -47,6 +50,9 @@ import sonia.scm.repository.GitTestHelper;
import sonia.scm.repository.GitWorkingCopyFactory;
import sonia.scm.repository.Person;
import sonia.scm.repository.api.MergeCommandResult;
import sonia.scm.repository.api.MergeDryRunCommandResult;
import sonia.scm.repository.api.MergePreventReason;
import sonia.scm.repository.api.MergePreventReasonType;
import sonia.scm.repository.api.MergeStrategy;
import sonia.scm.repository.work.NoneCachingWorkingCopyPool;
import sonia.scm.repository.work.WorkdirProvider;
@@ -61,7 +67,9 @@ import java.util.function.Consumer;
import java.util.function.Function;
import static org.assertj.core.api.Assertions.assertThat;
import static org.mockito.Mockito.when;
@RunWith(MockitoJUnitRunner.class)
@SubjectAware(configuration = "classpath:sonia/scm/configuration/shiro.ini", username = "admin", password = "secret")
public class GitMergeCommandTest extends AbstractGitCommandTestBase {
@@ -71,6 +79,8 @@ public class GitMergeCommandTest extends AbstractGitCommandTestBase {
public ShiroRule shiro = new ShiroRule();
@Rule
public BindTransportProtocolRule transportProtocolRule = new BindTransportProtocolRule();
@Mock
private AttributeAnalyzer attributeAnalyzer;
@BeforeClass
public static void setSigner() {
@@ -84,21 +94,60 @@ public class GitMergeCommandTest extends AbstractGitCommandTestBase {
request.setBranchToMerge("mergeable");
request.setTargetBranch("master");
boolean mergeable = command.dryRun(request).isMergeable();
MergeDryRunCommandResult result = command.dryRun(request);
assertThat(mergeable).isTrue();
assertThat(result.isMergeable()).isTrue();
assertThat(result.getReasons()).isEmpty();
}
@Test
public void shouldDetectNotMergeableBranches() {
public void shouldDetectNotMergeableBranches_FileConflict() {
GitMergeCommand command = createCommand();
MergeCommandRequest request = new MergeCommandRequest();
request.setBranchToMerge("test-branch");
request.setTargetBranch("master");
boolean mergeable = command.dryRun(request).isMergeable();
MergeDryRunCommandResult result = command.dryRun(request);
assertThat(mergeable).isFalse();
assertThat(result.isMergeable()).isFalse();
assertThat(result.getReasons().size()).isEqualTo(1);
assertThat(result.getReasons().stream().toList().get(0).getType()).isEqualTo(MergePreventReasonType.FILE_CONFLICTS);
}
@Test
public void shouldDetectNotMergeableBranches_ExternalMergeTool() {
String source = "mergeable";
String target = "master";
when(attributeAnalyzer.hasExternalMergeToolConflicts(source, target)).thenReturn(true);
GitMergeCommand command = createCommand();
MergeCommandRequest request = new MergeCommandRequest();
request.setBranchToMerge(source);
request.setTargetBranch(target);
MergeDryRunCommandResult result = command.dryRun(request);
assertThat(result.isMergeable()).isFalse();
assertThat(result.getReasons().size()).isEqualTo(1);
assertThat(result.getReasons().stream().toList().get(0).getType()).isEqualTo(MergePreventReasonType.EXTERNAL_MERGE_TOOL);
}
@Test
public void shouldDetectNotMergeableBranches_ExternalMergeToolAndFileConflict() {
String source = "test-branch";
String target = "master";
when(attributeAnalyzer.hasExternalMergeToolConflicts(source, target)).thenReturn(true);
GitMergeCommand command = createCommand();
MergeCommandRequest request = new MergeCommandRequest();
request.setBranchToMerge(source);
request.setTargetBranch(target);
MergeDryRunCommandResult result = command.dryRun(request);
assertThat(result.isMergeable()).isFalse();
assertThat(result.getReasons().size()).isEqualTo(2);
List<MergePreventReason> reasons = result.getReasons().stream().toList();
assertThat(reasons.get(0).getType()).isEqualTo(MergePreventReasonType.EXTERNAL_MERGE_TOOL);
assertThat(reasons.get(1).getType()).isEqualTo(MergePreventReasonType.FILE_CONFLICTS);
}
@Test
@@ -192,7 +241,7 @@ public class GitMergeCommandTest extends AbstractGitCommandTestBase {
}
@Test
public void shouldNotMergeConflictingBranches() {
public void shouldNotMergeConflictingBranches_FileConflict() {
GitMergeCommand command = createCommand();
MergeCommandRequest request = new MergeCommandRequest();
request.setBranchToMerge("test-branch");
@@ -419,7 +468,7 @@ public class GitMergeCommandTest extends AbstractGitCommandTestBase {
}
@Test(expected = NotFoundException.class)
public void shouldHandleNotExistingTargetBranchInDryRun() {
public void shouldHandleNotExistingTargetBranchInDryRun() throws IOException {
GitMergeCommand command = createCommand();
MergeCommandRequest request = new MergeCommandRequest();
request.setTargetBranch("not_existing");
@@ -517,7 +566,7 @@ public class GitMergeCommandTest extends AbstractGitCommandTestBase {
}
private GitMergeCommand createCommand(Consumer<Git> interceptor) {
return new GitMergeCommand(createContext(), new SimpleGitWorkingCopyFactory(new NoneCachingWorkingCopyPool(new WorkdirProvider(null, repositoryLocationResolver)), new SimpleMeterRegistry())) {
return new GitMergeCommand(createContext(), new SimpleGitWorkingCopyFactory(new NoneCachingWorkingCopyPool(new WorkdirProvider(null, repositoryLocationResolver)), new SimpleMeterRegistry()), attributeAnalyzer) {
@Override
<R, W extends GitCloneWorker<R>> R inClone(Function<Git, W> workerSupplier, GitWorkingCopyFactory workingCopyFactory, String initialBranch) {
Function<Git, W> interceptedWorkerSupplier = git -> {

View File

@@ -0,0 +1,20 @@
*.ipr text merge=mps
*.bat text
*.txt text
*.iml text
*.xml text
*.java text
*.mpr text merge=mps
*.css text
*.html text
*.dtd text
*.sh text
dependencies text merge=mps
generated text merge=mps
*.mps text merge=mps
trace.info text merge=mps
*.mpl text merge=mps
*.msd text merge=mps
*.devkit text merge=mps
*.mpsr text merge=mps
*.model text merge=mps