diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt index 265014e2cc..9876b53584 100644 --- a/Documentation/rest-api-changes.txt +++ b/Documentation/rest-api-changes.txt @@ -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 diff --git a/java/com/google/gerrit/extensions/api/changes/ChangeApi.java b/java/com/google/gerrit/extensions/api/changes/ChangeApi.java index 3364fc1d09..6c15c0c952 100644 --- a/java/com/google/gerrit/extensions/api/changes/ChangeApi.java +++ b/java/com/google/gerrit/extensions/api/changes/ChangeApi.java @@ -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 { diff --git a/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java b/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java index d349dda179..ce22eb4dc5 100644 --- a/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java +++ b/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java @@ -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); diff --git a/java/com/google/gerrit/server/comment/CommentContextCacheImpl.java b/java/com/google/gerrit/server/comment/CommentContextCacheImpl.java index 02a25b0dfa..532a685b7c 100644 --- a/java/com/google/gerrit/server/comment/CommentContextCacheImpl.java +++ b/java/com/google/gerrit/server/comment/CommentContextCacheImpl.java @@ -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 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 allContext = loader.getContext(commentsToKeys.keySet()); return allContext.entrySet().stream() diff --git a/java/com/google/gerrit/server/comment/CommentContextKey.java b/java/com/google/gerrit/server/comment/CommentContextKey.java index ccd50b7eed..af2ae92733 100644 --- a/java/com/google/gerrit/server/comment/CommentContextKey.java +++ b/java/com/google/gerrit/server/comment/CommentContextKey.java @@ -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(); } } diff --git a/java/com/google/gerrit/server/comment/CommentContextLoader.java b/java/com/google/gerrit/server/comment/CommentContextLoader.java index 46c430706d..63bb8d025b 100644 --- a/java/com/google/gerrit/server/comment/CommentContextLoader.java +++ b/java/com/google/gerrit/server/comment/CommentContextLoader.java @@ -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 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 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(); } } diff --git a/java/com/google/gerrit/server/restapi/change/CommentJson.java b/java/com/google/gerrit/server/restapi/change/CommentJson.java index 02f39ab783..77b58c657e 100644 --- a/java/com/google/gerrit/server/restapi/change/CommentJson.java +++ b/java/com/google/gerrit/server/restapi/change/CommentJson.java @@ -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(); } diff --git a/java/com/google/gerrit/server/restapi/change/ListChangeComments.java b/java/com/google/gerrit/server/restapi/change/ListChangeComments.java index fa7c1f55e2..c90e4fce86 100644 --- a/java/com/google/gerrit/server/restapi/change/ListChangeComments.java +++ b/java/com/google/gerrit/server/restapi/change/ListChangeComments.java @@ -42,6 +42,7 @@ public class ListChangeComments implements RestReadView { 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 { 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 { .setFillAccounts(true) .setFillPatchSet(true) .setFillCommentContext(includeContext) + .setContextPadding(contextPadding) .setProjectKey(rsrc.getProject()) .setChangeId(rsrc.getId()) .newHumanCommentFormatter(); diff --git a/javatests/com/google/gerrit/acceptance/server/change/CommentContextIT.java b/javatests/com/google/gerrit/acceptance/server/change/CommentContextIT.java index 8fa80cee94..52993e841c 100644 --- a/javatests/com/google/gerrit/acceptance/server/change/CommentContextIT.java +++ b/javatests/com/google/gerrit/acceptance/server/change/CommentContextIT.java @@ -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 expectedLines) throws Exception { + List 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; diff --git a/javatests/com/google/gerrit/server/cache/serialize/CommentContextSerializerTest.java b/javatests/com/google/gerrit/server/cache/serialize/CommentContextSerializerTest.java index 84f290cff8..643c7b7ef5 100644 --- a/javatests/com/google/gerrit/server/cache/serialize/CommentContextSerializerTest.java +++ b/javatests/com/google/gerrit/server/cache/serialize/CommentContextSerializerTest.java @@ -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)); diff --git a/proto/cache.proto b/proto/cache.proto index f3db71f05b..e827956c98 100644 --- a/proto/cache.proto +++ b/proto/cache.proto @@ -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