From 6197097ea5644bbe8706c4d4d711dcf5b6e342c6 Mon Sep 17 00:00:00 2001 From: Max Hohenegger Date: Tue, 8 Sep 2015 10:22:37 +0200 Subject: Fixed 'Merges do not appear to be using --no-ff flag' - use non-ff for finish operations on features with multiple commits - extended tests Bug: 473639 Signed-off-by: Max Hohenegger Change-Id: I7a7c3a57a78930f95c0775f99eed2e99bf856544 --- .../gitflow/op/AbstractFeatureOperationTest.java | 18 +++++++ .../gitflow/op/AbstractGitFlowOperationTest.java | 4 ++ .../gitflow/op/FeatureFinishOperationTest.java | 32 ++++++------ .../egit/gitflow/op/HotfixFinishOperationTest.java | 20 +++---- .../gitflow/op/ReleaseFinishOperationTest.java | 46 +++++++++++++--- org.eclipse.egit.gitflow/META-INF/MANIFEST.MF | 3 +- .../egit/gitflow/op/FeatureFinishOperation.java | 2 +- .../eclipse/egit/gitflow/op/GitFlowOperation.java | 61 +++++++++++++++++++--- .../egit/gitflow/op/ReleaseFinishOperation.java | 4 +- 9 files changed, 148 insertions(+), 42 deletions(-) diff --git a/org.eclipse.egit.gitflow.test/src/org/eclipse/egit/gitflow/op/AbstractFeatureOperationTest.java b/org.eclipse.egit.gitflow.test/src/org/eclipse/egit/gitflow/op/AbstractFeatureOperationTest.java index 5d2bc10b5a..d31ffa4a48 100644 --- a/org.eclipse.egit.gitflow.test/src/org/eclipse/egit/gitflow/op/AbstractFeatureOperationTest.java +++ b/org.eclipse.egit.gitflow.test/src/org/eclipse/egit/gitflow/op/AbstractFeatureOperationTest.java @@ -8,8 +8,14 @@ *******************************************************************************/ package org.eclipse.egit.gitflow.op; +import java.util.Iterator; + import org.eclipse.egit.gitflow.GitFlowRepository; +import org.eclipse.jgit.api.Git; +import org.eclipse.jgit.api.errors.GitAPIException; +import org.eclipse.jgit.api.errors.NoHeadException; import org.eclipse.jgit.lib.Repository; +import org.eclipse.jgit.revwalk.RevCommit; abstract public class AbstractFeatureOperationTest extends AbstractGitFlowOperationTest { @@ -20,4 +26,16 @@ abstract public class AbstractFeatureOperationTest extends new InitOperation(repository).execute(null); return new GitFlowRepository(repository); } + + protected int countCommits(Repository repository) throws GitAPIException, + NoHeadException { + int count = 0; + Iterable commits = Git.wrap(repository).log().call(); + Iterator iterator = commits.iterator(); + while (iterator.hasNext()) { + iterator.next(); + count++; + } + return count; + } } diff --git a/org.eclipse.egit.gitflow.test/src/org/eclipse/egit/gitflow/op/AbstractGitFlowOperationTest.java b/org.eclipse.egit.gitflow.test/src/org/eclipse/egit/gitflow/op/AbstractGitFlowOperationTest.java index bd89a7e558..f28776eb08 100644 --- a/org.eclipse.egit.gitflow.test/src/org/eclipse/egit/gitflow/op/AbstractGitFlowOperationTest.java +++ b/org.eclipse.egit.gitflow.test/src/org/eclipse/egit/gitflow/op/AbstractGitFlowOperationTest.java @@ -76,4 +76,8 @@ abstract public class AbstractGitFlowOperationTest extends GitTestCase { return testRepository.addAndCommit(project.project, new File(file.getLocationURI()), commitMessage); } + + protected String formatMergeCommitMessage(String branchName) { + return String.format("Merge branch '%s'", branchName); + } } \ No newline at end of file diff --git a/org.eclipse.egit.gitflow.test/src/org/eclipse/egit/gitflow/op/FeatureFinishOperationTest.java b/org.eclipse.egit.gitflow.test/src/org/eclipse/egit/gitflow/op/FeatureFinishOperationTest.java index 82b5255610..bda512e310 100644 --- a/org.eclipse.egit.gitflow.test/src/org/eclipse/egit/gitflow/op/FeatureFinishOperationTest.java +++ b/org.eclipse.egit.gitflow.test/src/org/eclipse/egit/gitflow/op/FeatureFinishOperationTest.java @@ -12,24 +12,20 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertTrue; import java.io.File; -import java.util.Iterator; import org.eclipse.egit.core.op.BranchOperation; import org.eclipse.egit.gitflow.GitFlowRepository; import org.eclipse.egit.gitflow.WrongGitFlowStateException; import org.eclipse.jgit.api.Git; import org.eclipse.jgit.api.Status; -import org.eclipse.jgit.api.errors.GitAPIException; -import org.eclipse.jgit.api.errors.NoHeadException; import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.revwalk.RevCommit; import org.junit.Test; public class FeatureFinishOperationTest extends AbstractFeatureOperationTest { @Test - public void testFeatureFinish() throws Exception { + public void testFeatureFinishFastForward() throws Exception { String fileName = "theFirstFile.txt"; - Repository repository = testRepository.getRepository(); GitFlowRepository gfRepo = init("testFeatureFinish\n\nfirst commit\n"); @@ -76,16 +72,22 @@ public class FeatureFinishOperationTest extends AbstractFeatureOperationTest { assertTrue(status.hasUncommittedChanges()); } - private int countCommits(Repository repository) throws GitAPIException, - NoHeadException { - int count = 0; - Iterable commits = Git.wrap(repository).log().call(); - Iterator iterator = commits.iterator(); - while (iterator.hasNext()) { - iterator.next(); - count++; - } - return count; + @Test + public void testFeatureFinish() throws Exception { + Repository repository = testRepository.getRepository(); + GitFlowRepository gfRepo = init("testFeatureFinish\n\nfirst commit\n"); + + new FeatureStartOperation(gfRepo, MY_FEATURE).execute(null); + addFileAndCommit("foo.txt", "testFeatureFinish\n\nbranch commit 1\n"); + addFileAndCommit("bar.txt", "testFeatureFinish\n\nbranch commit 2\n"); + new FeatureFinishOperation(gfRepo).execute(null); + assertEquals(gfRepo.getConfig().getDevelopFull(), + repository.getFullBranch()); + + String branchName = gfRepo.getConfig().getFeatureBranchName(MY_FEATURE); + + assertEquals(formatMergeCommitMessage(branchName) + " into develop", gfRepo.findHead() + .getFullMessage()); } @Test(expected = WrongGitFlowStateException.class) diff --git a/org.eclipse.egit.gitflow.test/src/org/eclipse/egit/gitflow/op/HotfixFinishOperationTest.java b/org.eclipse.egit.gitflow.test/src/org/eclipse/egit/gitflow/op/HotfixFinishOperationTest.java index 7678dc769b..6cccb0d543 100644 --- a/org.eclipse.egit.gitflow.test/src/org/eclipse/egit/gitflow/op/HotfixFinishOperationTest.java +++ b/org.eclipse.egit.gitflow.test/src/org/eclipse/egit/gitflow/op/HotfixFinishOperationTest.java @@ -11,6 +11,7 @@ package org.eclipse.egit.gitflow.op; import static org.eclipse.egit.gitflow.GitFlowDefaults.DEVELOP; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotEquals; +import static org.junit.Assert.assertNotNull; import java.io.File; @@ -23,7 +24,7 @@ import org.junit.Test; public class HotfixFinishOperationTest extends AbstractGitFlowOperationTest { @Test - public void testHotfixFinish() throws Exception { + public void testHotfixFinishSingleCommit() throws Exception { testRepository .createInitialCommit("testHotfixFinish\n\nfirst commit\n"); @@ -43,17 +44,17 @@ public class HotfixFinishOperationTest extends AbstractGitFlowOperationTest { String branchName = gfRepo.getConfig().getHotfixBranchName(MY_HOTFIX); // tag created? - assertEquals(branchCommit, gfRepo.findCommitForTag(MY_HOTFIX)); + RevCommit taggedCommit = gfRepo.findCommitForTag(MY_HOTFIX); + assertEquals(formatMergeCommitMessage(branchName), taggedCommit.getShortMessage()); // branch removed? assertEquals(findBranch(repository, branchName), null); RevCommit developHead = gfRepo.findHead(DEVELOP); - //TODO: as soon as we start using NO_FF for all finish operations, this must be not equals. - assertEquals(branchCommit, developHead); + assertNotEquals(branchCommit, developHead); RevCommit masterHead = gfRepo.findHead(MY_MASTER); - assertEquals(branchCommit, masterHead); + assertEquals(formatMergeCommitMessage(branchName), masterHead.getShortMessage()); } @Test @@ -73,12 +74,14 @@ public class HotfixFinishOperationTest extends AbstractGitFlowOperationTest { testRepository.appendContentAndCommit(project.getProject(), file, "Hello Release", "Release Commit"); + testRepository.appendContentAndCommit(project.getProject(), file, + "Hello Merge Commit", "Release Commit 2"); new ReleaseFinishOperation(gfRepo).execute(null); new HotfixStartOperation(gfRepo, MY_HOTFIX).execute(null); // modify on first branch - RevCommit hotfixCommit = testRepository.appendContentAndCommit( + testRepository.appendContentAndCommit( project.getProject(), file, "Hello Hotfix", "Hotfix Commit"); new BranchOperation(repository, gfRepo.getConfig().getDevelop()).execute(null); assertEquals(gfRepo.getConfig().getDevelopFull(), repository.getFullBranch()); @@ -93,9 +96,8 @@ public class HotfixFinishOperationTest extends AbstractGitFlowOperationTest { gfRepo); hotfixFinishOperation.execute(null); - // tag is created because of 473646 // TODO: check if the reference implementation cleans up in this case - assertEquals(hotfixCommit, gfRepo.findCommitForTag(MY_HOTFIX)); + assertNotNull(gfRepo.findCommitForTag(MY_HOTFIX)); // branch not removed? assertNotEquals(findBranch(repository, branchName), null); @@ -108,7 +110,7 @@ public class HotfixFinishOperationTest extends AbstractGitFlowOperationTest { // merged on master RevCommit masterHead = gfRepo.findHead(MY_MASTER); - assertEquals(hotfixCommit, masterHead); + assertEquals(String.format("Merge branch '%s'", branchName), masterHead.getFullMessage()); assertEquals(gfRepo.getConfig().getDevelopFull(), repository.getFullBranch()); } diff --git a/org.eclipse.egit.gitflow.test/src/org/eclipse/egit/gitflow/op/ReleaseFinishOperationTest.java b/org.eclipse.egit.gitflow.test/src/org/eclipse/egit/gitflow/op/ReleaseFinishOperationTest.java index c3073e7e0a..18e52b84ed 100644 --- a/org.eclipse.egit.gitflow.test/src/org/eclipse/egit/gitflow/op/ReleaseFinishOperationTest.java +++ b/org.eclipse.egit.gitflow.test/src/org/eclipse/egit/gitflow/op/ReleaseFinishOperationTest.java @@ -15,6 +15,7 @@ import static org.eclipse.egit.gitflow.GitFlowDefaults.MASTER; import static org.eclipse.egit.gitflow.GitFlowDefaults.RELEASE_PREFIX; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotEquals; import static org.junit.Assert.fail; import java.util.Iterator; @@ -33,7 +34,7 @@ import org.junit.Test; public class ReleaseFinishOperationTest extends AbstractGitFlowOperationTest { @Test - public void testReleaseFinish() throws Exception { + public void testReleaseFinishSingleCommit() throws Exception { testRepository .createInitialCommit("testReleaseFinish\n\nfirst commit\n"); @@ -56,19 +57,52 @@ public class ReleaseFinishOperationTest extends AbstractGitFlowOperationTest { String branchName = gfRepo.getConfig().getReleaseBranchName(MY_RELEASE); // tag created? - assertEquals(branchCommit, - gfRepo.findCommitForTag(MY_VERSION_TAG + MY_RELEASE)); + RevCommit taggedCommit = gfRepo.findCommitForTag(MY_VERSION_TAG + MY_RELEASE); + assertEquals(formatMergeCommitMessage(branchName), taggedCommit.getShortMessage()); // branch removed? assertEquals(findBranch(repository, branchName), null); RevCommit developHead = gfRepo.findHead(DEVELOP); - //TODO: as soon as we start using NO_FF for all finish operations, this must be not equals. - assertEquals(branchCommit, developHead); + assertNotEquals(branchCommit, developHead); RevCommit masterHead = gfRepo.findHead(MY_MASTER); - assertEquals(branchCommit, masterHead); + assertEquals(formatMergeCommitMessage(branchName), masterHead.getShortMessage()); } + @Test + public void testReleaseFinish() throws Exception { + testRepository + .createInitialCommit("testReleaseFinish\n\nfirst commit\n"); + + Repository repository = testRepository.getRepository(); + InitParameters initParameters = new InitParameters(); + initParameters.setDevelop(DEVELOP); + initParameters.setMaster(MASTER); + initParameters.setFeature(FEATURE_PREFIX); + initParameters.setRelease(RELEASE_PREFIX); + initParameters.setHotfix(HOTFIX_PREFIX); + initParameters.setVersionTag(MY_VERSION_TAG); + new InitOperation(repository, initParameters).execute(null); + GitFlowRepository gfRepo = new GitFlowRepository(repository); + + new ReleaseStartOperation(gfRepo, MY_RELEASE).execute(null); + addFileAndCommit("foo.txt", "testReleaseFinish\n\nbranch commit 1\n"); + addFileAndCommit("bar.txt", "testReleaseFinish\n\nbranch commit 2\n"); + ReleaseFinishOperation releaseFinishOperation = new ReleaseFinishOperation(gfRepo); + releaseFinishOperation.execute(null); + assertEquals(gfRepo.getConfig().getDevelopFull(), + repository.getFullBranch()); + + String branchName = gfRepo.getConfig().getReleaseBranchName(MY_RELEASE); + // tag created? + RevCommit taggedCommit = gfRepo.findCommitForTag(MY_VERSION_TAG + + MY_RELEASE); + assertEquals(formatMergeCommitMessage(branchName), + taggedCommit.getFullMessage()); + + // branch removed? + assertEquals(findBranch(repository, branchName), null); + } @Test public void testReleaseFinishFail() throws Exception { testRepository diff --git a/org.eclipse.egit.gitflow/META-INF/MANIFEST.MF b/org.eclipse.egit.gitflow/META-INF/MANIFEST.MF index 167643f33d..62a728faa0 100644 --- a/org.eclipse.egit.gitflow/META-INF/MANIFEST.MF +++ b/org.eclipse.egit.gitflow/META-INF/MANIFEST.MF @@ -31,8 +31,9 @@ Import-Package: org.eclipse.egit.core;version="[4.1.0,4.2.0)", org.eclipse.egit.core.internal.job;version="[4.1.0,4.2.0)", org.eclipse.egit.core.op;version="[4.1.0,4.2.0)", org.eclipse.jgit.api;version="[4.1.0,4.2.0)", + org.eclipse.jgit.api.errors;version="[4.1.0,4.2.0)", org.eclipse.jgit.errors;version="[4.1.0,4.2.0)", org.eclipse.jgit.lib;version="[4.1.0,4.2.0)", org.eclipse.jgit.revwalk;version="[4.1.0,4.2.0)", - org.eclipse.jgit.api.errors;version="[4.1.0,4.2.0)", + org.eclipse.jgit.revwalk.filter;version="[4.1.0,4.2.0)", org.eclipse.jgit.transport;version="[4.1.0,4.2.0)" diff --git a/org.eclipse.egit.gitflow/src/org/eclipse/egit/gitflow/op/FeatureFinishOperation.java b/org.eclipse.egit.gitflow/src/org/eclipse/egit/gitflow/op/FeatureFinishOperation.java index 013dde1010..ef979f383c 100644 --- a/org.eclipse.egit.gitflow/src/org/eclipse/egit/gitflow/op/FeatureFinishOperation.java +++ b/org.eclipse.egit.gitflow/src/org/eclipse/egit/gitflow/op/FeatureFinishOperation.java @@ -47,7 +47,7 @@ public final class FeatureFinishOperation extends AbstractFeatureOperation { @Override public void execute(IProgressMonitor monitor) throws CoreException { - finish(monitor, repository.getConfig().getFeatureBranchName(featureName), squash); + finish(monitor, repository.getConfig().getFeatureBranchName(featureName), squash, true); } /** diff --git a/org.eclipse.egit.gitflow/src/org/eclipse/egit/gitflow/op/GitFlowOperation.java b/org.eclipse.egit.gitflow/src/org/eclipse/egit/gitflow/op/GitFlowOperation.java index 186d30a0ba..68b74c083f 100644 --- a/org.eclipse.egit.gitflow/src/org/eclipse/egit/gitflow/op/GitFlowOperation.java +++ b/org.eclipse.egit.gitflow/src/org/eclipse/egit/gitflow/op/GitFlowOperation.java @@ -10,6 +10,7 @@ package org.eclipse.egit.gitflow.op; import static java.lang.String.format; import static org.eclipse.egit.gitflow.Activator.error; +import static org.eclipse.jgit.api.MergeCommand.FastForwardMode.NO_FF; import java.io.IOException; import java.lang.reflect.InvocationTargetException; @@ -33,12 +34,16 @@ import org.eclipse.jgit.api.MergeResult; import org.eclipse.jgit.api.errors.GitAPIException; import org.eclipse.jgit.lib.Ref; import org.eclipse.jgit.revwalk.RevCommit; +import org.eclipse.jgit.revwalk.RevWalk; +import org.eclipse.jgit.revwalk.RevWalkUtils; +import org.eclipse.jgit.revwalk.filter.RevFilter; import org.eclipse.jgit.transport.FetchResult; import org.eclipse.jgit.transport.RemoteConfig; /** * Common logic for Git Flow operations. */ +// TODO: This class should be called AbstractGitFlowOperation for consistency abstract public class GitFlowOperation implements IEGitOperation { /** * git path separator @@ -103,15 +108,16 @@ abstract public class GitFlowOperation implements IEGitOperation { * @param monitor * @param branchName * @param squash + * @param fastForwardSingleCommit Has no effect if {@code squash} is true. * @throws CoreException * @since 4.1 */ protected void finish(IProgressMonitor monitor, String branchName, - boolean squash) + boolean squash, boolean fastForwardSingleCommit) throws CoreException { try { mergeResult = mergeTo(monitor, branchName, repository.getConfig() - .getDevelop(), squash); + .getDevelop(), squash, fastForwardSingleCommit); if (!mergeResult.getMergeStatus().isSuccessful()) { return; } @@ -130,13 +136,16 @@ abstract public class GitFlowOperation implements IEGitOperation { } /** + * Finish without squash and NO_FF for single commit branches: + * {@link org.eclipse.egit.gitflow.op.GitFlowOperation#finish(IProgressMonitor, String, boolean, boolean)} + * * @param monitor * @param branchName * @throws CoreException */ protected void finish(IProgressMonitor monitor, String branchName) throws CoreException { - finish(monitor, branchName, false); + finish(monitor, branchName, false, false); } /** @@ -144,12 +153,13 @@ abstract public class GitFlowOperation implements IEGitOperation { * @param branchName * @param targetBranchName * @param squash + * @param fastForwardSingleCommit Has no effect if {@code squash} is true. * @return result of merging back to targetBranchName * @throws CoreException * @since 4.1 */ protected MergeResult mergeTo(IProgressMonitor monitor, String branchName, - String targetBranchName, boolean squash) throws CoreException { + String targetBranchName, boolean squash, boolean fastForwardSingleCommit) throws CoreException { try { if (!repository.hasBranch(targetBranchName)) { throw new RuntimeException(String.format( @@ -173,17 +183,52 @@ abstract public class GitFlowOperation implements IEGitOperation { if (squash) { mergeOperation.setCommit(true); } + if (!squash && (!fastForwardSingleCommit || hasMultipleCommits(branchName))) { + mergeOperation.setFastForwardMode(NO_FF); + } mergeOperation.execute(monitor); return mergeOperation.getResult(); - } catch (GitAPIException e) { + } catch (GitAPIException | IOException e) { throw new RuntimeException(e); } } + private boolean hasMultipleCommits(String branchName) throws IOException { + return getAheadOfDevelopCount(branchName) > 1; + } + + private int getAheadOfDevelopCount(String branchName) throws IOException { + String parentBranch = repository.getConfig().getDevelop(); + + Ref develop = repository.findBranch(parentBranch); + Ref branch = repository.findBranch(branchName); + + RevWalk walk = new RevWalk(repository.getRepository()); + + RevCommit branchCommit = walk.parseCommit(branch.getObjectId()); + RevCommit developCommit = walk.parseCommit(develop.getObjectId()); + + RevCommit mergeBase = findCommonBase(walk, branchCommit, developCommit); + + walk.reset(); + walk.setRevFilter(RevFilter.ALL); + int aheadCount = RevWalkUtils.count(walk, branchCommit, mergeBase); + + return aheadCount; + } + + private RevCommit findCommonBase(RevWalk walk, RevCommit branchCommit, + RevCommit developCommit) throws IOException { + walk.setRevFilter(RevFilter.MERGE_BASE); + walk.markStart(branchCommit); + walk.markStart(developCommit); + return walk.next(); + } + /** - * Merge without squash: - * {@link org.eclipse.egit.gitflow.op.GitFlowOperation#mergeTo(IProgressMonitor, String, String, boolean)} + * Merge without squash and NO_FF for single commit branches: + * {@link org.eclipse.egit.gitflow.op.GitFlowOperation#mergeTo(IProgressMonitor, String, String, boolean, boolean)} * * @param monitor * @param branchName @@ -193,7 +238,7 @@ abstract public class GitFlowOperation implements IEGitOperation { */ protected MergeResult mergeTo(IProgressMonitor monitor, String branchName, String targetBranchName) throws CoreException { - return mergeTo(monitor, branchName, targetBranchName, false); + return mergeTo(monitor, branchName, targetBranchName, false, false); } /** diff --git a/org.eclipse.egit.gitflow/src/org/eclipse/egit/gitflow/op/ReleaseFinishOperation.java b/org.eclipse.egit.gitflow/src/org/eclipse/egit/gitflow/op/ReleaseFinishOperation.java index 575d51a894..cc10ddfc5e 100644 --- a/org.eclipse.egit.gitflow/src/org/eclipse/egit/gitflow/op/ReleaseFinishOperation.java +++ b/org.eclipse.egit.gitflow/src/org/eclipse/egit/gitflow/op/ReleaseFinishOperation.java @@ -49,7 +49,7 @@ public final class ReleaseFinishOperation extends AbstractReleaseOperation { public void execute(IProgressMonitor monitor) throws CoreException { String releaseBranchName = repository.getConfig().getReleaseBranchName(versionName); String master = repository.getConfig().getMaster(); - mergeResult = mergeTo(monitor, releaseBranchName, master, false /* TODO */); + mergeResult = mergeTo(monitor, releaseBranchName, master, false /* TODO */, false); if (!mergeResult.getMergeStatus().isSuccessful()) { // problems during merge to master => this repository is not in a healthy state return; @@ -59,6 +59,6 @@ public final class ReleaseFinishOperation extends AbstractReleaseOperation { safeCreateTag(monitor, repository.getConfig().getVersionTagPrefix() + versionName, NLS.bind(CoreText.ReleaseFinishOperation_releaseOf, versionName)); - finish(monitor, releaseBranchName, false /* TODO: squash should also be supported for releases */); + finish(monitor, releaseBranchName, false /* TODO: squash should also be supported for releases */, false); } } -- cgit v1.2.3