Fix off-by-one error in RebaseTodoFile when reading a todo file

Commit messages of length 1 were not read. 'lineEnd' is the offset
of the last character in the line before the terminating LF or CR-LF,
and 'nextSpace' is actually the offset of the character _after_ the
next space. With a one-character commit message, nextSpace == lineEnd.

The code also assumes the commit message to be optional, but actually
failed in that case because it read beyond the line ending. Fix that,
too.

Add a test case for reading a todo file.

Bug: 546245
Change-Id: I368d63615930ea2398a6230e756442fd88870654
Signed-off-by: Thomas Wolf <thomas.wolf@paranor.ch>

diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/RebaseTodoFileTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/RebaseTodoFileTest.java
new file mode 100644 (file)
index 0000000..5cfc75c
--- /dev/null
+++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/RebaseTodoFileTest.java
@@ -0,0 +1,142 @@
+/*
+ * Copyright (C) 2019, Thomas Wolf <thomas.wolf@paranor.ch>
+ * and other copyright owners as documented in the project's IP log.
+ *
+ * This program and the accompanying materials are made available
+ * under the terms of the Eclipse Distribution License v1.0 which
+ * accompanies this distribution, is reproduced below, and is
+ * available at http://www.eclipse.org/org/documents/edl-v10.php
+ *
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or
+ * without modification, are permitted provided that the following
+ * conditions are met:
+ *
+ * - Redistributions of source code must retain the above copyright
+ *   notice, this list of conditions and the following disclaimer.
+ *
+ * - Redistributions in binary form must reproduce the above
+ *   copyright notice, this list of conditions and the following
+ *   disclaimer in the documentation and/or other materials provided
+ *   with the distribution.
+ *
+ * - Neither the name of the Eclipse Foundation, Inc. nor the
+ *   names of its contributors may be used to endorse or promote
+ *   products derived from this software without specific prior
+ *   written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND
+ * CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES,
+ * INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES
+ * OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+ * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR
+ * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT
+ * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
+ * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER
+ * CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT,
+ * STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF
+ * ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+package org.eclipse.jgit.lib;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.fail;
+
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.Arrays;
+import java.util.List;
+
+import org.eclipse.jgit.junit.RepositoryTestCase;
+import org.junit.Test;
+
+public class RebaseTodoFileTest extends RepositoryTestCase {
+
+       private static final String TEST_TODO = "test.todo";
+
+       private void createTodoList(String... lines) throws IOException {
+               Path p = Paths.get(db.getDirectory().getAbsolutePath(), TEST_TODO);
+               Files.write(p, Arrays.asList(lines));
+       }
+
+       @Test
+       public void testReadTodoFile() throws Exception {
+               String[] expected = { "reword " + ObjectId.zeroId().name() + " Foo",
+                               "# A comment in the the todo list",
+                               "pick " + ObjectId.zeroId().name() + " Foo fie",
+                               "squash " + ObjectId.zeroId().name() + " F",
+                               "fixup " + ObjectId.zeroId().name(),
+                               "edit " + ObjectId.zeroId().name() + " f",
+                               "edit " + ObjectId.zeroId().name() + ' ' };
+               createTodoList(expected);
+               RebaseTodoFile todo = new RebaseTodoFile(db);
+               List<RebaseTodoLine> lines = todo.readRebaseTodo(TEST_TODO, true);
+               assertEquals("Expected 7 lines", 7, lines.size());
+               int i = 0;
+               for (RebaseTodoLine line : lines) {
+                       switch (i) {
+                       case 0:
+                               assertEquals("Expected REWORD", RebaseTodoLine.Action.REWORD,
+                                               line.getAction());
+                               assertEquals("Unexpected ID", ObjectId.zeroId().abbreviate(40),
+                                               line.getCommit());
+                               assertEquals("Unexpected Message", "Foo",
+                                               line.getShortMessage());
+                               break;
+                       case 1:
+                               assertEquals("Expected COMMENT", RebaseTodoLine.Action.COMMENT,
+                                               line.getAction());
+                               assertEquals("Unexpected Message",
+                                               "# A comment in the the todo list",
+                                               line.getComment());
+                               break;
+                       case 2:
+                               assertEquals("Expected PICK", RebaseTodoLine.Action.PICK,
+                                               line.getAction());
+                               assertEquals("Unexpected ID", ObjectId.zeroId().abbreviate(40),
+                                               line.getCommit());
+                               assertEquals("Unexpected Message", "Foo fie",
+                                               line.getShortMessage());
+                               break;
+                       case 3:
+                               assertEquals("Expected SQUASH", RebaseTodoLine.Action.SQUASH,
+                                               line.getAction());
+                               assertEquals("Unexpected ID", ObjectId.zeroId().abbreviate(40),
+                                               line.getCommit());
+                               assertEquals("Unexpected Message", "F", line.getShortMessage());
+                               break;
+                       case 4:
+                               assertEquals("Expected FIXUP", RebaseTodoLine.Action.FIXUP,
+                                               line.getAction());
+                               assertEquals("Unexpected ID", ObjectId.zeroId().abbreviate(40),
+                                               line.getCommit());
+                               assertEquals("Unexpected Message", "", line.getShortMessage());
+                               break;
+                       case 5:
+                               assertEquals("Expected EDIT", RebaseTodoLine.Action.EDIT,
+                                               line.getAction());
+                               assertEquals("Unexpected ID", ObjectId.zeroId().abbreviate(40),
+                                               line.getCommit());
+                               assertEquals("Unexpected Message", "f", line.getShortMessage());
+                               break;
+                       case 6:
+                               assertEquals("Expected EDIT", RebaseTodoLine.Action.EDIT,
+                                               line.getAction());
+                               assertEquals("Unexpected ID", ObjectId.zeroId().abbreviate(40),
+                                               line.getCommit());
+                               assertEquals("Unexpected Message", "", line.getShortMessage());
+                               break;
+                       default:
+                               fail("Too many lines");
+                               return;
+                       }
+                       i++;
+               }
+       }
+}

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/lib/RebaseTodoFile.java b/org.eclipse.jgit/src/org/eclipse/jgit/lib/RebaseTodoFile.java
index 06b4b227c8722fd1a065f76c83c69263c0c481e9..c0fcd4161fa9d95e115797f59db4ad8db395d156 100644 (file)
--- a/org.eclipse.jgit/src/org/eclipse/jgit/lib/RebaseTodoFile.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/lib/RebaseTodoFile.java
@@ -179,7 +179,7 @@ public class RebaseTodoFile {
 
                int nextSpace = RawParseUtils.next(buf, tokenBegin, ' ');
                int tokenCount = 0;
-               while (tokenCount < 3 && nextSpace < lineEnd) {
+               while (tokenCount < 3 && nextSpace <= lineEnd) {
                        switch (tokenCount) {
                        case 0:
                                String actionToken = new String(buf, tokenBegin,
@@ -191,8 +191,14 @@ public class RebaseTodoFile {
                                break;
                        case 1:
                                nextSpace = RawParseUtils.next(buf, tokenBegin, ' ');
-                               String commitToken = new String(buf, tokenBegin,
-                                               nextSpace - tokenBegin - 1, UTF_8);
+                               String commitToken;
+                               if (nextSpace > lineEnd + 1) {
+                                       commitToken = new String(buf, tokenBegin,
+                                                       lineEnd - tokenBegin + 1, UTF_8);
+                               } else {
+                                       commitToken = new String(buf, tokenBegin,
+                                                       nextSpace - tokenBegin - 1, UTF_8);
+                               }
                                tokenBegin = nextSpace;
                                commit = AbbreviatedObjectId.fromString(commitToken);
                                break;