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>

1 files changed, 71 insertions, 60 deletions

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/api/CommitCommand.java b/org.eclipse.jgit/src/org/eclipse/jgit/api/CommitCommand.java
index 1f68a7df2c..162105b787 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/api/CommitCommand.java
+++ b/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();
-		if (!state.canCommit())
-			throw new WrongRepositoryStateException(MessageFormat.format(
-					JGitText.get().cannotCommitOnARepoWithState, state.name()));
-		processOptions(state);
+		RevWalk rw = new RevWalk(repo);
 
 		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)
-							.parseCommit(headId);
+					RevCommit previousCommit = rw.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);
-					try {
-						RevCommit revCommit = revWalk.parseCommit(commitId);
-						RefUpdate ru = repo.updateRef(Constants.HEAD);
-						ru.setNewObjectId(commitId);
-						if (reflogComment != null) {
-							ru.setRefLogMessage(reflogComment, false);
-						} else {
-							String prefix = amend ? "commit (amend): " //$NON-NLS-1$
-									: parents.size() == 0 ? "commit (initial): "
-											: "commit: ";
-							ru.setRefLogMessage(
-									prefix + revCommit.getShortMessage(), false);
-						}
-						if (headId != null)
-							ru.setExpectedOldObjectId(headId);
-						else
-							ru.setExpectedOldObjectId(ObjectId.zeroId());
-						Result rc = ru.forceUpdate();
-						switch (rc) {
-						case NEW:
-						case FORCED:
-						case FAST_FORWARD: {
-							setCallable(false);
-							if (state == RepositoryState.MERGING_RESOLVED) {
-								// Commit was successful. Now delete the files
-								// used for merge commits
-								repo.writeMergeCommitMsg(null);
-								repo.writeMergeHeads(null);
-							} else if (state == RepositoryState.CHERRY_PICKING_RESOLVED) {
-								repo.writeMergeCommitMsg(null);
-								repo.writeCherryPickHead(null);
-							} else if (state == RepositoryState.REVERTING_RESOLVED) {
-								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));
+					RevCommit revCommit = rw.parseCommit(commitId);
+					RefUpdate ru = repo.updateRef(Constants.HEAD);
+					ru.setNewObjectId(commitId);
+					if (reflogComment != null) {
+						ru.setRefLogMessage(reflogComment, false);
+					} else {
+						String prefix = amend ? "commit (amend): " //$NON-NLS-1$
+								: parents.size() == 0 ? "commit (initial): "
+										: "commit: ";
+						ru.setRefLogMessage(
+								prefix + revCommit.getShortMessage(), false);
+					}
+					if (headId != null)
+						ru.setExpectedOldObjectId(headId);
+					else
+						ru.setExpectedOldObjectId(ObjectId.zeroId());
+					Result rc = ru.forceUpdate();
+					switch (rc) {
+					case NEW:
+					case FORCED:
+					case FAST_FORWARD: {
+						setCallable(false);
+						if (state == RepositoryState.MERGING_RESOLVED) {
+							// Commit was successful. Now delete the files
+							// used for merge commits
+							repo.writeMergeCommitMsg(null);
+							repo.writeMergeHeads(null);
+						} else if (state == RepositoryState.CHERRY_PICKING_RESOLVED) {
+							repo.writeMergeCommitMsg(null);
+							repo.writeCherryPickHead(null);
+						} else if (state == RepositoryState.REVERTING_RESOLVED) {
+							repo.writeMergeCommitMsg(null);
+							repo.writeRevertHead(null);
 						}
-					} finally {
-						revWalk.release();
+						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 {
 					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,