diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/CommentSender.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/CommentSender.java index 70a4888718..33955e3ee7 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/CommentSender.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/CommentSender.java @@ -39,9 +39,12 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; import java.io.IOException; +import java.util.ArrayList; import java.util.Collections; +import java.util.HashMap; import java.util.HashSet; import java.util.List; +import java.util.Map; import java.util.Optional; import java.util.Set; @@ -55,6 +58,44 @@ public class CommentSender extends ReplyToChangeSender { CommentSender create(Project.NameKey project, Change.Id id); } + private class FileCommentGroup { + public String filename; + public int patchSetId; + public PatchFile fileData; + public List comments = new ArrayList<>(); + + /** + * @return a web link to the given patch set and file. + */ + public String getLink() { + String url = getGerritUrl(); + if (url == null) { + return null; + } + + return new StringBuilder() + .append(url) + .append("#/c/").append(change.getId()) + .append('/').append(patchSetId) + .append('/').append(KeyUtil.encode(filename)) + .toString(); + } + + /** + * @return A title for the group, i.e. "Commit Message", "Merge List", or + * "File [[filename]]". + */ + public String getTitle() { + if (Patch.COMMIT_MSG.equals(filename)) { + return "Commit Message"; + } else if (Patch.MERGE_LIST.equals(filename)) { + return "Merge List"; + } else { + return "File " + filename; + } + } + } + private List inlineComments = Collections.emptyList(); private final CommentsUtil commentsUtil; @@ -102,17 +143,53 @@ public class CommentSender extends ReplyToChangeSender { appendText(textTemplate("CommentFooter")); } + /** + * No longer used outside Velocity. Remove this method when VTL support is + * removed. + */ + @Deprecated public boolean hasInlineComments() { return !inlineComments.isEmpty(); } + /** + * No longer used outside Velocity. Remove this method when VTL support is + * removed. + */ + @Deprecated public String getInlineComments() { return getInlineComments(1); } + /** + * No longer used outside Velocity. Remove this method when VTL support is + * removed. + */ + @Deprecated public String getInlineComments(int lines) { StringBuilder cmts = new StringBuilder(); + for (FileCommentGroup group : getGroupedInlineComments()) { + String link = group.getLink(); + if (link != null) { + cmts.append(link).append('\n'); + } + cmts.append(group.getTitle()).append(":\n\n"); + for (Comment c : group.comments) { + appendComment(cmts, lines, group.fileData, c); + } + cmts.append("\n\n"); + } + return cmts.toString(); + } + + /** + * @return a list of FileCommentGroup objects representing the inline comments + * grouped by the file. + */ + private List getGroupedInlineComments() { + List groups = new ArrayList<>(); try (Repository repo = getRepository()) { + // Get the patch list: PatchList patchList = null; if (repo != null) { try { @@ -122,29 +199,21 @@ public class CommentSender extends ReplyToChangeSender { } } - String currentFileName = null; - int currentPatchSetId = -1; - PatchFile currentFileData = null; + // Loop over the comments and collect them into groups based on the file + // location of the comment. + FileCommentGroup currentGroup = null; for (Comment c : inlineComments) { - if (!c.key.filename.equals(currentFileName) - || c.key.patchSetId != currentPatchSetId) { - String link = makeLink(change.getId(), c.key); - if (link != null) { - cmts.append(link).append('\n'); - } - if (Patch.COMMIT_MSG.equals(c.key.filename)) { - cmts.append("Commit Message:\n\n"); - } else if (Patch.MERGE_LIST.equals(c.key.filename)) { - cmts.append("Merge List:\n\n"); - } else { - cmts.append("File ").append(c.key.filename).append(":\n\n"); - } - currentFileName = c.key.filename; - currentPatchSetId = c.key.patchSetId; - + // If it's a new group: + if (currentGroup == null + || !c.key.filename.equals(currentGroup.filename) + || c.key.patchSetId != currentGroup.patchSetId) { + currentGroup = new FileCommentGroup(); + currentGroup.filename = c.key.filename; + currentGroup.patchSetId = c.key.patchSetId; + groups.add(currentGroup); if (patchList != null) { try { - currentFileData = + currentGroup.fileData = new PatchFile(repo, patchList, c.key.filename); } catch (IOException e) { log.warn(String.format( @@ -152,20 +221,25 @@ public class CommentSender extends ReplyToChangeSender { c.key.filename, patchList.getNewId().name(), projectState.getProject().getName()), e); - currentFileData = null; + currentGroup.fileData = null; } } } - if (currentFileData != null) { - appendComment(cmts, lines, currentFileData, c); + if (currentGroup.fileData != null) { + currentGroup.comments.add(c); } - cmts.append("\n\n"); } } - return cmts.toString(); + + return groups; } + /** + * No longer used except for Velocity. Remove this method when VTL support is + * removed. + */ + @Deprecated private void appendComment(StringBuilder out, int contextLines, PatchFile currentFileData, Comment comment) { if (comment instanceof RobotComment) { @@ -176,62 +250,107 @@ public class CommentSender extends ReplyToChangeSender { .append(robotComment.robotRunId) .append("):\n"); } - short side = comment.side; - Comment.Range range = comment.range; - if (range != null) { - String prefix = "PS" + comment.key.patchSetId - + ", Line " + range.startLine + ": "; - for (int n = range.startLine; n <= range.endLine; n++) { - out.append(n == range.startLine - ? prefix - : Strings.padStart(": ", prefix.length(), ' ')); - String s = getLine(currentFileData, side, n); - if (n == range.startLine && n == range.endLine) { - s = s.substring( - Math.min(range.startChar, s.length()), - Math.min(range.endChar, s.length())); - } else if (n == range.startLine) { - s = s.substring(Math.min(range.startChar, s.length())); - } else if (n == range.endLine) { - s = s.substring(0, Math.min(range.endChar, s.length())); - } - out.append(s).append('\n'); - } - appendQuotedParent(out, comment); - out.append(comment.message.trim()).append('\n'); + if (comment.range != null) { + appendRangedComment(out, contextLines, currentFileData, comment); } else { - int lineNbr = comment.lineNbr; - - // Initialize maxLines to the known line number. - int maxLines = lineNbr; - - try { - maxLines = currentFileData.getLineCount(side); - } catch (IOException err) { - // The file could not be read, leave the max as is. - log.warn(String.format("Failed to read file %s on side %d", - comment.key.filename, side), err); - } catch (NoSuchEntityException err) { - // The file could not be read, leave the max as is. - log.warn(String.format("Side %d of file %s didn't exist", - side, comment.key.filename), err); - } - - final int startLine = Math.max(1, lineNbr - contextLines + 1); - final int stopLine = Math.min(maxLines, lineNbr + contextLines); - - for (int line = startLine; line <= lineNbr; ++line) { - appendFileLine(out, currentFileData, side, line); - } - appendQuotedParent(out, comment); - out.append(comment.message.trim()).append('\n'); - - for (int line = lineNbr + 1; line < stopLine; ++line) { - appendFileLine(out, currentFileData, side, line); - } + appendLineComment(out, contextLines, currentFileData, comment); } } + /** + * No longer used except for Velocity. Remove this method when VTL support is + * removed. + */ + @Deprecated + private void appendRangedComment(StringBuilder out, int contextLines, + PatchFile fileData, Comment comment) { + String prefix = getCommentLinePrefix(comment); + String emptyPrefix = Strings.padStart(": ", prefix.length(), ' '); + boolean firstLine = true; + for (String line : getLinesByRange(comment.range, fileData, comment.side)) { + out.append(firstLine ? prefix : emptyPrefix) + .append(line) + .append('\n'); + firstLine = false; + } + appendQuotedParent(out, comment); + out.append(comment.message.trim()).append('\n'); + } + + private String getCommentLinePrefix(Comment comment) { + int lineNbr = comment.range == null ? + comment.lineNbr : comment.range.startLine; + return "PS" + comment.key.patchSetId + ", Line " + lineNbr + ": "; + } + + /** + * @return the lines of file content in fileData that are encompassed by range + * on the given side. + */ + private List getLinesByRange(Comment.Range range, + PatchFile fileData, short side) { + List lines = new ArrayList<>(); + + for (int n = range.startLine; n <= range.endLine; n++) { + String s = getLine(fileData, side, n); + if (n == range.startLine && n == range.endLine) { + s = s.substring( + Math.min(range.startChar, s.length()), + Math.min(range.endChar, s.length())); + } else if (n == range.startLine) { + s = s.substring(Math.min(range.startChar, s.length())); + } else if (n == range.endLine) { + s = s.substring(0, Math.min(range.endChar, s.length())); + } + lines.add(s); + } + return lines; + } + + /** + * No longer used except for Velocity. Remove this method when VTL support is + * removed. + */ + @Deprecated + private void appendLineComment(StringBuilder out, int contextLines, + PatchFile currentFileData, Comment comment) { + short side = comment.side; + int lineNbr = comment.lineNbr; + + // Initialize maxLines to the known line number. + int maxLines = lineNbr; + + try { + maxLines = currentFileData.getLineCount(side); + } catch (IOException err) { + // The file could not be read, leave the max as is. + log.warn(String.format("Failed to read file %s on side %d", + comment.key.filename, side), err); + } catch (NoSuchEntityException err) { + // The file could not be read, leave the max as is. + log.warn(String.format("Side %d of file %s didn't exist", + side, comment.key.filename), err); + } + + int startLine = Math.max(1, lineNbr - contextLines + 1); + int stopLine = Math.min(maxLines, lineNbr + contextLines); + + for (int line = startLine; line <= lineNbr; ++line) { + appendFileLine(out, currentFileData, side, line); + } + appendQuotedParent(out, comment); + out.append(comment.message.trim()).append('\n'); + + for (int line = lineNbr + 1; line < stopLine; ++line) { + appendFileLine(out, currentFileData, side, line); + } + } + + /** + * No longer used except for Velocity. Remove this method when VTL support is + * removed. + */ + @Deprecated private void appendFileLine(StringBuilder cmts, PatchFile fileData, short side, int line) { String lineStr = getLine(fileData, side, line); @@ -242,45 +361,131 @@ public class CommentSender extends ReplyToChangeSender { .append("\n"); } + /** + * No longer used except for Velocity. Remove this method when VTL support is + * removed. + */ + @Deprecated private void appendQuotedParent(StringBuilder out, Comment child) { - if (child.parentUuid != null) { - Optional parent; - Comment.Key key = new Comment.Key(child.parentUuid, child.key.filename, - child.key.patchSetId); - try { - parent = commentsUtil.get(args.db.get(), changeData.notes(), key); - } catch (OrmException e) { - log.warn("Could not find the parent of this comment: " - + child.toString()); - parent = Optional.empty(); - } - if (parent.isPresent()) { - String msg = parent.get().message.trim(); - if (msg.length() > 75) { - msg = msg.substring(0, 75); - } - int lf = msg.indexOf('\n'); - if (lf > 0) { - msg = msg.substring(0, lf); - } - out.append("> ").append(msg).append('\n'); - } + Optional parent = getParent(child); + if (parent.isPresent()) { + out.append("> ") + .append(getShortenedCommentMessage(parent.get())) + .append('\n'); } } - // Makes a link back to the given patch set and file. - private String makeLink(Change.Id changeId, Comment.Key key) { - String url = getGerritUrl(); - if (url == null) { - return null; + /** + * Get the parent comment of a given comment. + * @param child the comment with a potential parent comment. + * @return an optional comment that will be present if the given comment has + * a parent, and is empty if it does not. + */ + private Optional getParent(Comment child) { + if (child.parentUuid == null) { + return Optional.empty(); } - return new StringBuilder() - .append(url) - .append("#/c/").append(changeId) - .append('/').append(key.patchSetId) - .append('/').append(KeyUtil.encode(key.filename)) - .toString(); + Optional parent; + Comment.Key key = new Comment.Key(child.parentUuid, child.key.filename, + child.key.patchSetId); + try { + return commentsUtil.get(args.db.get(), changeData.notes(), key); + } catch (OrmException e) { + log.warn("Could not find the parent of this comment: " + + child.toString()); + return Optional.empty(); + } + } + + /** + * Retrieve the file lines refered to by a comment. + * @param comment The comment that refers to some file contents. The comment + * may be a line comment or a ranged comment. + * @param fileData The file on which the comment appears. + * @return file contents referred to by the comment. If the comment is a line + * comment, the result will be a list of one string. Otherwise it will be + * a list of one or more strings. + */ + private List getLinesOfComment(Comment comment, PatchFile fileData) { + List lines = new ArrayList<>(); + if (comment.range == null) { + lines.add(getLine(fileData, comment.side, comment.lineNbr)); + } else { + lines.addAll(getLinesByRange(comment.range, fileData, comment.side)); + } + return lines; + } + + /** + * @return a shortened version of the given comment's message. Will be + * shortened to 75 characters or the first line, whichever is shorter. + */ + private String getShortenedCommentMessage(Comment comment) { + String msg = comment.message.trim(); + if (msg.length() > 75) { + msg = msg.substring(0, 75); + } + int lf = msg.indexOf('\n'); + if (lf > 0) { + msg = msg.substring(0, lf); + } + return msg; + } + + /** + * @return grouped inline comment data mapped to data structures that are + * suitable for passing into Soy. + */ + private List> getCommentGroupsTemplateData() { + List> commentGroups = new ArrayList<>(); + + for (CommentSender.FileCommentGroup group : getGroupedInlineComments()) { + Map groupData = new HashMap<>(); + groupData.put("link", group.getLink()); + groupData.put("title", group.getTitle()); + groupData.put("patchSetId", group.patchSetId); + + List> commentsList = new ArrayList<>(); + for (Comment comment : group.comments) { + Map commentData = new HashMap<>(); + commentData.put("lines", getLinesOfComment(comment, group.fileData)); + commentData.put("message", comment.message.trim()); + + String prefix = getCommentLinePrefix(comment); + commentData.put("linePrefix", prefix); + commentData.put("linePrefixEmpty", + Strings.padStart(": ", prefix.length(), ' ')); + + if (comment.range == null) { + commentData.put("startLine", comment.lineNbr); + } else { + commentData.put("startLine", comment.range.startLine); + commentData.put("endLine", comment.range.endLine); + } + + if (comment instanceof RobotComment) { + RobotComment robotComment = (RobotComment) comment; + commentData.put("isRobotComment", true); + commentData.put("robotId", robotComment.robotId); + commentData.put("robotRunId", robotComment.robotRunId); + } else { + commentData.put("isRobotComment", false); + } + + Optional parent = getParent(comment); + if (parent.isPresent()) { + commentData.put("parentMessage", + getShortenedCommentMessage(parent.get())); + } + + commentsList.add(commentData); + } + groupData.put("comments", commentsList); + + commentGroups.add(groupData); + } + return commentGroups; } private Repository getRepository() { @@ -294,8 +499,7 @@ public class CommentSender extends ReplyToChangeSender { @Override protected void setupSoyContext() { super.setupSoyContext(); - soyContextEmailData.put("inlineComments", getInlineComments()); - soyContextEmailData.put("hasInlineComments", hasInlineComments()); + soyContext.put("commentFiles", getCommentGroupsTemplateData()); } private String getLine(PatchFile fileInfo, short side, int lineNbr) { diff --git a/gerrit-server/src/main/resources/com/google/gerrit/server/mail/Comment.soy b/gerrit-server/src/main/resources/com/google/gerrit/server/mail/Comment.soy index 781d8a003e..ed574a8574 100644 --- a/gerrit-server/src/main/resources/com/google/gerrit/server/mail/Comment.soy +++ b/gerrit-server/src/main/resources/com/google/gerrit/server/mail/Comment.soy @@ -23,24 +23,47 @@ * @param coverLetter * @param email * @param fromName + * @param commentFiles */ {template .Comment autoescape="strict" kind="text"} - {if $coverLetter or $email.hasInlineComments} - {$fromName} has posted comments on this change. - {if $email.changeUrl} ( {$email.changeUrl} ){/if}{\n} - {\n} - Change subject: {$change.subject}{\n} - ......................................................................{\n} - {if $coverLetter} - {\n} - {\n} - {$coverLetter} - {/if} - {if $email.hasInlineComments} - {\n} - {\n} - {$email.inlineComments} - {/if} - {/if} + {$fromName} has posted comments on this change. + {if $email.changeUrl} ( {$email.changeUrl} ){/if}{\n} {\n} -{/template} \ No newline at end of file + Change subject: {$change.subject}{\n} + ......................................................................{\n} + {if $coverLetter} + {\n} + {\n} + {$coverLetter}{\n} + {\n} + {/if} + + {foreach $group in $commentFiles} + {$group.link}{\n} + {$group.title}:{\n} + {\n} + + {foreach $comment in $group.comments} + {if $comment.isRobotComment} + Robot Comment from {$comment.robotId} (run ID {$comment.robotRunId}): + {\n} + {/if} + + {foreach $line in $comment.lines} + {if isFirst($line)} + {$comment.linePrefix} + {else} + {$comment.linePrefixEmpty} + {/if} + {$line}{\n} + {/foreach} + {if $comment.parentMessage} + >{sp}{$comment.parentMessage}{\n} + {/if} + {$comment.message}{\n} + {\n} + {\n} + {/foreach} + {/foreach} + {\n} +{/template} diff --git a/gerrit-server/src/main/resources/com/google/gerrit/server/mail/CommentFooter.soy b/gerrit-server/src/main/resources/com/google/gerrit/server/mail/CommentFooter.soy index 3fcad6bb8d..7ef58b72a2 100644 --- a/gerrit-server/src/main/resources/com/google/gerrit/server/mail/CommentFooter.soy +++ b/gerrit-server/src/main/resources/com/google/gerrit/server/mail/CommentFooter.soy @@ -20,10 +20,10 @@ * The .CommentFooter template will determine the contents of the footer text * that will be appended to emails related to a user submitting comments on * changes. - * @param email + * @param commentFiles */ {template .CommentFooter autoescape="strict" kind="text"} - {if $email.hasInlineComments} + {if length($commentFiles) > 0} Gerrit-HasComments: Yes {else} Gerrit-HasComments: No