Merge changes Ie41c264a,I8c87a2d6
* changes:
Remove unused variables
Add more info into Comment.Range comments
diff --git a/java/com/google/gerrit/common/data/PatchScript.java b/java/com/google/gerrit/common/data/PatchScript.java
index 3428580..28fda17 100644
--- a/java/com/google/gerrit/common/data/PatchScript.java
+++ b/java/com/google/gerrit/common/data/PatchScript.java
@@ -56,7 +56,6 @@
private CommentDetail comments;
private List<Patch> history;
private boolean hugeFile;
- private boolean intralineDifference;
private boolean intralineFailure;
private boolean intralineTimeout;
private boolean binary;
@@ -83,7 +82,6 @@
CommentDetail cd,
List<Patch> hist,
boolean hf,
- boolean id,
boolean idf,
boolean idt,
boolean bin,
@@ -108,7 +106,6 @@
comments = cd;
history = hist;
hugeFile = hf;
- intralineDifference = id;
intralineFailure = idf;
intralineTimeout = idt;
binary = bin;
@@ -178,10 +175,6 @@
return diffPrefs.ignoreWhitespace != Whitespace.IGNORE_NONE;
}
- public boolean hasIntralineDifference() {
- return intralineDifference;
- }
-
public boolean hasIntralineFailure() {
return intralineFailure;
}
diff --git a/java/com/google/gerrit/reviewdb/client/Comment.java b/java/com/google/gerrit/reviewdb/client/Comment.java
index 94e7583..eb2aa52 100644
--- a/java/com/google/gerrit/reviewdb/client/Comment.java
+++ b/java/com/google/gerrit/reviewdb/client/Comment.java
@@ -124,6 +124,24 @@
}
}
+ /**
+ * The Range class defines continuous range of character.
+ *
+ * <p>The pair (startLine, startChar) defines the first character in the range. The pair (endLine,
+ * endChar) defines the first character AFTER the range (i.e. it doesn't belong the range).
+ * (endLine, endChar) must be a valid character inside text, except EOF case.</p>
+ * <p>Special cases:</p>
+ * <ul>
+ * <li>Zero length range: (startLine, startChar) = (endLine, endChar). Range defines insert
+ * position right before the (startLine, startChar) character (for {@link FixReplacement)
+ * <li>EOF case - range includes the last character in the file:
+ * <ul>
+ * <li>if a file ends with EOL mark, then (endLine, endChar) = (num_of_lines + 1, 0)
+ * <li>if a file doesn't end with EOL mark, then
+ * (endLine, endChar) = (num_of_lines, num_of_chars_in_last_line)
+ * </ul>
+ * </ul>
+ */
public static class Range implements Comparable<Range> {
private static final Comparator<Range> RANGE_COMPARATOR =
Comparator.<Range>comparingInt(range -> range.startLine)
@@ -131,10 +149,10 @@
.thenComparingInt(range -> range.endLine)
.thenComparingInt(range -> range.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 int startLine; // 1-based
+ public int startChar; // 0-based
+ public int endLine; // 1-based
+ public int endChar; // 0-based
public Range(Range r) {
this(r.startLine, r.startChar, r.endLine, r.endChar);
diff --git a/java/com/google/gerrit/server/patch/PatchScriptBuilder.java b/java/com/google/gerrit/server/patch/PatchScriptBuilder.java
index acf88e1..b9c13b1 100644
--- a/java/com/google/gerrit/server/patch/PatchScriptBuilder.java
+++ b/java/com/google/gerrit/server/patch/PatchScriptBuilder.java
@@ -121,7 +121,6 @@
private PatchScript build(PatchListEntry content, CommentDetail comments, List<Patch> history)
throws IOException {
- boolean intralineDifferenceIsPossible = true;
boolean intralineFailure = false;
boolean intralineTimeout = false;
@@ -134,9 +133,7 @@
edits = new ArrayList<>(content.getEdits());
ImmutableSet<Edit> editsDueToRebase = content.getEditsDueToRebase();
- if (!isModify(content)) {
- intralineDifferenceIsPossible = false;
- } else if (diffPrefs.intralineDifference) {
+ if (isModify(content) && diffPrefs.intralineDifference) {
IntraLineDiff d =
patchListCache.getIntraLineDiff(
IntraLineDiffKey.create(a.id, b.id, diffPrefs.ignoreWhitespace),
@@ -149,21 +146,17 @@
break;
case DISABLED:
- intralineDifferenceIsPossible = false;
break;
case ERROR:
- intralineDifferenceIsPossible = false;
intralineFailure = true;
break;
case TIMEOUT:
- intralineDifferenceIsPossible = false;
intralineTimeout = true;
break;
}
} else {
- intralineDifferenceIsPossible = false;
intralineFailure = true;
}
}
@@ -223,7 +216,6 @@
comments,
history,
hugeFile,
- intralineDifferenceIsPossible,
intralineFailure,
intralineTimeout,
content.getPatchType() == Patch.PatchType.BINARY,
diff --git a/javatests/com/google/gerrit/server/fixes/FixReplacementInterpreterTest.java b/javatests/com/google/gerrit/server/fixes/FixReplacementInterpreterTest.java
index 2174927..fb49657 100644
--- a/javatests/com/google/gerrit/server/fixes/FixReplacementInterpreterTest.java
+++ b/javatests/com/google/gerrit/server/fixes/FixReplacementInterpreterTest.java
@@ -148,6 +148,22 @@
.isEqualTo("First line\nA modification\nThird line\n");
}
+ @Test()
+ public void startAfterEndOfLineMarkThrowsAnException() throws Exception {
+ FixReplacement fixReplacement =
+ new FixReplacement(filePath1, new Range(1, 11, 2, 6), "A modification");
+ mockFileContent(filePath1, "First line\nSecond line\nThird line\n");
+ assertThrows(ResourceConflictException.class, () -> toTreeModifications(fixReplacement));
+ }
+
+ @Test()
+ public void endAfterEndOfLineMarkThrowsAnException() throws Exception {
+ FixReplacement fixReplacement =
+ new FixReplacement(filePath1, new Range(2, 0, 2, 12), "A modification");
+ mockFileContent(filePath1, "First line\nSecond line\nThird line\n");
+ assertThrows(ResourceConflictException.class, () -> toTreeModifications(fixReplacement));
+ }
+
@Test
public void replacementsMayTouch() throws Exception {
FixReplacement fixReplacement1 =
@@ -180,6 +196,34 @@
}
@Test
+ public void replacementsCanChangeLastLine() throws Exception {
+ FixReplacement fixReplacement =
+ new FixReplacement(filePath1, new Range(3, 0, 4, 0), "New content\n");
+ mockFileContent(filePath1, "First line\nSecond line\nThird line\n");
+
+ List<TreeModification> treeModifications = toTreeModifications(fixReplacement);
+ assertThatList(treeModifications)
+ .onlyElement()
+ .asChangeFileContentModification()
+ .newContent()
+ .isEqualTo("First line\nSecond line\nNew content\n");
+ }
+
+ @Test
+ public void replacementsCanChangeLastLineWithoutEOLMark() throws Exception {
+ FixReplacement fixReplacement =
+ new FixReplacement(filePath1, new Range(3, 0, 3, 10), "New content\n");
+ mockFileContent(filePath1, "First line\nSecond line\nThird line");
+
+ List<TreeModification> treeModifications = toTreeModifications(fixReplacement);
+ assertThatList(treeModifications)
+ .onlyElement()
+ .asChangeFileContentModification()
+ .newContent()
+ .isEqualTo("First line\nSecond line\nNew content\n");
+ }
+
+ @Test
public void replacementsCanModifySeveralFilesInAnyOrder() throws Exception {
FixReplacement fixReplacement1 =
new FixReplacement(filePath1, new Range(1, 1, 3, 2), "Modified content");
