From 2d07ee0143d0e41071d932d8bd5501d025aa8f4e Mon Sep 17 00:00:00 2001
From: Dave Borowitz <dborowitz@google.com>
Date: Wed, 20 Apr 2016 18:44:44 -0400
Subject: [PATCH] ChangeNoteUtil: Distinguish between 0 and NaN

RawParseUtils.parseBase10 returns 0 if the input was invalid. The only
way to distinguish between this and a real 0 is by checking whether
the pointer advanced. This was preventing reading changes in NoteDb
with 0 as the line or character offset, which is valid.

Change-Id: I7124abe8a2a2543671bf9da0a51792d44a8ac381
---
 .../gerrit/server/notedb/ChangeNoteUtil.java  | 28 ++++++++-----
 .../gerrit/server/notedb/ChangeNotesTest.java | 39 +++++++++++++++++++
 2 files changed, 58 insertions(+), 9 deletions(-)

diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNoteUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNoteUtil.java
index 14e40ab02d..7d3f3637ca 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNoteUtil.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNoteUtil.java
@@ -264,8 +264,11 @@ public class ChangeNoteUtil {
   private static CommentRange parseCommentRange(byte[] note, MutableInteger ptr) {
     CommentRange range = new CommentRange(-1, -1, -1, -1);
 
+    int last = ptr.value;
     int startLine = RawParseUtils.parseBase10(note, ptr.value, ptr);
-    if (startLine == 0) {
+    if (ptr.value == last) {
+      return null;
+    } else if (startLine == 0) {
       range.setEndLine(0);
       ptr.value += 1;
       return range;
@@ -282,30 +285,33 @@ public class ChangeNoteUtil {
       return null;
     }
 
+    last = ptr.value;
     int startChar = RawParseUtils.parseBase10(note, ptr.value, ptr);
-    if (note[ptr.value] == '-') {
+    if (ptr.value == last) {
+      return null;
+    } else if (note[ptr.value] == '-') {
       range.setStartCharacter(startChar);
       ptr.value += 1;
     } else {
       return null;
     }
 
+    last = ptr.value;
     int endLine = RawParseUtils.parseBase10(note, ptr.value, ptr);
-    if (endLine == 0) {
+    if (ptr.value == last) {
       return null;
-    }
-    if (note[ptr.value] == ':') {
+    } else if (note[ptr.value] == ':') {
       range.setEndLine(endLine);
       ptr.value += 1;
     } else {
       return null;
     }
 
+    last = ptr.value;
     int endChar = RawParseUtils.parseBase10(note, ptr.value, ptr);
-    if (endChar == 0) {
+    if (ptr.value == last) {
       return null;
-    }
-    if (note[ptr.value] == '\n') {
+    } else if (note[ptr.value] == '\n') {
       range.setEndCharacter(endChar);
       ptr.value += 1;
     } else {
@@ -377,11 +383,15 @@ public class ChangeNoteUtil {
     int startOfLength =
         RawParseUtils.endOfFooterLineKey(note, curr.value) + 1;
     MutableInteger i = new MutableInteger();
+    i.value = startOfLength;
     int commentLength =
         RawParseUtils.parseBase10(note, startOfLength, i);
+    if (i.value == startOfLength) {
+      throw parseException(changeId, "could not parse %s", LENGTH);
+    }
     int endOfLine = RawParseUtils.nextLF(note, curr.value);
     if (i.value != endOfLine - 1) {
-      throw parseException(changeId, "could not parse %s", PATCH_SET);
+      throw parseException(changeId, "could not parse %s", LENGTH);
     }
     curr.value = endOfLine;
     return checkResult(commentLength, "comment length", changeId);
diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java
index ad2f50abde..7cb74cba02 100644
--- a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java
+++ b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java
@@ -1223,6 +1223,45 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
     assertThat(cm.get(1).getPatchSetId()).isEqualTo(ps1);
   }
 
+  @Test
+  public void patchLineCommentsFileComment() throws Exception {
+    Change c = newChange();
+    ChangeUpdate update = newUpdate(c, otherUser);
+    PatchSet.Id psId = c.currentPatchSetId();
+    RevId revId = new RevId("abcd1234abcd1234abcd1234abcd1234abcd1234");
+
+    PatchLineComment comment = newPublishedComment(psId, "file1",
+        "uuid", null, 0, otherUser, null,
+        TimeUtil.nowTs(), "message", (short) 1, revId.get());
+    update.setPatchSetId(psId);
+    update.putComment(comment);
+    update.commit();
+
+    ChangeNotes notes = newNotes(c);
+    assertThat(notes.getComments())
+        .isEqualTo(ImmutableMultimap.of(revId, comment));
+  }
+
+  @Test
+  public void patchLineCommentsZeroColumns() throws Exception {
+    Change c = newChange();
+    ChangeUpdate update = newUpdate(c, otherUser);
+    PatchSet.Id psId = c.currentPatchSetId();
+    RevId revId = new RevId("abcd1234abcd1234abcd1234abcd1234abcd1234");
+    CommentRange range = new CommentRange(1, 0, 2, 0);
+
+    PatchLineComment comment = newPublishedComment(psId, "file1",
+        "uuid", range, range.getEndLine(), otherUser, null,
+        TimeUtil.nowTs(), "message", (short) 1, revId.get());
+    update.setPatchSetId(psId);
+    update.putComment(comment);
+    update.commit();
+
+    ChangeNotes notes = newNotes(c);
+    assertThat(notes.getComments())
+        .isEqualTo(ImmutableMultimap.of(revId, comment));
+  }
+
   @Test
   public void patchLineCommentNotesFormatSide1() throws Exception {
     Change c = newChange();