Merge "Document that single quotes are illegal in notify filters"
diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt
index 0fc733a..67ae65c 100644
--- a/Documentation/rest-api-changes.txt
+++ b/Documentation/rest-api-changes.txt
@@ -6017,6 +6017,10 @@
The numeric Change-Id of the change that this change reverts.
|`submission_id` |optional|
ID of the submission of this change. Only set if the status is `MERGED`.
+This ID is equal to the numeric ID of the change that triggered the submission.
+If the change that triggered the submission also has a topic, it will be
+"<id>-<topic>" of the change that triggered the submission.
+The callers must not rely on the format of the submission ID.
|==================================
[[change-input]]
diff --git a/java/com/google/gerrit/entities/SubmissionId.java b/java/com/google/gerrit/entities/SubmissionId.java
new file mode 100644
index 0000000..eb03a5a
--- /dev/null
+++ b/java/com/google/gerrit/entities/SubmissionId.java
@@ -0,0 +1,34 @@
+// Copyright (C) 2019 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.entities;
+
+import org.eclipse.jgit.annotations.Nullable;
+
+public class SubmissionId {
+ private final String submissionId;
+
+ public SubmissionId(Change.Id changeId, @Nullable String topic) {
+ submissionId = topic != null ? String.format("%s-%s", changeId, topic) : changeId.toString();
+ }
+
+ public SubmissionId(Change change) {
+ this(change.getId(), change.getTopic());
+ }
+
+ @Override
+ public String toString() {
+ return submissionId;
+ }
+}
diff --git a/java/com/google/gerrit/pgm/Daemon.java b/java/com/google/gerrit/pgm/Daemon.java
index 568fb60..da8411f 100644
--- a/java/com/google/gerrit/pgm/Daemon.java
+++ b/java/com/google/gerrit/pgm/Daemon.java
@@ -538,14 +538,14 @@
private Injector createWebInjector() {
final List<Module> modules = new ArrayList<>();
- if (sshd) {
- modules.add(new ProjectQoSFilter.Module());
- }
modules.add(RequestContextFilter.module());
modules.add(RequestMetricsFilter.module());
modules.add(H2CacheBasedWebSession.module());
modules.add(sysInjector.getInstance(GerritAuthModule.class));
modules.add(sysInjector.getInstance(GitOverHttpModule.class));
+ if (sshd) {
+ modules.add(new ProjectQoSFilter.Module());
+ }
modules.add(AllRequestFilter.module());
modules.add(sysInjector.getInstance(WebModule.class));
modules.add(sysInjector.getInstance(RequireSslFilter.Module.class));
diff --git a/java/com/google/gerrit/prettify/BUILD b/java/com/google/gerrit/prettify/BUILD
index 7c1241a..0a15fda 100644
--- a/java/com/google/gerrit/prettify/BUILD
+++ b/java/com/google/gerrit/prettify/BUILD
@@ -7,5 +7,7 @@
deps = [
"//lib:guava",
"//lib:jgit",
+ "//lib/auto:auto-value",
+ "//lib/auto:auto-value-annotations",
],
)
diff --git a/java/com/google/gerrit/prettify/common/SparseFileContent.java b/java/com/google/gerrit/prettify/common/SparseFileContent.java
index 348f9b2..1249b65 100644
--- a/java/com/google/gerrit/prettify/common/SparseFileContent.java
+++ b/java/com/google/gerrit/prettify/common/SparseFileContent.java
@@ -14,160 +14,175 @@
package com.google.gerrit.prettify.common;
-import java.util.ArrayList;
-import java.util.List;
+import com.google.auto.value.AutoValue;
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.collect.ImmutableList;
-public class SparseFileContent {
- protected List<Range> ranges;
- protected int size;
+/**
+ * A class to store subset of a file's lines in a memory efficient way. Internally, it stores lines
+ * as a list of ranges. Each range represents continuous set of lines and has information about line
+ * numbers in original file (zero-based).
+ *
+ * <p>{@link SparseFileContent.Accessor} must be used to work with the stored content.
+ */
+@AutoValue
+public abstract class SparseFileContent {
+ abstract ImmutableList<Range> getRanges();
- private transient int currentRangeIdx;
+ public abstract int getSize();
- public SparseFileContent() {
- ranges = new ArrayList<>();
+ public static SparseFileContent create(ImmutableList<Range> ranges, int size) {
+ return new AutoValue_SparseFileContent(ranges, size);
}
- public int size() {
- return size;
+ @VisibleForTesting
+ public int getRangesCount() {
+ return getRanges().size();
}
- public void setSize(int s) {
- size = s;
+ public Accessor createAccessor() {
+ return new Accessor(this);
}
- public String get(int idx) {
- final String line = getLine(idx);
- if (line == null) {
- throw new ArrayIndexOutOfBoundsException(idx);
- }
- return line;
- }
+ /**
+ * Provide a methods to work with the content of a {@link SparseFileContent}.
+ *
+ * <p>The class hides internal representation of a {@link SparseFileContent} and provides
+ * convenient way for accessing a content.
+ */
+ public static class Accessor {
+ private final SparseFileContent content;
+ private int currentRangeIdx;
- public boolean contains(int idx) {
- return getLine(idx) != null;
- }
-
- public int first() {
- return ranges.isEmpty() ? size() : ranges.get(0).base;
- }
-
- public int next(int idx) {
- // Most requests are sequential in nature, fetching the next
- // line from the current range, or the immediate next range.
- //
- int high = ranges.size();
- if (currentRangeIdx < high) {
- Range cur = ranges.get(currentRangeIdx);
- if (cur.contains(idx + 1)) {
- return idx + 1;
- }
-
- if (++currentRangeIdx < high) {
- // Its not plus one, its the base of the next range.
- //
- return ranges.get(currentRangeIdx).base;
- }
+ private Accessor(SparseFileContent content) {
+ this.content = content;
}
- // Binary search for the current value, since we know its a sorted list.
- //
- int low = 0;
- do {
- final int mid = (low + high) / 2;
- final Range cur = ranges.get(mid);
+ public String get(int idx) {
+ final String line = getLine(idx);
+ if (line == null) {
+ throw new ArrayIndexOutOfBoundsException(idx);
+ }
+ return line;
+ }
- if (cur.contains(idx)) {
+ public int getSize() {
+ return content.getSize();
+ }
+
+ public boolean contains(int idx) {
+ return getLine(idx) != null;
+ }
+
+ public int first() {
+ return content.getRanges().isEmpty() ? getSize() : content.getRanges().get(0).getBase();
+ }
+
+ public int next(int idx) {
+ // Most requests are sequential in nature, fetching the next
+ // line from the current range, or the immediate next range.
+ //
+ ImmutableList<Range> ranges = content.getRanges();
+ int high = ranges.size();
+ if (currentRangeIdx < high) {
+ Range cur = ranges.get(currentRangeIdx);
if (cur.contains(idx + 1)) {
- // Trivial plus one case above failed due to wrong currentRangeIdx.
- // Reset the cache so we don't miss in the future.
- //
- currentRangeIdx = mid;
return idx + 1;
}
- if (mid + 1 < ranges.size()) {
- // Its the base of the next range.
- currentRangeIdx = mid + 1;
- return ranges.get(currentRangeIdx).base;
- }
-
- // No more lines in the file.
- //
- return size();
- }
-
- if (idx < cur.base) {
- high = mid;
- } else {
- low = mid + 1;
- }
- } while (low < high);
-
- return size();
- }
-
- private String getLine(int idx) {
- // Most requests are sequential in nature, fetching the next
- // line from the current range, or the next range.
- //
- int high = ranges.size();
- if (currentRangeIdx < high) {
- Range cur = ranges.get(currentRangeIdx);
- if (cur.contains(idx)) {
- return cur.get(idx);
- }
-
- if (++currentRangeIdx < high) {
- final Range next = ranges.get(currentRangeIdx);
- if (next.contains(idx)) {
- return next.get(idx);
+ if (++currentRangeIdx < high) {
+ // Its not plus one, its the base of the next range.
+ //
+ return ranges.get(currentRangeIdx).getBase();
}
}
+
+ // Binary search for the current value, since we know its a sorted list.
+ //
+ int low = 0;
+ do {
+ final int mid = (low + high) / 2;
+ final Range cur = ranges.get(mid);
+
+ if (cur.contains(idx)) {
+ if (cur.contains(idx + 1)) {
+ // Trivial plus one case above failed due to wrong currentRangeIdx.
+ // Reset the cache so we don't miss in the future.
+ //
+ currentRangeIdx = mid;
+ return idx + 1;
+ }
+
+ if (mid + 1 < ranges.size()) {
+ // Its the base of the next range.
+ currentRangeIdx = mid + 1;
+ return ranges.get(currentRangeIdx).getBase();
+ }
+
+ // No more lines in the file.
+ //
+ return getSize();
+ }
+
+ if (idx < cur.getBase()) {
+ high = mid;
+ } else {
+ low = mid + 1;
+ }
+ } while (low < high);
+
+ return getSize();
}
- // Binary search for the range, since we know its a sorted list.
- //
- if (ranges.isEmpty()) {
+ private String getLine(int idx) {
+ // Most requests are sequential in nature, fetching the next
+ // line from the current range, or the next range.
+ //
+ ImmutableList<Range> ranges = content.getRanges();
+ int high = ranges.size();
+ if (currentRangeIdx < high) {
+ Range cur = ranges.get(currentRangeIdx);
+ if (cur.contains(idx)) {
+ return cur.get(idx);
+ }
+
+ if (++currentRangeIdx < high) {
+ final Range next = ranges.get(currentRangeIdx);
+ if (next.contains(idx)) {
+ return next.get(idx);
+ }
+ }
+ }
+
+ // Binary search for the range, since we know its a sorted list.
+ //
+ if (ranges.isEmpty()) {
+ return null;
+ }
+
+ int low = 0;
+ do {
+ final int mid = (low + high) / 2;
+ final Range cur = ranges.get(mid);
+ if (cur.contains(idx)) {
+ currentRangeIdx = mid;
+ return cur.get(idx);
+ }
+ if (idx < cur.getBase()) {
+ high = mid;
+ } else {
+ low = mid + 1;
+ }
+ } while (low < high);
return null;
}
-
- int low = 0;
- do {
- final int mid = (low + high) / 2;
- final Range cur = ranges.get(mid);
- if (cur.contains(idx)) {
- currentRangeIdx = mid;
- return cur.get(idx);
- }
- if (idx < cur.base) {
- high = mid;
- } else {
- low = mid + 1;
- }
- } while (low < high);
- return null;
- }
-
- public void addLine(int i, String content) {
- final Range r;
- if (!ranges.isEmpty() && i == last().end()) {
- r = last();
- } else {
- r = new Range(i);
- ranges.add(r);
- }
- r.lines.add(content);
- }
-
- private Range last() {
- return ranges.get(ranges.size() - 1);
}
@Override
- public String toString() {
+ public final String toString() {
final StringBuilder b = new StringBuilder();
b.append("SparseFileContent[\n");
- for (Range r : ranges) {
+ for (Range r : getRanges()) {
b.append(" ");
b.append(r.toString());
b.append('\n');
@@ -176,33 +191,32 @@
return b.toString();
}
- static class Range {
- protected int base;
- protected List<String> lines;
-
- private Range(int b) {
- base = b;
- lines = new ArrayList<>();
+ @AutoValue
+ abstract static class Range {
+ static Range create(int base, ImmutableList<String> lines) {
+ return new AutoValue_SparseFileContent_Range(base, lines);
}
- protected Range() {}
+ abstract int getBase();
+
+ abstract ImmutableList<String> getLines();
private String get(int i) {
- return lines.get(i - base);
+ return getLines().get(i - getBase());
}
private int end() {
- return base + lines.size();
+ return getBase() + getLines().size();
}
private boolean contains(int i) {
- return base <= i && i < end();
+ return getBase() <= i && i < end();
}
@Override
- public String toString() {
+ public final String toString() {
// Usage of [ and ) is intentional to denote inclusive/exclusive range
- return "Range[" + base + "," + end() + ")";
+ return "Range[" + getBase() + "," + end() + ")";
}
}
}
diff --git a/java/com/google/gerrit/prettify/common/SparseFileContentBuilder.java b/java/com/google/gerrit/prettify/common/SparseFileContentBuilder.java
new file mode 100644
index 0000000..04fb5d1
--- /dev/null
+++ b/java/com/google/gerrit/prettify/common/SparseFileContentBuilder.java
@@ -0,0 +1,90 @@
+// Copyright (C) 2019 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 com.google.common.collect.ImmutableList;
+import com.google.gerrit.prettify.common.SparseFileContent.Range;
+
+/**
+ * A builder for creating immutable {@link SparseFileContent}. Lines can be only be added in
+ * sequential (increased) order
+ */
+public class SparseFileContentBuilder {
+ private final ImmutableList.Builder<Range> ranges;
+ private final int size;
+ private int lastRangeBase;
+ private int lastRangeEnd;
+ private ImmutableList.Builder<String> lastRangeLines;
+
+ public SparseFileContentBuilder(int size) {
+ ranges = new ImmutableList.Builder<>();
+ startNextRange(0);
+ this.size = size;
+ }
+
+ public void addLine(int lineNumber, String content) {
+ if (lineNumber < 0) {
+ throw new IllegalArgumentException("Line number must be non-negative");
+ }
+ // if (lineNumber >= size) {
+ // The following 4 tests are failed if you uncomment this condition:
+ //
+ //
+ // diffOfFileWithMultilineRebaseHunkRemovingNewlineAtEndOfFileAndWithCommentReturnsFileContents
+ //
+ // diffOfFileWithMultilineRebaseHunkAddingNewlineAtEndOfFileAndWithCommentReturnsFileContents
+ //
+ //
+ // diffOfFileWithMultilineRebaseHunkRemovingNewlineAtEndOfFileAndWithCommentReturnsFileContents
+ //
+ // diffOfFileWithMultilineRebaseHunkAddingNewlineAtEndOfFileAndWithCommentReturnsFileContents
+ // Tests are failed because there are some bug with diff calculation.
+ // The condition must be uncommented after all these bugs are fixed.
+ // Also don't forget to remove ignore from for SparseFileContentBuilder
+ // throw new IllegalArgumentException(String.format("The zero-based line number %d is after
+ // the end of file. The file size is %d line(s).", lineNumber, size));
+ // }
+ if (lineNumber < lastRangeEnd) {
+ throw new IllegalArgumentException(
+ String.format(
+ "Invalid line number %d. You are trying to add a line before an already added line"
+ + " %d",
+ lineNumber, lastRangeEnd));
+ }
+ if (lineNumber > lastRangeEnd) {
+ finishLastRange();
+ startNextRange(lineNumber);
+ }
+ lastRangeLines.add(content);
+ lastRangeEnd++;
+ }
+
+ private void startNextRange(int base) {
+ lastRangeLines = new ImmutableList.Builder<>();
+ lastRangeBase = lastRangeEnd = base;
+ }
+
+ private void finishLastRange() {
+ if (lastRangeEnd > lastRangeBase) {
+ ranges.add(Range.create(lastRangeBase, lastRangeLines.build()));
+ lastRangeLines = null;
+ }
+ }
+
+ public SparseFileContent build() {
+ finishLastRange();
+ return SparseFileContent.create(ranges.build(), size);
+ }
+}
diff --git a/java/com/google/gerrit/prettify/common/testing/BUILD b/java/com/google/gerrit/prettify/common/testing/BUILD
new file mode 100644
index 0000000..5057fdb
--- /dev/null
+++ b/java/com/google/gerrit/prettify/common/testing/BUILD
@@ -0,0 +1,14 @@
+load("@rules_java//java:defs.bzl", "java_library")
+
+package(default_testonly = True)
+
+java_library(
+ name = "testing",
+ srcs = glob(["*.java"]),
+ visibility = ["//visibility:public"],
+ deps = [
+ "//java/com/google/gerrit/prettify:server",
+ "//lib:guava",
+ "//lib/truth",
+ ],
+)
diff --git a/java/com/google/gerrit/prettify/common/testing/SparseFileContentSubject.java b/java/com/google/gerrit/prettify/common/testing/SparseFileContentSubject.java
new file mode 100644
index 0000000..c1fe1ec
--- /dev/null
+++ b/java/com/google/gerrit/prettify/common/testing/SparseFileContentSubject.java
@@ -0,0 +1,65 @@
+// Copyright (C) 2019 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.testing;
+
+import static com.google.common.truth.Truth.assertAbout;
+
+import com.google.common.truth.FailureMetadata;
+import com.google.common.truth.IntegerSubject;
+import com.google.common.truth.MapSubject;
+import com.google.common.truth.Subject;
+import com.google.gerrit.prettify.common.SparseFileContent;
+import java.util.HashMap;
+import java.util.Map;
+
+public class SparseFileContentSubject extends Subject {
+ public static SparseFileContentSubject assertThat(SparseFileContent sparseFileContent) {
+ return assertAbout(sparseFileContent()).that(sparseFileContent);
+ }
+
+ private final SparseFileContent sparseFileContent;
+
+ private SparseFileContentSubject(FailureMetadata metadata, SparseFileContent actual) {
+ super(metadata, actual);
+ this.sparseFileContent = actual;
+ }
+
+ private static Subject.Factory<SparseFileContentSubject, SparseFileContent> sparseFileContent() {
+ return SparseFileContentSubject::new;
+ }
+
+ public IntegerSubject getSize() {
+ isNotNull();
+ return check("size()").that(sparseFileContent.getSize());
+ }
+
+ public IntegerSubject getRangesCount() {
+ isNotNull();
+ return check("rangesCount()").that(sparseFileContent.getRangesCount());
+ }
+
+ public MapSubject lines() {
+ isNotNull();
+ Map<Integer, String> lines = new HashMap<>();
+ SparseFileContent.Accessor accessor = sparseFileContent.createAccessor();
+ int size = accessor.getSize();
+ int idx = accessor.first();
+ while (idx < size) {
+ lines.put(idx, accessor.get(idx));
+ idx = accessor.next(idx);
+ }
+ return check("lines()").that(lines);
+ }
+}
diff --git a/java/com/google/gerrit/server/ExceptionHook.java b/java/com/google/gerrit/server/ExceptionHook.java
index a2bade0..019a715 100644
--- a/java/com/google/gerrit/server/ExceptionHook.java
+++ b/java/com/google/gerrit/server/ExceptionHook.java
@@ -34,6 +34,12 @@
* <p>Only affects operations that are executed with {@link
* com.google.gerrit.server.update.RetryHelper}.
*
+ * <p>Should return {@code true} only for exceptions that are caused by temporary issues where a
+ * retry of the operation has a chance to succeed.
+ *
+ * <p>If {@code false} is returned the operation is still retried once to capture a trace, unless
+ * {@link #skipRetryWithTrace(Throwable)} skips the auto-retry.
+ *
* @param throwable throwable that was thrown while executing the operation
* @return whether the operation should be retried
*/
@@ -42,6 +48,34 @@
}
/**
+ * Whether auto-retrying of an operation with tracing should be skipped for the given throwable.
+ *
+ * <p>Only affects operations that are executed with {@link
+ * com.google.gerrit.server.update.RetryHelper}.
+ *
+ * <p>This method is only called for exceptions for which the operation should not be retried
+ * ({@link #shouldRetry(Throwable)} returned {@code false}).
+ *
+ * <p>By default this method returns {@code false}, so that by default traces for unexpected
+ * exceptions are captured, which allows to investigate them.
+ *
+ * <p>Implementors may use this method to skip retry with tracing for exceptions that occur due to
+ * known causes that are permanent and where a trace is not needed for the investigation. For
+ * example, if an operation fails because persisted data is corrupt, it makes no sense to retry
+ * the operation with a trace, because the trace will not help with fixing the corrupt data.
+ *
+ * <p>This method is only invoked if retry with tracing is enabled on the server ({@code
+ * retry.retryWithTraceOnFailure} in {@code gerrit.config} is set to {@code true}).
+ *
+ * @param throwable throwable that was thrown while executing the operation
+ * @return whether auto-retrying of an operation with tracing should be skipped for the given
+ * throwable
+ */
+ default boolean skipRetryWithTrace(Throwable throwable) {
+ return false;
+ }
+
+ /**
* Formats the cause of an exception for use in metrics.
*
* <p>This method allows implementors to group exceptions that have the same cause into one metric
diff --git a/java/com/google/gerrit/server/change/ChangeInserter.java b/java/com/google/gerrit/server/change/ChangeInserter.java
index 4263373..d1c27d5 100644
--- a/java/com/google/gerrit/server/change/ChangeInserter.java
+++ b/java/com/google/gerrit/server/change/ChangeInserter.java
@@ -37,6 +37,7 @@
import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.entities.PatchSetApproval;
import com.google.gerrit.entities.PatchSetInfo;
+import com.google.gerrit.entities.SubmissionId;
import com.google.gerrit.extensions.api.changes.NotifyHandling;
import com.google.gerrit.extensions.client.ReviewerState;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
@@ -55,7 +56,6 @@
import com.google.gerrit.server.git.GroupCollector;
import com.google.gerrit.server.git.validators.CommitValidationException;
import com.google.gerrit.server.git.validators.CommitValidators;
-import com.google.gerrit.server.logging.RequestId;
import com.google.gerrit.server.mail.send.CreateChangeSender;
import com.google.gerrit.server.notedb.ChangeUpdate;
import com.google.gerrit.server.patch.PatchSetInfoFactory;
@@ -396,7 +396,7 @@
* instead of setting the status directly?
*/
if (change.getStatus() == Change.Status.MERGED) {
- update.fixStatusToMerged(new RequestId(ctx.getChange().getId().toString()));
+ update.fixStatusToMerged(new SubmissionId(change));
} else {
update.setStatus(change.getStatus());
}
diff --git a/java/com/google/gerrit/server/change/ConsistencyChecker.java b/java/com/google/gerrit/server/change/ConsistencyChecker.java
index 19db5ee..513f5ca 100644
--- a/java/com/google/gerrit/server/change/ConsistencyChecker.java
+++ b/java/com/google/gerrit/server/change/ConsistencyChecker.java
@@ -33,6 +33,7 @@
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.entities.Project;
+import com.google.gerrit.entities.SubmissionId;
import com.google.gerrit.exceptions.StorageException;
import com.google.gerrit.extensions.api.changes.FixInput;
import com.google.gerrit.extensions.common.ProblemInfo;
@@ -44,7 +45,6 @@
import com.google.gerrit.server.PatchSetUtil;
import com.google.gerrit.server.account.Accounts;
import com.google.gerrit.server.git.GitRepositoryManager;
-import com.google.gerrit.server.logging.RequestId;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.notedb.PatchSetState;
import com.google.gerrit.server.patch.PatchSetInfoFactory;
@@ -578,7 +578,7 @@
public boolean updateChange(ChangeContext ctx) {
ctx.getChange().setStatus(Change.Status.MERGED);
ctx.getUpdate(ctx.getChange().currentPatchSetId())
- .fixStatusToMerged(new RequestId(ctx.getChange().getId().toString()));
+ .fixStatusToMerged(new SubmissionId(ctx.getChange()));
p.status = Status.FIXED;
p.outcome = "Marked change as merged";
return true;
diff --git a/java/com/google/gerrit/server/diff/DiffInfoCreator.java b/java/com/google/gerrit/server/diff/DiffInfoCreator.java
new file mode 100644
index 0000000..c29ffc8
--- /dev/null
+++ b/java/com/google/gerrit/server/diff/DiffInfoCreator.java
@@ -0,0 +1,299 @@
+// Copyright (C) 2019 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.server.diff;
+
+import static com.google.common.base.Preconditions.checkState;
+
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.Lists;
+import com.google.common.collect.Maps;
+import com.google.common.flogger.FluentLogger;
+import com.google.gerrit.common.data.PatchScript;
+import com.google.gerrit.common.data.PatchScript.DisplayMethod;
+import com.google.gerrit.common.data.PatchScript.PatchScriptFileInfo;
+import com.google.gerrit.entities.Patch;
+import com.google.gerrit.extensions.common.ChangeType;
+import com.google.gerrit.extensions.common.DiffInfo;
+import com.google.gerrit.extensions.common.DiffInfo.ContentEntry;
+import com.google.gerrit.extensions.common.DiffInfo.FileMeta;
+import com.google.gerrit.extensions.common.DiffInfo.IntraLineStatus;
+import com.google.gerrit.extensions.common.DiffWebLinkInfo;
+import com.google.gerrit.extensions.common.WebLinkInfo;
+import com.google.gerrit.jgit.diff.ReplaceEdit;
+import com.google.gerrit.prettify.common.SparseFileContent;
+import com.google.gerrit.server.change.FileContentUtil;
+import com.google.gerrit.server.project.ProjectState;
+import java.util.List;
+import java.util.Optional;
+import java.util.Set;
+import org.eclipse.jgit.diff.Edit;
+
+/** Creates and fills a new {@link DiffInfo} object based on diff between files. */
+public class DiffInfoCreator {
+ private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+
+ private static final ImmutableMap<Patch.ChangeType, ChangeType> CHANGE_TYPE =
+ Maps.immutableEnumMap(
+ new ImmutableMap.Builder<Patch.ChangeType, ChangeType>()
+ .put(Patch.ChangeType.ADDED, ChangeType.ADDED)
+ .put(Patch.ChangeType.MODIFIED, ChangeType.MODIFIED)
+ .put(Patch.ChangeType.DELETED, ChangeType.DELETED)
+ .put(Patch.ChangeType.RENAMED, ChangeType.RENAMED)
+ .put(Patch.ChangeType.COPIED, ChangeType.COPIED)
+ .put(Patch.ChangeType.REWRITE, ChangeType.REWRITE)
+ .build());
+
+ private final DiffWebLinksProvider webLinksProvider;
+ private final boolean intraline;
+ private final ProjectState state;
+
+ public DiffInfoCreator(
+ ProjectState state, DiffWebLinksProvider webLinksProvider, boolean intraline) {
+ this.webLinksProvider = webLinksProvider;
+ this.state = state;
+ this.intraline = intraline;
+ }
+
+ /* Returns the {@link DiffInfo} to display for end-users */
+ public DiffInfo create(PatchScript ps, DiffSide sideA, DiffSide sideB) {
+ DiffInfo result = new DiffInfo();
+
+ ImmutableList<DiffWebLinkInfo> links = webLinksProvider.getDiffLinks();
+ result.webLinks = links.isEmpty() ? null : links;
+
+ if (ps.isBinary()) {
+ result.binary = true;
+ }
+ result.metaA = createFileMeta(sideA).orElse(null);
+ result.metaB = createFileMeta(sideB).orElse(null);
+
+ if (intraline) {
+ if (ps.hasIntralineTimeout()) {
+ result.intralineStatus = IntraLineStatus.TIMEOUT;
+ } else if (ps.hasIntralineFailure()) {
+ result.intralineStatus = IntraLineStatus.FAILURE;
+ } else {
+ result.intralineStatus = IntraLineStatus.OK;
+ }
+ logger.atFine().log("intralineStatus = %s", result.intralineStatus);
+ }
+
+ result.changeType = CHANGE_TYPE.get(ps.getChangeType());
+ logger.atFine().log("changeType = %s", result.changeType);
+ if (result.changeType == null) {
+ throw new IllegalStateException("unknown change type: " + ps.getChangeType());
+ }
+
+ if (ps.getPatchHeader().size() > 0) {
+ result.diffHeader = ps.getPatchHeader();
+ }
+ result.content = calculateDiffContentEntries(ps);
+ return result;
+ }
+
+ private static List<ContentEntry> calculateDiffContentEntries(PatchScript ps) {
+ ContentCollector contentCollector = new ContentCollector(ps);
+ Set<Edit> editsDueToRebase = ps.getEditsDueToRebase();
+ for (Edit edit : ps.getEdits()) {
+ logger.atFine().log("next edit = %s", edit);
+
+ if (edit.getType() == Edit.Type.EMPTY) {
+ logger.atFine().log("skip empty edit");
+ continue;
+ }
+ contentCollector.addCommon(edit.getBeginA());
+
+ checkState(
+ contentCollector.nextA == edit.getBeginA(),
+ "nextA = %s; want %s",
+ contentCollector.nextA,
+ edit.getBeginA());
+ checkState(
+ contentCollector.nextB == edit.getBeginB(),
+ "nextB = %s; want %s",
+ contentCollector.nextB,
+ edit.getBeginB());
+ switch (edit.getType()) {
+ case DELETE:
+ case INSERT:
+ case REPLACE:
+ List<Edit> internalEdit =
+ edit instanceof ReplaceEdit ? ((ReplaceEdit) edit).getInternalEdits() : null;
+ boolean dueToRebase = editsDueToRebase.contains(edit);
+ contentCollector.addDiff(edit.getEndA(), edit.getEndB(), internalEdit, dueToRebase);
+ break;
+ case EMPTY:
+ default:
+ throw new IllegalStateException();
+ }
+ }
+ contentCollector.addCommon(ps.getA().getSize());
+
+ return contentCollector.lines;
+ }
+
+ private Optional<FileMeta> createFileMeta(DiffSide side) {
+ PatchScriptFileInfo fileInfo = side.fileInfo();
+ if (fileInfo.displayMethod == DisplayMethod.NONE) {
+ return Optional.empty();
+ }
+ FileMeta result = new FileMeta();
+ result.name = side.fileName();
+ result.contentType =
+ FileContentUtil.resolveContentType(
+ state, side.fileName(), fileInfo.mode, fileInfo.mimeType);
+ result.lines = fileInfo.content.getSize();
+ ImmutableList<WebLinkInfo> links = webLinksProvider.getFileWebLinks(side.type());
+ result.webLinks = links.isEmpty() ? null : links;
+ result.commitId = fileInfo.commitId;
+ return Optional.of(result);
+ }
+
+ private static class ContentCollector {
+
+ private final List<ContentEntry> lines;
+ private final SparseFileContent.Accessor fileA;
+ private final SparseFileContent.Accessor fileB;
+ private final boolean ignoreWS;
+
+ private int nextA;
+ private int nextB;
+
+ ContentCollector(PatchScript ps) {
+ lines = Lists.newArrayListWithExpectedSize(ps.getEdits().size() + 2);
+ fileA = ps.getA().createAccessor();
+ fileB = ps.getB().createAccessor();
+ ignoreWS = ps.isIgnoreWhitespace();
+ }
+
+ void addCommon(int end) {
+ logger.atFine().log("addCommon: end = %d", end);
+
+ end = Math.min(end, fileA.getSize());
+ logger.atFine().log("end = %d", end);
+
+ if (nextA >= end) {
+ logger.atFine().log("nextA >= end: nextA = %d, end = %d", nextA, end);
+ return;
+ }
+
+ while (nextA < end) {
+ logger.atFine().log("nextA < end: nextA = %d, end = %d", nextA, end);
+
+ if (!fileA.contains(nextA)) {
+ logger.atFine().log("fileA does not contain nextA: nextA = %d", nextA);
+
+ int endRegion = Math.min(end, nextA == 0 ? fileA.first() : fileA.next(nextA - 1));
+ int len = endRegion - nextA;
+ entry().skip = len;
+ nextA = endRegion;
+ nextB += len;
+
+ logger.atFine().log("setting: nextA = %d, nextB = %d", nextA, nextB);
+ continue;
+ }
+
+ ContentEntry e = null;
+ for (int i = nextA; i == nextA && i < end; i = fileA.next(i), nextA++, nextB++) {
+ if (ignoreWS && fileB.contains(nextB)) {
+ if (e == null || e.common == null) {
+ logger.atFine().log("create new common entry: nextA = %d, nextB = %d", nextA, nextB);
+ e = entry();
+ e.a = Lists.newArrayListWithCapacity(end - nextA);
+ e.b = Lists.newArrayListWithCapacity(end - nextA);
+ e.common = true;
+ }
+ e.a.add(fileA.get(nextA));
+ e.b.add(fileB.get(nextB));
+ } else {
+ if (e == null || e.common != null) {
+ logger.atFine().log(
+ "create new non-common entry: nextA = %d, nextB = %d", nextA, nextB);
+ e = entry();
+ e.ab = Lists.newArrayListWithCapacity(end - nextA);
+ }
+ e.ab.add(fileA.get(nextA));
+ }
+ }
+ }
+ }
+
+ void addDiff(int endA, int endB, List<Edit> internalEdit, boolean dueToRebase) {
+ logger.atFine().log(
+ "addDiff: endA = %d, endB = %d, numberOfInternalEdits = %d, dueToRebase = %s",
+ endA, endB, internalEdit != null ? internalEdit.size() : 0, dueToRebase);
+
+ int lenA = endA - nextA;
+ int lenB = endB - nextB;
+ logger.atFine().log("lenA = %d, lenB = %d", lenA, lenB);
+ checkState(lenA > 0 || lenB > 0);
+
+ logger.atFine().log("create non-common entry");
+ ContentEntry e = entry();
+ if (lenA > 0) {
+ logger.atFine().log("lenA > 0: lenA = %d", lenA);
+ e.a = Lists.newArrayListWithCapacity(lenA);
+ for (; nextA < endA; nextA++) {
+ e.a.add(fileA.get(nextA));
+ }
+ }
+ if (lenB > 0) {
+ logger.atFine().log("lenB > 0: lenB = %d", lenB);
+ e.b = Lists.newArrayListWithCapacity(lenB);
+ for (; nextB < endB; nextB++) {
+ e.b.add(fileB.get(nextB));
+ }
+ }
+ if (internalEdit != null && !internalEdit.isEmpty()) {
+ logger.atFine().log("processing internal edits");
+
+ e.editA = Lists.newArrayListWithCapacity(internalEdit.size() * 2);
+ e.editB = Lists.newArrayListWithCapacity(internalEdit.size() * 2);
+ int lastA = 0;
+ int lastB = 0;
+ for (Edit edit : internalEdit) {
+ logger.atFine().log("internal edit = %s", edit);
+
+ if (edit.getBeginA() != edit.getEndA()) {
+ logger.atFine().log(
+ "edit.getBeginA() != edit.getEndA(): edit.getBeginA() = %d, edit.getEndA() = %d",
+ edit.getBeginA(), edit.getEndA());
+ e.editA.add(
+ ImmutableList.of(edit.getBeginA() - lastA, edit.getEndA() - edit.getBeginA()));
+ lastA = edit.getEndA();
+ logger.atFine().log("lastA = %d", lastA);
+ }
+ if (edit.getBeginB() != edit.getEndB()) {
+ logger.atFine().log(
+ "edit.getBeginB() != edit.getEndB(): edit.getBeginB() = %d, edit.getEndB() = %d",
+ edit.getBeginB(), edit.getEndB());
+ e.editB.add(
+ ImmutableList.of(edit.getBeginB() - lastB, edit.getEndB() - edit.getBeginB()));
+ lastB = edit.getEndB();
+ logger.atFine().log("lastB = %d", lastB);
+ }
+ }
+ }
+ e.dueToRebase = dueToRebase ? true : null;
+ }
+
+ private ContentEntry entry() {
+ ContentEntry e = new ContentEntry();
+ lines.add(e);
+ return e;
+ }
+ }
+}
diff --git a/java/com/google/gerrit/server/diff/DiffSide.java b/java/com/google/gerrit/server/diff/DiffSide.java
new file mode 100644
index 0000000..28c7810
--- /dev/null
+++ b/java/com/google/gerrit/server/diff/DiffSide.java
@@ -0,0 +1,37 @@
+// Copyright (C) 2019 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.server.diff;
+
+import com.google.auto.value.AutoValue;
+import com.google.gerrit.common.data.PatchScript.PatchScriptFileInfo;
+
+/** Contains settings for one of two sides in diff view. Each diff view has exactly 2 sides. */
+@AutoValue
+public abstract class DiffSide {
+ public enum Type {
+ SIDE_A,
+ SIDE_B
+ }
+
+ public static DiffSide create(PatchScriptFileInfo fileInfo, String fileName, Type type) {
+ return new AutoValue_DiffSide(fileInfo, fileName, type);
+ }
+
+ public abstract PatchScriptFileInfo fileInfo();
+
+ public abstract String fileName();
+
+ public abstract Type type();
+}
diff --git a/java/com/google/gerrit/server/diff/DiffWebLinksProvider.java b/java/com/google/gerrit/server/diff/DiffWebLinksProvider.java
new file mode 100644
index 0000000..0f71b17
--- /dev/null
+++ b/java/com/google/gerrit/server/diff/DiffWebLinksProvider.java
@@ -0,0 +1,29 @@
+// Copyright (C) 2019 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.server.diff;
+
+import com.google.common.collect.ImmutableList;
+import com.google.gerrit.extensions.common.DiffWebLinkInfo;
+import com.google.gerrit.extensions.common.WebLinkInfo;
+
+/** Provider for different types of links which can be displayed in a diff view. */
+public interface DiffWebLinksProvider {
+
+ /** Returns links associated with the diff view */
+ ImmutableList<DiffWebLinkInfo> getDiffLinks();
+
+ /** Returns links associated with the diff side */
+ ImmutableList<WebLinkInfo> getFileWebLinks(DiffSide.Type fileInfoType);
+}
diff --git a/java/com/google/gerrit/server/git/MergedByPushOp.java b/java/com/google/gerrit/server/git/MergedByPushOp.java
index 9aebebf..01d5380 100644
--- a/java/com/google/gerrit/server/git/MergedByPushOp.java
+++ b/java/com/google/gerrit/server/git/MergedByPushOp.java
@@ -22,11 +22,11 @@
import com.google.gerrit.entities.LabelId;
import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.entities.PatchSetInfo;
+import com.google.gerrit.entities.SubmissionId;
import com.google.gerrit.server.ChangeMessagesUtil;
import com.google.gerrit.server.PatchSetUtil;
import com.google.gerrit.server.config.SendEmailExecutor;
import com.google.gerrit.server.extensions.events.ChangeMerged;
-import com.google.gerrit.server.logging.RequestId;
import com.google.gerrit.server.mail.send.MergedSender;
import com.google.gerrit.server.notedb.ChangeUpdate;
import com.google.gerrit.server.patch.PatchSetInfoFactory;
@@ -52,7 +52,7 @@
MergedByPushOp create(
RequestScopePropagator requestScopePropagator,
PatchSet.Id psId,
- @Assisted RequestId submissionId,
+ @Assisted SubmissionId submissionId,
@Assisted("refName") String refName,
@Assisted("mergeResultRevId") String mergeResultRevId);
}
@@ -66,7 +66,7 @@
private final ChangeMerged changeMerged;
private final PatchSet.Id psId;
- private final RequestId submissionId;
+ private final SubmissionId submissionId;
private final String refName;
private final String mergeResultRevId;
@@ -86,7 +86,7 @@
ChangeMerged changeMerged,
@Assisted RequestScopePropagator requestScopePropagator,
@Assisted PatchSet.Id psId,
- @Assisted RequestId submissionId,
+ @Assisted SubmissionId submissionId,
@Assisted("refName") String refName,
@Assisted("mergeResultRevId") String mergeResultRevId) {
this.patchSetInfoFactory = patchSetInfoFactory;
@@ -137,7 +137,7 @@
}
change.setCurrentPatchSet(info);
change.setStatus(Change.Status.MERGED);
- change.setSubmissionId(submissionId.toStringForStorage());
+ change.setSubmissionId(submissionId.toString());
// we cannot reconstruct the submit records for when this change was
// submitted, this is why we must fix the status and other details.
update.fixStatusToMerged(submissionId);
diff --git a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
index cec9e4e..5155925 100644
--- a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
+++ b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
@@ -74,6 +74,7 @@
import com.google.gerrit.entities.PatchSetInfo;
import com.google.gerrit.entities.Project;
import com.google.gerrit.entities.RefNames;
+import com.google.gerrit.entities.SubmissionId;
import com.google.gerrit.exceptions.StorageException;
import com.google.gerrit.extensions.api.changes.HashtagsInput;
import com.google.gerrit.extensions.api.changes.NotifyHandling;
@@ -3023,7 +3024,8 @@
info,
groups,
magicBranch,
- receivePack.getPushCertificate())
+ receivePack.getPushCertificate(),
+ notes.getChange())
.setRequestScopePropagator(requestScopePropagator);
bu.addOp(notes.getChangeId(), replaceOp);
if (progress != null) {
@@ -3281,7 +3283,7 @@
int existingPatchSets = 0;
int newPatchSets = 0;
- RequestId submissionId = null;
+ SubmissionId submissionId = null;
COMMIT:
for (RevCommit c; (c = rw.next()) != null; ) {
rw.parseBody(c);
@@ -3290,10 +3292,10 @@
receivePackRefCache.tipsFromObjectId(c.copy(), RefNames.REFS_CHANGES)) {
PatchSet.Id psId = PatchSet.Id.fromRef(ref.getName());
Optional<ChangeNotes> notes = getChangeNotes(psId.changeId());
- if (submissionId == null) {
- submissionId = new RequestId(psId.changeId().toString());
- }
if (notes.isPresent() && notes.get().getChange().getDest().equals(branch)) {
+ if (submissionId == null) {
+ submissionId = new SubmissionId(notes.get().getChange());
+ }
existingPatchSets++;
bu.addOp(notes.get().getChangeId(), setPrivateOpFactory.create(false, null));
bu.addOp(
@@ -3333,7 +3335,7 @@
continue;
}
if (submissionId == null) {
- submissionId = new RequestId(id.toString());
+ submissionId = new SubmissionId(req.notes.getChange());
}
req.addOps(bu, null);
bu.addOp(id, setPrivateOpFactory.create(false, null));
diff --git a/java/com/google/gerrit/server/git/receive/ReplaceOp.java b/java/com/google/gerrit/server/git/receive/ReplaceOp.java
index 6c0d5d3..24154d60 100644
--- a/java/com/google/gerrit/server/git/receive/ReplaceOp.java
+++ b/java/com/google/gerrit/server/git/receive/ReplaceOp.java
@@ -36,6 +36,7 @@
import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.entities.PatchSetApproval;
import com.google.gerrit.entities.PatchSetInfo;
+import com.google.gerrit.entities.SubmissionId;
import com.google.gerrit.extensions.api.changes.AddReviewerInput;
import com.google.gerrit.extensions.api.changes.NotifyHandling;
import com.google.gerrit.extensions.client.ChangeKind;
@@ -61,7 +62,6 @@
import com.google.gerrit.server.extensions.events.RevisionCreated;
import com.google.gerrit.server.git.MergedByPushOp;
import com.google.gerrit.server.git.receive.ReceiveCommits.MagicBranchInput;
-import com.google.gerrit.server.logging.RequestId;
import com.google.gerrit.server.mail.MailUtil.MailRecipients;
import com.google.gerrit.server.mail.send.ReplacePatchSetSender;
import com.google.gerrit.server.notedb.ChangeNotes;
@@ -110,7 +110,8 @@
PatchSetInfo info,
List<String> groups,
@Nullable MagicBranchInput magicBranch,
- @Nullable PushCertificate pushCertificate);
+ @Nullable PushCertificate pushCertificate,
+ Change change);
}
private static final String CHANGE_IS_CLOSED = "change is closed";
@@ -143,6 +144,7 @@
private final PatchSetInfo info;
private final MagicBranchInput magicBranch;
private final PushCertificate pushCertificate;
+ private final Change change;
private List<String> groups;
private final Map<String, Short> approvals = new HashMap<>();
@@ -177,6 +179,7 @@
ProjectCache projectCache,
@SendEmailExecutor ExecutorService sendEmailExecutor,
ReviewerAdder reviewerAdder,
+ Change change,
@Assisted ProjectState projectState,
@Assisted BranchNameKey dest,
@Assisted boolean checkMergedInto,
@@ -218,6 +221,7 @@
this.groups = groups;
this.magicBranch = magicBranch;
this.pushCertificate = pushCertificate;
+ this.change = change;
}
@Override
@@ -239,7 +243,7 @@
mergedByPushOpFactory.create(
requestScopePropagator,
patchSetId,
- new RequestId(patchSetId.changeId().toString()),
+ new SubmissionId(change),
mergedInto,
mergeResultRevId);
}
diff --git a/java/com/google/gerrit/server/notedb/ChangeUpdate.java b/java/com/google/gerrit/server/notedb/ChangeUpdate.java
index 02a4dcc..6a900c0 100644
--- a/java/com/google/gerrit/server/notedb/ChangeUpdate.java
+++ b/java/com/google/gerrit/server/notedb/ChangeUpdate.java
@@ -55,11 +55,11 @@
import com.google.gerrit.entities.Comment;
import com.google.gerrit.entities.Project;
import com.google.gerrit.entities.RobotComment;
+import com.google.gerrit.entities.SubmissionId;
import com.google.gerrit.exceptions.StorageException;
import com.google.gerrit.mail.Address;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.GerritPersonIdent;
-import com.google.gerrit.server.logging.RequestId;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.util.LabelVote;
import com.google.inject.assistedinject.Assisted;
@@ -220,10 +220,10 @@
this.status = status;
}
- public void fixStatusToMerged(RequestId submissionId) {
+ public void fixStatusToMerged(SubmissionId submissionId) {
checkArgument(submissionId != null, "submission id must be set for merged changes");
this.status = Change.Status.MERGED;
- this.submissionId = submissionId.toStringForStorage();
+ this.submissionId = submissionId.toString();
}
public void putApproval(String label, short value) {
@@ -242,9 +242,9 @@
approvals.put(label, reviewer, Optional.empty());
}
- public void merge(RequestId submissionId, Iterable<SubmitRecord> submitRecords) {
+ public void merge(SubmissionId submissionId, Iterable<SubmitRecord> submitRecords) {
this.status = Change.Status.MERGED;
- this.submissionId = submissionId.toStringForStorage();
+ this.submissionId = submissionId.toString();
this.submitRecords = ImmutableList.copyOf(submitRecords);
checkArgument(!this.submitRecords.isEmpty(), "no submit records specified at submit time");
}
diff --git a/java/com/google/gerrit/server/patch/PatchScriptBuilder.java b/java/com/google/gerrit/server/patch/PatchScriptBuilder.java
index 1435c5e..0a17303 100644
--- a/java/com/google/gerrit/server/patch/PatchScriptBuilder.java
+++ b/java/com/google/gerrit/server/patch/PatchScriptBuilder.java
@@ -17,6 +17,7 @@
import static java.nio.charset.StandardCharsets.UTF_8;
import static java.util.Comparator.comparing;
+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;
@@ -24,11 +25,11 @@
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.Comment;
import com.google.gerrit.entities.Patch;
-import com.google.gerrit.entities.Project;
+import com.google.gerrit.entities.Patch.ChangeType;
import com.google.gerrit.extensions.client.DiffPreferencesInfo;
import com.google.gerrit.extensions.client.DiffPreferencesInfo.Whitespace;
import com.google.gerrit.prettify.common.EditList;
-import com.google.gerrit.prettify.common.SparseFileContent;
+import com.google.gerrit.prettify.common.SparseFileContentBuilder;
import com.google.gerrit.server.mime.FileTypeRegistry;
import com.google.inject.Inject;
import eu.medsea.mimeutil.MimeType;
@@ -39,10 +40,8 @@
import java.util.List;
import java.util.Objects;
import java.util.Optional;
+import java.util.Set;
import org.eclipse.jgit.diff.Edit;
-import org.eclipse.jgit.errors.CorruptObjectException;
-import org.eclipse.jgit.errors.IncorrectObjectTypeException;
-import org.eclipse.jgit.errors.MissingObjectException;
import org.eclipse.jgit.lib.Constants;
import org.eclipse.jgit.lib.FileMode;
import org.eclipse.jgit.lib.ObjectId;
@@ -59,28 +58,17 @@
private static final Comparator<Edit> EDIT_SORT = comparing(Edit::getBeginA);
- private Repository db;
- private Project.NameKey projectKey;
- private ObjectReader reader;
private Change change;
private DiffPreferencesInfo diffPrefs;
- private ComparisonType comparisonType;
- private ObjectId aId;
- private ObjectId bId;
private List<Edit> edits;
private final FileTypeRegistry registry;
- private final PatchListCache patchListCache;
private int context;
+ private IntraLineDiffCalculator intralineDiffCalculator;
+ private SidesResolver sidesResolver;
@Inject
- PatchScriptBuilder(FileTypeRegistry ftr, PatchListCache plc) {
+ PatchScriptBuilder(FileTypeRegistry ftr) {
registry = ftr;
- patchListCache = plc;
- }
-
- void setRepository(Repository r, Project.NameKey projectKey) {
- this.db = r;
- this.projectKey = projectKey;
}
void setChange(Change c) {
@@ -98,62 +86,39 @@
}
}
- void setTrees(ComparisonType ct, ObjectId a, ObjectId b) {
- comparisonType = ct;
- aId = a;
- bId = b;
+ void setIntraLineDiffCalculator(IntraLineDiffCalculator calculator) {
+ intralineDiffCalculator = calculator;
}
- PatchScript toPatchScript(PatchListEntry content, CommentDetail comments, List<Patch> history)
- throws IOException {
- reader = db.newObjectReader();
- try {
- return build(content, comments, history);
- } finally {
- reader.close();
- }
+ void setSidesResolver(SidesResolver resolver) {
+ sidesResolver = resolver;
}
- private PatchScript build(PatchListEntry content, CommentDetail comments, List<Patch> history)
+ PatchScript toPatchScript(PatchFileChange content, CommentDetail comments, List<Patch> history)
throws IOException {
- boolean intralineFailure = false;
- boolean intralineTimeout = false;
+ return build(content, comments, history);
+ }
- SideResolver resolver = new SideResolver();
- Side a = resolver.resolve(oldName(content), null, aId);
- Side b = resolver.resolve(newName(content), a, bId);
+ private PatchScript build(PatchFileChange content, CommentDetail comments, List<Patch> history)
+ throws IOException {
- edits = new ArrayList<>(content.getEdits());
+ ResolvedSides sides = sidesResolver.resolveSides(registry, oldName(content), newName(content));
+ PatchSide a = sides.a;
+ PatchSide b = sides.b;
+
+ ImmutableList<Edit> contentEdits = content.getEdits();
ImmutableSet<Edit> editsDueToRebase = content.getEditsDueToRebase();
- if (isModify(content) && diffPrefs.intralineDifference && isIntralineModeAllowed(b)) {
- IntraLineDiff d =
- patchListCache.getIntraLineDiff(
- IntraLineDiffKey.create(a.id, b.id, diffPrefs.ignoreWhitespace),
- IntraLineDiffArgs.create(
- a.src, b.src, edits, editsDueToRebase, projectKey, bId, b.path));
- if (d != null) {
- switch (d.getStatus()) {
- case EDIT_LIST:
- edits = new ArrayList<>(d.getEdits());
- break;
+ IntraLineDiffCalculatorResult intralineResult = IntraLineDiffCalculatorResult.NO_RESULT;
- case DISABLED:
- break;
-
- case ERROR:
- intralineFailure = true;
- break;
-
- case TIMEOUT:
- intralineTimeout = true;
- break;
- }
- } else {
- intralineFailure = true;
- }
+ if (isModify(content) && intralineDiffCalculator != null && isIntralineModeAllowed(b)) {
+ intralineResult =
+ intralineDiffCalculator.calculateIntraLineDiff(
+ contentEdits, editsDueToRebase, a.id, b.id, a.src, b.src, b.treeId, b.path);
}
+ edits = new ArrayList<>(intralineResult.edits.orElse(contentEdits));
+
correctForDifferencesInNewlineAtEnd(a, b);
if (comments != null) {
@@ -198,8 +163,8 @@
b.fileMode,
content.getHeaderLines(),
diffPrefs,
- a.dst,
- b.dst,
+ a.dst.build(),
+ b.dst.build(),
edits,
editsDueToRebase,
a.displayMethod,
@@ -209,14 +174,14 @@
comments,
history,
hugeFile,
- intralineFailure,
- intralineTimeout,
+ intralineResult.failure,
+ intralineResult.timeout,
content.getPatchType() == Patch.PatchType.BINARY,
- aId == null ? null : aId.getName(),
- bId == null ? null : bId.getName());
+ a.treeId == null ? null : a.treeId.getName(),
+ b.treeId == null ? null : b.treeId.getName());
}
- private static boolean isModify(PatchListEntry content) {
+ private static boolean isModify(PatchFileChange content) {
switch (content.getChangeType()) {
case MODIFIED:
case COPIED:
@@ -231,7 +196,7 @@
}
}
- private static String oldName(PatchListEntry entry) {
+ private static String oldName(PatchFileChange entry) {
switch (entry.getChangeType()) {
case ADDED:
return null;
@@ -246,7 +211,7 @@
}
}
- private static String newName(PatchListEntry entry) {
+ private static String newName(PatchFileChange entry) {
switch (entry.getChangeType()) {
case DELETED:
return null;
@@ -260,7 +225,7 @@
}
}
- private static boolean isIntralineModeAllowed(Side side) {
+ private static boolean isIntralineModeAllowed(PatchSide side) {
// The intraline diff cache keys are the same for these cases. It's better to not show
// intraline results than showing completely wrong diffs or to run into a server error.
return !Patch.isMagic(side.path) && !isSubmoduleCommit(side.mode);
@@ -270,7 +235,7 @@
return mode.getObjectType() == Constants.OBJ_COMMIT;
}
- private void correctForDifferencesInNewlineAtEnd(Side a, Side b) {
+ private void correctForDifferencesInNewlineAtEnd(PatchSide a, PatchSide b) {
// a.src.size() is the size ignoring a newline at the end whereas a.size() considers it.
int aSize = a.src.size();
int bSize = b.src.size();
@@ -312,11 +277,11 @@
return list.isEmpty() ? Optional.empty() : Optional.ofNullable(list.get(list.size() - 1));
}
- private boolean isNewlineAtEndDeleted(Side a, Side b) {
+ private boolean isNewlineAtEndDeleted(PatchSide a, PatchSide b) {
return !a.src.isMissingNewlineAtEnd() && b.src.isMissingNewlineAtEnd();
}
- private boolean isNewlineAtEndAdded(Side a, Side b) {
+ private boolean isNewlineAtEndAdded(PatchSide a, PatchSide b) {
return a.src.isMissingNewlineAtEnd() && !b.src.isMissingNewlineAtEnd();
}
@@ -434,7 +399,7 @@
return last.getEndA() + (b - last.getEndB());
}
- private void packContent(Side a, Side b, boolean ignoredWhitespace) {
+ private void packContent(PatchSide a, PatchSide b, boolean ignoredWhitespace) {
EditList list = new EditList(edits, context, a.size(), b.size());
for (EditList.Hunk hunk : list.getHunks()) {
while (hunk.next()) {
@@ -468,8 +433,8 @@
}
}
- private static class Side {
-
+ private static class PatchSide {
+ final ObjectId treeId;
final String path;
final ObjectId id;
final FileMode mode;
@@ -478,9 +443,10 @@
final MimeType mimeType;
final DisplayMethod displayMethod;
final PatchScript.FileMode fileMode;
- final SparseFileContent dst;
+ final SparseFileContentBuilder dst;
- public Side(
+ private PatchSide(
+ ObjectId treeId,
String path,
ObjectId id,
FileMode mode,
@@ -489,6 +455,7 @@
MimeType mimeType,
DisplayMethod displayMethod,
PatchScript.FileMode fileMode) {
+ this.treeId = treeId;
this.path = path;
this.id = id;
this.mode = mode;
@@ -497,8 +464,7 @@
this.mimeType = mimeType;
this.displayMethod = displayMethod;
this.fileMode = fileMode;
- dst = new SparseFileContent();
- dst.setSize(size());
+ dst = new SparseFileContentBuilder(size());
}
int size() {
@@ -521,15 +487,64 @@
}
}
- private class SideResolver {
+ interface SidesResolver {
- Side resolve(final String path, final Side other, final ObjectId within) throws IOException {
+ ResolvedSides resolveSides(FileTypeRegistry typeRegistry, String oldName, String newName)
+ throws IOException;
+ }
+
+ private static class ResolvedSides {
+ // Not an @AutoValue because PatchSide can't be AutoValue
+ public final PatchSide a;
+ public final PatchSide b;
+
+ ResolvedSides(PatchSide a, PatchSide b) {
+ this.a = a;
+ this.b = b;
+ }
+ }
+
+ static class SidesResolverImpl implements SidesResolver {
+
+ private final Repository db;
+ private ComparisonType comparisonType;
+ private ObjectId aId;
+ private ObjectId bId;
+
+ SidesResolverImpl(Repository db) {
+ this.db = db;
+ }
+
+ void setTrees(ComparisonType comparisonType, ObjectId a, ObjectId b) {
+ this.comparisonType = comparisonType;
+ this.aId = a;
+ this.bId = b;
+ }
+
+ @Override
+ public ResolvedSides resolveSides(FileTypeRegistry typeRegistry, String oldName, String newName)
+ throws IOException {
+ try (ObjectReader reader = db.newObjectReader()) {
+ PatchSide a = resolve(typeRegistry, reader, oldName, null, aId);
+ PatchSide b = resolve(typeRegistry, reader, newName, a, bId);
+ return new ResolvedSides(a, b);
+ }
+ }
+
+ PatchSide resolve(
+ final FileTypeRegistry registry,
+ final ObjectReader reader,
+ final String path,
+ final PatchSide other,
+ final ObjectId within)
+ throws IOException {
try {
boolean isCommitMsg = Patch.COMMIT_MSG.equals(path);
boolean isMergeList = Patch.MERGE_LIST.equals(path);
if (isCommitMsg || isMergeList) {
if (comparisonType.isAgainstParentOrAutoMerge() && Objects.equals(aId, within)) {
return createSide(
+ within,
path,
ObjectId.zeroId(),
FileMode.MISSING,
@@ -554,6 +569,7 @@
displayMethod = DisplayMethod.DIFF;
}
return createSide(
+ within,
path,
within,
mode,
@@ -563,7 +579,7 @@
displayMethod,
false);
}
- final TreeWalk tw = find(path, within);
+ final TreeWalk tw = find(reader, path, within);
ObjectId id = tw != null ? tw.getObjectId(0) : ObjectId.zeroId();
FileMode mode = tw != null ? tw.getFileMode(0) : FileMode.MISSING;
boolean reuse =
@@ -598,13 +614,15 @@
displayMethod = DisplayMethod.IMG;
}
}
- return createSide(path, id, mode, srcContent, src, mimeType, displayMethod, reuse);
+ return createSide(within, path, id, mode, srcContent, src, mimeType, displayMethod, reuse);
+
} catch (IOException err) {
throw new IOException("Cannot read " + within.name() + ":" + path, err);
}
}
- private Side createSide(
+ private PatchSide createSide(
+ ObjectId treeId,
String path,
ObjectId id,
FileMode mode,
@@ -629,12 +647,11 @@
} else if (mode == FileMode.GITLINK) {
fileMode = PatchScript.FileMode.GITLINK;
}
- return new Side(path, id, mode, srcContent, src, mimeType, displayMethod, fileMode);
+ return new PatchSide(
+ treeId, path, id, mode, srcContent, src, mimeType, displayMethod, fileMode);
}
- private TreeWalk find(String path, ObjectId within)
- throws MissingObjectException, IncorrectObjectTypeException, CorruptObjectException,
- IOException {
+ private TreeWalk find(ObjectReader reader, String path, ObjectId within) throws IOException {
if (path == null || within == null) {
return null;
}
@@ -649,4 +666,59 @@
return (a.getBits() & FileMode.TYPE_FILE) == FileMode.TYPE_FILE
&& (b.getBits() & FileMode.TYPE_FILE) == FileMode.TYPE_FILE;
}
+
+ static class IntraLineDiffCalculatorResult {
+ // Not an @AutoValue because Edit is mutable
+ final boolean failure;
+ final boolean timeout;
+ private final Optional<ImmutableList<Edit>> edits;
+
+ private IntraLineDiffCalculatorResult(
+ Optional<ImmutableList<Edit>> edits, boolean failure, boolean timeout) {
+ this.failure = failure;
+ this.timeout = timeout;
+ this.edits = edits;
+ }
+
+ static final IntraLineDiffCalculatorResult NO_RESULT =
+ new IntraLineDiffCalculatorResult(Optional.empty(), false, false);
+ static final IntraLineDiffCalculatorResult FAILURE =
+ new IntraLineDiffCalculatorResult(Optional.empty(), true, false);
+ static final IntraLineDiffCalculatorResult TIMEOUT =
+ new IntraLineDiffCalculatorResult(Optional.empty(), false, true);
+
+ static IntraLineDiffCalculatorResult success(ImmutableList<Edit> edits) {
+ return new IntraLineDiffCalculatorResult(Optional.of(edits), false, false);
+ }
+ }
+
+ interface IntraLineDiffCalculator {
+
+ IntraLineDiffCalculatorResult calculateIntraLineDiff(
+ ImmutableList<Edit> edits,
+ Set<Edit> editsDueToRebase,
+ ObjectId aId,
+ ObjectId bId,
+ Text aSrc,
+ Text bSrc,
+ ObjectId bTreeId,
+ String bPath);
+ }
+
+ interface PatchFileChange {
+
+ ImmutableList<Edit> getEdits();
+
+ ImmutableSet<Edit> getEditsDueToRebase();
+
+ List<String> getHeaderLines();
+
+ String getNewName();
+
+ String getOldName();
+
+ ChangeType getChangeType();
+
+ Patch.PatchType getPatchType();
+ }
}
diff --git a/java/com/google/gerrit/server/patch/PatchScriptFactory.java b/java/com/google/gerrit/server/patch/PatchScriptFactory.java
index ffeda3d..2c8de1d 100644
--- a/java/com/google/gerrit/server/patch/PatchScriptFactory.java
+++ b/java/com/google/gerrit/server/patch/PatchScriptFactory.java
@@ -17,6 +17,8 @@
import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.base.Preconditions.checkState;
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.common.data.CommentDetail;
@@ -27,6 +29,7 @@
import com.google.gerrit.entities.Patch;
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;
@@ -38,6 +41,10 @@
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.git.LargeObjectException;
import com.google.gerrit.server.notedb.ChangeNotes;
+import com.google.gerrit.server.patch.PatchScriptBuilder.IntraLineDiffCalculator;
+import com.google.gerrit.server.patch.PatchScriptBuilder.IntraLineDiffCalculatorResult;
+import com.google.gerrit.server.patch.PatchScriptBuilder.PatchFileChange;
+import com.google.gerrit.server.patch.PatchScriptBuilder.SidesResolverImpl;
import com.google.gerrit.server.permissions.ChangePermission;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
@@ -53,15 +60,19 @@
import java.util.List;
import java.util.Map;
import java.util.Optional;
+import java.util.Set;
import java.util.concurrent.Callable;
+import org.eclipse.jgit.diff.Edit;
import org.eclipse.jgit.errors.RepositoryNotFoundException;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.Repository;
public class PatchScriptFactory implements Callable<PatchScript> {
+
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
public interface Factory {
+
PatchScriptFactory create(
ChangeNotes notes,
String fileName,
@@ -225,7 +236,7 @@
loadCommentsAndHistory(content.getChangeType(), content.getOldName(), content.getNewName());
- return b.toPatchScript(content, comments, history);
+ return b.toPatchScript(new PatchListFileChange(content), comments, history);
} catch (PatchListNotAvailableException e) {
throw new NoSuchChangeException(changeId, e);
} catch (IOException e) {
@@ -256,10 +267,15 @@
private PatchScriptBuilder newBuilder(PatchList list, Repository git) {
final PatchScriptBuilder b = builderFactory.get();
- b.setRepository(git, notes.getProjectName());
b.setChange(notes.getChange());
b.setDiffPrefs(diffPrefs);
- b.setTrees(list.getComparisonType(), list.getOldId(), list.getNewId());
+ if (diffPrefs.intralineDifference) {
+ b.setIntraLineDiffCalculator(
+ new IntraLineDiffCalculatorImpl(patchListCache, notes.getProjectName(), diffPrefs));
+ }
+ SidesResolverImpl sidesResolver = new SidesResolverImpl(git);
+ sidesResolver.setTrees(list.getComparisonType(), list.getOldId(), list.getNewId());
+ b.setSidesResolver(sidesResolver);
return b;
}
@@ -402,4 +418,98 @@
}
}
}
+
+ private static class IntraLineDiffCalculatorImpl implements IntraLineDiffCalculator {
+
+ private final PatchListCache patchListCache;
+ private final Project.NameKey projectKey;
+ private final DiffPreferencesInfo diffPrefs;
+
+ public IntraLineDiffCalculatorImpl(
+ PatchListCache patchListCache, Project.NameKey projectKey, DiffPreferencesInfo diffPrefs) {
+ this.patchListCache = patchListCache;
+ this.projectKey = projectKey;
+ this.diffPrefs = diffPrefs;
+ }
+
+ @Override
+ public IntraLineDiffCalculatorResult calculateIntraLineDiff(
+ ImmutableList<Edit> edits,
+ Set<Edit> editsDueToRebase,
+ ObjectId aId,
+ ObjectId bId,
+ Text aSrc,
+ Text bSrc,
+ ObjectId bTreeId,
+ String bPath) {
+ IntraLineDiff d =
+ patchListCache.getIntraLineDiff(
+ IntraLineDiffKey.create(aId, bId, diffPrefs.ignoreWhitespace),
+ IntraLineDiffArgs.create(
+ aSrc, bSrc, edits, editsDueToRebase, projectKey, bTreeId, bPath));
+ if (d == null) {
+ return IntraLineDiffCalculatorResult.FAILURE;
+ }
+ switch (d.getStatus()) {
+ case EDIT_LIST:
+ return IntraLineDiffCalculatorResult.success(d.getEdits());
+
+ case DISABLED:
+ return IntraLineDiffCalculatorResult.NO_RESULT;
+
+ case ERROR:
+ return IntraLineDiffCalculatorResult.FAILURE;
+
+ case TIMEOUT:
+ return IntraLineDiffCalculatorResult.TIMEOUT;
+
+ default:
+ return IntraLineDiffCalculatorResult.NO_RESULT;
+ }
+ }
+ }
+
+ private static class PatchListFileChange implements PatchFileChange {
+
+ private final PatchListEntry patchListEntry;
+
+ PatchListFileChange(PatchListEntry patchListEntry) {
+ this.patchListEntry = patchListEntry;
+ }
+
+ @Override
+ public ImmutableList<Edit> getEdits() {
+ return patchListEntry.getEdits();
+ }
+
+ @Override
+ public ImmutableSet<Edit> getEditsDueToRebase() {
+ return patchListEntry.getEditsDueToRebase();
+ }
+
+ @Override
+ public String getNewName() {
+ return patchListEntry.getNewName();
+ }
+
+ @Override
+ public String getOldName() {
+ return patchListEntry.getOldName();
+ }
+
+ @Override
+ public ChangeType getChangeType() {
+ return patchListEntry.getChangeType();
+ }
+
+ @Override
+ public List<String> getHeaderLines() {
+ return patchListEntry.getHeaderLines();
+ }
+
+ @Override
+ public Patch.PatchType getPatchType() {
+ return patchListEntry.getPatchType();
+ }
+ }
}
diff --git a/java/com/google/gerrit/server/restapi/change/GetDiff.java b/java/com/google/gerrit/server/restapi/change/GetDiff.java
index a6536ce..82bdc34 100644
--- a/java/com/google/gerrit/server/restapi/change/GetDiff.java
+++ b/java/com/google/gerrit/server/restapi/change/GetDiff.java
@@ -14,27 +14,18 @@
package com.google.gerrit.server.restapi.change;
-import static com.google.common.base.Preconditions.checkState;
import static com.google.gerrit.util.cli.Localizable.localizable;
import com.google.common.base.MoreObjects;
import com.google.common.collect.ImmutableList;
-import com.google.common.collect.ImmutableMap;
-import com.google.common.collect.Lists;
-import com.google.common.collect.Maps;
import com.google.common.flogger.FluentLogger;
+import com.google.gerrit.common.Nullable;
import com.google.gerrit.common.data.PatchScript;
-import com.google.gerrit.common.data.PatchScript.DisplayMethod;
-import com.google.gerrit.entities.Patch;
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.common.ChangeType;
import com.google.gerrit.extensions.common.DiffInfo;
-import com.google.gerrit.extensions.common.DiffInfo.ContentEntry;
-import com.google.gerrit.extensions.common.DiffInfo.FileMeta;
-import com.google.gerrit.extensions.common.DiffInfo.IntraLineStatus;
import com.google.gerrit.extensions.common.DiffWebLinkInfo;
import com.google.gerrit.extensions.common.WebLinkInfo;
import com.google.gerrit.extensions.restapi.AuthException;
@@ -44,12 +35,12 @@
import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.extensions.restapi.Response;
import com.google.gerrit.extensions.restapi.RestReadView;
-import com.google.gerrit.jgit.diff.ReplaceEdit;
-import com.google.gerrit.prettify.common.SparseFileContent;
import com.google.gerrit.server.WebLinks;
-import com.google.gerrit.server.change.FileContentUtil;
import com.google.gerrit.server.change.FileResource;
import com.google.gerrit.server.change.RevisionResource;
+import com.google.gerrit.server.diff.DiffInfoCreator;
+import com.google.gerrit.server.diff.DiffSide;
+import com.google.gerrit.server.diff.DiffWebLinksProvider;
import com.google.gerrit.server.git.LargeObjectException;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.patch.PatchScriptFactory;
@@ -60,10 +51,7 @@
import com.google.gerrit.server.project.ProjectState;
import com.google.inject.Inject;
import java.io.IOException;
-import java.util.List;
-import java.util.Set;
import java.util.concurrent.TimeUnit;
-import org.eclipse.jgit.diff.Edit;
import org.kohsuke.args4j.CmdLineException;
import org.kohsuke.args4j.CmdLineParser;
import org.kohsuke.args4j.NamedOptionDef;
@@ -76,17 +64,6 @@
public class GetDiff implements RestReadView<FileResource> {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
- private static final ImmutableMap<Patch.ChangeType, ChangeType> CHANGE_TYPE =
- Maps.immutableEnumMap(
- new ImmutableMap.Builder<Patch.ChangeType, ChangeType>()
- .put(Patch.ChangeType.ADDED, ChangeType.ADDED)
- .put(Patch.ChangeType.MODIFIED, ChangeType.MODIFIED)
- .put(Patch.ChangeType.DELETED, ChangeType.DELETED)
- .put(Patch.ChangeType.RENAMED, ChangeType.RENAMED)
- .put(Patch.ChangeType.COPIED, ChangeType.COPIED)
- .put(Patch.ChangeType.REWRITE, ChangeType.REWRITE)
- .build());
-
private final ProjectCache projectCache;
private final PatchScriptFactory.Factory patchScriptFactoryFactory;
private final Revisions revisions;
@@ -164,111 +141,18 @@
psf.setLoadHistory(false);
psf.setLoadComments(context != DiffPreferencesInfo.WHOLE_FILE_CONTEXT);
PatchScript ps = psf.call();
- ContentCollector contentCollector = new ContentCollector(ps);
- Set<Edit> editsDueToRebase = ps.getEditsDueToRebase();
- for (Edit edit : ps.getEdits()) {
- logger.atFine().log("next edit = %s", edit);
-
- if (edit.getType() == Edit.Type.EMPTY) {
- logger.atFine().log("skip empty edit");
- continue;
- }
- contentCollector.addCommon(edit.getBeginA());
-
- checkState(
- contentCollector.nextA == edit.getBeginA(),
- "nextA = %s; want %s",
- contentCollector.nextA,
- edit.getBeginA());
- checkState(
- contentCollector.nextB == edit.getBeginB(),
- "nextB = %s; want %s",
- contentCollector.nextB,
- edit.getBeginB());
- switch (edit.getType()) {
- case DELETE:
- case INSERT:
- case REPLACE:
- List<Edit> internalEdit =
- edit instanceof ReplaceEdit ? ((ReplaceEdit) edit).getInternalEdits() : null;
- boolean dueToRebase = editsDueToRebase.contains(edit);
- contentCollector.addDiff(edit.getEndA(), edit.getEndB(), internalEdit, dueToRebase);
- break;
- case EMPTY:
- default:
- throw new IllegalStateException();
- }
- }
- contentCollector.addCommon(ps.getA().size());
-
- ProjectState state = projectCache.get(resource.getRevision().getChange().getProject());
-
- DiffInfo result = new DiffInfo();
- String revA = basePatchSet != null ? basePatchSet.refName() : ps.getFileInfoA().commitId;
- String revB =
- resource.getRevision().getEdit().isPresent()
- ? resource.getRevision().getEdit().get().getRefName()
- : resource.getRevision().getPatchSet().refName();
- logger.atFine().log("revA = %s, revB = %s", revA, revB);
-
- ImmutableList<DiffWebLinkInfo> links =
- webLinks.getDiffLinks(
- state.getName(),
- resource.getPatchKey().patchSetId().changeId().get(),
- basePatchSet != null ? basePatchSet.id().get() : null,
- revA,
+ Project.NameKey projectName = resource.getRevision().getChange().getProject();
+ ProjectState state = projectCache.get(projectName);
+ DiffSide sideA =
+ DiffSide.create(
+ ps.getFileInfoA(),
MoreObjects.firstNonNull(ps.getOldName(), ps.getNewName()),
- resource.getPatchKey().patchSetId().get(),
- revB,
- ps.getNewName());
- result.webLinks = links.isEmpty() ? null : links;
-
- if (ps.isBinary()) {
- result.binary = true;
- }
- if (ps.getDisplayMethodA() != DisplayMethod.NONE) {
- result.metaA = new FileMeta();
- result.metaA.name = MoreObjects.firstNonNull(ps.getOldName(), ps.getNewName());
- result.metaA.contentType =
- FileContentUtil.resolveContentType(
- state, result.metaA.name, ps.getFileModeA(), ps.getMimeTypeA());
- result.metaA.lines = ps.getA().size();
- result.metaA.webLinks = getFileWebLinks(state.getProject(), revA, result.metaA.name);
- result.metaA.commitId = ps.getFileInfoA().commitId;
- }
-
- if (ps.getDisplayMethodB() != DisplayMethod.NONE) {
- result.metaB = new FileMeta();
- result.metaB.name = ps.getNewName();
- result.metaB.contentType =
- FileContentUtil.resolveContentType(
- state, result.metaB.name, ps.getFileModeB(), ps.getMimeTypeB());
- result.metaB.lines = ps.getB().size();
- result.metaB.webLinks = getFileWebLinks(state.getProject(), revB, result.metaB.name);
- result.metaB.commitId = ps.getFileInfoB().commitId;
- }
-
- if (intraline) {
- if (ps.hasIntralineTimeout()) {
- result.intralineStatus = IntraLineStatus.TIMEOUT;
- } else if (ps.hasIntralineFailure()) {
- result.intralineStatus = IntraLineStatus.FAILURE;
- } else {
- result.intralineStatus = IntraLineStatus.OK;
- }
- logger.atFine().log("intralineStatus = %s", result.intralineStatus);
- }
-
- result.changeType = CHANGE_TYPE.get(ps.getChangeType());
- logger.atFine().log("changeType = %s", result.changeType);
- if (result.changeType == null) {
- throw new IllegalStateException("unknown change type: " + ps.getChangeType());
- }
-
- if (ps.getPatchHeader().size() > 0) {
- result.diffHeader = ps.getPatchHeader();
- }
- result.content = contentCollector.lines;
+ DiffSide.Type.SIDE_A);
+ DiffSide sideB = DiffSide.create(ps.getFileInfoB(), ps.getNewName(), DiffSide.Type.SIDE_B);
+ DiffWebLinksProvider webLinksProvider =
+ new DiffWebLinksProviderImpl(sideA, sideB, projectName, basePatchSet, webLinks, resource);
+ DiffInfoCreator diffInfoCreator = new DiffInfoCreator(state, webLinksProvider, intraline);
+ DiffInfo result = diffInfoCreator.create(ps, sideA, sideB);
Response<DiffInfo> r = Response.ok(result);
if (resource.isCacheable()) {
@@ -282,9 +166,69 @@
}
}
- private List<WebLinkInfo> getFileWebLinks(Project project, String rev, String file) {
- ImmutableList<WebLinkInfo> links = webLinks.getFileLinks(project.getName(), rev, file);
- return links.isEmpty() ? null : links;
+ private static class DiffWebLinksProviderImpl implements DiffWebLinksProvider {
+
+ private final WebLinks webLinks;
+ private final Project.NameKey projectName;
+ private final DiffSide sideA;
+ private final DiffSide sideB;
+ private final String revA;
+ private final String revB;
+ private final FileResource resource;
+ @Nullable private final PatchSet basePatchSet;
+
+ DiffWebLinksProviderImpl(
+ DiffSide sideA,
+ DiffSide sideB,
+ Project.NameKey projectName,
+ @Nullable PatchSet basePatchSet,
+ WebLinks webLinks,
+ FileResource resource) {
+ this.projectName = projectName;
+ this.webLinks = webLinks;
+ this.basePatchSet = basePatchSet;
+ this.resource = resource;
+ this.sideA = sideA;
+ this.sideB = sideB;
+
+ revA = basePatchSet != null ? basePatchSet.refName() : sideA.fileInfo().commitId;
+
+ RevisionResource revision = resource.getRevision();
+ revB =
+ revision
+ .getEdit()
+ .map(edit -> edit.getRefName())
+ .orElseGet(() -> revision.getPatchSet().refName());
+
+ logger.atFine().log("revA = %s, revB = %s", revA, revB);
+ }
+
+ @Override
+ public ImmutableList<DiffWebLinkInfo> getDiffLinks() {
+ return webLinks.getDiffLinks(
+ projectName.get(),
+ resource.getPatchKey().patchSetId().changeId().get(),
+ basePatchSet != null ? basePatchSet.id().get() : null,
+ revA,
+ sideA.fileName(),
+ resource.getPatchKey().patchSetId().get(),
+ revB,
+ sideB.fileName());
+ }
+
+ @Override
+ public ImmutableList<WebLinkInfo> getFileWebLinks(DiffSide.Type type) {
+ String rev;
+ DiffSide side;
+ if (type == DiffSide.Type.SIDE_A) {
+ rev = revA;
+ side = sideA;
+ } else {
+ rev = revB;
+ side = sideB;
+ }
+ return webLinks.getFileLinks(projectName.get(), rev, side.fileName());
+ }
}
public GetDiff setBase(String base) {
@@ -312,141 +256,6 @@
return this;
}
- private static class ContentCollector {
- final List<ContentEntry> lines;
- final SparseFileContent fileA;
- final SparseFileContent fileB;
- final boolean ignoreWS;
-
- int nextA;
- int nextB;
-
- ContentCollector(PatchScript ps) {
- lines = Lists.newArrayListWithExpectedSize(ps.getEdits().size() + 2);
- fileA = ps.getA();
- fileB = ps.getB();
- ignoreWS = ps.isIgnoreWhitespace();
- }
-
- void addCommon(int end) {
- logger.atFine().log("addCommon: end = %d", end);
-
- end = Math.min(end, fileA.size());
- logger.atFine().log("end = %d", end);
-
- if (nextA >= end) {
- logger.atFine().log("nextA >= end: nextA = %d, end = %d", nextA, end);
- return;
- }
-
- while (nextA < end) {
- logger.atFine().log("nextA < end: nextA = %d, end = %d", nextA, end);
-
- if (!fileA.contains(nextA)) {
- logger.atFine().log("fileA does not contain nextA: nextA = %d", nextA);
-
- int endRegion = Math.min(end, nextA == 0 ? fileA.first() : fileA.next(nextA - 1));
- int len = endRegion - nextA;
- entry().skip = len;
- nextA = endRegion;
- nextB += len;
-
- logger.atFine().log("setting: nextA = %d, nextB = %d", nextA, nextB);
- continue;
- }
-
- ContentEntry e = null;
- for (int i = nextA; i == nextA && i < end; i = fileA.next(i), nextA++, nextB++) {
- if (ignoreWS && fileB.contains(nextB)) {
- if (e == null || e.common == null) {
- logger.atFine().log("create new common entry: nextA = %d, nextB = %d", nextA, nextB);
- e = entry();
- e.a = Lists.newArrayListWithCapacity(end - nextA);
- e.b = Lists.newArrayListWithCapacity(end - nextA);
- e.common = true;
- }
- e.a.add(fileA.get(nextA));
- e.b.add(fileB.get(nextB));
- } else {
- if (e == null || e.common != null) {
- logger.atFine().log(
- "create new non-common entry: nextA = %d, nextB = %d", nextA, nextB);
- e = entry();
- e.ab = Lists.newArrayListWithCapacity(end - nextA);
- }
- e.ab.add(fileA.get(nextA));
- }
- }
- logger.atFine().log("nextA = %d, nextB = %d", nextA, nextB);
- }
- }
-
- void addDiff(int endA, int endB, List<Edit> internalEdit, boolean dueToRebase) {
- logger.atFine().log(
- "addDiff: endA = %d, endB = %d, numberOfInternalEdits = %d, dueToRebase = %s",
- endA, endB, internalEdit != null ? internalEdit.size() : 0, dueToRebase);
-
- int lenA = endA - nextA;
- int lenB = endB - nextB;
- logger.atFine().log("lenA = %d, lenB = %d", lenA, lenB);
- checkState(lenA > 0 || lenB > 0);
-
- logger.atFine().log("create non-common entry");
- ContentEntry e = entry();
- if (lenA > 0) {
- logger.atFine().log("lenA > 0: lenA = %d", lenA);
- e.a = Lists.newArrayListWithCapacity(lenA);
- for (; nextA < endA; nextA++) {
- e.a.add(fileA.get(nextA));
- }
- }
- if (lenB > 0) {
- logger.atFine().log("lenB > 0: lenB = %d", lenB);
- e.b = Lists.newArrayListWithCapacity(lenB);
- for (; nextB < endB; nextB++) {
- e.b.add(fileB.get(nextB));
- }
- }
- if (internalEdit != null && !internalEdit.isEmpty()) {
- logger.atFine().log("processing internal edits");
-
- e.editA = Lists.newArrayListWithCapacity(internalEdit.size() * 2);
- e.editB = Lists.newArrayListWithCapacity(internalEdit.size() * 2);
- int lastA = 0;
- int lastB = 0;
- for (Edit edit : internalEdit) {
- logger.atFine().log("internal edit = %s", edit);
-
- if (edit.getBeginA() != edit.getEndA()) {
- logger.atFine().log(
- "edit.getBeginA() != edit.getEndA(): edit.getBeginA() = %d, edit.getEndA() = %d",
- edit.getBeginA(), edit.getEndA());
- e.editA.add(
- ImmutableList.of(edit.getBeginA() - lastA, edit.getEndA() - edit.getBeginA()));
- lastA = edit.getEndA();
- logger.atFine().log("lastA = %d", lastA);
- }
- if (edit.getBeginB() != edit.getEndB()) {
- logger.atFine().log(
- "edit.getBeginB() != edit.getEndB(): edit.getBeginB() = %d, edit.getEndB() = %d",
- edit.getBeginB(), edit.getEndB());
- e.editB.add(
- ImmutableList.of(edit.getBeginB() - lastB, edit.getEndB() - edit.getBeginB()));
- lastB = edit.getEndB();
- logger.atFine().log("lastB = %d", lastB);
- }
- }
- }
- e.dueToRebase = dueToRebase ? true : null;
- }
-
- private ContentEntry entry() {
- ContentEntry e = new ContentEntry();
- lines.add(e);
- return e;
- }
- }
-
@Deprecated
enum IgnoreWhitespace {
NONE(DiffPreferencesInfo.Whitespace.IGNORE_NONE),
@@ -462,6 +271,7 @@
}
public static class ContextOptionHandler extends OptionHandler<Short> {
+
public ContextOptionHandler(CmdLineParser parser, OptionDef option, Setter<Short> setter) {
super(parser, option, setter);
}
diff --git a/java/com/google/gerrit/server/submit/MergeOp.java b/java/com/google/gerrit/server/submit/MergeOp.java
index 75ae62d..8bdd98e 100644
--- a/java/com/google/gerrit/server/submit/MergeOp.java
+++ b/java/com/google/gerrit/server/submit/MergeOp.java
@@ -41,6 +41,7 @@
import com.google.gerrit.entities.ChangeMessage;
import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.entities.Project;
+import com.google.gerrit.entities.SubmissionId;
import com.google.gerrit.exceptions.StorageException;
import com.google.gerrit.extensions.api.changes.NotifyHandling;
import com.google.gerrit.extensions.api.changes.SubmitInput;
@@ -237,7 +238,7 @@
private final Map<Change.Id, Change> updatedChanges;
private Timestamp ts;
- private RequestId submissionId;
+ private SubmissionId submissionId;
private IdentifiedUser caller;
private MergeOpRepoManager orm;
@@ -449,7 +450,7 @@
this.dryrun = dryrun;
this.caller = caller;
this.ts = TimeUtil.nowTs();
- this.submissionId = new RequestId(change.getId().toString());
+ this.submissionId = new SubmissionId(change);
try (TraceContext traceContext =
TraceContext.open().addTag(RequestId.Type.SUBMISSION_ID, submissionId)) {
diff --git a/java/com/google/gerrit/server/submit/SubmitStrategy.java b/java/com/google/gerrit/server/submit/SubmitStrategy.java
index 4c68e1b..efb2f76 100644
--- a/java/com/google/gerrit/server/submit/SubmitStrategy.java
+++ b/java/com/google/gerrit/server/submit/SubmitStrategy.java
@@ -21,6 +21,7 @@
import com.google.common.collect.Sets;
import com.google.gerrit.entities.BranchNameKey;
import com.google.gerrit.entities.Change;
+import com.google.gerrit.entities.SubmissionId;
import com.google.gerrit.extensions.api.changes.SubmitInput;
import com.google.gerrit.extensions.client.SubmitType;
import com.google.gerrit.extensions.config.FactoryModule;
@@ -42,7 +43,6 @@
import com.google.gerrit.server.git.MergeUtil;
import com.google.gerrit.server.git.TagCache;
import com.google.gerrit.server.git.validators.OnSubmitValidators;
-import com.google.gerrit.server.logging.RequestId;
import com.google.gerrit.server.patch.PatchSetInfoFactory;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.project.ProjectConfig;
@@ -94,7 +94,7 @@
RevFlag canMergeFlag,
Set<RevCommit> alreadyAccepted,
Set<CodeReviewCommit> incoming,
- RequestId submissionId,
+ SubmissionId submissionId,
SubmitInput submitInput,
SubmoduleOp submoduleOp,
boolean dryrun);
@@ -125,7 +125,7 @@
final MergeTip mergeTip;
final RevFlag canMergeFlag;
final Set<RevCommit> alreadyAccepted;
- final RequestId submissionId;
+ final SubmissionId submissionId;
final SubmitType submitType;
final SubmitInput submitInput;
final SubmoduleOp submoduleOp;
@@ -164,7 +164,7 @@
@Assisted RevFlag canMergeFlag,
@Assisted Set<RevCommit> alreadyAccepted,
@Assisted Set<CodeReviewCommit> incoming,
- @Assisted RequestId submissionId,
+ @Assisted SubmissionId submissionId,
@Assisted SubmitType submitType,
@Assisted SubmitInput submitInput,
@Assisted SubmoduleOp submoduleOp,
diff --git a/java/com/google/gerrit/server/submit/SubmitStrategyFactory.java b/java/com/google/gerrit/server/submit/SubmitStrategyFactory.java
index cba572bc..a015665 100644
--- a/java/com/google/gerrit/server/submit/SubmitStrategyFactory.java
+++ b/java/com/google/gerrit/server/submit/SubmitStrategyFactory.java
@@ -16,13 +16,13 @@
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.entities.BranchNameKey;
+import com.google.gerrit.entities.SubmissionId;
import com.google.gerrit.extensions.api.changes.SubmitInput;
import com.google.gerrit.extensions.client.SubmitType;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.git.CodeReviewCommit;
import com.google.gerrit.server.git.CodeReviewCommit.CodeReviewRevWalk;
import com.google.gerrit.server.git.MergeTip;
-import com.google.gerrit.server.logging.RequestId;
import com.google.gerrit.server.submit.MergeOp.CommitStatus;
import com.google.inject.Inject;
import com.google.inject.Singleton;
@@ -52,7 +52,7 @@
IdentifiedUser caller,
MergeTip mergeTip,
CommitStatus commitStatus,
- RequestId submissionId,
+ SubmissionId submissionId,
SubmitInput submitInput,
SubmoduleOp submoduleOp,
boolean dryrun)
diff --git a/java/com/google/gerrit/server/submit/SubmitStrategyOp.java b/java/com/google/gerrit/server/submit/SubmitStrategyOp.java
index b7bc58a..6be4a91 100644
--- a/java/com/google/gerrit/server/submit/SubmitStrategyOp.java
+++ b/java/com/google/gerrit/server/submit/SubmitStrategyOp.java
@@ -453,7 +453,7 @@
Change c = ctx.getChange();
logger.atFine().log("Setting change %s merged", c.getId());
c.setStatus(Change.Status.MERGED);
- c.setSubmissionId(args.submissionId.toStringForStorage());
+ c.setSubmissionId(args.submissionId.toString());
// TODO(dborowitz): We need to be able to change the author of the message,
// which is not the user from the update context. addMergedMessage was able
diff --git a/java/com/google/gerrit/server/update/RetryHelper.java b/java/com/google/gerrit/server/update/RetryHelper.java
index 4f9a67c..f3636c9 100644
--- a/java/com/google/gerrit/server/update/RetryHelper.java
+++ b/java/com/google/gerrit/server/update/RetryHelper.java
@@ -335,6 +335,12 @@
if (retryWithTraceOnFailure
&& opts.retryWithTrace().isPresent()
&& opts.retryWithTrace().get().test(t)) {
+ // Exception hooks may identify exceptions for which retrying with trace should be
+ // skipped.
+ if (exceptionHooks.stream().anyMatch(h -> h.skipRetryWithTrace(t))) {
+ return false;
+ }
+
String caller = opts.caller().orElse("N/A");
String cause = formatCause(t);
if (!traceContext.isTracing()) {
diff --git a/javatests/com/google/gerrit/acceptance/server/change/ConsistencyCheckerIT.java b/javatests/com/google/gerrit/acceptance/server/change/ConsistencyCheckerIT.java
index 08719d3..ee5f117 100644
--- a/javatests/com/google/gerrit/acceptance/server/change/ConsistencyCheckerIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/change/ConsistencyCheckerIT.java
@@ -29,6 +29,7 @@
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.entities.RefNames;
+import com.google.gerrit.entities.SubmissionId;
import com.google.gerrit.extensions.api.changes.FixInput;
import com.google.gerrit.extensions.client.ChangeStatus;
import com.google.gerrit.extensions.common.ChangeInfo;
@@ -39,7 +40,6 @@
import com.google.gerrit.server.change.ConsistencyChecker;
import com.google.gerrit.server.change.NotifyResolver;
import com.google.gerrit.server.change.PatchSetInserter;
-import com.google.gerrit.server.logging.RequestId;
import com.google.gerrit.server.notedb.ChangeNoteUtil;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.notedb.Sequences;
@@ -315,7 +315,7 @@
public boolean updateChange(ChangeContext ctx) {
ctx.getChange().setStatus(Change.Status.MERGED);
ctx.getUpdate(ctx.getChange().currentPatchSetId())
- .fixStatusToMerged(new RequestId(ctx.getChange().getId().toString()));
+ .fixStatusToMerged(new SubmissionId(ctx.getChange()));
return true;
}
});
@@ -865,7 +865,7 @@
public boolean updateChange(ChangeContext ctx) {
ctx.getChange().setStatus(Change.Status.MERGED);
ctx.getUpdate(ctx.getChange().currentPatchSetId())
- .fixStatusToMerged(new RequestId(ctx.getChange().getId().toString()));
+ .fixStatusToMerged(new SubmissionId(ctx.getChange()));
return true;
}
});
diff --git a/javatests/com/google/gerrit/integration/git/GitProtocolV2IT.java b/javatests/com/google/gerrit/integration/git/GitProtocolV2IT.java
index 9c4bdef4..8577c16 100644
--- a/javatests/com/google/gerrit/integration/git/GitProtocolV2IT.java
+++ b/javatests/com/google/gerrit/integration/git/GitProtocolV2IT.java
@@ -35,6 +35,7 @@
import com.google.gerrit.entities.RefNames;
import com.google.gerrit.extensions.api.GerritApi;
import com.google.gerrit.extensions.common.ChangeInput;
+import com.google.gerrit.server.config.AllProjectsName;
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.group.SystemGroupBackend;
import com.google.inject.Inject;
@@ -47,12 +48,15 @@
@UseSsh
public class GitProtocolV2IT extends StandaloneSiteTest {
+ private static final String ADMIN_PASSWORD = "secret";
private final String[] SSH_KEYGEN_CMD =
new String[] {"ssh-keygen", "-t", "rsa", "-q", "-P", "", "-f"};
private final String[] GIT_LS_REMOTE =
new String[] {"git", "-c", "protocol.version=2", "ls-remote", "-o", "trace=12345"};
private final String[] GIT_CLONE_MIRROR =
new String[] {"git", "-c", "protocol.version=2", "clone", "--mirror"};
+ private final String[] GIT_FETCH = new String[] {"git", "-c", "protocol.version=2", "fetch"};
+ private final String[] GIT_INIT = new String[] {"git", "init"};
private final String GIT_SSH_COMMAND =
"ssh -o UserKnownHostsFile=/dev/null -o StrictHostKeyChecking=no -i";
@@ -61,6 +65,7 @@
@Inject private ProjectOperations projectOperations;
@Inject private @TestSshServerAddress InetSocketAddress sshAddress;
@Inject private @GerritServerConfig Config config;
+ @Inject private AllProjectsName allProjectsName;
@BeforeClass
public static void assertGitClientVersion() throws Exception {
@@ -131,7 +136,7 @@
.commit;
// Prepare new change on secret branch
- in = new ChangeInput(project.get(), "secret", "Test secret change");
+ in = new ChangeInput(project.get(), ADMIN_PASSWORD, "Test secret change");
in.newBranch = true;
// Create new change and retrieve SHA1 for the created patch set
@@ -257,9 +262,82 @@
}
}
+ @Test
+ public void testGitWireProtocolV2FetchIndividualRef() throws Exception {
+ try (ServerContext ctx = startServer()) {
+ ctx.getInjector().injectMembers(this);
+
+ // Setup admin password
+ gApi.accounts().id(admin.username()).setHttpPassword(ADMIN_PASSWORD);
+
+ // Get authenticated Git/HTTP URL
+ String urlWithCredentials =
+ config
+ .getString("gerrit", null, "canonicalweburl")
+ .replace("http://", "http://" + admin.username() + ":" + ADMIN_PASSWORD + "@");
+
+ // Create project
+ Project.NameKey privateProject = Project.nameKey("private-project");
+ gApi.projects().create(privateProject.get());
+
+ // Set protocol.version=2 in target repository
+ execute(
+ ImmutableList.of("git", "config", "protocol.version", "2"),
+ sitePaths
+ .site_path
+ .resolve("git")
+ .resolve(privateProject.get() + Constants.DOT_GIT)
+ .toFile());
+
+ // Disallow general read permissions for anonymous users
+ projectOperations
+ .project(allProjectsName)
+ .forUpdate()
+ .add(deny(Permission.READ).ref("refs/*").group(SystemGroupBackend.ANONYMOUS_USERS))
+ .add(
+ allow(Permission.READ)
+ .ref("refs/heads/master")
+ .group(SystemGroupBackend.REGISTERED_USERS))
+ .update();
+
+ // Set up project permission to allow registered users fetching changes/*
+ projectOperations
+ .project(privateProject)
+ .forUpdate()
+ .add(
+ allow(Permission.READ)
+ .ref("refs/changes/*")
+ .group(SystemGroupBackend.REGISTERED_USERS))
+ .update();
+
+ // Create new change and retrieve refs for the created patch set
+ ChangeInput visibleChangeIn =
+ new ChangeInput(privateProject.get(), "master", "Test private change");
+ visibleChangeIn.newBranch = true;
+ int visibleChangeNumber = gApi.changes().create(visibleChangeIn).info()._number;
+ Change.Id changeId = Change.id(visibleChangeNumber);
+ String visibleChangeNumberRef = RefNames.patchSetRef(PatchSet.id(changeId, 1));
+
+ // Fetch a single ref using git wire protocol v2 over HTTP with authentication
+ execute(GIT_INIT);
+
+ String outFetchRef =
+ execute(
+ ImmutableList.<String>builder()
+ .add(GIT_FETCH)
+ .add(urlWithCredentials + "/" + privateProject.get())
+ .add(visibleChangeNumberRef)
+ .build(),
+ ImmutableMap.of("GIT_TRACE_PACKET", "1"));
+
+ assertThat(outFetchRef).contains("git< version 2");
+ assertThat(outFetchRef).contains(visibleChangeNumberRef);
+ }
+ }
+
private void setUpUserAuthentication(String username) throws Exception {
// Assign HTTP password to user
- gApi.accounts().id(username).setHttpPassword("secret");
+ gApi.accounts().id(username).setHttpPassword(ADMIN_PASSWORD);
// Generate private/public key for user
execute(
@@ -285,6 +363,10 @@
assertThat(out).contains(commit);
}
+ private String execute(String... cmds) throws Exception {
+ return execute(ImmutableList.<String>builder().add(cmds).build());
+ }
+
private String execute(ImmutableList<String> cmd) throws Exception {
return execute(cmd, sitePaths.data_dir.toFile(), ImmutableMap.of());
}
diff --git a/javatests/com/google/gerrit/prettify/BUILD b/javatests/com/google/gerrit/prettify/BUILD
new file mode 100644
index 0000000..0eb7cee
--- /dev/null
+++ b/javatests/com/google/gerrit/prettify/BUILD
@@ -0,0 +1,13 @@
+load("//tools/bzl:junit.bzl", "junit_tests")
+
+junit_tests(
+ name = "prettify_tests",
+ srcs = glob(["**/*.java"]),
+ deps = [
+ "//java/com/google/gerrit/prettify:server",
+ "//java/com/google/gerrit/prettify/common/testing",
+ "//java/com/google/gerrit/testing:gerrit-test-util",
+ "//lib:guava",
+ "//lib/truth",
+ ],
+)
diff --git a/javatests/com/google/gerrit/prettify/common/SparseFileContentBuilderTest.java b/javatests/com/google/gerrit/prettify/common/SparseFileContentBuilderTest.java
new file mode 100644
index 0000000..a751d50
--- /dev/null
+++ b/javatests/com/google/gerrit/prettify/common/SparseFileContentBuilderTest.java
@@ -0,0 +1,170 @@
+// Copyright (C) 2019 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 static com.google.gerrit.prettify.common.testing.SparseFileContentSubject.assertThat;
+import static com.google.gerrit.testing.GerritJUnit.assertThrows;
+
+import com.google.common.collect.ImmutableMap;
+import org.junit.Ignore;
+import org.junit.Test;
+
+public class SparseFileContentBuilderTest {
+
+ @Test
+ public void addLineWithNegativeNumber() {
+ SparseFileContentBuilder builder = new SparseFileContentBuilder(10);
+ assertThrows(IllegalArgumentException.class, () -> builder.addLine(-1, "First line"));
+
+ assertThrows(IllegalArgumentException.class, () -> builder.addLine(-5, "First line"));
+ }
+
+ @Test
+ @Ignore
+ public void addLineNumberZeroFileSize() {
+ // Temporary ignore - see comments in SparseFileContentBuilder.build() method
+ SparseFileContentBuilder builder = new SparseFileContentBuilder(0);
+ assertThrows(IllegalArgumentException.class, () -> builder.addLine(0, "First line"));
+ }
+
+ @Test
+ @Ignore
+ public void addLineNumberNonZeroFileSize() {
+ // Temporary ignore - see comments in SparseFileContentBuilder.build() method
+ SparseFileContentBuilder builder = new SparseFileContentBuilder(5);
+ assertThrows(IllegalArgumentException.class, () -> builder.addLine(5, "First line"));
+ assertThrows(IllegalArgumentException.class, () -> builder.addLine(6, "First line"));
+ assertThrows(IllegalArgumentException.class, () -> builder.addLine(7, "First line"));
+ }
+
+ @Test
+ public void addLineIncorrectOrder() {
+ SparseFileContentBuilder builder = new SparseFileContentBuilder(5);
+
+ builder.addLine(0, "First line");
+ builder.addLine(1, "Second line");
+ builder.addLine(3, "Third line");
+ builder.addLine(4, "Fourth line");
+ assertThrows(IllegalArgumentException.class, () -> builder.addLine(4, "Other Line"));
+
+ assertThrows(IllegalArgumentException.class, () -> builder.addLine(2, "Other Line"));
+ }
+
+ @Test
+ public void emptyContentZeroSize() {
+ SparseFileContentBuilder builder = new SparseFileContentBuilder(0);
+
+ SparseFileContent content = builder.build();
+ assertThat(content).getSize().isEqualTo(0);
+ assertThat(content).getRangesCount().isEqualTo(0);
+ assertThat(content).lines().isEmpty();
+ }
+
+ @Test
+ public void emptyContentNonZeroSize() {
+ SparseFileContentBuilder builder = new SparseFileContentBuilder(4);
+ SparseFileContent content = builder.build();
+ assertThat(content).getSize().isEqualTo(4);
+ assertThat(content).getRangesCount().isEqualTo(0);
+ assertThat(content).lines().isEmpty();
+ }
+
+ @Test
+ public void oneLineContentLineNumberZero() {
+ SparseFileContentBuilder builder = new SparseFileContentBuilder(1);
+
+ builder.addLine(0, "First line");
+ SparseFileContent content = builder.build();
+ assertThat(content).getSize().isEqualTo(1);
+ assertThat(content).getRangesCount().isEqualTo(1);
+ assertThat(content).lines().containsExactlyEntriesIn(ImmutableMap.of(0, "First line"));
+ }
+
+ @Test
+ public void oneLineContentLineNumberNotZero() {
+ SparseFileContentBuilder builder = new SparseFileContentBuilder(6);
+
+ builder.addLine(5, "First line");
+ SparseFileContent content = builder.build();
+ assertThat(content).getSize().isEqualTo(6);
+ assertThat(content).getRangesCount().isEqualTo(1);
+ assertThat(content).lines().containsExactlyEntriesIn(ImmutableMap.of(5, "First line"));
+ }
+
+ @Test
+ public void multiLineContinuousContentStartingFromZero() {
+ SparseFileContentBuilder builder = new SparseFileContentBuilder(5);
+
+ builder.addLine(0, "First line");
+ builder.addLine(1, "Second line");
+ builder.addLine(2, "Third line");
+ SparseFileContent content = builder.build();
+ assertThat(content).getSize().isEqualTo(5);
+ assertThat(content).getRangesCount().isEqualTo(1);
+ assertThat(content)
+ .lines()
+ .containsExactlyEntriesIn(
+ ImmutableMap.of(
+ 0, "First line",
+ 1, "Second line",
+ 2, "Third line"));
+ }
+
+ @Test
+ public void multiLineContentStartingFromNonZeroLine() {
+ SparseFileContentBuilder builder = new SparseFileContentBuilder(8);
+
+ builder.addLine(5, "First line");
+ builder.addLine(6, "Second line");
+ builder.addLine(7, "Third line");
+ SparseFileContent content = builder.build();
+ assertThat(content).getSize().isEqualTo(8);
+ assertThat(content).getRangesCount().isEqualTo(1);
+ assertThat(content)
+ .lines()
+ .containsExactlyEntriesIn(
+ ImmutableMap.of(
+ 5, "First line",
+ 6, "Second line",
+ 7, "Third line"));
+ }
+
+ @Test
+ public void multiLineContentWithGaps() {
+ SparseFileContentBuilder builder = new SparseFileContentBuilder(10000);
+ builder.addLine(0, "First line");
+ builder.addLine(1, "Second line");
+ builder.addLine(3, "Third line");
+ builder.addLine(4, "Fourth line");
+ builder.addLine(5, "Fifth line");
+ builder.addLine(6, "Sixth line");
+ builder.addLine(10, "Seventh line");
+ SparseFileContent content = builder.build();
+ assertThat(content).getSize().isEqualTo(10000);
+ assertThat(content).getRangesCount().isEqualTo(3);
+ assertThat(content)
+ .lines()
+ .containsExactlyEntriesIn(
+ ImmutableMap.builder()
+ .put(0, "First line")
+ .put(1, "Second line")
+ .put(3, "Third line")
+ .put(4, "Fourth line")
+ .put(5, "Fifth line")
+ .put(6, "Sixth line")
+ .put(10, "Seventh line")
+ .build());
+ }
+}
diff --git a/javatests/com/google/gerrit/server/notedb/ChangeNotesTest.java b/javatests/com/google/gerrit/server/notedb/ChangeNotesTest.java
index 145e914..db0dec8 100644
--- a/javatests/com/google/gerrit/server/notedb/ChangeNotesTest.java
+++ b/javatests/com/google/gerrit/server/notedb/ChangeNotesTest.java
@@ -45,6 +45,7 @@
import com.google.gerrit.entities.CommentRange;
import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.entities.PatchSetApproval;
+import com.google.gerrit.entities.SubmissionId;
import com.google.gerrit.exceptions.StorageException;
import com.google.gerrit.mail.Address;
import com.google.gerrit.server.AssigneeStatusUpdate;
@@ -52,7 +53,6 @@
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.ReviewerSet;
import com.google.gerrit.server.config.GerritServerId;
-import com.google.gerrit.server.logging.RequestId;
import com.google.gerrit.server.notedb.ChangeNotesCommit.ChangeNotesRevWalk;
import com.google.gerrit.server.util.time.TimeUtil;
import com.google.gerrit.testing.TestChanges;
@@ -432,7 +432,7 @@
@Test
public void approvalsPostSubmit() throws Exception {
Change c = newChange();
- RequestId submissionId = submissionId(c);
+ SubmissionId submissionId = new SubmissionId(c);
ChangeUpdate update = newUpdate(c, changeOwner);
update.putApproval("Code-Review", (short) 1);
update.putApproval("Verified", (short) 1);
@@ -467,7 +467,7 @@
@Test
public void approvalsDuringSubmit() throws Exception {
Change c = newChange();
- RequestId submissionId = submissionId(c);
+ SubmissionId submissionId = new SubmissionId(c);
ChangeUpdate update = newUpdate(c, changeOwner);
update.putApproval("Code-Review", (short) 1);
update.putApproval("Verified", (short) 1);
@@ -604,7 +604,7 @@
@Test
public void submitRecords() throws Exception {
Change c = newChange();
- RequestId submissionId = submissionId(c);
+ SubmissionId submissionId = new SubmissionId(c);
ChangeUpdate update = newUpdate(c, changeOwner);
update.setSubjectForCommit("Submit patch set 1");
@@ -640,13 +640,13 @@
null,
submitLabel("Verified", "OK", changeOwner.getAccountId()),
submitLabel("Alternative-Code-Review", "NEED", null)));
- assertThat(notes.getChange().getSubmissionId()).isEqualTo(submissionId.toStringForStorage());
+ assertThat(notes.getChange().getSubmissionId()).isEqualTo(submissionId.toString());
}
@Test
public void latestSubmitRecordsOnly() throws Exception {
Change c = newChange();
- RequestId submissionId = submissionId(c);
+ SubmissionId submissionId = new SubmissionId(c);
ChangeUpdate update = newUpdate(c, changeOwner);
update.setSubjectForCommit("Submit patch set 1");
update.merge(
@@ -669,7 +669,7 @@
assertThat(notes.getSubmitRecords())
.containsExactly(
submitRecord("OK", null, submitLabel("Code-Review", "OK", changeOwner.getAccountId())));
- assertThat(notes.getChange().getSubmissionId()).isEqualTo(submissionId.toStringForStorage());
+ assertThat(notes.getChange().getSubmissionId()).isEqualTo(submissionId.toString());
}
@Test
@@ -977,7 +977,7 @@
// Finish off by merging the change.
update = newUpdate(c, changeOwner);
update.merge(
- submissionId(c),
+ new SubmissionId(c),
ImmutableList.of(
submitRecord(
"NOT_READY",
@@ -3140,8 +3140,4 @@
update.commit();
return tr.parseBody(commit);
}
-
- private RequestId submissionId(Change c) {
- return new RequestId(c.getId().toString());
- }
}
diff --git a/javatests/com/google/gerrit/server/notedb/CommitMessageOutputTest.java b/javatests/com/google/gerrit/server/notedb/CommitMessageOutputTest.java
index 97781a4..5e25c13 100644
--- a/javatests/com/google/gerrit/server/notedb/CommitMessageOutputTest.java
+++ b/javatests/com/google/gerrit/server/notedb/CommitMessageOutputTest.java
@@ -21,9 +21,9 @@
import com.google.common.collect.ImmutableList;
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.Change;
+import com.google.gerrit.entities.SubmissionId;
import com.google.gerrit.mail.Address;
import com.google.gerrit.server.CurrentUser;
-import com.google.gerrit.server.logging.RequestId;
import com.google.gerrit.server.util.time.TimeUtil;
import com.google.gerrit.testing.ConfigSuite;
import com.google.gerrit.testing.TestChanges;
@@ -151,7 +151,7 @@
ChangeUpdate update = newUpdate(c, changeOwner);
update.setSubjectForCommit("Submit patch set 1");
- RequestId submissionId = submissionId(c);
+ SubmissionId submissionId = new SubmissionId(c);
update.merge(
submissionId,
ImmutableList.of(
@@ -174,7 +174,7 @@
+ "Patch-set: 1\n"
+ "Status: merged\n"
+ "Submission-id: "
- + submissionId.toStringForStorage()
+ + submissionId.toString()
+ "\n"
+ "Submitted-with: NOT_READY\n"
+ "Submitted-with: OK: Verified: Gerrit User 1 <1@gerrit>\n"
@@ -223,7 +223,7 @@
ChangeUpdate update = newUpdate(c, changeOwner);
update.setSubjectForCommit("Submit patch set 1");
- RequestId submissionId = submissionId(c);
+ SubmissionId submissionId = new SubmissionId(c);
update.merge(
submissionId, ImmutableList.of(submitRecord("RULE_ERROR", "Problem with patch set:\n1")));
update.commit();
@@ -234,7 +234,7 @@
+ "Patch-set: 1\n"
+ "Status: merged\n"
+ "Submission-id: "
- + submissionId.toStringForStorage()
+ + submissionId.toString()
+ "\n"
+ "Submitted-with: RULE_ERROR Problem with patch set: 1\n",
update.getResult());
@@ -427,8 +427,4 @@
RevCommit commit = parseCommit(commitId);
assertThat(commit.getFullMessage()).isEqualTo(expected);
}
-
- private RequestId submissionId(Change c) {
- return new RequestId(c.getId().toString());
- }
}
diff --git a/javatests/com/google/gerrit/server/update/BatchUpdateTest.java b/javatests/com/google/gerrit/server/update/BatchUpdateTest.java
index 5860c48..c7ca887 100644
--- a/javatests/com/google/gerrit/server/update/BatchUpdateTest.java
+++ b/javatests/com/google/gerrit/server/update/BatchUpdateTest.java
@@ -26,12 +26,12 @@
import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.entities.Project;
import com.google.gerrit.entities.RefNames;
+import com.google.gerrit.entities.SubmissionId;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.change.ChangeInserter;
import com.google.gerrit.server.change.PatchSetInserter;
import com.google.gerrit.server.git.GitRepositoryManager;
-import com.google.gerrit.server.logging.RequestId;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.notedb.ChangeUpdate;
import com.google.gerrit.server.notedb.Sequences;
@@ -332,7 +332,7 @@
cr.label = "Code-Review";
sr.labels = ImmutableList.of(cr);
ChangeUpdate update = ctx.getUpdate(ctx.getChange().currentPatchSetId());
- update.merge(new RequestId(), ImmutableList.of(sr));
+ update.merge(new SubmissionId(ctx.getChange()), ImmutableList.of(sr));
update.setChangeMessage("Submitted");
return true;
}
diff --git a/polygerrit-ui/app/elements/change/gr-confirm-rebase-dialog/gr-confirm-rebase-dialog_test.html b/polygerrit-ui/app/elements/change/gr-confirm-rebase-dialog/gr-confirm-rebase-dialog_test.html
index b116171..d801638 100644
--- a/polygerrit-ui/app/elements/change/gr-confirm-rebase-dialog/gr-confirm-rebase-dialog_test.html
+++ b/polygerrit-ui/app/elements/change/gr-confirm-rebase-dialog/gr-confirm-rebase-dialog_test.html
@@ -182,6 +182,11 @@
test('input text change triggers function', () => {
sandbox.spy(element, '_getRecentChanges');
element.$.parentInput.noDebounce = true;
+ MockInteractions.pressAndReleaseKeyOn(
+ element.$.parentInput.$.input,
+ 13,
+ null,
+ 'enter');
element._text = '1';
assert.isTrue(element._getRecentChanges.calledOnce);
element._text = '12';
diff --git a/polygerrit-ui/app/elements/core/gr-reporting/gr-reporting.js b/polygerrit-ui/app/elements/core/gr-reporting/gr-reporting.js
index d169728..3f2f4bd 100644
--- a/polygerrit-ui/app/elements/core/gr-reporting/gr-reporting.js
+++ b/polygerrit-ui/app/elements/core/gr-reporting/gr-reporting.js
@@ -24,7 +24,6 @@
CATEGORY_RPC: 'RPC Timing',
// Reported events - alphabetize below.
APP_STARTED: 'App Started',
- PAGE_LOADED: 'Page Loaded',
};
// Plugin-related reporting constants.
@@ -298,38 +297,21 @@
console.error('pageLoaded should be called after window.onload');
this.async(this.pageLoaded, 100);
} else {
- const loadTime = this.performanceTiming.loadEventEnd -
+ const perfEvents = Object.keys(this.performanceTiming.toJSON());
+ perfEvents.forEach(
+ eventName => this._reportPerformanceTiming(eventName)
+ );
+ }
+ },
+
+ _reportPerformanceTiming(eventName) {
+ const eventTiming = this.performanceTiming[eventName];
+ if (eventTiming > 0) {
+ const elapsedTime = eventTiming -
this.performanceTiming.navigationStart;
+ // NavResTime - Navigation and resource timings.
this.reporter(TIMING.TYPE, TIMING.CATEGORY_UI_LATENCY,
- TIMING.PAGE_LOADED, loadTime, true);
-
- const requestStart = this.performanceTiming.requestStart -
- this.performanceTiming.navigationStart;
- this.reporter(TIMING.TYPE, TIMING.CATEGORY_UI_LATENCY,
- 'requestStart', requestStart, true);
-
- const responseEnd = this.performanceTiming.responseEnd -
- this.performanceTiming.navigationStart;
- this.reporter(TIMING.TYPE, TIMING.CATEGORY_UI_LATENCY,
- 'responseEnd', responseEnd, true);
-
- const domLoading = this.performanceTiming.domLoading -
- this.performanceTiming.navigationStart;
- this.reporter(TIMING.TYPE, TIMING.CATEGORY_UI_LATENCY,
- 'domLoading', domLoading, true);
-
- const domContentLoadedEventStart =
- this.performanceTiming.domContentLoadedEventStart -
- this.performanceTiming.navigationStart;
- this.reporter(TIMING.TYPE, TIMING.CATEGORY_UI_LATENCY,
- 'domContentLoadedEventStart', domContentLoadedEventStart, true);
-
- if (this.performanceTiming.redirectEnd > 0) {
- const redirectEnd = this.performanceTiming.redirectEnd -
- this.performanceTiming.navigationStart;
- this.reporter(TIMING.TYPE, TIMING.CATEGORY_UI_LATENCY,
- 'redirectEnd', redirectEnd, true);
- }
+ `NavResTime - ${eventName}`, elapsedTime, true);
}
},
diff --git a/polygerrit-ui/app/elements/core/gr-reporting/gr-reporting_test.html b/polygerrit-ui/app/elements/core/gr-reporting/gr-reporting_test.html
index c2c0297..430b41e 100644
--- a/polygerrit-ui/app/elements/core/gr-reporting/gr-reporting_test.html
+++ b/polygerrit-ui/app/elements/core/gr-reporting/gr-reporting_test.html
@@ -52,6 +52,7 @@
navigationStart: 1,
loadEventEnd: 2,
};
+ fakePerformance.toJSON = () => fakePerformance;
sinon.stub(element, 'performanceTiming',
{get() { return fakePerformance; }});
sandbox.stub(element, 'reporter');
@@ -83,7 +84,7 @@
element.pageLoaded();
assert.isTrue(
element.reporter.calledWithExactly(
- 'timing-report', 'UI Latency', 'Page Loaded',
+ 'timing-report', 'UI Latency', 'NavResTime - loadEventEnd',
fakePerformance.loadEventEnd - fakePerformance.navigationStart,
true)
);
diff --git a/polygerrit-ui/app/elements/shared/gr-autocomplete/gr-autocomplete.js b/polygerrit-ui/app/elements/shared/gr-autocomplete/gr-autocomplete.js
index 3a60255..be3c48f 100644
--- a/polygerrit-ui/app/elements/shared/gr-autocomplete/gr-autocomplete.js
+++ b/polygerrit-ui/app/elements/shared/gr-autocomplete/gr-autocomplete.js
@@ -264,13 +264,23 @@
return;
}
+ // Reset _suggestions for every update
+ // This will also prevent from carrying over suggestions:
+ // @see Issue 12039
+ this._suggestions = [];
+
+ // TODO(taoalpha): Also skip if text has not changed
+
if (this._disableSuggestions) { return; }
if (text.length < threshold) {
- this._suggestions = [];
this.value = '';
return;
}
+ if (!this._focused) {
+ return;
+ }
+
const update = () => {
this.query(text).then(suggestions => {
if (text !== this.text) {
diff --git a/polygerrit-ui/app/elements/shared/gr-autocomplete/gr-autocomplete_test.html b/polygerrit-ui/app/elements/shared/gr-autocomplete/gr-autocomplete_test.html
index 39329e5..9df3cfc 100644
--- a/polygerrit-ui/app/elements/shared/gr-autocomplete/gr-autocomplete_test.html
+++ b/polygerrit-ui/app/elements/shared/gr-autocomplete/gr-autocomplete_test.html
@@ -38,6 +38,10 @@
suite('gr-autocomplete tests', () => {
let element;
let sandbox;
+ const focusOnInput = element => {
+ MockInteractions.pressAndReleaseKeyOn(element.$.input, 13, null,
+ 'enter');
+ };
setup(() => {
element = fixture('basic');
@@ -63,6 +67,7 @@
assert.isTrue(element.$.suggestions.isHidden);
assert.equal(element.$.suggestions.$.cursor.index, -1);
+ focusOnInput(element);
element.text = 'blah';
assert.isTrue(queryStub.called);
@@ -186,6 +191,7 @@
return promise = Promise.resolve([{name: 'suggestion', value: 0}]);
});
element.query = queryStub;
+ focusOnInput(element);
element.text = 'blah';
promise.then(() => {
@@ -207,6 +213,7 @@
return promise = Promise.resolve([{name: 'suggestion', value: 0}]);
});
element.query = queryStub;
+ focusOnInput(element);
element.text = 'blah';
element.clearOnCommit = true;
@@ -228,15 +235,11 @@
return Promise.resolve([]);
});
element.query = queryStub;
-
element.threshold = 2;
-
+ focusOnInput(element);
element.text = 'a';
-
assert.isFalse(queryStub.called);
-
element.text = 'ab';
-
assert.isTrue(queryStub.called);
});
@@ -249,6 +252,7 @@
(name, cb) => { callback = cb; });
element.query = queryStub;
element.noDebounce = false;
+ focusOnInput(element);
element.text = 'a';
assert.isFalse(queryStub.called);
assert.isTrue(debounceStub.called);
@@ -269,11 +273,60 @@
assert.equal(element._suggestions.length, 0);
});
+ test('when focused', done => {
+ let promise;
+ const queryStub = sandbox.stub()
+ .returns(promise = Promise.resolve([{name: 'suggestion', value: 0}]));
+ element.query = queryStub;
+ element.suggestOnlyWhenFocus = true;
+ focusOnInput(element);
+ element.text = 'bla';
+ assert.equal(element._focused, true);
+ flushAsynchronousOperations();
+ promise.then(() => {
+ assert.equal(element._suggestions.length, 1);
+ assert.equal(queryStub.notCalled, false);
+ done();
+ });
+ });
+
+ test('when not focused', done => {
+ let promise;
+ const queryStub = sandbox.stub()
+ .returns(promise = Promise.resolve([{name: 'suggestion', value: 0}]));
+ element.query = queryStub;
+ element.suggestOnlyWhenFocus = true;
+ element.text = 'bla';
+ assert.equal(element._focused, false);
+ flushAsynchronousOperations();
+ promise.then(() => {
+ assert.equal(element._suggestions.length, 0);
+ done();
+ });
+ });
+
+ test('suggestions should not carry over', done => {
+ let promise;
+ const queryStub = sandbox.stub()
+ .returns(promise = Promise.resolve([{name: 'suggestion', value: 0}]));
+ element.query = queryStub;
+ focusOnInput(element);
+ element.text = 'bla';
+ flushAsynchronousOperations();
+ promise.then(() => {
+ assert.equal(element._suggestions.length, 1);
+ element._updateSuggestions('', 0, false);
+ assert.equal(element._suggestions.length, 0);
+ done();
+ });
+ });
+
test('multi completes only the last part of the query', done => {
let promise;
const queryStub = sandbox.stub()
.returns(promise = Promise.resolve([{name: 'suggestion', value: 0}]));
element.query = queryStub;
+ focusOnInput(element);
element.text = 'blah blah';
element.multi = true;