opendev/gerrit
Code Issues Proposed changes

Remove handling of ignored diff context parameter (just backend logic)

Browse Source 
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
This commit is contained in:
Alice Kober-Sotzek
2020-09-25 11:38:44 +02:00
parent 55c531c9f1
commit 03674da5c4
12 changed files with 169 additions and 660 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -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;
}

6
java/com/google/gerrit/extensions/client/DiffPreferencesInfo.java
View File

@@ -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,