opendev/gerrit
Code Issues Proposed changes

Add the context-padding parameter to the comment context

Browse Source 
This parameter allows padding the context lines with extra lines before
and after the comment range. This parameter works only if the
enable-context parameter is set to true.

This change is backward compatible: if the new parameter is not set or
set to zero, only the lines of the comment range are returned.

Change-Id: I2d44fb852c68ba484cbf94adff912b7ee522f9c5
This commit is contained in:
Youssef Elghareeb
2021-02-05 18:53:05 +01:00
parent abfec19e6d
commit 87b7450210
11 changed files with 159 additions and 18 deletions
Download Patch File Download Diff File Expand all files Collapse all files

4
Documentation/rest-api-changes.txt
View File

@@ -2056,6 +2056,10 @@ If the `enable-context` request parameter is set to true, the comment entries
will contain a list of link:#context-line[ContextLine] containing the lines of
the source file where the comment was written.
The `context-padding` request parameter can be used to specify an extra number
of context lines to be added before and after the comment range. This parameter
only works if `enable-context` is set to true.
.Request
----
GET /changes/myProject~master~I8473b95934b5732ac55d26311a706c9c2bde9940/comments HTTP/1.0

10
java/com/google/gerrit/extensions/api/changes/ChangeApi.java
View File

@@ -413,6 +413,7 @@ public interface ChangeApi {
abstract class CommentsRequest {
private boolean enableContext;
private int contextPadding;
/**
* Get all published comments on a change.
@@ -436,6 +437,11 @@ public interface ChangeApi {
return this;
}
public CommentsRequest contextPadding(int contextPadding) {
this.contextPadding = contextPadding;
return this;
}
public CommentsRequest withContext() {
this.enableContext = true;
return this;
@@ -444,6 +450,10 @@ public interface ChangeApi {
public boolean getContext() {
return enableContext;
}
public int getContextPadding() {
return contextPadding;
}
}
abstract class SuggestedReviewersRequest {

2
java/com/google/gerrit/server/api/changes/ChangeApiImpl.java
View File

@@ -612,6 +612,7 @@ class ChangeApiImpl implements ChangeApi {
try {
ListChangeComments listComments = listCommentsProvider.get();
listComments.setContext(this.getContext());
listComments.setContextPadding(this.getContextPadding());
return listComments.apply(change).value();
} catch (Exception e) {
throw asRestApiException("Cannot get comments", e);
@@ -623,6 +624,7 @@ class ChangeApiImpl implements ChangeApi {
try {
ListChangeComments listComments = listCommentsProvider.get();
listComments.setContext(this.getContext());
listComments.setContextPadding(this.getContextPadding());
return listComments.getComments(change);
} catch (Exception e) {
throw asRestApiException("Cannot get comments", e);

4
java/com/google/gerrit/server/comment/CommentContextCacheImpl.java
View File

@@ -58,7 +58,7 @@ public class CommentContextCacheImpl implements CommentContextCache {
@Override
protected void configure() {
persist(CACHE_NAME, CommentContextKey.class, CommentContext.class)
.version(1)
.version(2)
.diskLimit(1 << 30) // limit the total cache size to 1 GB
.maximumWeight(1 << 23) // Limit the size of the in-memory cache to 8 MB
.weigher(CommentContextWeigher.class)
@@ -220,7 +220,7 @@ public class CommentContextCacheImpl implements CommentContextCache {
Map<ContextInput, CommentContextKey> commentsToKeys = new HashMap<>();
for (CommentContextKey key : keys) {
Comment comment = getCommentForKey(humanComments, key);
commentsToKeys.put(ContextInput.fromComment(comment), key);
commentsToKeys.put(ContextInput.fromComment(comment, key.contextPadding()), key);
}
Map<ContextInput, CommentContext> allContext = loader.getContext(commentsToKeys.keySet());
return allContext.entrySet().stream()

7
java/com/google/gerrit/server/comment/CommentContextKey.java
View File

@@ -28,6 +28,9 @@ public abstract class CommentContextKey {
abstract Integer patchset();
/** Number of extra lines of context that should be added before and after the comment range. */
abstract int contextPadding();
abstract Builder toBuilder();
public static Builder builder() {
@@ -47,6 +50,8 @@ public abstract class CommentContextKey {
public abstract Builder patchset(Integer patchset);
public abstract Builder contextPadding(Integer numLines);
public abstract CommentContextKey build();
}
@@ -62,6 +67,7 @@ public abstract class CommentContextKey {
.setPatchset(key.patchset())
.setPathHash(key.path())
.setCommentId(key.id())
.setContextPadding(key.contextPadding())
.build());
}
@@ -75,6 +81,7 @@ public abstract class CommentContextKey {
.patchset(proto.getPatchset())
.id(proto.getCommentId())
.path(proto.getPathHash())
.contextPadding(proto.getContextPadding())
.build();
}
}

68
java/com/google/gerrit/server/comment/CommentContextLoader.java
View File

@@ -98,15 +98,20 @@ public class CommentContextLoader {
case COMMIT_MSG:
result.put(
contextInput,
getContextForCommitMessage(rw.getObjectReader(), commit, range.get()));
getContextForCommitMessage(
rw.getObjectReader(), commit, range.get(), contextInput.contextPadding()));
break;
case MERGE_LIST:
result.put(
contextInput, getContextForMergeList(rw.getObjectReader(), commit, range.get()));
contextInput,
getContextForMergeList(
rw.getObjectReader(), commit, range.get(), contextInput.contextPadding()));
break;
default:
result.put(
contextInput, getContextForFilePath(repo, rw, commit, filePath, range.get()));
contextInput,
getContextForFilePath(
repo, rw, commit, filePath, range.get(), contextInput.contextPadding()));
}
}
}
@@ -115,20 +120,27 @@ public class CommentContextLoader {
}
private CommentContext getContextForCommitMessage(
ObjectReader reader, RevCommit commit, Range range) throws IOException {
ObjectReader reader, RevCommit commit, Range commentRange, int contextPadding)
throws IOException {
Text text = Text.forCommit(reader, commit);
return createContext(text, range);
return createContext(text, commentRange, contextPadding);
}
private CommentContext getContextForMergeList(ObjectReader reader, RevCommit commit, Range range)
private CommentContext getContextForMergeList(
ObjectReader reader, RevCommit commit, Range commentRange, int contextPadding)
throws IOException {
ComparisonType cmp = ComparisonType.againstParent(1);
Text text = Text.forMergeList(cmp, reader, commit);
return createContext(text, range);
return createContext(text, commentRange, contextPadding);
}
private CommentContext getContextForFilePath(
Repository repo, RevWalk rw, RevCommit commit, String filePath, Range range)
Repository repo,
RevWalk rw,
RevCommit commit,
String filePath,
Range commentRange,
int contextPadding)
throws IOException {
// TODO(ghareeb): We can further group the comments by file paths to avoid opening
// the same file multiple times.
@@ -140,23 +152,38 @@ public class CommentContextLoader {
}
ObjectId id = tw.getObjectId(0);
Text src = new Text(repo.open(id, Constants.OBJ_BLOB));
return createContext(src, range);
return createContext(src, commentRange, contextPadding);
}
}
private static CommentContext createContext(Text src, Range range) {
if (range.start() < 1 || range.end() > src.size()) {
private static CommentContext createContext(Text src, Range commentRange, int contextPadding) {
if (commentRange.start() < 1 || commentRange.end() - 1 > src.size()) {
throw new StorageException(
"Invalid comment range " + range + ". Text only contains " + src.size() + " lines.");
"Invalid comment range "
+ commentRange
+ ". Text only contains "
+ src.size()
+ " lines.");
}
commentRange = adjustRange(commentRange, contextPadding, src.size());
ImmutableMap.Builder<Integer, String> context =
ImmutableMap.builderWithExpectedSize(range.end() - range.start());
for (int i = range.start(); i < range.end(); i++) {
ImmutableMap.builderWithExpectedSize(commentRange.end() - commentRange.start());
for (int i = commentRange.start(); i < commentRange.end(); i++) {
context.put(i, src.getString(i - 1));
}
return CommentContext.create(context.build());
}
/**
* Adjust the {@code commentRange} parameter by adding {@code contextPadding} lines before and
* after the comment range.
*/
private static Range adjustRange(Range commentRange, int contextPadding, int fileLines) {
int newStartLine = commentRange.start() - contextPadding;
int newEndLine = commentRange.end() + contextPadding;
return Range.create(Math.max(1, newStartLine), Math.min(fileLines + 1, newEndLine));
}
private static Optional<Range> getStartAndEndLines(ContextInput comment) {
if (comment.range() != null) {
return Optional.of(Range.create(comment.range().startLine, comment.range().endLine + 1));
@@ -177,17 +204,23 @@ public class CommentContextLoader {
/** End line of the comment (exclusive). */
abstract int end();
/** Number of lines covered by this range. */
int size() {
return end() - start();
}
}
/** This entity only contains comment fields needed to load the comment context. */
@AutoValue
abstract static class ContextInput {
static ContextInput fromComment(Comment comment) {
static ContextInput fromComment(Comment comment, int contextPadding) {
return new AutoValue_CommentContextLoader_ContextInput.Builder()
.commitId(comment.getCommitId())
.filePath(comment.key.filename)
.range(comment.range)
.lineNumber(comment.lineNbr)
.contextPadding(contextPadding)
.build();
}
@@ -210,6 +243,9 @@ public class CommentContextLoader {
*/
abstract Integer lineNumber();
/** Number of extra lines of context that should be added before and after the comment range. */
abstract Integer contextPadding();
@AutoValue.Builder
public abstract static class Builder {
@@ -221,6 +257,8 @@ public class CommentContextLoader {
public abstract Builder lineNumber(Integer lineNumber);
public abstract Builder contextPadding(Integer contextPadding);
public abstract ContextInput build();
}
}

7
java/com/google/gerrit/server/restapi/change/CommentJson.java
View File

@@ -61,6 +61,7 @@ public class CommentJson {
private boolean fillAccounts = true;
private boolean fillPatchSet;
private boolean fillCommentContext;
private int contextPadding;
@Inject
CommentJson(AccountLoader.Factory accountLoaderFactory, CommentContextCache commentContextCache) {
@@ -83,6 +84,11 @@ public class CommentJson {
return this;
}
CommentJson setContextPadding(int contextPadding) {
this.contextPadding = contextPadding;
return this;
}
CommentJson setProjectKey(Project.NameKey project) {
this.project = project;
return this;
@@ -184,6 +190,7 @@ public class CommentJson {
.id(r.id)
.path(r.path)
.patchset(r.patchSet)
.contextPadding(contextPadding)
.build();
}

12
java/com/google/gerrit/server/restapi/change/ListChangeComments.java
View File

@@ -42,6 +42,7 @@ public class ListChangeComments implements RestReadView<ChangeResource> {
private final CommentsUtil commentsUtil;
private boolean includeContext;
private int contextPadding;
/**
* Optional parameter. If set, the contextLines field of the {@link ContextLineInfo} of the
@@ -54,6 +55,16 @@ public class ListChangeComments implements RestReadView<ChangeResource> {
this.includeContext = context;
}
/**
* Optional parameter. Works only if {@link #includeContext} is set to true. If {@link
* #contextPadding} is set, the context lines in the response will be padded with {@link
* #contextPadding} extra lines before and after the comment range.
*/
@Option(name = "--context-padding")
public void setContextPadding(int contextPadding) {
this.contextPadding = contextPadding;
}
@Inject
ListChangeComments(
ChangeData.Factory changeDataFactory,
@@ -105,6 +116,7 @@ public class ListChangeComments implements RestReadView<ChangeResource> {
.setFillAccounts(true)
.setFillPatchSet(true)
.setFillCommentContext(includeContext)
.setContextPadding(contextPadding)
.setProjectKey(rsrc.getProject())
.setChangeId(rsrc.getId())
.newHumanCommentFormatter();

58
javatests/com/google/gerrit/acceptance/server/change/CommentContextIT.java
View File

@@ -286,6 +286,64 @@ public class CommentContextIT extends AbstractDaemonTest {
assertThat(comments.get(0).contextLines).isEmpty();
}
@Test
public void commentContextWithZeroPadding() throws Exception {
String changeId = createChangeWithComment(3, 4);
assertContextLines(changeId, /* contextPadding= */ 0, ImmutableList.of(3, 4));
}
@Test
public void commentContextWithSmallPadding() throws Exception {
String changeId = createChangeWithComment(3, 4);
assertContextLines(changeId, /* contextPadding= */ 1, ImmutableList.of(2, 3, 4, 5));
}
@Test
public void commentContextWithSmallPaddingAtTheBeginningOfFile() throws Exception {
String changeId = createChangeWithComment(1, 2);
assertContextLines(changeId, /* contextPadding= */ 2, ImmutableList.of(1, 2, 3, 4));
}
@Test
public void commentContextWithPaddingLargerThanFileSize() throws Exception {
String changeId = createChangeWithComment(3, 3);
assertContextLines(
changeId,
/* contextPadding= */ 20,
ImmutableList.of(1, 2, 3, 4, 5, 6)); // file only contains six lines.
}
private String createChangeWithComment(int startLine, int endLine) throws Exception {
PushOneCommit.Result result =
createChange(testRepo, "master", SUBJECT, FILE_NAME, FILE_CONTENT, "topic");
String changeId = result.getChangeId();
String ps1 = result.getCommit().name();
Comment.Range commentRange = createCommentRange(startLine, endLine);
CommentInput comment =
CommentsUtil.newComment(FILE_NAME, Side.REVISION, commentRange, "comment", false);
CommentsUtil.addComments(gApi, changeId, ps1, comment);
return changeId;
}
private void assertContextLines(
String changeId, int contextPadding, ImmutableList<Integer> expectedLines) throws Exception {
List<CommentInfo> comments =
gApi.changes()
.id(changeId)
.commentsRequest()
.withContext(true)
.contextPadding(contextPadding)
.getAsList();
assertThat(comments).hasSize(1);
assertThat(
comments.get(0).contextLines.stream()
.map(c -> c.lineNumber)
.collect(Collectors.toList()))
.containsExactlyElementsIn(expectedLines);
}
private Comment.Range createCommentRange(int startLine, int endLine) {
Comment.Range range = new Comment.Range();
range.startLine = startLine;

1
javatests/com/google/gerrit/server/cache/serialize/CommentContextSerializerTest.java vendored
View File

@@ -34,6 +34,7 @@ public class CommentContextSerializerTest {
.id("commentId")
.path("pathHash")
.patchset(1)
.contextPadding(3)
.build();
byte[] serialized = CommentContextKey.Serializer.INSTANCE.serialize(k);
assertThat(k).isEqualTo(CommentContextKey.Serializer.INSTANCE.deserialize(serialized));

4
proto/cache.proto
View File

@@ -511,7 +511,7 @@ message ProjectCacheKeyProto {
}
// Serialized form of com.google.gerrit.server.comment.CommentContextCacheImpl.Key
// Next ID: 6
// Next ID: 7
message CommentContextKeyProto {
string project = 1;
string change_id = 2;
@@ -520,6 +520,8 @@ message CommentContextKeyProto {
// hashed with the murmur3_128 hash function
string path_hash = 5;
int32 context_padding = 6;
}
// Serialized form of a list of com.google.gerrit.extensions.common.ContextLineInfo