]> source.dussan.org Git - jgit.git/commitdiff
Improve ours/theirs conflict markers for rebase, cherry-pick 22/7222/3
authorRobin Stocker <robin@nibor.org>
Fri, 17 Aug 2012 22:11:45 +0000 (00:11 +0200)
committerMatthias Sohn <matthias.sohn@sap.com>
Fri, 17 Aug 2012 22:11:45 +0000 (00:11 +0200)
On conflicts in rebase or cherry-pick, the conflict markers were like
this:

    <<<<<<< OURS
    a
    =======
    b
    >>>>>>> THEIRS

This is technically correct, but it could be better.

It's especially confusing during a rebase, where the meaning of
OURS/THEIRS is not obvious. The intuition is that "ours" is the commits
that "I" did before the rebase, but it's the other way around because of
the way rebase works. See various bug reports and stackoverflow
discussions.

With this change, in the case of a cherry-pick while on master, the
markers will be like this:

    <<<<<<< master
    a
    =======
    b
    >>>>>>> bad1dea Message of the commit I'm cherry-picking

In the case of a "git rebase master":

    <<<<<<< Upstream, based on master
    a
    =======
    b
    >>>>>>> b161dea Message of a commit I'm rebasing

It's not "master" because that would only be correct for the first
cherry-pick during a rebase, after that, it's master + already
cherry-picked commits.

And in the case of a "git pull --rebase":

    <<<<<<< Upstream, based on branch 'master' of git@example.org:repo
    a
    =======
    b
    >>>>>>> b161dea Message of a commit I'm rebasing

Bug: 336819
Change-Id: I1333a8dd170bb0077f491962013485efb6f2a926
Signed-off-by: Robin Stocker <robin@nibor.org>
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
org.eclipse.jgit.test/tst/org/eclipse/jgit/api/CherryPickCommandTest.java
org.eclipse.jgit.test/tst/org/eclipse/jgit/api/PullCommandWithRebaseTest.java
org.eclipse.jgit.test/tst/org/eclipse/jgit/api/RebaseCommandTest.java
org.eclipse.jgit/src/org/eclipse/jgit/api/CherryPickCommand.java
org.eclipse.jgit/src/org/eclipse/jgit/api/PullCommand.java
org.eclipse.jgit/src/org/eclipse/jgit/api/RebaseCommand.java

index 469739c57c45f595a8434f8599858f3bb7f08d5d..7984d9f2421bde93210d65f0474f8a16f2758cb5 100644 (file)
@@ -223,6 +223,32 @@ public class CherryPickCommandTest extends RepositoryTestCase {
                assertEquals(CherryPickStatus.OK, result.getStatus());
        }
 
+       @Test
+       public void testCherryPickConflictMarkers() throws Exception {
+               Git git = new Git(db);
+               RevCommit sideCommit = prepareCherryPick(git);
+
+               CherryPickResult result = git.cherryPick().include(sideCommit.getId())
+                               .call();
+               assertEquals(CherryPickStatus.CONFLICTING, result.getStatus());
+
+               String expected = "<<<<<<< master\na(master)\n=======\na(side)\n>>>>>>> 527460a side\n";
+               checkFile(new File(db.getWorkTree(), "a"), expected);
+       }
+
+       @Test
+       public void testCherryPickOurCommitName() throws Exception {
+               Git git = new Git(db);
+               RevCommit sideCommit = prepareCherryPick(git);
+
+               CherryPickResult result = git.cherryPick().include(sideCommit.getId())
+                               .setOurCommitName("custom name").call();
+               assertEquals(CherryPickStatus.CONFLICTING, result.getStatus());
+
+               String expected = "<<<<<<< custom name\na(master)\n=======\na(side)\n>>>>>>> 527460a side\n";
+               checkFile(new File(db.getWorkTree(), "a"), expected);
+       }
+
        private RevCommit prepareCherryPick(final Git git) throws Exception {
                // create, add and commit file a
                writeTrashFile("a", "a");
index a014071a5606bab026ab6e8b9e2a4bab4f16f295..1686ce1fa4ae8da9e0b52a2b3ec54124b531d292 100644 (file)
@@ -56,6 +56,7 @@ import java.io.IOException;
 import org.eclipse.jgit.api.CreateBranchCommand.SetupUpstreamMode;
 import org.eclipse.jgit.api.MergeResult.MergeStatus;
 import org.eclipse.jgit.api.RebaseResult.Status;
+import org.eclipse.jgit.lib.ConfigConstants;
 import org.eclipse.jgit.lib.Constants;
 import org.eclipse.jgit.lib.RepositoryState;
 import org.eclipse.jgit.lib.RepositoryTestCase;
@@ -164,9 +165,17 @@ public class PullCommandWithRebaseTest extends RepositoryTestCase {
 
                res = target.pull().call();
 
+               String remoteUri = target
+                               .getRepository()
+                               .getConfig()
+                               .getString(ConfigConstants.CONFIG_REMOTE_SECTION, "origin",
+                                               ConfigConstants.CONFIG_KEY_URL);
+
                assertFalse(res.getFetchResult().getTrackingRefUpdates().isEmpty());
                assertTrue(res.getRebaseResult().getStatus().equals(Status.STOPPED));
-               String result = "<<<<<<< OURS\nSource change\n=======\nTarget change\n>>>>>>> THEIRS\n";
+               String result = "<<<<<<< Upstream, based on branch 'master' of "
+                               + remoteUri
+                               + "\nSource change\n=======\nTarget change\n>>>>>>> 42453fd Target change in local\n";
                assertFileContentsEqual(targetFile, result);
                assertEquals(RepositoryState.REBASING_INTERACTIVE, target
                                .getRepository().getRepositoryState());
@@ -210,7 +219,8 @@ public class PullCommandWithRebaseTest extends RepositoryTestCase {
 
                assertNull(res.getFetchResult());
                assertEquals(Status.STOPPED, res.getRebaseResult().getStatus());
-               String result = "<<<<<<< OURS\nMaster change\n=======\nSlave change\n>>>>>>> THEIRS\n";
+               String result = "<<<<<<< Upstream, based on branch 'master' of local repository\n"
+                               + "Master change\n=======\nSlave change\n>>>>>>> 4049c9e Source change in based on master\n";
                assertFileContentsEqual(targetFile, result);
                assertEquals(RepositoryState.REBASING_INTERACTIVE, target
                                .getRepository().getRepositoryState());
index edb36b81fe5b629d5facfdb1042f086833890f57..dbad322be20c448919f396cef1e34edfd367f537 100644 (file)
@@ -420,7 +420,8 @@ public class RebaseCommandTest extends RepositoryTestCase {
                assertEquals(Status.STOPPED, res.getStatus());
                assertEquals(conflicting, res.getCurrentCommit());
                checkFile(FILE1,
-                               "<<<<<<< OURS\n1master\n=======\n1topic\n>>>>>>> THEIRS\n2\n3\ntopic4");
+                               "<<<<<<< Upstream, based on master\n1master\n=======\n1topic",
+                               ">>>>>>> e0d1dea change file1 in topic\n2\n3\ntopic4");
 
                assertEquals(RepositoryState.REBASING_INTERACTIVE, db
                                .getRepositoryState());
@@ -778,8 +779,10 @@ public class RebaseCommandTest extends RepositoryTestCase {
 
                res = git.rebase().setOperation(Operation.SKIP).call();
                // TODO is this correct? It is what the command line returns
-               checkFile(FILE1,
-                               "1master\n2\n<<<<<<< OURS\n3master\n=======\n3topic\n>>>>>>> THEIRS\n4\n5topic");
+               checkFile(
+                               FILE1,
+                               "1master\n2\n<<<<<<< Upstream, based on master\n3master\n=======\n3topic",
+                               ">>>>>>> 5afc8df change file1 in topic again\n4\n5topic");
                assertEquals(Status.STOPPED, res.getStatus());
        }
 
index b5bf119082d7cf26c714f4232b909013c5026af1..3d3b47d0896152562899128b5469b6b693b4a9aa 100644 (file)
@@ -84,6 +84,8 @@ import org.eclipse.jgit.treewalk.FileTreeIterator;
 public class CherryPickCommand extends GitCommand<CherryPickResult> {
        private List<Ref> commits = new LinkedList<Ref>();
 
+       private String ourCommitName = null;
+
        /**
         * @param repo
         */
@@ -144,10 +146,16 @@ public class CherryPickCommand extends GitCommand<CherryPickResult> {
                                RevCommit srcParent = srcCommit.getParent(0);
                                revWalk.parseHeaders(srcParent);
 
+                               String ourName = calculateOurName(headRef);
+                               String cherryPickName = srcCommit.getId().abbreviate(7).name()
+                                               + " " + srcCommit.getShortMessage();
+
                                ResolveMerger merger = (ResolveMerger) MergeStrategy.RESOLVE
                                                .newMerger(repo);
                                merger.setWorkingTreeIterator(new FileTreeIterator(repo));
                                merger.setBase(srcParent.getTree());
+                               merger.setCommitNames(new String[] { "BASE", ourName,
+                                               cherryPickName });
                                if (merger.merge(headCommit, srcCommit)) {
                                        if (AnyObjectId.equals(headCommit.getTree().getId(), merger
                                                        .getResultTreeId()))
@@ -223,4 +231,24 @@ public class CherryPickCommand extends GitCommand<CherryPickResult> {
                return include(new ObjectIdRef.Unpeeled(Storage.LOOSE, name,
                                commit.copy()));
        }
+
+       /**
+        * @param ourCommitName
+        *            the name that should be used in the "OURS" place for conflict
+        *            markers
+        * @return {@code this}
+        */
+       public CherryPickCommand setOurCommitName(String ourCommitName) {
+               this.ourCommitName = ourCommitName;
+               return this;
+       }
+
+       private String calculateOurName(Ref headRef) {
+               if (ourCommitName != null)
+                       return ourCommitName;
+
+               String targetRefName = headRef.getTarget().getName();
+               String headName = Repository.shortenRefName(targetRefName);
+               return headName;
+       }
 }
index fa425d37fbdd8661c9ef0f1100166baee906780e..5b8c776fd7dd860655e659b0f2eddc9da7991077 100644 (file)
@@ -242,19 +242,21 @@ public class PullCommand extends TransportCommand<PullCommand, PullResult> {
                        }
                }
 
+               String upstreamName = "branch \'"
+                               + Repository.shortenRefName(remoteBranchName) + "\' of "
+                               + remoteUri;
+
                PullResult result;
                if (doRebase) {
                        RebaseCommand rebase = new RebaseCommand(repo);
                        RebaseResult rebaseRes = rebase.setUpstream(commitToMerge)
+                                       .setUpstreamName(upstreamName)
                                        .setProgressMonitor(monitor).setOperation(Operation.BEGIN)
                                        .call();
                        result = new PullResult(fetchRes, remote, rebaseRes);
                } else {
                        MergeCommand merge = new MergeCommand(repo);
-                       String name = "branch \'"
-                                       + Repository.shortenRefName(remoteBranchName) + "\' of "
-                                       + remoteUri;
-                       merge.include(name, commitToMerge);
+                       merge.include(upstreamName, commitToMerge);
                        MergeResult mergeRes = merge.call();
                        monitor.update(1);
                        result = new PullResult(fetchRes, remote, mergeRes);
index 9f8aa7bfb2613fbf26662ba1389f13a0e2f8b0e9..ac7430ae2e87952a2650f1a49ef1db7bc09bfe1d 100644 (file)
@@ -137,6 +137,8 @@ public class RebaseCommand extends GitCommand<RebaseResult> {
 
        private static final String ONTO = "onto";
 
+       private static final String ONTO_NAME = "onto-name";
+
        private static final String PATCH = "patch";
 
        private static final String REBASE_HEAD = "head";
@@ -167,6 +169,8 @@ public class RebaseCommand extends GitCommand<RebaseResult> {
 
        private RevCommit upstreamCommit;
 
+       private String upstreamCommitName;
+
        private ProgressMonitor monitor = NullProgressMonitor.INSTANCE;
 
        private final RevWalk walk;
@@ -211,9 +215,16 @@ public class RebaseCommand extends GitCommand<RebaseResult> {
                        case SKIP:
                                // fall through
                        case CONTINUE:
-                               String upstreamCommitName = readFile(rebaseDir, ONTO);
+                               String upstreamCommitId = readFile(rebaseDir, ONTO);
+                               try {
+                                       upstreamCommitName = readFile(rebaseDir, ONTO_NAME);
+                               } catch (FileNotFoundException e) {
+                                       // Fall back to commit ID if file doesn't exist (e.g. rebase
+                                       // was started by C Git)
+                                       upstreamCommitName = upstreamCommitId;
+                               }
                                this.upstreamCommit = walk.parseCommit(repo
-                                               .resolve(upstreamCommitName));
+                                               .resolve(upstreamCommitId));
                                break;
                        case BEGIN:
                                RebaseResult res = initFilesAndRewind();
@@ -267,8 +278,10 @@ public class RebaseCommand extends GitCommand<RebaseResult> {
                                                // TODO if the content of this commit is already merged
                                                // here we should skip this step in order to avoid
                                                // confusing pseudo-changed
+                                               String ourCommitName = getOurCommitName();
                                                CherryPickResult cherryPickResult = new Git(repo)
-                                                               .cherryPick().include(commitToPick).call();
+                                                               .cherryPick().include(commitToPick)
+                                                               .setOurCommitName(ourCommitName).call();
                                                switch (cherryPickResult.getStatus()) {
                                                case FAILED:
                                                        if (operation == Operation.BEGIN)
@@ -300,6 +313,14 @@ public class RebaseCommand extends GitCommand<RebaseResult> {
                }
        }
 
+       private String getOurCommitName() {
+               // If onto is different from upstream, this should say "onto", but
+               // RebaseCommand doesn't support a different "onto" at the moment.
+               String ourCommitName = "Upstream, based on "
+                               + Repository.shortenRefName(upstreamCommitName);
+               return ourCommitName;
+       }
+
        private void updateHead(String headName, RevCommit newHead)
                        throws IOException {
                // point the previous head (if any) to the new commit
@@ -584,6 +605,7 @@ public class RebaseCommand extends GitCommand<RebaseResult> {
                createFile(rebaseDir, REBASE_HEAD, headId.name());
                createFile(rebaseDir, HEAD_NAME, headName);
                createFile(rebaseDir, ONTO, upstreamCommit.name());
+               createFile(rebaseDir, ONTO_NAME, upstreamCommitName);
                createFile(rebaseDir, INTERACTIVE, "");
                BufferedWriter fw = new BufferedWriter(new OutputStreamWriter(
                                new FileOutputStream(new File(rebaseDir, GIT_REBASE_TODO)),
@@ -884,6 +906,7 @@ public class RebaseCommand extends GitCommand<RebaseResult> {
         */
        public RebaseCommand setUpstream(RevCommit upstream) {
                this.upstreamCommit = upstream;
+               this.upstreamCommitName = upstream.name();
                return this;
        }
 
@@ -895,6 +918,7 @@ public class RebaseCommand extends GitCommand<RebaseResult> {
        public RebaseCommand setUpstream(AnyObjectId upstream) {
                try {
                        this.upstreamCommit = walk.parseCommit(upstream);
+                       this.upstreamCommitName = upstream.name();
                } catch (IOException e) {
                        throw new JGitInternalException(MessageFormat.format(
                                        JGitText.get().couldNotReadObjectWhileParsingCommit,
@@ -917,12 +941,30 @@ public class RebaseCommand extends GitCommand<RebaseResult> {
                                throw new RefNotFoundException(MessageFormat.format(JGitText
                                                .get().refNotResolved, upstream));
                        upstreamCommit = walk.parseCommit(repo.resolve(upstream));
+                       upstreamCommitName = upstream;
                        return this;
                } catch (IOException ioe) {
                        throw new JGitInternalException(ioe.getMessage(), ioe);
                }
        }
 
+       /**
+        * Optionally override the name of the upstream. If this is used, it has to
+        * come after any {@link #setUpstream} call.
+        *
+        * @param upstreamName
+        *            the name which will be used to refer to upstream in conflicts
+        * @return {@code this}
+        */
+       public RebaseCommand setUpstreamName(String upstreamName) {
+               if (upstreamCommit == null) {
+                       throw new IllegalStateException(
+                                       "setUpstreamName must be called after setUpstream.");
+               }
+               this.upstreamCommitName = upstreamName;
+               return this;
+       }
+
        /**
         * @param operation
         *            the operation to perform