Add backend API and implementation for ported comments

When a reviewer leaves comments on a patchset, those comments stay
still attached to that patchset when the change owner uploads further
patchsets. Replies to such comments (even when newer patchsets are
available) are also attached to these older patchsets. This doesn't make
a difference for views which simply list comments like the "Comments"
tab or the "Change log". However, diff views only show comments of the
chosen patchsets. Hence, changes with comments scattered all over
different patchsets require quite some navigation. Sometimes, users
might even miss some of these comments if they don't go to the right
views.

Ported comments represent a solution for the described situation. A
ported comment is a comment which was left on an earlier patchset and
is shown on a later patchset. The idea is that users can simply go to
a diff view and see all comments which were left until that patchset.
The plan is to make the ported comments distinguishable from the actual
comments on the patchset. However, exact details will be left to the
frontend implementation.

To show the comments left until a patchset, the frontend will need to
ask the backend both for the actual comments and the ported comments
of that patchset (-> two separate REST API calls).

The logic which comments are ported is contained in the backend. The
backend always ports a complete comment thread. In the first version
of ported comments, we decided to exclude resolved comment threads
as showing all comments left on earlier patchsets might produce too much
unrelated noise for users. We can improve on that in later versions of
this feature. This change still also ports resolved comments as we'll
only add the relevant filtering in a follow-up change.

Comments on the parent commit of a patchset should also be ported.
Those comments require additional considerations and hence are also
deferred to a follow-up change.

As for other comment endpoints, we distinguish between ported published
comments and ported draft comments. The endpoint for the latter ones
is added in a follow-up change.

This change focuses on adding the new REST endpoint for ported
published comments along with its implementation and extensive tests.
Adding these detailed tests proved useful as we even noticed a bug in
Gerrit's previous code (see I75a65bd57). We also know from past
experience with diffs and edits due to rebase that only such extensive
tests help to prevent regressions for corner cases.

One major part of the implementation of the ported comments endpoint is
the mapping of the comment positions from the original patchset to the
target patchset. For edits due to rebase, we already have an algorithm
to map a diff hunk between two commits to a diff hunk between two other
commits. With I4062e6a22, we generalized this algorithm so that we can
re-use it to map a comment position in one commit to the corresponding
position in another commit. That algorithm currently drops any positions
which run into a mapping conflict. Deleted files are also not properly
supported. We'll adjust those aspects in follow-up changes.

The position transformation algorithm also requires the diff between
patchset commits as input. At the moment, we get that diff from the
existing diff caches. However, the current structure of the diff caches
makes this a very expensive operation as the computed diff encompasses
all files between the patchsets. This should improve when we have a new
diff cache structure in place which is currently in the making and
which allows to only retrieve the diffs of necessary files (= those on
which comments were left).

Summary of open aspects addressed in follow-up changes:
- Filtering of resolved comment threads
- Handling of comments on parent commits (= "left side comments")
  including special considerations for merge commits
- Separate endpoint for ported draft comments (implemented in
If8802a4f9)
- Correct handling for file deletions (implemented in If8ed1e511)
- Adjustment of transformation logic to not exclude transformed
  positions on conflicts and to map those to file comments instead
  (implemented in I99f80bee3)
- Making endpoint more robust by catching diff errors and omitting
  corresponding comments
- REST API documentation (done last when the final state is available)
For some of them, we already added tests which we can enable as soon
as we have the correct implementation in place.

Change-Id: I4e96af5acf428939a3a130c7ca6f6b74d40dab12
This commit is contained in:
Alice Kober-Sotzek
2020-08-14 19:09:47 +02:00
parent 8fd62bede9
commit fda8c4fc92
6 changed files with 1771 additions and 0 deletions

View File

@@ -106,6 +106,8 @@ public interface RevisionApi {
List<RobotCommentInfo> robotCommentsAsList() throws RestApiException;
Map<String, List<CommentInfo>> portedComments() throws RestApiException;
/**
* Applies the indicated fix by creating a new change edit or integrating the fix with the
* existing change edit. If no change edit exists before this call, the fix must refer to the
@@ -293,6 +295,11 @@ public interface RevisionApi {
throw new NotImplementedException();
}
@Override
public Map<String, List<CommentInfo>> portedComments() throws RestApiException {
throw new NotImplementedException();
}
@Override
public EditInfo applyFix(String fixId) throws RestApiException {
throw new NotImplementedException();

View File

@@ -27,6 +27,7 @@ import com.google.common.truth.Subject;
import com.google.gerrit.extensions.client.Side;
import com.google.gerrit.extensions.common.CommentInfo;
import com.google.gerrit.truth.ListSubject;
import java.sql.Timestamp;
import java.util.List;
public class CommentInfoSubject extends Subject {
@@ -91,6 +92,10 @@ public class CommentInfoSubject extends Subject {
return check("inReplyTo").that(commentInfo().inReplyTo);
}
public StringSubject commitId() {
return check("commitId").that(commentInfo().commitId);
}
public AccountInfoSubject author() {
return check("author").about(accounts()).that(commentInfo().author);
}
@@ -99,6 +104,14 @@ public class CommentInfoSubject extends Subject {
return check("tag").that(commentInfo().tag);
}
public ComparableSubject<Timestamp> updated() {
return check("updated").that(commentInfo().updated);
}
public StringSubject changeMessageId() {
return check("changeMessageId").that(commentInfo().changeMessageId);
}
private CommentInfo commentInfo() {
isNotNull();
return commentInfo;