Merge "Update Reviewers type in common.ts"
diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt
index 7e1cb56..8bd02a8 100644
--- a/Documentation/rest-api-changes.txt
+++ b/Documentation/rest-api-changes.txt
@@ -5585,9 +5585,6 @@
differences are reported in the result. Valid values are `IGNORE_NONE`,
`IGNORE_TRAILING`, `IGNORE_LEADING_AND_TRAILING` or `IGNORE_ALL`.
-The `context` parameter can be specified to control the number of lines of surrounding context
-in the diff. Valid values are `ALL` or number of lines.
-
[[preview-fix]]
=== Preview fix
--
@@ -7810,7 +7807,8 @@
The `RobotCommentInfo` entity contains information about a robot inline
comment.
-`RobotCommentInfo` has the same fields as <<comment-info,CommentInfo>>.
+`RobotCommentInfo` has the same fields as <<comment-info,CommentInfo>>
+except for the `unresolved` field which doesn't exist for robot comments.
In addition `RobotCommentInfo` has the following fields:
[options="header",cols="1,^1,5"]
@@ -7830,8 +7828,35 @@
The `RobotCommentInput` entity contains information for creating an inline
robot comment.
-`RobotCommentInput` has the same fields as
-<<robot-comment-info,RobotCommentInfo>>.
+[options="header",cols="1,^1,5"]
+|===========================
+|Field Name ||Description
+|`path` ||
+link:#file-id[The file path] for which the inline comment should be added.
+|`side` |optional|
+The side on which the comment should be added. +
+Allowed values are `REVISION` and `PARENT`. +
+If not set, the default is `REVISION`.
+|`line` |optional|
+The number of the line for which the comment should be added. +
+`0` if it is a file comment. +
+If neither line nor range is set, a file comment is added. +
+If range is set, this value is ignored in favor of the `end_line` of the range.
+|`range` |optional|
+The range of the comment as a link:#comment-range[CommentRange]
+entity.
+|`in_reply_to` |optional|
+The URL encoded UUID of the comment to which this comment is a reply.
+|`message` |optional|
+The comment message.
+|`robot_id` ||The ID of the robot that generated this comment.
+|`robot_run_id` ||An ID of the run of the robot.
+|`url` |optional|URL to more information.
+|`properties` |optional|Robot specific properties as map that maps arbitrary
+keys to values.
+|`fix_suggestions`|optional|Suggested fixes for this robot comment as a list of
+<<fix-suggestion-info,FixSuggestionInfo>> entities.
+|===========================
[[rule-input]]
=== RuleInput
diff --git a/java/com/google/gerrit/acceptance/testsuite/change/PerPatchsetOperationsImpl.java b/java/com/google/gerrit/acceptance/testsuite/change/PerPatchsetOperationsImpl.java
index b8c841c..eda6c7e 100644
--- a/java/com/google/gerrit/acceptance/testsuite/change/PerPatchsetOperationsImpl.java
+++ b/java/com/google/gerrit/acceptance/testsuite/change/PerPatchsetOperationsImpl.java
@@ -259,9 +259,6 @@
.ifPresent(range -> newRobotComment.setLineNbrAndRange(null, range));
robotCommentCreation
- .unresolved()
- .ifPresent(unresolved -> newRobotComment.unresolved = unresolved);
- robotCommentCreation
.parentUuid()
.ifPresent(parentUuid -> newRobotComment.parentUuid = parentUuid);
robotCommentCreation.url().ifPresent(url -> newRobotComment.url = url);
diff --git a/java/com/google/gerrit/acceptance/testsuite/change/TestRobotCommentCreation.java b/java/com/google/gerrit/acceptance/testsuite/change/TestRobotCommentCreation.java
index 809190d..558af3f 100644
--- a/java/com/google/gerrit/acceptance/testsuite/change/TestRobotCommentCreation.java
+++ b/java/com/google/gerrit/acceptance/testsuite/change/TestRobotCommentCreation.java
@@ -41,8 +41,6 @@
public abstract Optional<CommentSide> side();
- public abstract Optional<Boolean> unresolved();
-
public abstract Optional<String> parentUuid();
public abstract Optional<String> tag();
@@ -152,18 +150,6 @@
abstract Builder side(CommentSide side);
- /** Indicates a resolved comment. */
- public Builder resolved() {
- return unresolved(false);
- }
-
- /** Indicates an unresolved comment. */
- public Builder unresolved() {
- return unresolved(true);
- }
-
- abstract Builder unresolved(boolean unresolved);
-
/**
* UUID of another comment to which this comment is a reply. This comment must have similar
* attributes (e.g. file, line, side) as the parent comment. The parent comment must be a
diff --git a/java/com/google/gerrit/entities/Comment.java b/java/com/google/gerrit/entities/Comment.java
index e7ac4e4..37b8620 100644
--- a/java/com/google/gerrit/entities/Comment.java
+++ b/java/com/google/gerrit/entities/Comment.java
@@ -231,7 +231,6 @@
private String revId;
public String serverId;
- public boolean unresolved;
public Comment(Comment c) {
this(
@@ -240,15 +239,13 @@
new Timestamp(c.writtenOn.getTime()),
c.side,
c.message,
- c.serverId,
- c.unresolved);
+ c.serverId);
this.lineNbr = c.lineNbr;
this.realAuthor = c.realAuthor;
this.parentUuid = c.parentUuid;
this.range = c.range != null ? new Range(c.range) : null;
this.tag = c.tag;
this.revId = c.revId;
- this.unresolved = c.unresolved;
}
public Comment(
@@ -257,8 +254,7 @@
Timestamp writtenOn,
short side,
String message,
- String serverId,
- boolean unresolved) {
+ String serverId) {
this.key = key;
this.author = new Comment.Identity(author);
this.realAuthor = this.author;
@@ -266,7 +262,6 @@
this.side = side;
this.message = message;
this.serverId = serverId;
- this.unresolved = unresolved;
}
public void setLineNbrAndRange(
@@ -334,8 +329,7 @@
&& Objects.equals(range, c.range)
&& Objects.equals(tag, c.tag)
&& Objects.equals(revId, c.revId)
- && Objects.equals(serverId, c.serverId)
- && unresolved == c.unresolved;
+ && Objects.equals(serverId, c.serverId);
}
@Override
@@ -352,8 +346,7 @@
range,
tag,
revId,
- serverId,
- unresolved);
+ serverId);
}
@Override
@@ -373,7 +366,6 @@
.add("parentUuid", Objects.toString(parentUuid, ""))
.add("range", Objects.toString(range, ""))
.add("revId", Objects.toString(revId, ""))
- .add("tag", Objects.toString(tag, ""))
- .add("unresolved", unresolved);
+ .add("tag", Objects.toString(tag, ""));
}
}
diff --git a/java/com/google/gerrit/entities/HumanComment.java b/java/com/google/gerrit/entities/HumanComment.java
index 8b687cc..50bee8d 100644
--- a/java/com/google/gerrit/entities/HumanComment.java
+++ b/java/com/google/gerrit/entities/HumanComment.java
@@ -15,6 +15,7 @@
package com.google.gerrit.entities;
import java.sql.Timestamp;
+import java.util.Objects;
/**
* This class represents inline human comments in NoteDb. This means it determines the JSON format
@@ -27,6 +28,8 @@
*/
public class HumanComment extends Comment {
+ public boolean unresolved;
+
public HumanComment(
Key key,
Account.Id author,
@@ -35,7 +38,8 @@
String message,
String serverId,
boolean unresolved) {
- super(key, author, writtenOn, side, message, serverId, unresolved);
+ super(key, author, writtenOn, side, message, serverId);
+ this.unresolved = unresolved;
}
public HumanComment(HumanComment comment) {
@@ -49,19 +53,23 @@
@Override
public String toString() {
- return toStringHelper().toString();
+ return toStringHelper().add("unresolved", unresolved).toString();
}
@Override
- public boolean equals(Object o) {
- if (!(o instanceof HumanComment)) {
+ public boolean equals(Object otherObject) {
+ if (!(otherObject instanceof HumanComment)) {
return false;
}
- return super.equals(o);
+ if (!super.equals(otherObject)) {
+ return false;
+ }
+ HumanComment otherComment = (HumanComment) otherObject;
+ return unresolved == otherComment.unresolved;
}
@Override
public int hashCode() {
- return super.hashCode();
+ return Objects.hash(super.hashCode(), unresolved);
}
}
diff --git a/java/com/google/gerrit/entities/RobotComment.java b/java/com/google/gerrit/entities/RobotComment.java
index 03ddad5..e2e4114 100644
--- a/java/com/google/gerrit/entities/RobotComment.java
+++ b/java/com/google/gerrit/entities/RobotComment.java
@@ -35,7 +35,7 @@
String serverId,
String robotId,
String robotRunId) {
- super(key, author, writtenOn, side, message, serverId, false);
+ super(key, author, writtenOn, side, message, serverId);
this.robotId = robotId;
this.robotRunId = robotRunId;
}
diff --git a/java/com/google/gerrit/extensions/api/changes/DraftInput.java b/java/com/google/gerrit/extensions/api/changes/DraftInput.java
index b3c2786..74f626f 100644
--- a/java/com/google/gerrit/extensions/api/changes/DraftInput.java
+++ b/java/com/google/gerrit/extensions/api/changes/DraftInput.java
@@ -19,18 +19,19 @@
public class DraftInput extends Comment {
public String tag;
+ public Boolean unresolved;
@Override
public boolean equals(Object o) {
if (super.equals(o)) {
DraftInput di = (DraftInput) o;
- return Objects.equals(tag, di.tag);
+ return Objects.equals(tag, di.tag) && Objects.equals(unresolved, di.unresolved);
}
return false;
}
@Override
public int hashCode() {
- return Objects.hash(super.hashCode(), tag);
+ return Objects.hash(super.hashCode(), tag, unresolved);
}
}
diff --git a/java/com/google/gerrit/extensions/api/changes/FileApi.java b/java/com/google/gerrit/extensions/api/changes/FileApi.java
index 8d9b2d5..26f9452 100644
--- a/java/com/google/gerrit/extensions/api/changes/FileApi.java
+++ b/java/com/google/gerrit/extensions/api/changes/FileApi.java
@@ -52,7 +52,6 @@
abstract class DiffRequest {
private String base;
- private Integer context;
private Boolean intraline;
private Whitespace whitespace;
private OptionalInt parent = OptionalInt.empty();
@@ -64,11 +63,6 @@
return this;
}
- public DiffRequest withContext(int context) {
- this.context = context;
- return this;
- }
-
public DiffRequest withIntraline(boolean intraline) {
this.intraline = intraline;
return this;
@@ -88,10 +82,6 @@
return base;
}
- public Integer getContext() {
- return context;
- }
-
public Boolean getIntraline() {
return intraline;
}
diff --git a/java/com/google/gerrit/extensions/api/changes/ReviewInput.java b/java/com/google/gerrit/extensions/api/changes/ReviewInput.java
index a213ed6..7ecc0a6 100644
--- a/java/com/google/gerrit/extensions/api/changes/ReviewInput.java
+++ b/java/com/google/gerrit/extensions/api/changes/ReviewInput.java
@@ -100,9 +100,11 @@
PUBLISH_ALL_REVISIONS
}
- public static class CommentInput extends Comment {}
+ public static class CommentInput extends Comment {
+ public Boolean unresolved;
+ }
- public static class RobotCommentInput extends CommentInput {
+ public static class RobotCommentInput extends Comment {
public String robotId;
public String robotRunId;
public String url;
diff --git a/java/com/google/gerrit/extensions/client/Comment.java b/java/com/google/gerrit/extensions/client/Comment.java
index faa9f69..634992e 100644
--- a/java/com/google/gerrit/extensions/client/Comment.java
+++ b/java/com/google/gerrit/extensions/client/Comment.java
@@ -37,7 +37,6 @@
public String inReplyTo;
public Timestamp updated;
public String message;
- public Boolean unresolved;
/**
* Hex commit SHA1 (as 40 characters hex string) of the commit of the patchset to which this
@@ -128,7 +127,6 @@
&& Objects.equals(inReplyTo, c.inReplyTo)
&& Objects.equals(updated, c.updated)
&& Objects.equals(message, c.message)
- && Objects.equals(unresolved, c.unresolved)
&& Objects.equals(commitId, c.commitId);
}
return false;
diff --git a/java/com/google/gerrit/extensions/client/DiffPreferencesInfo.java b/java/com/google/gerrit/extensions/client/DiffPreferencesInfo.java
index ed01a4d..6d52a93 100644
--- a/java/com/google/gerrit/extensions/client/DiffPreferencesInfo.java
+++ b/java/com/google/gerrit/extensions/client/DiffPreferencesInfo.java
@@ -28,12 +28,6 @@
/** Default line length. */
public static final int DEFAULT_LINE_LENGTH = 100;
- /** Context setting to display the entire file. */
- public static final short WHOLE_FILE_CONTEXT = -1;
-
- /** Typical valid choices for the default context setting. */
- public static final short[] CONTEXT_CHOICES = {3, 10, 25, 50, 75, 100, WHOLE_FILE_CONTEXT};
-
public enum Whitespace {
IGNORE_NONE,
IGNORE_TRAILING,
diff --git a/java/com/google/gerrit/extensions/common/CommentInfo.java b/java/com/google/gerrit/extensions/common/CommentInfo.java
index 19d721b..fcce2b3 100644
--- a/java/com/google/gerrit/extensions/common/CommentInfo.java
+++ b/java/com/google/gerrit/extensions/common/CommentInfo.java
@@ -22,6 +22,7 @@
public AccountInfo author;
public String tag;
public String changeMessageId;
+ public Boolean unresolved;
/**
* A list of {@link ContextLineInfo}, that is, a list of pairs of {line_num, line_text} of the
@@ -33,13 +34,15 @@
public boolean equals(Object o) {
if (super.equals(o)) {
CommentInfo ci = (CommentInfo) o;
- return Objects.equals(author, ci.author) && Objects.equals(tag, ci.tag);
+ return Objects.equals(author, ci.author)
+ && Objects.equals(tag, ci.tag)
+ && Objects.equals(unresolved, ci.unresolved);
}
return false;
}
@Override
public int hashCode() {
- return Objects.hash(super.hashCode(), author, tag);
+ return Objects.hash(super.hashCode(), author, tag, unresolved);
}
}
diff --git a/java/com/google/gerrit/prettify/common/EditHunk.java b/java/com/google/gerrit/prettify/common/EditHunk.java
new file mode 100644
index 0000000..68cb796
--- /dev/null
+++ b/java/com/google/gerrit/prettify/common/EditHunk.java
@@ -0,0 +1,92 @@
+// Copyright (C) 2009 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.prettify.common;
+
+import java.util.List;
+import org.eclipse.jgit.diff.Edit;
+
+/**
+ * This is a legacy class. It was only simplified but not improved regarding readability or code
+ * health. Feel free to completely rewrite it or replace it with some other, better code.
+ */
+public class EditHunk {
+ private final List<Edit> edits;
+
+ private int curIdx;
+ private Edit curEdit;
+
+ private int aCur;
+ private int bCur;
+ private final int aEnd;
+ private final int bEnd;
+
+ public EditHunk(List<Edit> edits, int aSize, int bSize) {
+ this.edits = edits;
+
+ curIdx = 0;
+ curEdit = edits.get(curIdx);
+
+ aCur = 0;
+ bCur = 0;
+ aEnd = aSize;
+ bEnd = bSize;
+ }
+
+ public int getCurA() {
+ return aCur;
+ }
+
+ public int getCurB() {
+ return bCur;
+ }
+
+ public void incA() {
+ aCur++;
+ }
+
+ public void incB() {
+ bCur++;
+ }
+
+ public void incBoth() {
+ incA();
+ incB();
+ }
+
+ public boolean isUnmodifiedLine() {
+ return !isDeletedA() && !isInsertedB();
+ }
+
+ public boolean isDeletedA() {
+ return curEdit.getBeginA() <= aCur && aCur < curEdit.getEndA();
+ }
+
+ public boolean isInsertedB() {
+ return curEdit.getBeginB() <= bCur && bCur < curEdit.getEndB();
+ }
+
+ public boolean next() {
+ if (!in(curEdit)) {
+ if (curIdx < edits.size() - 1) {
+ curEdit = edits.get(++curIdx);
+ }
+ }
+ return aCur < aEnd || bCur < bEnd;
+ }
+
+ private boolean in(Edit edit) {
+ return aCur < edit.getEndA() || bCur < edit.getEndB();
+ }
+}
diff --git a/java/com/google/gerrit/prettify/common/EditList.java b/java/com/google/gerrit/prettify/common/EditList.java
deleted file mode 100644
index 172a346..0000000
--- a/java/com/google/gerrit/prettify/common/EditList.java
+++ /dev/null
@@ -1,174 +0,0 @@
-// Copyright (C) 2009 The Android Open Source Project
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-// http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-
-package com.google.gerrit.prettify.common;
-
-import java.util.Iterator;
-import java.util.List;
-import org.eclipse.jgit.diff.Edit;
-
-public class EditList {
- private final List<Edit> edits;
- private final int context;
- private final int aSize;
- private final int bSize;
-
- public EditList(final List<Edit> edits, int contextLines, int aSize, int bSize) {
- this.edits = edits;
- this.context = contextLines;
- this.aSize = aSize;
- this.bSize = bSize;
- }
-
- public List<Edit> getEdits() {
- return edits;
- }
-
- public Iterable<Hunk> getHunks() {
- return () ->
- new Iterator<Hunk>() {
- private int curIdx;
-
- @Override
- public boolean hasNext() {
- return curIdx < edits.size();
- }
-
- @Override
- public Hunk next() {
- final int c = curIdx;
- final int e = findCombinedEnd(c);
- curIdx = e + 1;
- return new Hunk(c, e);
- }
-
- @Override
- public void remove() {
- throw new UnsupportedOperationException();
- }
- };
- }
-
- private int findCombinedEnd(int i) {
- int end = i + 1;
- while (end < edits.size() && (combineA(end) || combineB(end))) {
- end++;
- }
- return end - 1;
- }
-
- private boolean combineA(int i) {
- final Edit s = edits.get(i);
- final Edit e = edits.get(i - 1);
- // + 1 to prevent '... skipping 1 common line ...' messages.
- return s.getBeginA() - e.getEndA() <= 2 * context + 1;
- }
-
- private boolean combineB(int i) {
- final int s = edits.get(i).getBeginB();
- final int e = edits.get(i - 1).getEndB();
- // + 1 to prevent '... skipping 1 common line ...' messages.
- return s - e <= 2 * context + 1;
- }
-
- public class Hunk {
- private int curIdx;
- private Edit curEdit;
- private final int endIdx;
- private final Edit endEdit;
-
- private int aCur;
- private int bCur;
- private final int aEnd;
- private final int bEnd;
-
- private Hunk(int ci, int ei) {
- curIdx = ci;
- endIdx = ei;
- curEdit = edits.get(curIdx);
- endEdit = edits.get(endIdx);
-
- aCur = Math.max(0, curEdit.getBeginA() - context);
- bCur = Math.max(0, curEdit.getBeginB() - context);
- aEnd = Math.min(aSize, endEdit.getEndA() + context);
- bEnd = Math.min(bSize, endEdit.getEndB() + context);
- }
-
- public int getCurA() {
- return aCur;
- }
-
- public int getCurB() {
- return bCur;
- }
-
- public Edit getCurEdit() {
- return curEdit;
- }
-
- public int getEndA() {
- return aEnd;
- }
-
- public int getEndB() {
- return bEnd;
- }
-
- public void incA() {
- aCur++;
- }
-
- public void incB() {
- bCur++;
- }
-
- public void incBoth() {
- incA();
- incB();
- }
-
- public boolean isStartOfFile() {
- return aCur == 0 && bCur == 0;
- }
-
- public boolean isContextLine() {
- return !isModifiedLine();
- }
-
- public boolean isDeletedA() {
- return curEdit.getBeginA() <= aCur && aCur < curEdit.getEndA();
- }
-
- public boolean isInsertedB() {
- return curEdit.getBeginB() <= bCur && bCur < curEdit.getEndB();
- }
-
- public boolean isModifiedLine() {
- return isDeletedA() || isInsertedB();
- }
-
- public boolean next() {
- if (!in(curEdit)) {
- if (curIdx < endIdx) {
- curEdit = edits.get(++curIdx);
- }
- }
- return aCur < aEnd || bCur < bEnd;
- }
-
- private boolean in(Edit edit) {
- return aCur < edit.getEndA() || bCur < edit.getEndB();
- }
- }
-}
diff --git a/java/com/google/gerrit/server/api/changes/FileApiImpl.java b/java/com/google/gerrit/server/api/changes/FileApiImpl.java
index c506b2e..cf9f243 100644
--- a/java/com/google/gerrit/server/api/changes/FileApiImpl.java
+++ b/java/com/google/gerrit/server/api/changes/FileApiImpl.java
@@ -123,9 +123,6 @@
if (r.getBase() != null) {
getDiff.setBase(r.getBase());
}
- if (r.getContext() != null) {
- getDiff.setContext(r.getContext());
- }
if (r.getIntraline() != null) {
getDiff.setIntraline(r.getIntraline());
}
diff --git a/java/com/google/gerrit/server/change/CommentThread.java b/java/com/google/gerrit/server/change/CommentThread.java
index 7b729d2..0265f60 100644
--- a/java/com/google/gerrit/server/change/CommentThread.java
+++ b/java/com/google/gerrit/server/change/CommentThread.java
@@ -17,9 +17,11 @@
import com.google.auto.value.AutoValue;
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
-import com.google.common.collect.Iterables;
+import com.google.common.collect.Streams;
import com.google.gerrit.entities.Comment;
+import com.google.gerrit.entities.HumanComment;
import java.util.List;
+import java.util.Optional;
/**
* Representation of a comment thread.
@@ -37,7 +39,14 @@
/** Whether the whole thread is considered as unresolved. */
public boolean unresolved() {
- return Iterables.getLast(comments()).unresolved;
+ Optional<HumanComment> lastHumanComment =
+ Streams.findLast(
+ comments().stream()
+ .filter(HumanComment.class::isInstance)
+ .map(HumanComment.class::cast));
+ // We often use false == null for boolean fields. It's also a safe fall-back if no human comment
+ // is part of the thread.
+ return lastHumanComment.map(comment -> comment.unresolved).orElse(false);
}
public static <T extends Comment> Builder<T> builder() {
diff --git a/java/com/google/gerrit/server/logging/LoggingContext.java b/java/com/google/gerrit/server/logging/LoggingContext.java
index 36c7e9e..671c224 100644
--- a/java/com/google/gerrit/server/logging/LoggingContext.java
+++ b/java/com/google/gerrit/server/logging/LoggingContext.java
@@ -42,6 +42,15 @@
private static final ThreadLocal<MutableTags> tags = new ThreadLocal<>();
private static final ThreadLocal<Boolean> forceLogging = new ThreadLocal<>();
private static final ThreadLocal<Boolean> performanceLogging = new ThreadLocal<>();
+
+ /**
+ * When copying the logging context to a new thread we need to ensure that the performance log
+ * records that are added in the new thread are added to the same {@link
+ * MutablePerformanceLogRecords} instance (see {@link LoggingContextAwareRunnable} and {@link
+ * LoggingContextAwareCallable}). This is important since performance log records are processed
+ * only at the end of the request and performance log records that are created in another thread
+ * should not get lost.
+ */
private static final ThreadLocal<MutablePerformanceLogRecords> performanceLogRecords =
new ThreadLocal<>();
@@ -57,11 +66,6 @@
return runnable;
}
- // Pass the MutablePerformanceLogRecords instance into the LoggingContextAwareRunnable
- // constructor so that performance log records that are created in the wrapped runnable are
- // added to this MutablePerformanceLogRecords instance. This is important since performance
- // log records are processed only at the end of the request and performance log records that
- // are created in another thread should not get lost.
return new LoggingContextAwareRunnable(
runnable, getInstance().getMutablePerformanceLogRecords());
}
@@ -71,11 +75,6 @@
return callable;
}
- // Pass the MutablePerformanceLogRecords instance into the LoggingContextAwareCallable
- // constructor so that performance log records that are created in the wrapped runnable are
- // added to this MutablePerformanceLogRecords instance. This is important since performance
- // log records are processed only at the end of the request and performance log records that
- // are created in another thread should not get lost.
return new LoggingContextAwareCallable<>(
callable, getInstance().getMutablePerformanceLogRecords());
}
@@ -233,12 +232,7 @@
* <p><strong>Attention:</strong> The passed in {@link MutablePerformanceLogRecords} instance is
* directly stored in the logging context.
*
- * <p>This method is intended to be only used when the logging context is copied to a new thread
- * to ensure that the performance log records that are added in the new thread are added to the
- * same {@link MutablePerformanceLogRecords} instance (see {@link LoggingContextAwareRunnable} and
- * {@link LoggingContextAwareCallable}). This is important since performance log records are
- * processed only at the end of the request and performance log records that are created in
- * another thread should not get lost.
+ * <p>This method is intended to be only used when the logging context is copied to a new thread.
*
* @param mutablePerformanceLogRecords the {@link MutablePerformanceLogRecords} instance in which
* performance log records should be stored
diff --git a/java/com/google/gerrit/server/logging/LoggingContextAwareCallable.java b/java/com/google/gerrit/server/logging/LoggingContextAwareCallable.java
index d2701d7..1adee1b 100644
--- a/java/com/google/gerrit/server/logging/LoggingContextAwareCallable.java
+++ b/java/com/google/gerrit/server/logging/LoggingContextAwareCallable.java
@@ -75,13 +75,6 @@
loggingCtx.setTags(tags);
loggingCtx.forceLogging(forceLogging);
loggingCtx.performanceLogging(performanceLogging);
-
- // For the performance log records use the {@link MutablePerformanceLogRecords} instance from
- // the logging context of the calling thread in the logging context of the new thread. This way
- // performance log records that are created from the new thread are available from the logging
- // context of the calling thread. This is important since performance log records are processed
- // only at the end of the request and performance log records that are created in another thread
- // should not get lost.
loggingCtx.setMutablePerformanceLogRecords(mutablePerformanceLogRecords);
try {
return callable.call();
diff --git a/java/com/google/gerrit/server/logging/LoggingContextAwareRunnable.java b/java/com/google/gerrit/server/logging/LoggingContextAwareRunnable.java
index 23162b1..d0559cc 100644
--- a/java/com/google/gerrit/server/logging/LoggingContextAwareRunnable.java
+++ b/java/com/google/gerrit/server/logging/LoggingContextAwareRunnable.java
@@ -98,13 +98,6 @@
loggingCtx.setTags(tags);
loggingCtx.forceLogging(forceLogging);
loggingCtx.performanceLogging(performanceLogging);
-
- // For the performance log records use the {@link MutablePerformanceLogRecords} instance from
- // the logging context of the calling thread in the logging context of the new thread. This way
- // performance log records that are created from the new thread are available from the logging
- // context of the calling thread. This is important since performance log records are processed
- // only at the end of the request and performance log records that are created in another thread
- // should not get lost.
loggingCtx.setMutablePerformanceLogRecords(mutablePerformanceLogRecords);
try {
runnable.run();
diff --git a/java/com/google/gerrit/server/patch/DiffContentCalculator.java b/java/com/google/gerrit/server/patch/DiffContentCalculator.java
index 53f7ca6..a387da6 100644
--- a/java/com/google/gerrit/server/patch/DiffContentCalculator.java
+++ b/java/com/google/gerrit/server/patch/DiffContentCalculator.java
@@ -14,28 +14,19 @@
package com.google.gerrit.server.patch;
-import static java.util.Comparator.comparing;
-
import com.google.common.collect.ImmutableList;
-import com.google.gerrit.common.data.CommentDetail;
-import com.google.gerrit.entities.Comment;
import com.google.gerrit.extensions.client.DiffPreferencesInfo;
import com.google.gerrit.extensions.client.DiffPreferencesInfo.Whitespace;
import com.google.gerrit.jgit.diff.ReplaceEdit;
-import com.google.gerrit.prettify.common.EditList;
+import com.google.gerrit.prettify.common.EditHunk;
import com.google.gerrit.prettify.common.SparseFileContent;
import com.google.gerrit.prettify.common.SparseFileContentBuilder;
-import java.util.Comparator;
import java.util.List;
import java.util.Optional;
import org.eclipse.jgit.diff.Edit;
/** Collects all lines and their content to be displayed in diff view. */
class DiffContentCalculator {
- private static final int MAX_CONTEXT = 5000000;
-
- private static final Comparator<Edit> EDIT_SORT = comparing(Edit::getBeginA);
-
private final DiffPreferencesInfo diffPrefs;
DiffContentCalculator(DiffPreferencesInfo diffPrefs) {
@@ -60,13 +51,11 @@
* @param srcA Original text content
* @param srcB New text content
* @param edits List of edits which was applied to srcA to produce srcB
- * @param comments Existing comments for srcA and srcB
* @return an instance of {@link DiffCalculatorResult}.
*/
DiffCalculatorResult calculateDiffContent(
- TextSource srcA, TextSource srcB, ImmutableList<Edit> edits, CommentDetail comments) {
- int context = getContext();
- if (srcA.src == srcB.src && srcA.size() <= context && edits.isEmpty()) {
+ TextSource srcA, TextSource srcB, ImmutableList<Edit> edits) {
+ if (srcA.src == srcB.src && edits.isEmpty()) {
// Odd special case; the files are identical (100% rename or copy)
// and the user has asked for context that is larger than the file.
// Send them the entire file, with an empty edit after the last line.
@@ -80,43 +69,13 @@
Edit emptyEdit = new Edit(srcA.size(), srcA.size());
return new DiffCalculatorResult(diffContent, ImmutableList.of(emptyEdit));
}
- ImmutableList.Builder<Edit> builder = ImmutableList.builder();
+ ImmutableList<Edit> sortedEdits = correctForDifferencesInNewlineAtEnd(srcA, srcB, edits);
- builder.addAll(correctForDifferencesInNewlineAtEnd(srcA, srcB, edits));
-
- boolean nonsortedEdits = false;
- if (comments != null) {
- ImmutableList<Edit> commentEdits = ensureCommentsVisible(comments, edits);
- builder.addAll(commentEdits);
- nonsortedEdits = !commentEdits.isEmpty();
- }
-
- ImmutableList<Edit> sortedEdits = builder.build();
- if (nonsortedEdits) {
- sortedEdits = ImmutableList.sortedCopyOf(EDIT_SORT, sortedEdits);
- }
-
- // In order to expand the skipped common lines or syntax highlight the
- // file properly we need to give the client the complete file contents.
- // So force our context temporarily to the complete file size.
- //
DiffContent diffContent =
- packContent(
- srcA,
- srcB,
- diffPrefs.ignoreWhitespace != Whitespace.IGNORE_NONE,
- sortedEdits,
- MAX_CONTEXT);
+ packContent(srcA, srcB, diffPrefs.ignoreWhitespace != Whitespace.IGNORE_NONE, sortedEdits);
return new DiffCalculatorResult(diffContent, sortedEdits);
}
- private int getContext() {
- if (diffPrefs.context == DiffPreferencesInfo.WHOLE_FILE_CONTEXT) {
- return MAX_CONTEXT;
- }
- return Math.min(diffPrefs.context, MAX_CONTEXT);
- }
-
private ImmutableList<Edit> correctForDifferencesInNewlineAtEnd(
TextSource a, TextSource b, ImmutableList<Edit> edits) {
// a.src.size() is the size ignoring a newline at the end whereas a.size() considers it.
@@ -205,128 +164,14 @@
return a.src.isMissingNewlineAtEnd() && !b.src.isMissingNewlineAtEnd();
}
- private ImmutableList<Edit> ensureCommentsVisible(
- CommentDetail comments, ImmutableList<Edit> edits) {
- if (comments.getCommentsA().isEmpty() && comments.getCommentsB().isEmpty()) {
- // No comments, no additional dummy edits are required.
- //
- return ImmutableList.of();
- }
-
- // Construct empty Edit blocks around each location where a comment is.
- // This will force the later packContent method to include the regions
- // containing comments, potentially combining those regions together if
- // they have overlapping contexts. UI renders will also be able to make
- // correct hunks from this, but because the Edit is empty they will not
- // style it specially.
- //
- final ImmutableList.Builder<Edit> commmentEdits = ImmutableList.builder();
- int lastLine;
-
- lastLine = -1;
- for (Comment c : comments.getCommentsA()) {
- final int a = c.lineNbr;
- if (lastLine != a) {
- final int b = mapA2B(a - 1, edits);
- if (0 <= b) {
- getNewEditForComment(edits, new Edit(a - 1, b)).ifPresent(commmentEdits::add);
- }
- lastLine = a;
- }
- }
-
- lastLine = -1;
- for (Comment c : comments.getCommentsB()) {
- int b = c.lineNbr;
- if (lastLine != b) {
- final int a = mapB2A(b - 1, edits);
- if (0 <= a) {
- getNewEditForComment(edits, new Edit(a, b - 1)).ifPresent(commmentEdits::add);
- }
- lastLine = b;
- }
- }
- return commmentEdits.build();
- }
-
- private Optional<Edit> getNewEditForComment(ImmutableList<Edit> edits, Edit toAdd) {
- final int a = toAdd.getBeginA();
- final int b = toAdd.getBeginB();
- for (Edit e : edits) {
- if (e.getBeginA() <= a && a <= e.getEndA()) {
- return Optional.empty();
- }
- if (e.getBeginB() <= b && b <= e.getEndB()) {
- return Optional.empty();
- }
- }
- return Optional.of(toAdd);
- }
-
- private int mapA2B(int a, ImmutableList<Edit> edits) {
- if (edits.isEmpty()) {
- // Magic special case of an unmodified file.
- //
- return a;
- }
-
- for (int i = 0; i < edits.size(); i++) {
- final Edit e = edits.get(i);
- if (a < e.getBeginA()) {
- if (i == 0) {
- // Special case of context at start of file.
- //
- return a;
- }
- return e.getBeginB() - (e.getBeginA() - a);
- }
- if (e.getBeginA() <= a && a <= e.getEndA()) {
- return -1;
- }
- }
-
- final Edit last = edits.get(edits.size() - 1);
- return last.getEndB() + (a - last.getEndA());
- }
-
- private int mapB2A(int b, ImmutableList<Edit> edits) {
- if (edits.isEmpty()) {
- // Magic special case of an unmodified file.
- //
- return b;
- }
-
- for (int i = 0; i < edits.size(); i++) {
- final Edit e = edits.get(i);
- if (b < e.getBeginB()) {
- if (i == 0) {
- // Special case of context at start of file.
- //
- return b;
- }
- return e.getBeginA() - (e.getBeginB() - b);
- }
- if (e.getBeginB() <= b && b <= e.getEndB()) {
- return -1;
- }
- }
-
- final Edit last = edits.get(edits.size() - 1);
- return last.getEndA() + (b - last.getEndB());
- }
-
private DiffContent packContent(
- TextSource a,
- TextSource b,
- boolean ignoredWhitespace,
- ImmutableList<Edit> edits,
- int context) {
+ TextSource a, TextSource b, boolean ignoredWhitespace, ImmutableList<Edit> edits) {
SparseFileContentBuilder diffA = new SparseFileContentBuilder(a.size());
SparseFileContentBuilder diffB = new SparseFileContentBuilder(b.size());
- EditList list = new EditList(edits, context, a.size(), b.size());
- for (EditList.Hunk hunk : list.getHunks()) {
+ if (!edits.isEmpty()) {
+ EditHunk hunk = new EditHunk(edits, a.size(), b.size());
while (hunk.next()) {
- if (hunk.isContextLine()) {
+ if (hunk.isUnmodifiedLine()) {
String lineA = a.getSourceLine(hunk.getCurA());
diffA.addLine(hunk.getCurA(), lineA);
diff --git a/java/com/google/gerrit/server/patch/PatchScriptBuilder.java b/java/com/google/gerrit/server/patch/PatchScriptBuilder.java
index 9b8409d..c6f7acf 100644
--- a/java/com/google/gerrit/server/patch/PatchScriptBuilder.java
+++ b/java/com/google/gerrit/server/patch/PatchScriptBuilder.java
@@ -18,7 +18,6 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
-import com.google.gerrit.common.data.CommentDetail;
import com.google.gerrit.common.data.PatchScript;
import com.google.gerrit.common.data.PatchScript.DisplayMethod;
import com.google.gerrit.entities.FixReplacement;
@@ -69,8 +68,7 @@
intralineDiffCalculator = calculator;
}
- PatchScript toPatchScript(
- Repository git, PatchList list, PatchListEntry content, CommentDetail comments)
+ PatchScript toPatchScript(Repository git, PatchList list, PatchListEntry content)
throws IOException {
PatchFileChange change =
@@ -86,7 +84,7 @@
ResolvedSides sides =
resolveSides(
git, sidesResolver, oldName(change), newName(change), list.getOldId(), list.getNewId());
- return build(sides.a, sides.b, change, comments);
+ return build(sides.a, sides.b, change);
}
private ResolvedSides resolveSides(
@@ -136,7 +134,7 @@
ChangeType.MODIFIED,
PatchType.UNIFIED);
- return build(a, b, change, null);
+ return build(a, b, change);
}
private PatchSide resolveSideA(
@@ -147,9 +145,7 @@
}
}
- private PatchScript build(
- PatchSide a, PatchSide b, PatchFileChange content, CommentDetail comments) {
-
+ private PatchScript build(PatchSide a, PatchSide b, PatchFileChange content) {
ImmutableList<Edit> contentEdits = content.getEdits();
ImmutableSet<Edit> editsDueToRebase = content.getEditsDueToRebase();
@@ -163,8 +159,7 @@
ImmutableList<Edit> finalEdits = intralineResult.edits.orElse(contentEdits);
DiffContentCalculator calculator = new DiffContentCalculator(diffPrefs);
DiffCalculatorResult diffCalculatorResult =
- calculator.calculateDiffContent(
- new TextSource(a.src), new TextSource(b.src), finalEdits, comments);
+ calculator.calculateDiffContent(new TextSource(a.src), new TextSource(b.src), finalEdits);
return new PatchScript(
content.getChangeType(),
diff --git a/java/com/google/gerrit/server/patch/PatchScriptFactory.java b/java/com/google/gerrit/server/patch/PatchScriptFactory.java
index 30930ec..60a0688 100644
--- a/java/com/google/gerrit/server/patch/PatchScriptFactory.java
+++ b/java/com/google/gerrit/server/patch/PatchScriptFactory.java
@@ -20,19 +20,13 @@
import com.google.common.collect.ImmutableList;
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.common.Nullable;
-import com.google.gerrit.common.data.CommentDetail;
import com.google.gerrit.common.data.PatchScript;
-import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.Change;
-import com.google.gerrit.entities.HumanComment;
-import com.google.gerrit.entities.Patch.ChangeType;
import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.entities.Project;
import com.google.gerrit.extensions.client.DiffPreferencesInfo;
import com.google.gerrit.extensions.client.DiffPreferencesInfo.Whitespace;
import com.google.gerrit.extensions.restapi.AuthException;
-import com.google.gerrit.server.CommentsUtil;
-import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.PatchSetUtil;
import com.google.gerrit.server.edit.ChangeEdit;
import com.google.gerrit.server.edit.ChangeEditUtil;
@@ -84,7 +78,6 @@
private final PatchSetUtil psUtil;
private final Provider<PatchScriptBuilder> builderFactory;
private final PatchListCache patchListCache;
- private final CommentsUtil commentsUtil;
private final String fileName;
@Nullable private final PatchSet.Id psa;
@@ -92,12 +85,10 @@
private final PatchSet.Id psb;
private final DiffPreferencesInfo diffPrefs;
private final ChangeEditUtil editReader;
- private final Provider<CurrentUser> userProvider;
private final PermissionBackend permissionBackend;
private final ProjectCache projectCache;
private final Change.Id changeId;
- private boolean loadComments = true;
private ChangeNotes notes;
@@ -107,9 +98,7 @@
PatchSetUtil psUtil,
Provider<PatchScriptBuilder> builderFactory,
PatchListCache patchListCache,
- CommentsUtil commentsUtil,
ChangeEditUtil editReader,
- Provider<CurrentUser> userProvider,
PermissionBackend permissionBackend,
ProjectCache projectCache,
@Assisted ChangeNotes notes,
@@ -122,9 +111,7 @@
this.builderFactory = builderFactory;
this.patchListCache = patchListCache;
this.notes = notes;
- this.commentsUtil = commentsUtil;
this.editReader = editReader;
- this.userProvider = userProvider;
this.permissionBackend = permissionBackend;
this.projectCache = projectCache;
@@ -143,9 +130,7 @@
PatchSetUtil psUtil,
Provider<PatchScriptBuilder> builderFactory,
PatchListCache patchListCache,
- CommentsUtil commentsUtil,
ChangeEditUtil editReader,
- Provider<CurrentUser> userProvider,
PermissionBackend permissionBackend,
ProjectCache projectCache,
@Assisted ChangeNotes notes,
@@ -158,9 +143,7 @@
this.builderFactory = builderFactory;
this.patchListCache = patchListCache;
this.notes = notes;
- this.commentsUtil = commentsUtil;
this.editReader = editReader;
- this.userProvider = userProvider;
this.permissionBackend = permissionBackend;
this.projectCache = projectCache;
@@ -174,10 +157,6 @@
checkArgument(parentNum >= 0, "parentNum must be >= 0");
}
- public void setLoadComments(boolean load) {
- loadComments = load;
- }
-
@Override
public PatchScript call()
throws LargeObjectException, AuthException, InvalidChangeOperationException, IOException,
@@ -218,9 +197,7 @@
final PatchScriptBuilder b = newBuilder();
final PatchListEntry content = list.get(fileName);
- Optional<CommentDetail> comments = loadComments(content, changeEdit);
-
- return b.toPatchScript(git, list, content, comments.orElse(null));
+ return b.toPatchScript(git, list, content);
} catch (PatchListNotAvailableException e) {
throw new NoSuchChangeException(changeId, e);
} catch (IOException e) {
@@ -238,14 +215,6 @@
}
}
- private Optional<CommentDetail> loadComments(PatchListEntry content, boolean changeEdit) {
- if (!loadComments) {
- return Optional.empty();
- }
- return new CommentsLoader(psa, psb, userProvider, notes, commentsUtil)
- .load(changeEdit, content.getChangeType(), content.getOldName(), content.getNewName());
- }
-
private Optional<ObjectId> getAId() {
if (psa == null) {
return Optional.empty();
@@ -300,99 +269,6 @@
}
}
- private static class CommentsLoader {
- private final PatchSet.Id psa;
- private final PatchSet.Id psb;
- private final Provider<CurrentUser> userProvider;
- private final ChangeNotes notes;
- private final CommentsUtil commentsUtil;
- private CommentDetail comments;
-
- CommentsLoader(
- PatchSet.Id psa,
- PatchSet.Id psb,
- Provider<CurrentUser> userProvider,
- ChangeNotes notes,
- CommentsUtil commentsUtil) {
- this.psa = psa;
- this.psb = psb;
- this.userProvider = userProvider;
- this.notes = notes;
- this.commentsUtil = commentsUtil;
- }
-
- private Optional<CommentDetail> load(
- boolean changeEdit, ChangeType changeType, String oldName, String newName) {
- // TODO: Implement this method with CommentDetailBuilder (this class doesn't exists yet).
- // This is a legacy code which create final object and populate it and then returns it.
- if (changeEdit) {
- return Optional.empty();
- }
-
- comments = new CommentDetail(psa, psb);
- switch (changeType) {
- case ADDED:
- case MODIFIED:
- loadPublished(newName);
- break;
-
- case DELETED:
- loadPublished(newName);
- break;
-
- case COPIED:
- case RENAMED:
- if (psa != null) {
- loadPublished(oldName);
- }
- loadPublished(newName);
- break;
-
- case REWRITE:
- break;
- }
-
- CurrentUser user = userProvider.get();
- if (user.isIdentifiedUser()) {
- Account.Id me = user.getAccountId();
- switch (changeType) {
- case ADDED:
- case MODIFIED:
- loadDrafts(me, newName);
- break;
-
- case DELETED:
- loadDrafts(me, newName);
- break;
-
- case COPIED:
- case RENAMED:
- if (psa != null) {
- loadDrafts(me, oldName);
- }
- loadDrafts(me, newName);
- break;
-
- case REWRITE:
- break;
- }
- }
- return Optional.of(comments);
- }
-
- private void loadPublished(String file) {
- for (HumanComment c : commentsUtil.publishedByChangeFile(notes, file)) {
- comments.include(notes.getChangeId(), c);
- }
- }
-
- private void loadDrafts(Account.Id me, String file) {
- for (HumanComment c : commentsUtil.draftByChangeFileAuthor(notes, file, me)) {
- comments.include(notes.getChangeId(), c);
- }
- }
- }
-
private static class IntraLineDiffCalculator
implements PatchScriptBuilder.IntraLineDiffCalculator {
diff --git a/java/com/google/gerrit/server/restapi/change/CommentJson.java b/java/com/google/gerrit/server/restapi/change/CommentJson.java
index 67049e8..4de9b63 100644
--- a/java/com/google/gerrit/server/restapi/change/CommentJson.java
+++ b/java/com/google/gerrit/server/restapi/change/CommentJson.java
@@ -166,7 +166,6 @@
r.updated = c.writtenOn;
r.range = toRange(c.range);
r.tag = c.tag;
- r.unresolved = c.unresolved;
if (loader != null) {
r.author = loader.get(c.author.getId());
}
@@ -194,6 +193,7 @@
protected CommentInfo toInfo(HumanComment c, AccountLoader loader) {
CommentInfo ci = new CommentInfo();
fillCommentInfo(c, ci, loader);
+ ci.unresolved = c.unresolved;
return ci;
}
diff --git a/java/com/google/gerrit/server/restapi/change/GetDiff.java b/java/com/google/gerrit/server/restapi/change/GetDiff.java
index f4e2ddd..8d51786 100644
--- a/java/com/google/gerrit/server/restapi/change/GetDiff.java
+++ b/java/com/google/gerrit/server/restapi/change/GetDiff.java
@@ -15,7 +15,6 @@
package com.google.gerrit.server.restapi.change;
import static com.google.gerrit.server.project.ProjectCache.illegalState;
-import static com.google.gerrit.util.cli.Localizable.localizable;
import com.google.common.base.MoreObjects;
import com.google.common.collect.ImmutableList;
@@ -54,9 +53,7 @@
import com.google.inject.Inject;
import java.io.IOException;
import java.util.concurrent.TimeUnit;
-import org.kohsuke.args4j.CmdLineException;
import org.kohsuke.args4j.CmdLineParser;
-import org.kohsuke.args4j.NamedOptionDef;
import org.kohsuke.args4j.Option;
import org.kohsuke.args4j.OptionDef;
import org.kohsuke.args4j.spi.OptionHandler;
@@ -84,8 +81,9 @@
@Option(name = "--whitespace")
Whitespace whitespace;
+ // TODO(hiesel): Remove parameter when not used by callers (e.g. frontend) anymore.
@Option(name = "--context", handler = ContextOptionHandler.class)
- int context = DiffPreferencesInfo.DEFAULT_CONTEXT;
+ int context;
@Option(name = "--intraline")
boolean intraline;
@@ -114,11 +112,10 @@
} else {
prefs.ignoreWhitespace = Whitespace.IGNORE_LEADING_AND_TRAILING;
}
- prefs.context = context;
prefs.intralineDifference = intraline;
logger.atFine().log(
- "diff preferences: ignoreWhitespace = %s, context = %s, intralineDifference = %s",
- prefs.ignoreWhitespace, prefs.context, prefs.intralineDifference);
+ "diff preferences: ignoreWhitespace = %s, intralineDifference = %s",
+ prefs.ignoreWhitespace, prefs.intralineDifference);
PatchScriptFactory psf;
PatchSet basePatchSet = null;
@@ -143,7 +140,6 @@
}
try {
- psf.setLoadComments(context != DiffPreferencesInfo.WHOLE_FILE_CONTEXT);
PatchScript ps = psf.call();
Project.NameKey projectName = resource.getRevision().getChange().getProject();
ProjectState state = projectCache.get(projectName).orElseThrow(illegalState(projectName));
@@ -245,11 +241,6 @@
return this;
}
- public GetDiff setContext(int context) {
- this.context = context;
- return this;
- }
-
public GetDiff setIntraline(boolean intraline) {
this.intraline = intraline;
return this;
@@ -274,6 +265,7 @@
}
}
+ // TODO(hiesel): Remove this class once clients don't send the context parameter anymore.
public static class ContextOptionHandler extends OptionHandler<Short> {
public ContextOptionHandler(CmdLineParser parser, OptionDef option, Setter<Short> setter) {
@@ -281,33 +273,14 @@
}
@Override
- public final int parseArguments(Parameters params) throws CmdLineException {
- final String value = params.getParameter(0);
- short context;
- if ("all".equalsIgnoreCase(value)) {
- context = DiffPreferencesInfo.WHOLE_FILE_CONTEXT;
- } else {
- try {
- context = Short.parseShort(value, 10);
- if (context < 0) {
- throw new NumberFormatException();
- }
- } catch (NumberFormatException e) {
- logger.atFine().withCause(e).log("invalid numeric value");
- throw new CmdLineException(
- owner,
- localizable("\"%s\" is not a valid value for \"%s\""),
- value,
- ((NamedOptionDef) option).name());
- }
- }
- setter.addValue(context);
+ public final int parseArguments(Parameters params) {
+ // Return 1 to consume the context parameter.
return 1;
}
@Override
public final String getDefaultMetaVariable() {
- return "ALL|# LINES";
+ return "ignored";
}
}
}
diff --git a/java/com/google/gerrit/server/restapi/change/PostReview.java b/java/com/google/gerrit/server/restapi/change/PostReview.java
index 3367ca6..604c87f 100644
--- a/java/com/google/gerrit/server/restapi/change/PostReview.java
+++ b/java/com/google/gerrit/server/restapi/change/PostReview.java
@@ -568,8 +568,8 @@
}
}
- private static <T extends CommentInput> Map<String, List<T>> cleanUpComments(
- Map<String, List<T>> commentsPerPath) {
+ private static <T extends com.google.gerrit.extensions.client.Comment>
+ Map<String, List<T>> cleanUpComments(Map<String, List<T>> commentsPerPath) {
Map<String, List<T>> cleanedUpCommentMap = new HashMap<>();
for (Map.Entry<String, List<T>> e : commentsPerPath.entrySet()) {
String path = e.getKey();
@@ -587,7 +587,8 @@
return cleanedUpCommentMap;
}
- private static <T extends CommentInput> List<T> cleanUpComments(List<T> comments) {
+ private static <T extends com.google.gerrit.extensions.client.Comment> List<T> cleanUpComments(
+ List<T> comments) {
return comments.stream()
.filter(Objects::nonNull)
.filter(comment -> !Strings.nullToEmpty(comment.message).trim().isEmpty())
@@ -598,7 +599,7 @@
return TraceContext.newTimer(getClass().getSimpleName() + "#" + method, Metadata.empty());
}
- private <T extends CommentInput> void checkComments(
+ private <T extends com.google.gerrit.extensions.client.Comment> void checkComments(
RevisionResource revision, Map<String, List<T>> commentsPerPath)
throws BadRequestException, PatchListNotAvailableException {
logger.atFine().log("checking comments");
@@ -646,15 +647,16 @@
}
}
- private static <T extends CommentInput> void ensureCommentNotOnMagicFilesOfAutoMerge(
- String path, T comment) throws BadRequestException {
+ private static <T extends com.google.gerrit.extensions.client.Comment>
+ void ensureCommentNotOnMagicFilesOfAutoMerge(String path, T comment)
+ throws BadRequestException {
if (Patch.isMagic(path) && comment.side == Side.PARENT && comment.parent == null) {
throw new BadRequestException(String.format("cannot comment on %s on auto-merge", path));
}
}
- private static <T extends CommentInput> void ensureValidPatchsetLevelComment(
- String path, T comment) throws BadRequestException {
+ private static <T extends com.google.gerrit.extensions.client.Comment>
+ void ensureValidPatchsetLevelComment(String path, T comment) throws BadRequestException {
if (path.equals(PATCHSET_LEVEL)
&& (comment.side != null || comment.range != null || comment.line != null)) {
throw new BadRequestException("Patchset-level comments can't have side, range, or line");
diff --git a/java/com/google/gerrit/server/restapi/change/ReplyAttentionSetUpdates.java b/java/com/google/gerrit/server/restapi/change/ReplyAttentionSetUpdates.java
index c523036..65c0cda 100644
--- a/java/com/google/gerrit/server/restapi/change/ReplyAttentionSetUpdates.java
+++ b/java/com/google/gerrit/server/restapi/change/ReplyAttentionSetUpdates.java
@@ -125,7 +125,6 @@
}
if (serviceUserClassifier.isServiceUser(currentUser.getAccountId())) {
botsWithNegativeLabelsAddOwnerAndUploader(bu, changeNotes, input);
- robotCommentAddsOwnerAndUploader(bu, changeNotes, input);
return;
}
@@ -254,8 +253,8 @@
}
/**
- * Bots don't process automatic rules, but they do have special rules; One of them: If voted
- * negatively on a label, add the owner and uploader.
+ * Bots don't process automatic rules, the only attention set change they do is this rule: Add
+ * owner and uploader when a bot votes negatively.
*/
private void botsWithNegativeLabelsAddOwnerAndUploader(
BatchUpdate bu, ChangeNotes changeNotes, ReviewInput input) {
@@ -270,22 +269,6 @@
}
/**
- * Bots don't process automatic rules, but they do have special rules; One of them: When adding a
- * robot comment, add the owner and uploader. This only applies on open changes.
- */
- private void robotCommentAddsOwnerAndUploader(
- BatchUpdate bu, ChangeNotes changeNotes, ReviewInput input) {
- if (input.robotComments != null && changeNotes.getChange().isNew()) {
- Account.Id uploader = changeNotes.getCurrentPatchSet().uploader();
- Account.Id owner = changeNotes.getChange().getOwner();
- addToAttentionSet(bu, changeNotes, owner, "A robot comment was added", false);
- if (!owner.equals(uploader)) {
- addToAttentionSet(bu, changeNotes, uploader, "A robot comment was added", false);
- }
- }
- }
-
- /**
* Adds the user to the attention set
*
* @param bu BatchUpdate to perform the updates to the attention set
diff --git a/java/com/google/gerrit/testing/TestCommentHelper.java b/java/com/google/gerrit/testing/TestCommentHelper.java
index 76a5521..da8f871 100644
--- a/java/com/google/gerrit/testing/TestCommentHelper.java
+++ b/java/com/google/gerrit/testing/TestCommentHelper.java
@@ -82,7 +82,6 @@
c.parent = null;
c.line = line != 0 ? line : null;
c.message = message;
- c.unresolved = false;
c.inReplyTo = inReplyTo;
if (line != 0) c.range = range;
return c;
@@ -96,7 +95,6 @@
c.parent = null;
c.line = line != 0 ? line : null;
c.message = message;
- c.unresolved = false;
if (line != 0) c.range = range;
return c;
}
diff --git a/javatests/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java b/javatests/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java
index 5684b1f..82215b6 100644
--- a/javatests/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java
@@ -36,16 +36,12 @@
import com.google.gerrit.common.RawInputUtil;
import com.google.gerrit.extensions.api.changes.FileApi;
import com.google.gerrit.extensions.api.changes.RebaseInput;
-import com.google.gerrit.extensions.api.changes.ReviewInput;
-import com.google.gerrit.extensions.api.changes.ReviewInput.CommentInput;
-import com.google.gerrit.extensions.client.Comment;
import com.google.gerrit.extensions.client.DiffPreferencesInfo;
import com.google.gerrit.extensions.common.ChangeType;
import com.google.gerrit.extensions.common.DiffInfo;
import com.google.gerrit.extensions.common.FileInfo;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.BinaryResult;
-import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.testing.ConfigSuite;
import java.awt.image.BufferedImage;
import java.io.ByteArrayOutputStream;
@@ -64,6 +60,7 @@
import org.eclipse.jgit.lib.PersonIdent;
import org.eclipse.jgit.revwalk.RevCommit;
import org.junit.Before;
+import org.junit.Ignore;
import org.junit.Test;
public class RevisionDiffIT extends AbstractDaemonTest {
@@ -676,12 +673,6 @@
String baseFileContent = FILE_CONTENT.concat("Line 101");
ObjectId commit2 = addCommit(commit1, FILE_NAME, baseFileContent);
rebaseChangeOn(changeId, commit2);
- // Add a comment so that file contents are not 'skipped'. To be able to add a comment, touch
- // (= modify) the file in the change.
- addModifiedPatchSet(
- changeId, FILE_NAME, fileContent -> fileContent.replace("Line 2\n", "Line two\n"));
- CommentInput comment = createCommentInput(3, 0, 4, 0, "Comment to not skip file content.");
- addCommentTo(changeId, CURRENT, FILE_NAME, comment);
String previousPatchSetId = gApi.changes().id(changeId).get().currentRevision;
String newBaseFileContent = baseFileContent.concat("\n");
ObjectId commit3 = addCommit(commit2, FILE_NAME, newBaseFileContent);
@@ -2517,17 +2508,14 @@
}
@Test
- public void diffOfUnmodifiedFileWithWholeFileContextReturnsFileContents() throws Exception {
+ public void diffOfUnmodifiedFileReturnsAllFileContents() throws Exception {
addModifiedPatchSet(changeId, FILE_NAME, content -> content.replace("Line 2\n", "Line two\n"));
String previousPatchSetId = gApi.changes().id(changeId).get().currentRevision;
addModifiedPatchSet(
changeId, FILE_NAME2, content -> content.replace("2nd line\n", "Second line\n"));
DiffInfo diffInfo =
- getDiffRequest(changeId, CURRENT, FILE_NAME)
- .withBase(previousPatchSetId)
- .withContext(DiffPreferencesInfo.WHOLE_FILE_CONTEXT)
- .get();
+ getDiffRequest(changeId, CURRENT, FILE_NAME).withBase(previousPatchSetId).get();
// We don't list the full file contents here as that is not the focus of this test.
assertThat(diffInfo)
.content()
@@ -2538,40 +2526,16 @@
}
@Test
- public void diffOfUnmodifiedFileWithCommentAndWholeFileContextReturnsFileContents()
+ // TODO(ghareeb): Don't exclude diffs which only contain rebase hunks within the diff caches. Only
+ // filter such files in the GetFiles REST endpoint.
+ @Ignore
+ public void diffOfFileWithOnlyRebaseHunksConsideringWhitespaceReturnsFileContents()
throws Exception {
addModifiedPatchSet(changeId, FILE_NAME, content -> content.replace("Line 2\n", "Line two\n"));
String previousPatchSetId = gApi.changes().id(changeId).get().currentRevision;
- CommentInput comment = createCommentInput(2, 0, 3, 0, "Should be 'Line 2'.");
- addCommentTo(changeId, previousPatchSetId, FILE_NAME, comment);
- addModifiedPatchSet(
- changeId, FILE_NAME2, content -> content.replace("2nd line\n", "Second line\n"));
-
- DiffInfo diffInfo =
- getDiffRequest(changeId, CURRENT, FILE_NAME)
- .withBase(previousPatchSetId)
- .withContext(DiffPreferencesInfo.WHOLE_FILE_CONTEXT)
- .get();
- // We don't list the full file contents here as that is not the focus of this test.
- assertThat(diffInfo)
- .content()
- .element(0)
- .commonLines()
- .containsAtLeast("Line 1", "Line two", "Line 3", "Line 4", "Line 5")
- .inOrder();
- }
-
- @Test
- public void
- diffOfFileWithOnlyRebaseHunksAndWithCommentAndConsideringWhitespaceReturnsFileContents()
- throws Exception {
- addModifiedPatchSet(changeId, FILE_NAME, content -> content.replace("Line 2\n", "Line two\n"));
- String previousPatchSetId = gApi.changes().id(changeId).get().currentRevision;
String newBaseFileContent = FILE_CONTENT.replace("Line 70\n", "Line seventy\n");
ObjectId commit2 = addCommit(commit1, FILE_NAME, newBaseFileContent);
rebaseChangeOn(changeId, commit2);
- CommentInput comment = createCommentInput(2, 0, 3, 0, "Should be 'Line 2'.");
- addCommentTo(changeId, previousPatchSetId, FILE_NAME, comment);
DiffInfo diffInfo =
getDiffRequest(changeId, CURRENT, FILE_NAME)
@@ -2585,18 +2549,23 @@
.commonLines()
.containsAtLeast("Line 1", "Line two", "Line 3", "Line 4", "Line 5")
.inOrder();
+ // It's crucial that the line changed in the rebase is reported correctly.
+ assertThat(diffInfo).content().element(1).linesOfA().containsExactly("Line 70");
+ assertThat(diffInfo).content().element(1).linesOfB().containsExactly("Line seventy");
+ assertThat(diffInfo).content().element(1).isDueToRebase();
}
@Test
- public void diffOfFileWithOnlyRebaseHunksAndWithCommentAndIgnoringWhitespaceReturnsFileContents()
+ // TODO(ghareeb): Don't exclude diffs which only contain rebase hunks within the diff caches. Only
+ // filter such files in the GetFiles REST endpoint.
+ @Ignore
+ public void diffOfFileWithOnlyRebaseHunksAndIgnoringWhitespaceReturnsFileContents()
throws Exception {
addModifiedPatchSet(changeId, FILE_NAME, content -> content.replace("Line 2\n", "Line two\n"));
String previousPatchSetId = gApi.changes().id(changeId).get().currentRevision;
String newBaseFileContent = FILE_CONTENT.replace("Line 70\n", "Line seventy\n");
ObjectId commit2 = addCommit(commit1, FILE_NAME, newBaseFileContent);
rebaseChangeOn(changeId, commit2);
- CommentInput comment = createCommentInput(2, 0, 3, 0, "Should be 'Line 2'.");
- addCommentTo(changeId, previousPatchSetId, FILE_NAME, comment);
DiffInfo diffInfo =
getDiffRequest(changeId, CURRENT, FILE_NAME)
@@ -2610,12 +2579,18 @@
.commonLines()
.containsAtLeast("Line 1", "Line two", "Line 3", "Line 4", "Line 5")
.inOrder();
+ // It's crucial that the line changed in the rebase is reported correctly.
+ assertThat(diffInfo).content().element(1).linesOfA().containsExactly("Line 70");
+ assertThat(diffInfo).content().element(1).linesOfB().containsExactly("Line seventy");
+ assertThat(diffInfo).content().element(1).isDueToRebase();
}
@Test
- public void
- diffOfFileWithMultilineRebaseHunkAddingNewlineAtEndOfFileAndWithCommentReturnsFileContents()
- throws Exception {
+ // TODO(ghareeb): Don't exclude diffs which only contain rebase hunks within the diff caches. Only
+ // filter such files in the GetFiles REST endpoint.
+ @Ignore
+ public void diffOfFileWithMultilineRebaseHunkAddingNewlineAtEndOfFileReturnsFileContents()
+ throws Exception {
String baseFileContent = FILE_CONTENT.concat("Line 101");
ObjectId commit2 = addCommit(commit1, FILE_NAME, baseFileContent);
rebaseChangeOn(changeId, commit2);
@@ -2624,8 +2599,6 @@
String newBaseFileContent = baseFileContent.concat("\nLine 102\nLine 103\n");
ObjectId commit3 = addCommit(commit2, FILE_NAME, newBaseFileContent);
rebaseChangeOn(changeId, commit3);
- CommentInput comment = createCommentInput(2, 0, 3, 0, "Should be 'Line 2'.");
- addCommentTo(changeId, previousPatchSetId, FILE_NAME, comment);
DiffInfo diffInfo =
getDiffRequest(changeId, CURRENT, FILE_NAME)
@@ -2639,19 +2612,27 @@
.commonLines()
.containsAtLeast("Line 1", "Line two", "Line 3", "Line 4", "Line 5")
.inOrder();
+ // It's crucial that the lines changed in the rebase are reported correctly.
+ assertThat(diffInfo).content().element(1).linesOfA().containsExactly("Line 101");
+ assertThat(diffInfo)
+ .content()
+ .element(1)
+ .linesOfB()
+ .containsExactly("Line 101", "Line 102", "Line 103", "");
+ assertThat(diffInfo).content().element(1).isDueToRebase();
}
@Test
- public void
- diffOfFileWithMultilineRebaseHunkRemovingNewlineAtEndOfFileAndWithCommentReturnsFileContents()
- throws Exception {
+ // TODO(ghareeb): Don't exclude diffs which only contain rebase hunks within the diff caches. Only
+ // filter such files in the GetFiles REST endpoint.
+ @Ignore
+ public void diffOfFileWithMultilineRebaseHunkRemovingNewlineAtEndOfFileReturnsFileContents()
+ throws Exception {
addModifiedPatchSet(changeId, FILE_NAME, content -> content.replace("Line 2\n", "Line two\n"));
String previousPatchSetId = gApi.changes().id(changeId).get().currentRevision;
- String newBaseFileContent = FILE_CONTENT.concat("Line 101\nLine 103\nLine 104");
+ String newBaseFileContent = FILE_CONTENT.concat("Line 101\nLine 102\nLine 103");
ObjectId commit2 = addCommit(commit1, FILE_NAME, newBaseFileContent);
rebaseChangeOn(changeId, commit2);
- CommentInput comment = createCommentInput(2, 0, 3, 0, "Should be 'Line 2'.");
- addCommentTo(changeId, previousPatchSetId, FILE_NAME, comment);
DiffInfo diffInfo =
getDiffRequest(changeId, CURRENT, FILE_NAME)
@@ -2665,6 +2646,14 @@
.commonLines()
.containsAtLeast("Line 1", "Line two", "Line 3", "Line 4", "Line 5")
.inOrder();
+ // It's crucial that the lines changed in the rebase are reported correctly.
+ assertThat(diffInfo).content().element(1).linesOfA().containsExactly("Line 100", "");
+ assertThat(diffInfo)
+ .content()
+ .element(1)
+ .linesOfB()
+ .containsExactly("Line 100", "Line 101", "Line 102", "Line 103");
+ assertThat(diffInfo).content().element(1).isDueToRebase();
}
@Test
@@ -2674,49 +2663,10 @@
DiffInfo diffInfo =
getDiffRequest(changeId, CURRENT, "a_non-existent_file.txt")
.withBase(initialPatchSetId)
- .withContext(DiffPreferencesInfo.WHOLE_FILE_CONTEXT)
.get();
assertThat(diffInfo).content().isEmpty();
}
- // This behavior is likely a bug. A fix might not be easy as it might break syntax highlighting.
- // TODO: Fix this issue or remove the broken parameter (at least in the documentation).
- @Test
- public void contextParameterIsIgnored() throws Exception {
- addModifiedPatchSet(
- changeId, FILE_NAME, content -> content.replace("Line 20\n", "Line twenty\n"));
-
- DiffInfo diffInfo =
- getDiffRequest(changeId, CURRENT, FILE_NAME)
- .withBase(initialPatchSetId)
- .withContext(5)
- .get();
- assertThat(diffInfo).content().element(0).commonLines().hasSize(19);
- assertThat(diffInfo).content().element(1).linesOfA().containsExactly("Line 20");
- assertThat(diffInfo).content().element(1).linesOfB().containsExactly("Line twenty");
- assertThat(diffInfo).content().element(2).commonLines().hasSize(81);
- }
-
- // This behavior is likely a bug. A fix might not be easy as it might break syntax highlighting.
- // TODO: Fix this issue or remove the broken parameter (at least in the documentation).
- @Test
- public void contextParameterIsIgnoredForUnmodifiedFileWithComment() throws Exception {
- addModifiedPatchSet(
- changeId, FILE_NAME, content -> content.replace("Line 20\n", "Line twenty\n"));
- String previousPatchSetId = gApi.changes().id(changeId).get().currentRevision;
- CommentInput comment = createCommentInput(20, 0, 21, 0, "Should be 'Line 20'.");
- addCommentTo(changeId, previousPatchSetId, FILE_NAME, comment);
- addModifiedPatchSet(
- changeId, FILE_NAME2, content -> content.replace("2nd line\n", "Second line\n"));
-
- DiffInfo diffInfo =
- getDiffRequest(changeId, CURRENT, FILE_NAME)
- .withBase(previousPatchSetId)
- .withContext(5)
- .get();
- assertThat(diffInfo).content().element(0).commonLines().hasSize(101);
- }
-
@Test
public void requestingDiffForOldFileNameOfRenamedFileYieldsReasonableResult() throws Exception {
addModifiedPatchSet(changeId, FILE_NAME, content -> content.replace("Line 2\n", "Line two\n"));
@@ -2726,40 +2676,13 @@
gApi.changes().id(changeId).edit().publish();
DiffInfo diffInfo =
- getDiffRequest(changeId, CURRENT, FILE_NAME)
- .withBase(previousPatchSetId)
- .withContext(DiffPreferencesInfo.WHOLE_FILE_CONTEXT)
- .get();
+ getDiffRequest(changeId, CURRENT, FILE_NAME).withBase(previousPatchSetId).get();
// This behavior has been present in Gerrit for quite some time. It differs from the results
- // returned for other cases (e.g. requesting the diff with whole file context for an unmodified
- // file; requesting the diff with whole file context for a non-existent file). However, it's not
- // completely clear what should be returned. The closest would be the result of a file deletion
- // but that might also be misleading for users as actually a file rename occurred. In fact,
- // requesting the diff result for the old file name of a renamed file is not a reasonable use
- // case at all. We at least guarantee that we don't run into an internal error.
- assertThat(diffInfo).content().element(0).commonLines().isNull();
- assertThat(diffInfo).content().element(0).numberOfSkippedLines().isGreaterThan(0);
- }
-
- @Test
- public void requestingDiffForOldFileNameOfRenamedFileWithCommentOnOldFileYieldsReasonableResult()
- throws Exception {
- addModifiedPatchSet(changeId, FILE_NAME, content -> content.replace("Line 2\n", "Line two\n"));
- String previousPatchSetId = gApi.changes().id(changeId).get().currentRevision;
- CommentInput comment = createCommentInput(2, 0, 3, 0, "Should be 'Line 2'.");
- addCommentTo(changeId, previousPatchSetId, FILE_NAME, comment);
- String newFilePath = "a_new_file.txt";
- gApi.changes().id(changeId).edit().renameFile(FILE_NAME, newFilePath);
- gApi.changes().id(changeId).edit().publish();
-
- DiffInfo diffInfo =
- getDiffRequest(changeId, CURRENT, FILE_NAME)
- .withBase(previousPatchSetId)
- .withContext(DiffPreferencesInfo.WHOLE_FILE_CONTEXT)
- .get();
- // See comment for requestingDiffForOldFileNameOfRenamedFileYieldsReasonableResult().
- // This test should additionally ensure that we also don't run into an internal error when
- // a comment is present.
+ // returned for other cases (e.g. requesting the diff for an unmodified file; requesting the
+ // diff for a non-existent file). After a rename, the original file doesn't exist anymore.
+ // Hence, the most reasonable thing would be to match the behavior of requesting the diff for a
+ // non-existent file, which returns an empty diff.
+ // This test at least guarantees that we don't run into an internal error.
assertThat(diffInfo).content().element(0).commonLines().isNull();
assertThat(diffInfo).content().element(0).numberOfSkippedLines().isGreaterThan(0);
}
@@ -2781,26 +2704,6 @@
assertThat(e).hasMessageThat().isEqualTo("edit not allowed as base");
}
- private static CommentInput createCommentInput(
- int startLine, int startCharacter, int endLine, int endCharacter, String message) {
- CommentInput comment = new CommentInput();
- comment.range = new Comment.Range();
- comment.range.startLine = startLine;
- comment.range.startCharacter = startCharacter;
- comment.range.endLine = endLine;
- comment.range.endCharacter = endCharacter;
- comment.message = message;
- return comment;
- }
-
- private void addCommentTo(
- String changeId, String previousPatchSetId, String fileName, CommentInput comment)
- throws RestApiException {
- ReviewInput reviewInput = new ReviewInput();
- reviewInput.comments = ImmutableMap.of(fileName, ImmutableList.of(comment));
- gApi.changes().id(changeId).revision(previousPatchSetId).review(reviewInput);
- }
-
private void assertDiffForNewFile(
PushOneCommit.Result pushResult, String path, String expectedContentSideB) throws Exception {
DiffInfo diff =
diff --git a/javatests/com/google/gerrit/acceptance/edit/ChangeEditIT.java b/javatests/com/google/gerrit/acceptance/edit/ChangeEditIT.java
index 22cecdb..5dbbe96 100644
--- a/javatests/com/google/gerrit/acceptance/edit/ChangeEditIT.java
+++ b/javatests/com/google/gerrit/acceptance/edit/ChangeEditIT.java
@@ -1016,11 +1016,7 @@
}
private String urlDiff(String changeId, String fileName) {
- return "/changes/"
- + changeId
- + "/revisions/0/files/"
- + fileName
- + "/diff?context=ALL&intraline";
+ return "/changes/" + changeId + "/revisions/0/files/" + fileName + "/diff?intraline";
}
private String urlDiff(String changeId, String revisionId, String fileName) {
@@ -1030,7 +1026,7 @@
+ revisionId
+ "/files/"
+ fileName
- + "/diff?context=ALL&intraline";
+ + "/diff?intraline";
}
private <T> T readContentFromJson(RestResponse r, Class<T> clazz) throws Exception {
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/AttentionSetIT.java b/javatests/com/google/gerrit/acceptance/rest/change/AttentionSetIT.java
index cc6b199..976be96 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/AttentionSetIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/AttentionSetIT.java
@@ -1279,25 +1279,6 @@
}
@Test
- public void robotCommentAddsOwnerOnNewChanges() throws Exception {
- TestAccount robot =
- accountCreator.create("robot2", "robot2@example.com", "Ro Bot", "Ro", "Service Users");
- PushOneCommit.Result r = createChange();
- requestScopeOperations.setApiUser(robot.id());
- ReviewInput reviewInput = new ReviewInput();
- ReviewInput.RobotCommentInput robotCommentInput =
- TestCommentHelper.createRobotCommentInputWithMandatoryFields("a.txt");
- reviewInput.robotComments = ImmutableMap.of("a.txt", ImmutableList.of(robotCommentInput));
- change(r).current().review(reviewInput);
-
- AttentionSetUpdate attentionSet =
- Iterables.getOnlyElement(getAttentionSetUpdatesForUser(r, admin));
- assertThat(attentionSet).hasAccountIdThat().isEqualTo(admin.id());
- assertThat(attentionSet).hasOperationThat().isEqualTo(AttentionSetUpdate.Operation.ADD);
- assertThat(attentionSet).hasReasonThat().isEqualTo("A robot comment was added");
- }
-
- @Test
public void robotCommentDoesNotAddOwnerOnClosedChanges() throws Exception {
TestAccount robot =
accountCreator.create("robot2", "robot2@example.com", "Ro Bot", "Ro", "Service Users");
diff --git a/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java b/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java
index 903ce1a..b4dd4b3 100644
--- a/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java
@@ -78,7 +78,6 @@
import java.util.Map;
import java.util.Optional;
import java.util.function.Function;
-import java.util.function.Supplier;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
import java.util.stream.Collectors;
@@ -1883,39 +1882,46 @@
private static CommentInput newCommentWithOnlyMandatoryFields(String path, String message) {
CommentInput c = new CommentInput();
- return populate(c, path, null, null, null, null, message, false);
+ c.unresolved = false;
+ return populate(c, path, null, null, null, null, message);
}
private static CommentInput newComment(
String path, Side side, int line, String message, Boolean unresolved) {
CommentInput c = new CommentInput();
- return populate(c, path, side, null, line, message, unresolved);
+ c.unresolved = unresolved;
+ return populate(c, path, side, null, line, message);
}
private static CommentInput newCommentOnParent(
String path, int parent, int line, String message) {
CommentInput c = new CommentInput();
- return populate(c, path, Side.PARENT, parent, line, message, false);
+ c.unresolved = false;
+ return populate(c, path, Side.PARENT, parent, line, message);
}
private DraftInput newDraft(String path, Side side, int line, String message) {
DraftInput d = new DraftInput();
- return populate(d, path, side, null, line, message, false);
+ d.unresolved = false;
+ return populate(d, path, side, null, line, message);
}
private DraftInput newDraft(String path, Side side, Comment.Range range, String message) {
DraftInput d = new DraftInput();
- return populate(d, path, side, null, range.startLine, range, message, false);
+ d.unresolved = false;
+ return populate(d, path, side, null, range.startLine, range, message);
}
private DraftInput newDraftOnParent(String path, int parent, int line, String message) {
DraftInput d = new DraftInput();
- return populate(d, path, Side.PARENT, parent, line, message, false);
+ d.unresolved = false;
+ return populate(d, path, Side.PARENT, parent, line, message);
}
private DraftInput newDraftWithOnlyMandatoryFields(String path, String message) {
DraftInput d = new DraftInput();
- return populate(d, path, null, null, null, null, message, false);
+ d.unresolved = false;
+ return populate(d, path, null, null, null, null, message);
}
private static <C extends Comment> C populate(
@@ -1925,14 +1931,12 @@
Integer parent,
Integer line,
Comment.Range range,
- String message,
- Boolean unresolved) {
+ String message) {
c.path = path;
c.side = side;
c.parent = parent;
c.line = line != null && line != 0 ? line : null;
c.message = message;
- c.unresolved = unresolved;
if (range != null) {
c.range = range;
}
@@ -1940,8 +1944,8 @@
}
private static <C extends Comment> C populate(
- C c, String path, Side side, Integer parent, int line, String message, Boolean unresolved) {
- return populate(c, path, side, parent, line, null, message, unresolved);
+ C c, String path, Side side, Integer parent, int line, String message) {
+ return populate(c, path, side, parent, line, null, message);
}
private static Comment.Range createLineRange(int startChar, int endChar) {
@@ -1954,20 +1958,22 @@
}
private static Function<CommentInfo, CommentInput> infoToInput(String path) {
- return infoToInput(path, CommentInput::new);
+ return info -> {
+ CommentInput commentInput = new CommentInput();
+ commentInput.path = path;
+ commentInput.unresolved = info.unresolved;
+ copy(info, commentInput);
+ return commentInput;
+ };
}
private static Function<CommentInfo, DraftInput> infoToDraft(String path) {
- return infoToInput(path, DraftInput::new);
- }
-
- private static <I extends Comment> Function<CommentInfo, I> infoToInput(
- String path, Supplier<I> supplier) {
return info -> {
- I i = supplier.get();
- i.path = path;
- copy(info, i);
- return i;
+ DraftInput draftInput = new DraftInput();
+ draftInput.path = path;
+ draftInput.unresolved = info.unresolved;
+ copy(info, draftInput);
+ return draftInput;
};
}
@@ -1977,7 +1983,6 @@
to.line = from.line;
to.message = from.message;
to.range = from.range;
- to.unresolved = from.unresolved;
to.inReplyTo = from.inReplyTo;
}
}
diff --git a/javatests/com/google/gerrit/acceptance/testsuite/change/PatchsetOperationsImplTest.java b/javatests/com/google/gerrit/acceptance/testsuite/change/PatchsetOperationsImplTest.java
index 9161928..46d9abf 100644
--- a/javatests/com/google/gerrit/acceptance/testsuite/change/PatchsetOperationsImplTest.java
+++ b/javatests/com/google/gerrit/acceptance/testsuite/change/PatchsetOperationsImplTest.java
@@ -1067,28 +1067,6 @@
}
@Test
- public void robotCommentCanBeCreatedAsResolved() throws Exception {
- Change.Id changeId = changeOperations.newChange().create();
-
- String commentUuid =
- changeOperations.change(changeId).currentPatchset().newRobotComment().resolved().create();
-
- CommentInfo comment = getRobotCommentFromServerInCurrentPatchset(changeId, commentUuid);
- assertThat(comment).unresolved().isFalse();
- }
-
- @Test
- public void robotCommentCanBeCreatedAsUnresolved() throws Exception {
- Change.Id changeId = changeOperations.newChange().create();
-
- String commentUuid =
- changeOperations.change(changeId).currentPatchset().newRobotComment().unresolved().create();
-
- CommentInfo comment = getRobotCommentFromServerInCurrentPatchset(changeId, commentUuid);
- assertThat(comment).unresolved().isTrue();
- }
-
- @Test
public void replyToRobotCommentCanBeCreated() throws Exception {
Change.Id changeId = changeOperations.newChange().create();
String parentCommentUuid =
diff --git a/plugins/replication b/plugins/replication
index 7ad27c7..293ee35 160000
--- a/plugins/replication
+++ b/plugins/replication
@@ -1 +1 @@
-Subproject commit 7ad27c7ac49731e70dd5ad3691b0b410492792f0
+Subproject commit 293ee3590d9fb57f6882e11126d8c97532cce968
diff --git a/polygerrit-ui/app/elements/change/gr-message/gr-message.ts b/polygerrit-ui/app/elements/change/gr-message/gr-message.ts
index d627071..87d443b 100644
--- a/polygerrit-ui/app/elements/change/gr-message/gr-message.ts
+++ b/polygerrit-ui/app/elements/change/gr-message/gr-message.ts
@@ -44,8 +44,8 @@
import {CommentThread} from '../../diff/gr-comment-api/gr-comment-api';
import {hasOwnProperty} from '../../../utils/common-util';
-const PATCH_SET_PREFIX_PATTERN = /^Patch Set \d+:\s*(.*)/;
-const LABEL_TITLE_SCORE_PATTERN = /^(-?)([A-Za-z0-9-]+?)([+-]\d+)?$/;
+const PATCH_SET_PREFIX_PATTERN = /^(?:Uploaded\s*)?(?:P|p)atch (?:S|s)et \d+:\s*(.*)/;
+const LABEL_TITLE_SCORE_PATTERN = /^(-?)([A-Za-z0-9-]+?)([+-]\d+)?[.]?$/;
declare global {
interface HTMLElementTagNameMap {
diff --git a/polygerrit-ui/app/elements/change/gr-message/gr-message_test.js b/polygerrit-ui/app/elements/change/gr-message/gr-message_test.js
index 2e2b278..a705d45 100644
--- a/polygerrit-ui/app/elements/change/gr-message/gr-message_test.js
+++ b/polygerrit-ui/app/elements/change/gr-message/gr-message_test.js
@@ -315,6 +315,32 @@
assert.isFalse(scoreChips[2].classList.contains('min'));
});
+ test('Uploaded patch set X', () => {
+ element.message = {
+ author: {},
+ expanded: false,
+ message: 'Uploaded patch set 1:' +
+ 'Verified+1 Code-Review-2 Trybot-Label3+1 Blub+1',
+ };
+ element.labelExtremes = {
+ 'Verified': {max: 1, min: -1},
+ 'Code-Review': {max: 2, min: -2},
+ 'Trybot-Label3': {max: 3, min: 0},
+ };
+ flush();
+ const scoreChips = element.root.querySelectorAll('.score');
+ assert.equal(scoreChips.length, 3);
+
+ assert.isTrue(scoreChips[0].classList.contains('positive'));
+ assert.isTrue(scoreChips[0].classList.contains('max'));
+
+ assert.isTrue(scoreChips[1].classList.contains('negative'));
+ assert.isTrue(scoreChips[1].classList.contains('min'));
+
+ assert.isTrue(scoreChips[2].classList.contains('positive'));
+ assert.isFalse(scoreChips[2].classList.contains('min'));
+ });
+
test('removed votes', () => {
element.message = {
author: {},
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_html.ts b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_html.ts
index a289be4..ef3e848 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_html.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_html.ts
@@ -225,6 +225,7 @@
on-change="_handleReviewedChange"
hidden$="[[!_loggedIn]]"
hidden=""
+ title="Toggle reviewed status of file"
aria-label="file reviewed"
/><!--
-->
diff --git a/polygerrit-ui/app/elements/shared/gr-hovercard/gr-hovercard-behavior.ts b/polygerrit-ui/app/elements/shared/gr-hovercard/gr-hovercard-behavior.ts
index a3233be..90dac88 100644
--- a/polygerrit-ui/app/elements/shared/gr-hovercard/gr-hovercard-behavior.ts
+++ b/polygerrit-ui/app/elements/shared/gr-hovercard/gr-hovercard-behavior.ts
@@ -193,10 +193,12 @@
* Hovercard elements are created outside of <gr-app>, so if you want to fire
* events, then you probably want to do that through the target element.
*/
- dispatchEventThroughTarget(eventName: string) {
+ dispatchEventThroughTarget(eventName: string, detail?: unknown) {
+ if (!detail) detail = {};
if (this._target)
this._target.dispatchEvent(
new CustomEvent(eventName, {
+ detail,
bubbles: true,
composed: true,
})
@@ -463,7 +465,7 @@
removeListeners(): void;
debounceHide(): void;
cancelHideDebouncer(): void;
- dispatchEventThroughTarget(eventName: string): void;
+ dispatchEventThroughTarget(eventName: string, detail?: unknown): void;
hide(e?: MouseEvent): void;
debounceShow(): void;
debounceShowBy(delayMs: number): void;
diff --git a/polygerrit-ui/app/services/flags/flags.ts b/polygerrit-ui/app/services/flags/flags.ts
index b45710b..047e9e0 100644
--- a/polygerrit-ui/app/services/flags/flags.ts
+++ b/polygerrit-ui/app/services/flags/flags.ts
@@ -26,4 +26,5 @@
export enum KnownExperimentId {
PATCHSET_COMMENTS = 'UiFeature__patchset_comments',
PATCHSET_CHOICE_FOR_COMMENT_LINKS = 'UiFeature__patchset_choice_for_comment_links',
+ NEW_CONTEXT_CONTROLS = 'UiFeature__new_context_controls',
}
diff --git a/polygerrit-ui/app/types/common.ts b/polygerrit-ui/app/types/common.ts
index 06e3810..caa5f7f 100644
--- a/polygerrit-ui/app/types/common.ts
+++ b/polygerrit-ui/app/types/common.ts
@@ -1830,7 +1830,7 @@
labels?: LabelNameToValuesMap;
comments?: PathToCommentsInputMap;
robot_comments?: PathToRobotCommentsMap;
- drafts: DraftsAction;
+ drafts?: DraftsAction;
notify?: NotifyType;
notify_details?: RecipientTypeToNotifyInfoMap;
omit_duplicate_comments?: boolean;
diff --git a/polygerrit-ui/server.go b/polygerrit-ui/server.go
index 12b3516..3844b5b 100644
--- a/polygerrit-ui/server.go
+++ b/polygerrit-ui/server.go
@@ -527,38 +527,56 @@
)
type typescriptLogWriter struct {
- logger *log.Logger
+ unfinishedLine string
+ logger *log.Logger
// when WaitGroup counter is 0 the compilation is complete
compilationDoneWaiter *sync.WaitGroup
}
func newTypescriptLogWriter(compilationCompleteWaiter *sync.WaitGroup) *typescriptLogWriter {
return &typescriptLogWriter{
+ unfinishedLine: "",
logger: log.New(log.Writer(), "TSC - ", log.Flags()),
compilationDoneWaiter: compilationCompleteWaiter,
}
}
func (lw typescriptLogWriter) Write(p []byte) (n int, err error) {
- text := strings.TrimSpace(string(p))
- if strings.HasSuffix(text, tsFileChangeDetectedMsg) ||
- strings.HasSuffix(text, tsStartingCompilation) {
- lw.compilationDoneWaiter.Add(1)
+ // The input p can contain several lines and/or the partial line
+ // Code splits the input by EOL marker (\n) and stores the unfinished line
+ // for the next call to Write.
+ partialText := lw.unfinishedLine + string(p)
+ lines := strings.Split(partialText, "\n")
+ fullLines := lines
+ if strings.HasSuffix(partialText, "\n") {
+ lw.unfinishedLine = ""
+ } else {
+ fullLines = lines[:len(lines)-1]
+ lw.unfinishedLine = lines[len(lines)-1]
}
- if tsStartWatchingMsg.MatchString(text) {
- // A source code can be changed while previous compiler run is in progress.
- // In this case typescript reruns compilation again almost immediately
- // after the previous run finishes. To detect this situation, we are
- // waiting waitForNextChangeInterval before decreasing the counter.
- // If another compiler run is started in this interval, we will wait
- // again until it finishes.
- go func() {
- time.Sleep(waitForNextChangeInterval)
- lw.compilationDoneWaiter.Add(-1)
- }()
-
+ for _, fullLine := range fullLines {
+ text := strings.TrimSpace(fullLine)
+ if text == "" {
+ continue
+ }
+ if strings.HasSuffix(text, tsFileChangeDetectedMsg) ||
+ strings.HasSuffix(text, tsStartingCompilation) {
+ lw.compilationDoneWaiter.Add(1)
+ }
+ if tsStartWatchingMsg.MatchString(text) {
+ // A source code can be changed while previous compiler run is in progress.
+ // In this case typescript reruns compilation again almost immediately
+ // after the previous run finishes. To detect this situation, we are
+ // waiting waitForNextChangeInterval before decreasing the counter.
+ // If another compiler run is started in this interval, we will wait
+ // again until it finishes.
+ go func() {
+ time.Sleep(waitForNextChangeInterval)
+ lw.compilationDoneWaiter.Add(-1)
+ }()
+ }
+ lw.logger.Print(text)
}
- lw.logger.Print(text)
return len(p), nil
}
