From cd06fb6ddfbcca01481c25b2a1ccdf217c879645 Mon Sep 17 00:00:00 2001
From: Alice Kober-Sotzek <aliceks@google.com>
Date: Tue, 21 Mar 2017 15:19:45 +0100
Subject: [PATCH] Reject (robot) comments with ranges referring to line 0

Line numbers are 1-based except when they are used to indicate a file
comment. To highlight that behavior, comments are added to the REST
documentation as well as to the code. Those comments additionally
indicate whether the values are inclusive or exclusive.

Change-Id: I4b4409ac25acd2124b3e74cb2ee075206ebae0eb
---
 Documentation/rest-api-changes.txt            |  8 +++----
 .../gerrit/extensions/client/Comment.java     | 14 +++++------
 .../gerrit/extensions/client/RangeTest.java   | 24 +++++++++++++++++++
 .../gerrit/reviewdb/client/Comment.java       |  8 +++----
 4 files changed, 39 insertions(+), 15 deletions(-)

diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt
index 45648e597b..b52582638a 100644
--- a/Documentation/rest-api-changes.txt
+++ b/Documentation/rest-api-changes.txt
@@ -5386,10 +5386,10 @@ The `CommentRange` entity describes the range of an inline comment.
 [options="header",cols="1,^1,5"]
 |===========================
 |Field Name    ||Description
-|`start_line`        ||The start line number of the range.
-|`start_character`   ||The character position in the start line.
-|`end_line`          ||The end line number of the range.
-|`end_character`     ||The character position in the end line.
+|`start_line`        ||The start line number of the range. (1-based, inclusive)
+|`start_character`   ||The character position in the start line. (0-based, inclusive)
+|`end_line`          ||The end line number of the range. (1-based, exclusive)
+|`end_character`     ||The character position in the end line. (0-based, exclusive)
 |===========================
 
 [[commit-info]]
diff --git a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/client/Comment.java b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/client/Comment.java
index 5e9ca94d18..2225a99368 100644
--- a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/client/Comment.java
+++ b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/client/Comment.java
@@ -29,7 +29,7 @@ public abstract class Comment {
   public String path;
   public Side side;
   public Integer parent;
-  public Integer line;
+  public Integer line; // value 0 or null indicates a file comment, normal lines start at 1
   public Range range;
   public String inReplyTo;
   public Timestamp updated;
@@ -37,15 +37,15 @@ public abstract class Comment {
   public Boolean unresolved;
 
   public static class Range {
-    public int startLine;
-    public int startCharacter;
-    public int endLine;
-    public int endCharacter;
+    public int startLine; // 1-based, inclusive
+    public int startCharacter; // 0-based, inclusive
+    public int endLine; // 1-based, exclusive
+    public int endCharacter; // 0-based, exclusive
 
     public boolean isValid() {
-      return startLine >= 0
+      return startLine > 0
           && startCharacter >= 0
-          && endLine >= 0
+          && endLine > 0
           && endCharacter >= 0
           && startLine <= endLine
           && (startLine != endLine || startCharacter <= endCharacter);
diff --git a/gerrit-extension-api/src/test/java/com/google/gerrit/extensions/client/RangeTest.java b/gerrit-extension-api/src/test/java/com/google/gerrit/extensions/client/RangeTest.java
index f209e7c17a..9695933679 100644
--- a/gerrit-extension-api/src/test/java/com/google/gerrit/extensions/client/RangeTest.java
+++ b/gerrit-extension-api/src/test/java/com/google/gerrit/extensions/client/RangeTest.java
@@ -62,6 +62,30 @@ public class RangeTest {
     assertThat(range).isInvalid();
   }
 
+  @Test
+  public void zeroStartLineResultsInInvalidRange() {
+    Comment.Range range = createRange(0, 2, 19, 10);
+    assertThat(range).isInvalid();
+  }
+
+  @Test
+  public void zeroEndLineResultsInInvalidRange() {
+    Comment.Range range = createRange(13, 2, 0, 10);
+    assertThat(range).isInvalid();
+  }
+
+  @Test
+  public void zeroStartCharacterResultsInValidRange() {
+    Comment.Range range = createRange(13, 0, 19, 10);
+    assertThat(range).isValid();
+  }
+
+  @Test
+  public void zeroEndCharacterResultsInValidRange() {
+    Comment.Range range = createRange(13, 31, 19, 0);
+    assertThat(range).isValid();
+  }
+
   @Test
   public void startLineGreaterThanEndLineResultsInInvalidRange() {
     Comment.Range range = createRange(20, 2, 19, 10);
diff --git a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/Comment.java b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/Comment.java
index b2c72d9568..cadd52cc0c 100644
--- a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/Comment.java
+++ b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/Comment.java
@@ -131,10 +131,10 @@ public class Comment {
   }
 
   public static class Range {
-    public int startLine;
-    public int startChar;
-    public int endLine;
-    public int endChar;
+    public int startLine; // 1-based, inclusive
+    public int startChar; // 0-based, inclusive
+    public int endLine; // 1-based, exclusive
+    public int endChar; // 0-based, exclusive
 
     public Range(Range r) {
       this(r.startLine, r.startChar, r.endLine, r.endChar);