Fix CommitCommand not to destroy repo

There was a severe bug in CommitCommand which could corrupt
repos. When merging an annotated tag the JGit MergeCommand writes
correctly the ID of the tag (and not the id of the commit the tag was
pointing to) into MERGE_HEAD. Native git does the same. But
CommitCommand was reading this file and trusting blindly that it will
contain only IDs of commits. Then the CommitCommand created a
commit which has as parent a non-commit object (the tag object). That's
so corrupt that even native git gives up when you call "git log" in
such a repo.

To reproduce that with EGit simply right-click on a tag in the
Repository View and select Merge. The result was a corrupt repo!

Bug: 336291
Change-Id: I24cd5de19ce6ca7b68b4052c9e73dcc6d207b57c
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>

org.eclipse.jgit.test/tst/org/eclipse/jgit/api/MergeCommandTest.java

@@ -326,6 +326,33 @@ public class MergeCommandTest extends RepositoryTestCase {
 		assertEquals(RepositoryState.MERGING, db.getRepositoryState());
 	}
 
+	@Test
+	public void testMergeTag() throws Exception {
+		Git git = new Git(db);
+
+		writeTrashFile("a", "a");
+		git.add().addFilepattern("a").call();
+		RevCommit initialCommit = git.commit().setMessage("initial").call();
+
+		createBranch(initialCommit, "refs/heads/side");
+		checkoutBranch("refs/heads/side");
+
+		writeTrashFile("b", "b");
+		git.add().addFilepattern("b").call();
+		RevCommit secondCommit = git.commit().setMessage("side").call();
+		Ref tag = git.tag().setAnnotated(true).setMessage("my tag 01")
+				.setName("tag01").setObjectId(secondCommit).call();
+
+		checkoutBranch("refs/heads/master");
+
+		writeTrashFile("a", "a2");
+		git.add().addFilepattern("a").call();
+		git.commit().setMessage("main").call();
+
+		MergeResult result = git.merge().include(tag).setStrategy(MergeStrategy.RESOLVE).call();
+		assertEquals(MergeStatus.MERGED, result.getMergeStatus());
+	}
+
 	@Test
 	public void testMergeMessage() throws Exception {
 		Git git = new Git(db);

org.eclipse.jgit/src/org/eclipse/jgit/api/CommitCommand.java

@@ -77,6 +77,8 @@ import org.eclipse.jgit.lib.RefUpdate.Result;
 import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.lib.RepositoryState;
 import org.eclipse.jgit.revwalk.RevCommit;
+import org.eclipse.jgit.revwalk.RevObject;
+import org.eclipse.jgit.revwalk.RevTag;
 import org.eclipse.jgit.revwalk.RevWalk;
 import org.eclipse.jgit.treewalk.CanonicalTreeParser;
 import org.eclipse.jgit.treewalk.FileTreeIterator;
@@ -150,13 +152,16 @@ public class CommitCommand extends GitCommand<RevCommit> {
 		checkCallable();
 		Collections.sort(only);
 
-		RepositoryState state = repo.getRepositoryState();
+		RevWalk rw = new RevWalk(repo);
-		if (!state.canCommit())
-			throw new WrongRepositoryStateException(MessageFormat.format(
-					JGitText.get().cannotCommitOnARepoWithState, state.name()));
-		processOptions(state);
 
 		try {
+			RepositoryState state = repo.getRepositoryState();
+			if (!state.canCommit())
+				throw new WrongRepositoryStateException(MessageFormat.format(
+						JGitText.get().cannotCommitOnARepoWithState,
+						state.name()));
+			processOptions(state, rw);
+
 			if (all && !repo.isBare() && repo.getWorkTree() != null) {
 				Git git = new Git(repo);
 				try {
@@ -182,8 +187,7 @@ public class CommitCommand extends GitCommand<RevCommit> {
 
 			if (headId != null)
 				if (amend) {
-					RevCommit previousCommit = new RevWalk(repo)
+					RevCommit previousCommit = rw.parseCommit(headId);
-							.parseCommit(headId);
 					for (RevCommit p : previousCommit.getParents())
 						parents.add(p.getId());
 					if (author == null)
@@ -196,7 +200,7 @@ public class CommitCommand extends GitCommand<RevCommit> {
 			DirCache index = repo.lockDirCache();
 			try {
 				if (!only.isEmpty())
-					index = createTemporaryIndex(headId, index);
+					index = createTemporaryIndex(headId, index, rw);
 
 				ObjectInserter odi = repo.newObjectInserter();
 				try {
@@ -219,56 +223,51 @@ public class CommitCommand extends GitCommand<RevCommit> {
 					ObjectId commitId = odi.insert(commit);
 					odi.flush();
 
-					RevWalk revWalk = new RevWalk(repo);
+					RevCommit revCommit = rw.parseCommit(commitId);
-					try {
+					RefUpdate ru = repo.updateRef(Constants.HEAD);
-						RevCommit revCommit = revWalk.parseCommit(commitId);
+					ru.setNewObjectId(commitId);
-						RefUpdate ru = repo.updateRef(Constants.HEAD);
+					if (reflogComment != null) {
-						ru.setNewObjectId(commitId);
+						ru.setRefLogMessage(reflogComment, false);
-						if (reflogComment != null) {
+					} else {
-							ru.setRefLogMessage(reflogComment, false);
+						String prefix = amend ? "commit (amend): " //$NON-NLS-1$
-						} else {
+								: parents.size() == 0 ? "commit (initial): "
-							String prefix = amend ? "commit (amend): " //$NON-NLS-1$
+										: "commit: ";
-									: parents.size() == 0 ? "commit (initial): "
+						ru.setRefLogMessage(
-											: "commit: ";
+								prefix + revCommit.getShortMessage(), false);
-							ru.setRefLogMessage(
+					}
-									prefix + revCommit.getShortMessage(), false);
+					if (headId != null)
-						}
+						ru.setExpectedOldObjectId(headId);
-						if (headId != null)
+					else
-							ru.setExpectedOldObjectId(headId);
+						ru.setExpectedOldObjectId(ObjectId.zeroId());
-						else
+					Result rc = ru.forceUpdate();
-							ru.setExpectedOldObjectId(ObjectId.zeroId());
+					switch (rc) {
-						Result rc = ru.forceUpdate();
+					case NEW:
-						switch (rc) {
+					case FORCED:
-						case NEW:
+					case FAST_FORWARD: {
-						case FORCED:
+						setCallable(false);
-						case FAST_FORWARD: {
+						if (state == RepositoryState.MERGING_RESOLVED) {
-							setCallable(false);
+							// Commit was successful. Now delete the files
-							if (state == RepositoryState.MERGING_RESOLVED) {
+							// used for merge commits
-								// Commit was successful. Now delete the files
+							repo.writeMergeCommitMsg(null);
-								// used for merge commits
+							repo.writeMergeHeads(null);
-								repo.writeMergeCommitMsg(null);
+						} else if (state == RepositoryState.CHERRY_PICKING_RESOLVED) {
-								repo.writeMergeHeads(null);
+							repo.writeMergeCommitMsg(null);
-							} else if (state == RepositoryState.CHERRY_PICKING_RESOLVED) {
+							repo.writeCherryPickHead(null);
-								repo.writeMergeCommitMsg(null);
+						} else if (state == RepositoryState.REVERTING_RESOLVED) {
-								repo.writeCherryPickHead(null);
+							repo.writeMergeCommitMsg(null);
-							} else if (state == RepositoryState.REVERTING_RESOLVED) {
+							repo.writeRevertHead(null);
-								repo.writeMergeCommitMsg(null);
-								repo.writeRevertHead(null);
-							}
-							return revCommit;
-						}
-						case REJECTED:
-						case LOCK_FAILURE:
-							throw new ConcurrentRefUpdateException(JGitText
-									.get().couldNotLockHEAD, ru.getRef(), rc);
-						default:
-							throw new JGitInternalException(MessageFormat
-									.format(JGitText.get().updatingRefFailed,
-											Constants.HEAD,
-											commitId.toString(), rc));
 						}
-					} finally {
+						return revCommit;
-						revWalk.release();
+					}
+					case REJECTED:
+					case LOCK_FAILURE:
+						throw new ConcurrentRefUpdateException(
+								JGitText.get().couldNotLockHEAD, ru.getRef(),
+								rc);
+					default:
+						throw new JGitInternalException(MessageFormat.format(
+								JGitText.get().updatingRefFailed,
+								Constants.HEAD, commitId.toString(), rc));
 					}
 				} finally {
 					odi.release();
@@ -281,6 +280,8 @@ public class CommitCommand extends GitCommand<RevCommit> {
 		} catch (IOException e) {
 			throw new JGitInternalException(
 					JGitText.get().exceptionCaughtDuringExecutionOfCommitCommand, e);
+		} finally {
+			rw.dispose();
 		}
 	}
 
@@ -297,7 +298,8 @@ public class CommitCommand extends GitCommand<RevCommit> {
 					+ changeId.getName() + "\n"); //$NON-NLS-1$
 	}
 
-	private DirCache createTemporaryIndex(ObjectId headId, DirCache index)
+	private DirCache createTemporaryIndex(ObjectId headId, DirCache index,
+			RevWalk rw)
 			throws IOException {
 		ObjectInserter inserter = null;
 
@@ -317,7 +319,7 @@ public class CommitCommand extends GitCommand<RevCommit> {
 		int fIdx = treeWalk.addTree(new FileTreeIterator(repo));
 		int hIdx = -1;
 		if (headId != null)
-			hIdx = treeWalk.addTree(new RevWalk(repo).parseTree(headId));
+			hIdx = treeWalk.addTree(rw.parseTree(headId));
 		treeWalk.setRecursive(true);
 
 		String lastAddedFile = null;
@@ -473,11 +475,14 @@ public class CommitCommand extends GitCommand<RevCommit> {
 	 *
 	 * @param state
 	 *            the state of the repository we are working on
+	 * @param rw
+	 *            the RevWalk to use
 	 *
 	 * @throws NoMessageException
 	 *             if the commit message has not been specified
 	 */
-	private void processOptions(RepositoryState state) throws NoMessageException {
+	private void processOptions(RepositoryState state, RevWalk rw)
+			throws NoMessageException {
 		if (committer == null)
 			committer = new PersonIdent(repo);
 		if (author == null && !amend)
@@ -487,6 +492,12 @@ public class CommitCommand extends GitCommand<RevCommit> {
 		if (state == RepositoryState.MERGING_RESOLVED) {
 			try {
 				parents = repo.readMergeHeads();
+				if (parents != null)
+					for (int i = 0; i < parents.size(); i++) {
+						RevObject ro = rw.parseAny(parents.get(i));
+						if (ro instanceof RevTag)
+							parents.set(i, rw.peel(ro));
+					}
 			} catch (IOException e) {
 				throw new JGitInternalException(MessageFormat.format(
 						JGitText.get().exceptionOccurredDuringReadingOfGIT_DIR,