Parse commitId and shortMessage of a commented out line in rebase todo

A rebase todo file consists of regular non-comment action lines and
comment lines. In case that a regular action line has been commented out
(i.e. prefixed with a hash '#'), the RebaseTodoLine that is representing
this line should hold the values for commitId and shortMessage even
though it's a comment line. This allows to switch between comment and
non-comment easily even after the file has been persisted and reread.

Change-Id: I56ec2ba08eaf3772e2d74d937dd496209a744d4b
Signed-off-by: Tobias Pfeifer <to.pfeifer@web.de>
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>

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

@@ -63,11 +63,13 @@ import org.eclipse.jgit.api.MergeResult.MergeStatus;
 import org.eclipse.jgit.api.RebaseCommand.InteractiveHandler;
 import org.eclipse.jgit.api.RebaseCommand.Operation;
 import org.eclipse.jgit.api.RebaseResult.Status;
+import org.eclipse.jgit.api.errors.JGitInternalException;
 import org.eclipse.jgit.api.errors.RefNotFoundException;
 import org.eclipse.jgit.api.errors.UnmergedPathsException;
 import org.eclipse.jgit.api.errors.WrongRepositoryStateException;
 import org.eclipse.jgit.dircache.DirCacheCheckout;
 import org.eclipse.jgit.junit.RepositoryTestCase;
+import org.eclipse.jgit.lib.AbbreviatedObjectId;
 import org.eclipse.jgit.lib.Constants;
 import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.PersonIdent;
@@ -1745,6 +1747,123 @@ public class RebaseCommandTest extends RepositoryTestCase {
 		assertEquals(Action.REWORD, steps.get(1).getAction());
 	}
 
+	@Test
+	public void testRebaseShouldTryToParseValidLineMarkedAsComment()
+			throws IOException {
+		String todo = "# pick 1111111 Valid line commented out with space\n"
+				+ "#edit 2222222 Valid line commented out without space\n"
+				+ "# pick invalidLine Comment line at end\n";
+		write(getTodoFile(), todo);
+
+		List<RebaseTodoLine> steps = db.readRebaseTodo(GIT_REBASE_TODO, true);
+		assertEquals(3, steps.size());
+
+		RebaseTodoLine firstLine = steps.get(0);
+
+		assertEquals("1111111", firstLine.getCommit().name());
+		assertEquals("Valid line commented out with space",
+				firstLine.getShortMessage());
+		assertEquals("comment", firstLine.getAction().toToken());
+
+		try {
+			firstLine.setAction(Action.PICK);
+			assertEquals("1111111", firstLine.getCommit().name());
+			assertEquals("pick", firstLine.getAction().toToken());
+		} catch (Exception e) {
+			fail("Valid parsable RebaseTodoLine that has been commented out should allow to change the action, but failed");
+		}
+
+		assertEquals("2222222", steps.get(1).getCommit().name());
+		assertEquals("comment", steps.get(1).getAction().toToken());
+
+		assertEquals(null, steps.get(2).getCommit());
+		assertEquals(null, steps.get(2).getShortMessage());
+		assertEquals("comment", steps.get(2).getAction().toToken());
+		assertEquals("# pick invalidLine Comment line at end", steps.get(2)
+				.getComment());
+		try {
+			steps.get(2).setAction(Action.PICK);
+			fail("A comment RebaseTodoLine that doesn't contain a valid parsable line should fail, but doesn't");
+		} catch (Exception e) {
+			// expected
+		}
+
+	}
+
+	@SuppressWarnings("unused")
+	@Test
+	public void testRebaseTodoLineSetComment() throws Exception {
+		try {
+			new RebaseTodoLine("This is a invalid comment");
+			fail("Constructing a comment line with invalid comment string should fail, but doesn't");
+		} catch (JGitInternalException e) {
+			// expected
+		}
+		RebaseTodoLine validCommentLine = new RebaseTodoLine(
+				"# This is a comment");
+		assertEquals(Action.COMMENT, validCommentLine.getAction());
+		assertEquals("# This is a comment", validCommentLine.getComment());
+
+		RebaseTodoLine actionLineToBeChanged = new RebaseTodoLine(Action.EDIT,
+				AbbreviatedObjectId.fromString("1111111"), "short Message");
+		assertEquals(null, actionLineToBeChanged.getComment());
+
+		try {
+			actionLineToBeChanged.setComment("invalid comment");
+			fail("Setting a invalid comment string should fail but doesn't");
+		} catch (JGitInternalException e) {
+			assertEquals(null, actionLineToBeChanged.getComment());
+		}
+
+		actionLineToBeChanged.setComment("# valid comment");
+		assertEquals("# valid comment", actionLineToBeChanged.getComment());
+		try {
+			actionLineToBeChanged.setComment("invalid comment");
+			fail("Setting a invalid comment string should fail but doesn't");
+		} catch (JGitInternalException e) {
+			// expected
+			// setting comment failed, but was successfully set before,
+			// therefore it may not be altered since then
+			assertEquals("# valid comment", actionLineToBeChanged.getComment());
+		}
+		try {
+			actionLineToBeChanged.setComment("# line1 \n line2");
+			actionLineToBeChanged.setComment("line1 \n line2");
+			actionLineToBeChanged.setComment("\n");
+			actionLineToBeChanged.setComment("# line1 \r line2");
+			actionLineToBeChanged.setComment("line1 \r line2");
+			actionLineToBeChanged.setComment("\r");
+			actionLineToBeChanged.setComment("# line1 \n\r line2");
+			actionLineToBeChanged.setComment("line1 \n\r line2");
+			actionLineToBeChanged.setComment("\n\r");
+			fail("Setting a multiline comment string should fail but doesn't");
+		} catch (JGitInternalException e) {
+			// expected
+		}
+		// Try setting valid comments
+		actionLineToBeChanged.setComment("# valid comment");
+		assertEquals("# valid comment", actionLineToBeChanged.getComment());
+
+		actionLineToBeChanged.setComment("# \t \t valid comment");
+		assertEquals("# \t \t valid comment",
+				actionLineToBeChanged.getComment());
+
+		actionLineToBeChanged.setComment("#       ");
+		assertEquals("#       ", actionLineToBeChanged.getComment());
+
+		actionLineToBeChanged.setComment("");
+		assertEquals("", actionLineToBeChanged.getComment());
+
+		actionLineToBeChanged.setComment("  ");
+		assertEquals("  ", actionLineToBeChanged.getComment());
+
+		actionLineToBeChanged.setComment("\t\t");
+		assertEquals("\t\t", actionLineToBeChanged.getComment());
+
+		actionLineToBeChanged.setComment(null);
+		assertEquals(null, actionLineToBeChanged.getComment());
+	}
+
 	@Test
 	public void testRebaseInteractiveReword() throws Exception {
 		// create file1 on master

org.eclipse.jgit/src/org/eclipse/jgit/lib/RebaseTodoFile.java

@@ -51,6 +51,7 @@ import java.io.OutputStreamWriter;
 import java.util.LinkedList;
 import java.util.List;
 
+import org.eclipse.jgit.lib.RebaseTodoLine.Action;
 import org.eclipse.jgit.util.IO;
 import org.eclipse.jgit.util.RawParseUtils;
 
@@ -88,8 +89,6 @@ public class RebaseTodoFile {
 		int tokenBegin = 0;
 		List<RebaseTodoLine> r = new LinkedList<RebaseTodoLine>();
 		while (ptr < buf.length) {
-			RebaseTodoLine.Action action = null;
-			AbbreviatedObjectId commit = null;
 			tokenBegin = ptr;
 			ptr = RawParseUtils.nextLF(buf, ptr);
 			int lineStart = tokenBegin;
@@ -99,35 +98,91 @@ public class RebaseTodoFile {
 			// Handle comments
 			if (buf[tokenBegin] == '#') {
 				if (includeComments)
-					r.add(new RebaseTodoLine(RawParseUtils.decode(buf,
+					parseComments(buf, tokenBegin, r, lineEnd);
-							tokenBegin, lineEnd + 1)));
+			} else {
-				continue;
+				// skip leading spaces+tabs+cr
-			}
+				tokenBegin = nextParsableToken(buf, tokenBegin, lineEnd);
-			// skip leading spaces, tabs, CR
-			while (tokenBegin <= lineEnd
-					&& (buf[tokenBegin] == ' ' || buf[tokenBegin] == '\t' || buf[tokenBegin] == '\r'))
-				tokenBegin++;
 				// Handle empty lines (maybe empty after skipping leading
 				// whitespace)
-			if (tokenBegin > lineEnd) {
+				if (tokenBegin == -1) {
 					if (includeComments)
 						r.add(new RebaseTodoLine(RawParseUtils.decode(buf,
 								lineStart, 1 + lineEnd)));
 					continue;
 				}
+				RebaseTodoLine line = parseLine(buf, tokenBegin, lineEnd);
+				if (line == null)
+					continue;
+				r.add(line);
+			}
+		}
+		return r;
+	}
+
+	private static void parseComments(byte[] buf, int tokenBegin,
+			List<RebaseTodoLine> r, int lineEnd) {
+		RebaseTodoLine line = null;
+		String commentString = RawParseUtils.decode(buf,
+				tokenBegin, lineEnd + 1);
+		try {
+			int skip = tokenBegin + 1; // skip '#'
+			skip = nextParsableToken(buf, skip, lineEnd);
+			if (skip != -1) {
+				// try to parse the line as non-comment
+				line = parseLine(buf, skip, lineEnd);
+				// successfully parsed as non-comment line
+				// mark this line as a comment explicitly
+				line.setAction(Action.COMMENT);
+				// use the read line as comment string
+				line.setComment(commentString);
+			}
+		} catch (Exception e) {
+			// parsing as non-comment line failed
+			line = null;
+		} finally {
+			if (line == null)
+				line = new RebaseTodoLine(commentString);
+			r.add(line);
+		}
+	}
+
+	/**
+	 * Skip leading space, tab, CR and LF characters
+	 *
+	 * @param buf
+	 * @param tokenBegin
+	 * @param lineEnd
+	 * @return the token within the range of the given {@code buf} that doesn't
+	 *         need to be skipped, {@code -1} if no such token found within the
+	 *         range (i.e. empty line)
+	 */
+	private static int nextParsableToken(byte[] buf, int tokenBegin, int lineEnd) {
+		while (tokenBegin <= lineEnd
+				&& (buf[tokenBegin] == ' ' || buf[tokenBegin] == '\t' || buf[tokenBegin] == '\r'))
+			tokenBegin++;
+		if (tokenBegin > lineEnd)
+			return -1;
+		return tokenBegin;
+	}
+
+	private static RebaseTodoLine parseLine(byte[] buf, int tokenBegin,
+			int lineEnd) {
+		RebaseTodoLine.Action action = null;
+		AbbreviatedObjectId commit = null;
+
 		int nextSpace = RawParseUtils.next(buf, tokenBegin, ' ');
 		int tokenCount = 0;
-			while (tokenCount < 3 && nextSpace < ptr) {
+		while (tokenCount < 3 && nextSpace < lineEnd) {
 			switch (tokenCount) {
 			case 0:
 				String actionToken = new String(buf, tokenBegin, nextSpace
 						- tokenBegin - 1);
 				tokenBegin = nextSpace;
 				action = RebaseTodoLine.Action.parse(actionToken);
-					break;
-				case 1:
 				if (action == null)
+					return null; // parsing failed
 				break;
+			case 1:
 				nextSpace = RawParseUtils.next(buf, tokenBegin, ' ');
 				String commitToken = new String(buf, tokenBegin, nextSpace
 						- tokenBegin - 1);
@@ -135,16 +190,12 @@ public class RebaseTodoFile {
 				commit = AbbreviatedObjectId.fromString(commitToken);
 				break;
 			case 2:
-					if (action == null)
+				return new RebaseTodoLine(action, commit, RawParseUtils.decode(
-						break;
+						buf, tokenBegin, 1 + lineEnd));
-					r.add(new RebaseTodoLine(action, commit, RawParseUtils
-							.decode(buf, tokenBegin, 1 + lineEnd)));
-					break;
 			}
 			tokenCount++;
 		}
-		}
+		return null;
-		return r;
 	}
 
 	/**