commit 03674da5c408c91db11ce7e765f131f52f30ecaf Author: Alice Kober-Sotzek Date: Fri Sep 25 11:38:44 2020 +0200 Remove handling of ignored diff context parameter (just backend logic) Since Ib70c6526a and I6f33b2141, it hasn't mattered whether a request asked for a specific context as we internally 'upgraded' the context to MAX_CONTEXT (= 5,000,000 lines). As a result, Gerrit either always returned the full file content or none at all (-> special 'skip' behavior, which seems to be outdated and problematic as well). In Iae2b098c3, we documented with tests that the context parameter was ignored. We've kept around this effectively dead code for years and carried it through several refactorings. In addition, the handling of the context parameter increased the complexity of the code, making the code base harder to understand for developers. The different behavior when comments are present also had some unintended side effects as one of the tests in Iae2b098c3 showed. Furthermore, we don't even know whether the original code for the context handling still works as we don't have any tests covering a less-than-file context. Hence, we decided to remove the context parameter from the diff calculation in the backend. This doesn't mean that Gerrit diffs shown on the UI now always expand to the full file content. Since Ib70c6526a, the frontend has been responsible for honoring the diff context setting and we'll keep it that way. If it turns out that we'll need such a context parameter in the backend for optimizations in the future, we can always add it back (and potentially use the deleted code as guidance). Instead of just deleting the context parameter from the involved methods, we also adjusted some coding constructs which can now be much simpler. One example is the EditList class, which was simplified to the EditHunk class as we now have just one large EditHunk instead of a list. The involved code could be improved much further with newer Java practices and functionality in mind. That's beyond the scope of this change, though. For the moment, we still need to keep the context parameter as request parameter as the frontend unnecessarily sends it along. Without adapting it, requests would result in errors as GET request parameters are checked for existence. I79e307b63 will remove the parameter from the frontend calls. Change-Id: I6f5acd4e2cbe2a6a0b296b1bc323982ef0049d3a diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt index f25e27d..9889ade 100644 --- a/Documentation/rest-api-changes.txt +++ b/Documentation/rest-api-changes.txt @@ -5585,9 +5585,6 @@ The `whitespace` parameter can be specified to control how whitespace 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 -- 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 @@ public interface FileApi { abstract class DiffRequest { private String base; - private Integer context; private Boolean intraline; private Whitespace whitespace; private OptionalInt parent = OptionalInt.empty(); @@ -64,11 +63,6 @@ public interface FileApi { 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 @@ public interface FileApi { return base; } - public Integer getContext() { - return context; - } - public Boolean getIntraline() { return intraline; } 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 @@ public class DiffPreferencesInfo { /** 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/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 edits; + + private int curIdx; + private Edit curEdit; + + private int aCur; + private int bCur; + private final int aEnd; + private final int bEnd; + + public EditHunk(List 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 904995a..0000000 --- a/java/com/google/gerrit/prettify/common/EditList.java +++ /dev/null @@ -1,149 +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 edits; - private final int context; - private final int aSize; - private final int bSize; - - public EditList(final List edits, int contextLines, int aSize, int bSize) { - this.edits = edits; - this.context = contextLines; - this.aSize = aSize; - this.bSize = bSize; - } - - public Iterable getHunks() { - return () -> - new Iterator() { - 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 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); - Edit 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 void incA() { - aCur++; - } - - public void incB() { - bCur++; - } - - public void incBoth() { - incA(); - incB(); - } - - public boolean isContextLine() { - 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 < 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 @@ class FileApiImpl implements FileApi { 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/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_SORT = comparing(Edit::getBeginA); - private final DiffPreferencesInfo diffPrefs; DiffContentCalculator(DiffPreferencesInfo diffPrefs) { @@ -60,13 +51,11 @@ class DiffContentCalculator { * @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 edits, CommentDetail comments) { - int context = getContext(); - if (srcA.src == srcB.src && srcA.size() <= context && edits.isEmpty()) { + TextSource srcA, TextSource srcB, ImmutableList 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 @@ class DiffContentCalculator { Edit emptyEdit = new Edit(srcA.size(), srcA.size()); return new DiffCalculatorResult(diffContent, ImmutableList.of(emptyEdit)); } - ImmutableList.Builder builder = ImmutableList.builder(); - - builder.addAll(correctForDifferencesInNewlineAtEnd(srcA, srcB, edits)); + ImmutableList sortedEdits = correctForDifferencesInNewlineAtEnd(srcA, srcB, edits); - boolean nonsortedEdits = false; - if (comments != null) { - ImmutableList commentEdits = ensureCommentsVisible(comments, edits); - builder.addAll(commentEdits); - nonsortedEdits = !commentEdits.isEmpty(); - } - - ImmutableList 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 correctForDifferencesInNewlineAtEnd( TextSource a, TextSource b, ImmutableList edits) { // a.src.size() is the size ignoring a newline at the end whereas a.size() considers it. @@ -205,128 +164,14 @@ class DiffContentCalculator { return a.src.isMissingNewlineAtEnd() && !b.src.isMissingNewlineAtEnd(); } - private ImmutableList ensureCommentsVisible( - CommentDetail comments, ImmutableList 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 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 getNewEditForComment(ImmutableList 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 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 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 edits, - int context) { + TextSource a, TextSource b, boolean ignoredWhitespace, ImmutableList 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 static java.nio.charset.StandardCharsets.UTF_8; 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 @@ class PatchScriptBuilder { 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 @@ class PatchScriptBuilder { 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 @@ class PatchScriptBuilder { ChangeType.MODIFIED, PatchType.UNIFIED); - return build(a, b, change, null); + return build(a, b, change); } private PatchSide resolveSideA( @@ -147,9 +145,7 @@ class PatchScriptBuilder { } } - private PatchScript build( - PatchSide a, PatchSide b, PatchFileChange content, CommentDetail comments) { - + private PatchScript build(PatchSide a, PatchSide b, PatchFileChange content) { ImmutableList contentEdits = content.getEdits(); ImmutableSet editsDueToRebase = content.getEditsDueToRebase(); @@ -163,8 +159,7 @@ class PatchScriptBuilder { ImmutableList 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 static com.google.common.base.Preconditions.checkState; 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 @@ public class PatchScriptFactory implements Callable { private final PatchSetUtil psUtil; private final Provider builderFactory; private final PatchListCache patchListCache; - private final CommentsUtil commentsUtil; private final String fileName; @Nullable private final PatchSet.Id psa; @@ -92,12 +85,10 @@ public class PatchScriptFactory implements Callable { private final PatchSet.Id psb; private final DiffPreferencesInfo diffPrefs; private final ChangeEditUtil editReader; - private final Provider 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 @@ public class PatchScriptFactory implements Callable { PatchSetUtil psUtil, Provider builderFactory, PatchListCache patchListCache, - CommentsUtil commentsUtil, ChangeEditUtil editReader, - Provider userProvider, PermissionBackend permissionBackend, ProjectCache projectCache, @Assisted ChangeNotes notes, @@ -122,9 +111,7 @@ public class PatchScriptFactory implements Callable { 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 @@ public class PatchScriptFactory implements Callable { PatchSetUtil psUtil, Provider builderFactory, PatchListCache patchListCache, - CommentsUtil commentsUtil, ChangeEditUtil editReader, - Provider userProvider, PermissionBackend permissionBackend, ProjectCache projectCache, @Assisted ChangeNotes notes, @@ -158,9 +143,7 @@ public class PatchScriptFactory implements Callable { 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 @@ public class PatchScriptFactory implements Callable { 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 @@ public class PatchScriptFactory implements Callable { final PatchScriptBuilder b = newBuilder(); final PatchListEntry content = list.get(fileName); - Optional 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 @@ public class PatchScriptFactory implements Callable { } } - private Optional 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 getAId() { if (psa == null) { return Optional.empty(); @@ -300,99 +269,6 @@ public class PatchScriptFactory implements Callable { } } - private static class CommentsLoader { - private final PatchSet.Id psa; - private final PatchSet.Id psb; - private final Provider userProvider; - private final ChangeNotes notes; - private final CommentsUtil commentsUtil; - private CommentDetail comments; - - CommentsLoader( - PatchSet.Id psa, - PatchSet.Id psb, - Provider userProvider, - ChangeNotes notes, - CommentsUtil commentsUtil) { - this.psa = psa; - this.psb = psb; - this.userProvider = userProvider; - this.notes = notes; - this.commentsUtil = commentsUtil; - } - - private Optional 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/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.gerrit.server.project.ProjectState; 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 @@ public class GetDiff implements RestReadView { @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 @@ public class GetDiff implements RestReadView { } 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 @@ public class GetDiff implements RestReadView { } 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 @@ public class GetDiff implements RestReadView { 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 @@ public class GetDiff implements RestReadView { } } + // TODO(hiesel): Remove this class once clients don't send the context parameter anymore. public static class ContextOptionHandler extends OptionHandler { public ContextOptionHandler(CmdLineParser parser, OptionDef option, Setter setter) { @@ -281,33 +273,14 @@ public class GetDiff implements RestReadView { } @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/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.acceptance.PushOneCommit.Result; 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.ObjectId; 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 @@ public class RevisionDiffIT extends AbstractDaemonTest { 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 @@ public class RevisionDiffIT extends AbstractDaemonTest { } @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 @@ public class RevisionDiffIT extends AbstractDaemonTest { } @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 @@ public class RevisionDiffIT extends AbstractDaemonTest { .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 @@ public class RevisionDiffIT extends AbstractDaemonTest { .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 @@ public class RevisionDiffIT extends AbstractDaemonTest { 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 @@ public class RevisionDiffIT extends AbstractDaemonTest { .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 @@ public class RevisionDiffIT extends AbstractDaemonTest { .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 @@ public class RevisionDiffIT extends AbstractDaemonTest { 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 @@ public class RevisionDiffIT extends AbstractDaemonTest { 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 @@ public class RevisionDiffIT extends AbstractDaemonTest { 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 @@ public class ChangeEditIT extends AbstractDaemonTest { } 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 @@ public class ChangeEditIT extends AbstractDaemonTest { + revisionId + "/files/" + fileName - + "/diff?context=ALL&intraline"; + + "/diff?intraline"; } private T readContentFromJson(RestResponse r, Class clazz) throws Exception {