Merge "Test tracing git push for a project via config"
diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt
index 6b3b79f..0c218c8 100644
--- a/Documentation/config-gerrit.txt
+++ b/Documentation/config-gerrit.txt
@@ -5976,6 +5976,19 @@
+
By default, unset (all projects are matched).
+[[tracing.metric]]
+==== Subsection tracing.metric
+
+Section to control for which operations latency and counts should be recorded
+in the link:metrics.html#performance[performance metrics].
+
+[[tracing.metric.operation]]tracing.metric.operation::
++
+Name of a Gerrit operation for which latency and counts should be recorded in
+the link:metrics.html#performance[performance metrics].
++
+The operation name must match the operation name that is used with TraceTimer.
+
[[deadline.id]]
==== Subsection deadline.<id>
diff --git a/Documentation/metrics.txt b/Documentation/metrics.txt
index 3b56391..a9b82d6 100644
--- a/Documentation/metrics.txt
+++ b/Documentation/metrics.txt
@@ -73,6 +73,30 @@
** `cancellation_type`:
The cancellation type (graceful or forceful).
+[[performance]]
+=== Performance
+
+* `performance/operations`: Latency of performing operations
+** `operation_name`:
+ The operation that was performed.
+** `request`:
+ The request for which the operation was performed (format = '<request-type>
+ <redacted-request-uri>').
+** `plugin`:
+ The name of the plugin that performed the operation.
+* `performance/operations_count`: Number of performed operations
+** `operation_name`:
+ The operation that was performed.
+** `request`:
+ The request for which the operation was performed (format = '<request-type>
+ <redacted-request-uri>').
+** `plugin`:
+ The name of the plugin that performed the operation.
+
+Performance metrics can be enabled via the
+link:config.gerrit.html#tracing.exportPerformanceMetrics[`tracing.exportPerformanceMetrics`]
+setting.
+
=== Pushes
* `receivecommits/changes`: histogram of number of changes processed in a single
@@ -100,8 +124,10 @@
DELETE).
* `receivecommits/reject_count`: number of rejected pushes
** `kind`:
- The push kind ('magic' if it was a push for code review, 'direct' if it was
- a direct push or 'n/a' if the push kind couldn't be detected).
+ The push kind ('magic push'/'magic push by service user' if it was a push for
+ code review, 'direct push'/'direct push by service user' if it was a direct
+ push, 'magic push by service, 'magic or direct push'/'magic or direct push by
+ service user' if the push kind couldn't be detected).
** `reason`:
The rejection reason.
** `status`:
diff --git a/java/com/google/gerrit/metrics/Timer1.java b/java/com/google/gerrit/metrics/Timer1.java
index 9dc32da..cdf457c 100644
--- a/java/com/google/gerrit/metrics/Timer1.java
+++ b/java/com/google/gerrit/metrics/Timer1.java
@@ -54,6 +54,8 @@
}
}
+ private boolean suppressLogging;
+
protected final String name;
protected final Field<F1> field;
@@ -87,15 +89,24 @@
field.metadataMapper().accept(metadataBuilder, fieldValue);
Metadata metadata = metadataBuilder.build();
- LoggingContext.getInstance()
- .addPerformanceLogRecord(() -> PerformanceLogRecord.create(name, durationNanos, metadata));
- logger.atFinest().log(
- "%s (%s = %s) took %.2f ms", name, field.name(), fieldValue, durationNanos / 1000000.0);
+ if (!suppressLogging) {
+ LoggingContext.getInstance()
+ .addPerformanceLogRecord(
+ () -> PerformanceLogRecord.create(name, durationNanos, metadata));
+ logger.atFinest().log(
+ "%s (%s = %s) took %.2f ms", name, field.name(), fieldValue, durationNanos / 1000000.0);
+ }
doRecord(fieldValue, value, unit);
RequestStateContext.abortIfCancelled();
}
+ /** Suppress logging (debug log and performance log) when values are recorded. */
+ public final Timer1<F1> suppressLogging() {
+ this.suppressLogging = true;
+ return this;
+ }
+
/**
* Record a value in the distribution.
*
diff --git a/java/com/google/gerrit/metrics/Timer2.java b/java/com/google/gerrit/metrics/Timer2.java
index 46dd617..9f08711 100644
--- a/java/com/google/gerrit/metrics/Timer2.java
+++ b/java/com/google/gerrit/metrics/Timer2.java
@@ -57,6 +57,8 @@
}
}
+ private boolean suppressLogging;
+
protected final String name;
protected final Field<F1> field1;
protected final Field<F2> field2;
@@ -95,16 +97,25 @@
field2.metadataMapper().accept(metadataBuilder, fieldValue2);
Metadata metadata = metadataBuilder.build();
- LoggingContext.getInstance()
- .addPerformanceLogRecord(() -> PerformanceLogRecord.create(name, durationNanos, metadata));
- logger.atFinest().log(
- "%s (%s = %s, %s = %s) took %.2f ms",
- name, field1.name(), fieldValue1, field2.name(), fieldValue2, durationNanos / 1000000.0);
+ if (!suppressLogging) {
+ LoggingContext.getInstance()
+ .addPerformanceLogRecord(
+ () -> PerformanceLogRecord.create(name, durationNanos, metadata));
+ logger.atFinest().log(
+ "%s (%s = %s, %s = %s) took %.2f ms",
+ name, field1.name(), fieldValue1, field2.name(), fieldValue2, durationNanos / 1000000.0);
+ }
doRecord(fieldValue1, fieldValue2, value, unit);
RequestStateContext.abortIfCancelled();
}
+ /** Suppress logging (debug log and performance log) when values are recorded. */
+ public final Timer2<F1, F2> suppressLogging() {
+ this.suppressLogging = true;
+ return this;
+ }
+
/**
* Record a value in the distribution.
*
diff --git a/java/com/google/gerrit/metrics/Timer3.java b/java/com/google/gerrit/metrics/Timer3.java
index 922cdd6..2a01d5d 100644
--- a/java/com/google/gerrit/metrics/Timer3.java
+++ b/java/com/google/gerrit/metrics/Timer3.java
@@ -60,6 +60,8 @@
}
}
+ private boolean suppressLogging;
+
protected final String name;
protected final Field<F1> field1;
protected final Field<F2> field2;
@@ -104,23 +106,32 @@
field3.metadataMapper().accept(metadataBuilder, fieldValue3);
Metadata metadata = metadataBuilder.build();
- LoggingContext.getInstance()
- .addPerformanceLogRecord(() -> PerformanceLogRecord.create(name, durationNanos, metadata));
- logger.atFinest().log(
- "%s (%s = %s, %s = %s, %s = %s) took %.2f ms",
- name,
- field1.name(),
- fieldValue1,
- field2.name(),
- fieldValue2,
- field3.name(),
- fieldValue3,
- durationNanos / 1000000.0);
+ if (!suppressLogging) {
+ LoggingContext.getInstance()
+ .addPerformanceLogRecord(
+ () -> PerformanceLogRecord.create(name, durationNanos, metadata));
+ logger.atFinest().log(
+ "%s (%s = %s, %s = %s, %s = %s) took %.2f ms",
+ name,
+ field1.name(),
+ fieldValue1,
+ field2.name(),
+ fieldValue2,
+ field3.name(),
+ fieldValue3,
+ durationNanos / 1000000.0);
+ }
doRecord(fieldValue1, fieldValue2, fieldValue3, value, unit);
RequestStateContext.abortIfCancelled();
}
+ /** Suppress logging (debug log and performance log) when values are recorded. */
+ public final Timer3<F1, F2, F3> suppressLogging() {
+ this.suppressLogging = true;
+ return this;
+ }
+
/**
* Record a value in the distribution.
*
diff --git a/java/com/google/gerrit/server/PerformanceMetrics.java b/java/com/google/gerrit/server/PerformanceMetrics.java
new file mode 100644
index 0000000..3d21fed
--- /dev/null
+++ b/java/com/google/gerrit/server/PerformanceMetrics.java
@@ -0,0 +1,104 @@
+// Copyright (C) 2021 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;
+
+import com.google.common.collect.ImmutableList;
+import com.google.gerrit.common.Nullable;
+import com.google.gerrit.metrics.Counter3;
+import com.google.gerrit.metrics.Description;
+import com.google.gerrit.metrics.Field;
+import com.google.gerrit.metrics.MetricMaker;
+import com.google.gerrit.metrics.Timer3;
+import com.google.gerrit.server.config.GerritServerConfig;
+import com.google.gerrit.server.logging.Metadata;
+import com.google.gerrit.server.logging.PerformanceLogger;
+import com.google.gerrit.server.logging.TraceContext;
+import com.google.inject.Inject;
+import com.google.inject.Singleton;
+import java.time.Instant;
+import java.util.concurrent.TimeUnit;
+import org.eclipse.jgit.lib.Config;
+
+/** Performance logger that records the execution times as a metric. */
+@Singleton
+public class PerformanceMetrics implements PerformanceLogger {
+ private static final String OPERATION_LATENCY_METRIC_NAME = "performance/operations";
+ private static final String OPERATION_COUNT_METRIC_NAME = "performance/operations_count";
+
+ private final ImmutableList<String> tracedOperations;
+ public final Timer3<String, String, String> operationsLatency;
+ public final Counter3<String, String, String> operationsCounter;
+
+ @Inject
+ PerformanceMetrics(@GerritServerConfig Config cfg, MetricMaker metricMaker) {
+ this.tracedOperations =
+ ImmutableList.copyOf(cfg.getStringList("tracing", "metric", "operation"));
+
+ Field<String> operationNameField =
+ Field.ofString(
+ "operation_name",
+ (metadataBuilder, fieldValue) -> metadataBuilder.operationName(fieldValue))
+ .description("The operation that was performed.")
+ .build();
+ Field<String> requestField =
+ Field.ofString("request", (metadataBuilder, fieldValue) -> {})
+ .description(
+ "The request for which the operation was performed"
+ + " (format = '<request-type> <redacted-request-uri>').")
+ .build();
+ Field<String> pluginField =
+ Field.ofString(
+ "plugin", (metadataBuilder, fieldValue) -> metadataBuilder.pluginName(fieldValue))
+ .description("The name of the plugin that performed the operation.")
+ .build();
+
+ this.operationsLatency =
+ metricMaker
+ .newTimer(
+ OPERATION_LATENCY_METRIC_NAME,
+ new Description("Latency of performing operations")
+ .setCumulative()
+ .setUnit(Description.Units.MILLISECONDS),
+ operationNameField,
+ requestField,
+ pluginField)
+ .suppressLogging();
+ this.operationsCounter =
+ metricMaker.newCounter(
+ OPERATION_COUNT_METRIC_NAME,
+ new Description("Number of performed operations").setRate(),
+ operationNameField,
+ requestField,
+ pluginField);
+ }
+
+ @Override
+ public void logNanos(String operation, long durationNanos, Instant endTime) {
+ logNanos(operation, durationNanos, endTime, /* metadata= */ null);
+ }
+
+ @Override
+ public void logNanos(
+ String operation, long durationNanos, Instant endTime, @Nullable Metadata metadata) {
+ if (!tracedOperations.contains(operation)) {
+ return;
+ }
+
+ String requestTag = TraceContext.getTag(TraceRequestListener.TAG_REQUEST).orElse("");
+ String pluginTag = TraceContext.getPluginTag().orElse("");
+ operationsLatency.record(operation, requestTag, pluginTag, durationNanos, TimeUnit.NANOSECONDS);
+ operationsCounter.increment(operation, requestTag, pluginTag);
+ }
+}
diff --git a/java/com/google/gerrit/server/config/GerritGlobalModule.java b/java/com/google/gerrit/server/config/GerritGlobalModule.java
index 2b26956..c4ecc80 100644
--- a/java/com/google/gerrit/server/config/GerritGlobalModule.java
+++ b/java/com/google/gerrit/server/config/GerritGlobalModule.java
@@ -89,6 +89,7 @@
import com.google.gerrit.server.ExceptionHookImpl;
import com.google.gerrit.server.ExternalUser;
import com.google.gerrit.server.IdentifiedUser;
+import com.google.gerrit.server.PerformanceMetrics;
import com.google.gerrit.server.RequestListener;
import com.google.gerrit.server.TraceRequestListener;
import com.google.gerrit.server.account.AccountControl;
@@ -455,6 +456,7 @@
DynamicSet.setOf(binder(), SubmitRequirement.class);
DynamicSet.setOf(binder(), QuotaEnforcer.class);
DynamicSet.setOf(binder(), PerformanceLogger.class);
+ DynamicSet.bind(binder(), PerformanceLogger.class).to(PerformanceMetrics.class);
DynamicSet.setOf(binder(), RequestListener.class);
DynamicSet.bind(binder(), RequestListener.class).to(TraceRequestListener.class);
DynamicSet.setOf(binder(), ExceptionHook.class);
diff --git a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
index 9f7b763..536e1fb 100644
--- a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
+++ b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
@@ -124,6 +124,7 @@
import com.google.gerrit.server.Sequences;
import com.google.gerrit.server.account.AccountResolver;
import com.google.gerrit.server.account.AccountResolver.UnresolvableAccountException;
+import com.google.gerrit.server.account.ServiceUserClassifier;
import com.google.gerrit.server.approval.ApprovalsUtil;
import com.google.gerrit.server.cancellation.RequestCancelledException;
import com.google.gerrit.server.cancellation.RequestStateContext;
@@ -234,6 +235,7 @@
import java.util.concurrent.ConcurrentLinkedQueue;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.Future;
+import java.util.logging.Level;
import java.util.stream.Collectors;
import java.util.stream.Stream;
import java.util.stream.StreamSupport;
@@ -426,6 +428,7 @@
private final Sequences seq;
private final SetHashtagsOp.Factory hashtagsFactory;
private final SetTopicOp.Factory setTopicFactory;
+ private final ServiceUserClassifier serviceUserClassifier;
private final ImmutableList<SubmissionListener> superprojectUpdateSubmissionListeners;
private final TagCache tagCache;
private final ProjectConfig.Factory projectConfigFactory;
@@ -516,6 +519,7 @@
Sequences seq,
SetHashtagsOp.Factory hashtagsFactory,
SetTopicOp.Factory setTopicFactory,
+ ServiceUserClassifier serviceUserClassifier,
@SuperprojectUpdateOnSubmission
ImmutableList<SubmissionListener> superprojectUpdateSubmissionListeners,
TagCache tagCache,
@@ -550,6 +554,7 @@
this.exceptionHooks = exceptionHooks;
this.hashtagsFactory = hashtagsFactory;
this.setTopicFactory = setTopicFactory;
+ this.serviceUserClassifier = serviceUserClassifier;
this.indexer = indexer;
this.initializers = initializers;
this.mergeOpProvider = mergeOpProvider;
@@ -762,8 +767,15 @@
logger.atFine().log("Processing commands done.");
} catch (RuntimeException e) {
String formattedCause = getFormattedCause(e).orElse(e.getClass().getSimpleName());
- logger.atFine().withCause(e).log("ReceiveCommits failed due to %s", formattedCause);
- metrics.rejectCount.increment("n/a", formattedCause, SC_INTERNAL_SERVER_ERROR);
+ int statusCode =
+ getStatus(e).map(ExceptionHook.Status::statusCode).orElse(SC_INTERNAL_SERVER_ERROR);
+ logger.at(statusCode < SC_INTERNAL_SERVER_ERROR ? Level.INFO : Level.SEVERE).withCause(e).log(
+ "ReceiveCommits failed due to %s", formattedCause);
+ String pushKind = "magic or direct push";
+ if (serviceUserClassifier.isServiceUser(user.getAccountId())) {
+ pushKind += " by service user";
+ }
+ metrics.rejectCount.increment(pushKind, formattedCause, statusCode);
throw e;
}
progress.end();
@@ -778,6 +790,14 @@
.findFirst();
}
+ private Optional<ExceptionHook.Status> getStatus(Throwable err) {
+ return exceptionHooks.stream()
+ .map(h -> h.getStatus(err))
+ .filter(Optional::isPresent)
+ .map(Optional::get)
+ .findFirst();
+ }
+
// Process as many commands as possible, but may leave some commands in state NOT_ATTEMPTED.
private void processCommandsUnsafe(
Collection<ReceiveCommand> commands, MultiProgressMonitor progress) {
@@ -3934,10 +3954,13 @@
private void reject(ReceiveCommand cmd, RejectionReason reason) {
logger.atFine().log("Rejecting command '%s': %s", cmd, reason.why());
- metrics.rejectCount.increment(
- MagicBranch.isMagicBranch(cmd.getRefName()) ? "magic" : "direct",
- reason.metricBucket(),
- reason.statusCode());
+
+ String pushKind = (MagicBranch.isMagicBranch(cmd.getRefName()) ? "magic push" : "direct push");
+ if (serviceUserClassifier.isServiceUser(user.getAccountId())) {
+ pushKind += " by service user";
+ }
+ metrics.rejectCount.increment(pushKind, reason.metricBucket(), reason.statusCode());
+
cmd.setResult(REJECTED_OTHER_REASON, reason.why());
}
diff --git a/java/com/google/gerrit/server/logging/PerformanceLogger.java b/java/com/google/gerrit/server/logging/PerformanceLogger.java
index 2f90dfd..fa61eb4 100644
--- a/java/com/google/gerrit/server/logging/PerformanceLogger.java
+++ b/java/com/google/gerrit/server/logging/PerformanceLogger.java
@@ -16,7 +16,6 @@
import com.google.gerrit.extensions.annotations.ExtensionPoint;
import java.time.Instant;
-import java.util.concurrent.TimeUnit;
/**
* Extension point for logging performance records.
@@ -35,27 +34,6 @@
* Record the execution time of an operation in a performance log.
*
* @param operation operation that was performed
- * @param durationMs time that the execution of the operation took (in milliseconds)
- */
- @Deprecated
- default void log(String operation, long durationMs, Instant endTime) {
- log(operation, durationMs, endTime, Metadata.empty());
- }
-
- /**
- * Record the execution time of an operation in a performance log.
- *
- * @param operation operation that was performed
- * @param durationMs time that the execution of the operation took (in milliseconds)
- * @param metadata metadata
- */
- @Deprecated
- void log(String operation, long durationMs, Instant endTime, Metadata metadata);
-
- /**
- * Record the execution time of an operation in a performance log.
- *
- * @param operation operation that was performed
* @param durationNanos time that the execution of the operation took (in nanoseconds)
*/
default void logNanos(String operation, long durationNanos, Instant endTime) {
@@ -69,7 +47,5 @@
* @param durationNanos time that the execution of the operation took (in nanoseconds)
* @param metadata metadata
*/
- default void logNanos(String operation, long durationNanos, Instant endTime, Metadata metadata) {
- log(operation, TimeUnit.NANOSECONDS.toMillis(durationNanos), endTime, metadata);
- }
+ void logNanos(String operation, long durationNanos, Instant endTime, Metadata metadata);
}
diff --git a/javatests/com/google/gerrit/acceptance/ssh/SshTraceIT.java b/javatests/com/google/gerrit/acceptance/ssh/SshTraceIT.java
index c4497dc..8153a5d 100644
--- a/javatests/com/google/gerrit/acceptance/ssh/SshTraceIT.java
+++ b/javatests/com/google/gerrit/acceptance/ssh/SshTraceIT.java
@@ -124,7 +124,7 @@
private ImmutableList.Builder<PerformanceLogEntry> logEntries = ImmutableList.builder();
@Override
- public void log(String operation, long durationMs, Instant endTime, Metadata metadata) {
+ public void logNanos(String operation, long durationNanos, Instant endTime, Metadata metadata) {
logEntries.add(PerformanceLogEntry.create(operation, endTime, metadata));
}
diff --git a/javatests/com/google/gerrit/server/logging/LoggingContextAwareExecutorServiceTest.java b/javatests/com/google/gerrit/server/logging/LoggingContextAwareExecutorServiceTest.java
index c1b9f13..8cdf16a 100644
--- a/javatests/com/google/gerrit/server/logging/LoggingContextAwareExecutorServiceTest.java
+++ b/javatests/com/google/gerrit/server/logging/LoggingContextAwareExecutorServiceTest.java
@@ -52,7 +52,8 @@
testPerformanceLogger =
new PerformanceLogger() {
@Override
- public void log(String operation, long durationMs, Instant endTime, Metadata metadata) {
+ public void logNanos(
+ String operation, long durationNanos, Instant endTime, Metadata metadata) {
// do nothing
}
};
diff --git a/javatests/com/google/gerrit/server/logging/PerformanceLogContextTest.java b/javatests/com/google/gerrit/server/logging/PerformanceLogContextTest.java
index c93061d..4b3c658 100644
--- a/javatests/com/google/gerrit/server/logging/PerformanceLogContextTest.java
+++ b/javatests/com/google/gerrit/server/logging/PerformanceLogContextTest.java
@@ -365,7 +365,7 @@
private ImmutableList.Builder<PerformanceLogEntry> logEntries = ImmutableList.builder();
@Override
- public void log(String operation, long durationMs, Instant endTime, Metadata metadata) {
+ public void logNanos(String operation, long durationNanos, Instant endTime, Metadata metadata) {
logEntries.add(PerformanceLogEntry.create(operation, metadata));
}
diff --git a/polygerrit-ui/app/embed/diff/gr-context-controls/gr-context-controls.ts b/polygerrit-ui/app/embed/diff/gr-context-controls/gr-context-controls.ts
index e1fc2ed..1ec5262e 100644
--- a/polygerrit-ui/app/embed/diff/gr-context-controls/gr-context-controls.ts
+++ b/polygerrit-ui/app/embed/diff/gr-context-controls/gr-context-controls.ts
@@ -271,6 +271,16 @@
.breadcrumbTooltip {
white-space: nowrap;
}
+ .unrelatedChanges {
+ color: var(--primary-button-text-color);
+ background-color: var(--primary-button-background-color);
+
+ &:hover {
+ // TODO(anuragpathak): Update hover colors as per specification.
+ color: var(--primary-button-text-color);
+ background-color: var(--primary-button-background-color);
+ }
+ }
`,
];
}
@@ -370,8 +380,14 @@
let classes = 'contextControlButton showContext ';
if (type === ContextButtonType.ALL) {
- text = `+${pluralize(linesToExpand, 'common line')}`;
- ariaLabel = `Show ${pluralize(linesToExpand, 'common line')}`;
+ if (this.group.hasNonCommonDeltaGroup()) {
+ text = '+ Unrelated changes';
+ ariaLabel = 'Show unrelated changes';
+ classes += ' unrelatedChanges ';
+ } else {
+ text = `+${pluralize(linesToExpand, 'common line')}`;
+ ariaLabel = `Show ${pluralize(linesToExpand, 'common line')}`;
+ }
classes += this.showBoth()
? 'centeredButton'
: this.showAbove()
@@ -483,7 +499,7 @@
* Creates a container div with partial (+10) expansion buttons (above and/or below).
*/
private createPartialExpansionButtons() {
- if (!this.showPartialLinks()) {
+ if (!this.showPartialLinks() || this.group?.hasNonCommonDeltaGroup()) {
return undefined;
}
let aboveButton;
@@ -515,7 +531,8 @@
if (
!this.showPartialLinks() ||
!this.renderPreferences?.use_block_expansion ||
- this.group?.hasSkipGroup()
+ this.group?.hasSkipGroup() ||
+ this.group?.hasNonCommonDeltaGroup()
) {
return undefined;
}
diff --git a/polygerrit-ui/app/embed/diff/gr-context-controls/gr-context-controls_test.ts b/polygerrit-ui/app/embed/diff/gr-context-controls/gr-context-controls_test.ts
index 20fc9c4..74726bf 100644
--- a/polygerrit-ui/app/embed/diff/gr-context-controls/gr-context-controls_test.ts
+++ b/polygerrit-ui/app/embed/diff/gr-context-controls/gr-context-controls_test.ts
@@ -10,7 +10,10 @@
import {SyntaxBlock} from '../../../api/diff';
import {fixture, html, assert} from '@open-wc/testing';
import {waitEventLoop} from '../../../test/test-utils';
-import {createContextGroup} from '../../../test/test-data-generators';
+import {
+ createContextGroup,
+ createContextGroupWithDelta,
+} from '../../../test/test-data-generators';
suite('gr-context-control tests', () => {
let element: GrContextControls;
@@ -333,4 +336,16 @@
assert.equal(tooltipAbove.getAttribute('position'), 'top');
assert.equal(tooltipBelow.getAttribute('position'), 'bottom');
});
+
+ test('context control with delta group', async () => {
+ element.group = createContextGroupWithDelta();
+ await waitEventLoop();
+
+ const buttons = element.shadowRoot!.querySelectorAll(
+ 'paper-button.showContext'
+ );
+ assert.equal(buttons.length, 1);
+ assert.equal(buttons[0].textContent!.trim(), '+ Unrelated changes');
+ assert.include([...buttons[0].classList.values()], 'unrelatedChanges');
+ });
});
diff --git a/polygerrit-ui/app/embed/diff/gr-diff/gr-diff-group.ts b/polygerrit-ui/app/embed/diff/gr-diff/gr-diff-group.ts
index c5366e7..c7e9f44 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff/gr-diff-group.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff/gr-diff-group.ts
@@ -165,7 +165,8 @@
* Groups where all lines are before or all lines are after the split will be
* retained as is and put into the first or second list respectively. Groups
* with some lines before and some lines after the split will be split into
- * two groups, which will be put into the first and second list.
+ * two groups, which will be put into the first and second list. Groups with
+ * type DELTA which are not common will not be split.
*
* @param split A line number offset relative to the first group's
* start line at which the groups should be split.
@@ -179,31 +180,41 @@
if (groups.length === 0) return [[], []];
const leftSplit = groups[0].lineRange.left.start_line + split;
const rightSplit = groups[0].lineRange.right.start_line + split;
-
+ let isSplitDone = false;
const beforeGroups = [];
const afterGroups = [];
for (const group of groups) {
- const isCompletelyBefore =
- group.lineRange.left.end_line < leftSplit ||
- group.lineRange.right.end_line < rightSplit;
- const isCompletelyAfter =
- leftSplit <= group.lineRange.left.start_line ||
- rightSplit <= group.lineRange.right.start_line;
- if (isCompletelyBefore) {
- beforeGroups.push(group);
- } else if (isCompletelyAfter) {
+ if (isSplitDone) {
afterGroups.push(group);
+ } else if (
+ group.type === GrDiffGroupType.DELTA &&
+ !group.ignoredWhitespaceOnly
+ ) {
+ beforeGroups.push(group);
} else {
- const {beforeSplit, afterSplit} = splitGroupInTwo(
- group,
- leftSplit,
- rightSplit
- );
- if (beforeSplit) {
- beforeGroups.push(beforeSplit);
- }
- if (afterSplit) {
- afterGroups.push(afterSplit);
+ const isCompletelyBefore =
+ group.lineRange.left.end_line < leftSplit ||
+ group.lineRange.right.end_line < rightSplit;
+ const isCompletelyAfter =
+ leftSplit <= group.lineRange.left.start_line ||
+ rightSplit <= group.lineRange.right.start_line;
+ if (isCompletelyBefore) {
+ beforeGroups.push(group);
+ } else if (isCompletelyAfter) {
+ afterGroups.push(group);
+ } else {
+ const {beforeSplit, afterSplit} = splitGroupInTwo(
+ group,
+ leftSplit,
+ rightSplit
+ );
+ if (beforeSplit) {
+ beforeGroups.push(beforeSplit);
+ }
+ if (afterSplit) {
+ afterGroups.push(afterSplit);
+ }
+ isSplitDone = true;
}
}
}
@@ -438,6 +449,15 @@
);
}
+ /** Returns true if it contains a DELTA group excluding whitespace only
+ * changes.
+ */
+ hasNonCommonDeltaGroup() {
+ return this.contextGroups?.some(
+ g => g.type === GrDiffGroupType.DELTA && !g.ignoredWhitespaceOnly
+ );
+ }
+
containsLine(side: Side, line: LineNumber) {
if (typeof line !== 'number') {
// For FILE and LOST, beforeNumber and afterNumber are the same
diff --git a/polygerrit-ui/app/embed/diff/gr-diff/gr-diff-group_test.ts b/polygerrit-ui/app/embed/diff/gr-diff/gr-diff-group_test.ts
index bbbb4ad..c8446f0 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff/gr-diff-group_test.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff/gr-diff-group_test.ts
@@ -97,6 +97,8 @@
suite('hideInContextControl', () => {
let groups: GrDiffGroup[];
+ let groupsWithDelta: GrDiffGroup[];
+ let groupsWithWhiteSpaceOnlyChange: GrDiffGroup[];
setup(() => {
groups = [
new GrDiffGroup({
@@ -108,6 +110,46 @@
],
}),
new GrDiffGroup({
+ type: GrDiffGroupType.BOTH,
+ lines: [
+ new GrDiffLine(GrDiffLineType.BOTH, 8, 10),
+ new GrDiffLine(GrDiffLineType.BOTH, 9, 11),
+ new GrDiffLine(GrDiffLineType.BOTH, 10, 12),
+ new GrDiffLine(GrDiffLineType.BOTH, 11, 13),
+ ],
+ }),
+ new GrDiffGroup({
+ type: GrDiffGroupType.BOTH,
+ lines: [
+ new GrDiffLine(GrDiffLineType.BOTH, 12, 14),
+ new GrDiffLine(GrDiffLineType.BOTH, 13, 15),
+ new GrDiffLine(GrDiffLineType.BOTH, 14, 16),
+ ],
+ }),
+ ];
+
+ groupsWithWhiteSpaceOnlyChange = [
+ groups[0],
+ new GrDiffGroup({
+ type: GrDiffGroupType.DELTA,
+ lines: [
+ new GrDiffLine(GrDiffLineType.REMOVE, 8),
+ new GrDiffLine(GrDiffLineType.ADD, 0, 10),
+ new GrDiffLine(GrDiffLineType.REMOVE, 9),
+ new GrDiffLine(GrDiffLineType.ADD, 0, 11),
+ new GrDiffLine(GrDiffLineType.REMOVE, 10),
+ new GrDiffLine(GrDiffLineType.ADD, 0, 12),
+ new GrDiffLine(GrDiffLineType.REMOVE, 11),
+ new GrDiffLine(GrDiffLineType.ADD, 0, 13),
+ ],
+ ignoredWhitespaceOnly: true,
+ }),
+ groups[2],
+ ];
+
+ groupsWithDelta = [
+ groups[0],
+ new GrDiffGroup({
type: GrDiffGroupType.DELTA,
lines: [
new GrDiffLine(GrDiffLineType.REMOVE, 8),
@@ -120,14 +162,7 @@
new GrDiffLine(GrDiffLineType.ADD, 0, 13),
],
}),
- new GrDiffGroup({
- type: GrDiffGroupType.BOTH,
- lines: [
- new GrDiffLine(GrDiffLineType.BOTH, 12, 14),
- new GrDiffLine(GrDiffLineType.BOTH, 13, 15),
- new GrDiffLine(GrDiffLineType.BOTH, 14, 16),
- ],
- }),
+ groups[2],
];
});
@@ -144,29 +179,33 @@
assert.equal(collapsedGroups[2], groups[2]);
});
+ test('does not hides when split is at delta group in context control', () => {
+ const collapsedGroups = hideInContextControl(groupsWithDelta, 3, 7);
+ assert.equal(collapsedGroups.length, 3);
+
+ assert.equal(collapsedGroups[0], groupsWithDelta[0]);
+ assert.equal(collapsedGroups[1], groupsWithDelta[1]);
+ assert.equal(collapsedGroups[2], groupsWithDelta[2]);
+ });
+
test('splits partially hidden groups', () => {
const collapsedGroups = hideInContextControl(groups, 4, 8);
assert.equal(collapsedGroups.length, 4);
assert.equal(collapsedGroups[0], groups[0]);
- assert.equal(collapsedGroups[1].type, GrDiffGroupType.DELTA);
- assert.deepEqual(collapsedGroups[1].adds, [groups[1].adds[0]]);
- assert.deepEqual(collapsedGroups[1].removes, [groups[1].removes[0]]);
+ assert.equal(collapsedGroups[1].type, GrDiffGroupType.BOTH);
+ assert.deepEqual(collapsedGroups[1].lines, [groups[1].lines[0]]);
assert.equal(collapsedGroups[2].type, GrDiffGroupType.CONTEXT_CONTROL);
assert.equal(collapsedGroups[2].contextGroups.length, 2);
assert.equal(
collapsedGroups[2].contextGroups[0].type,
- GrDiffGroupType.DELTA
+ GrDiffGroupType.BOTH
);
assert.deepEqual(
- collapsedGroups[2].contextGroups[0].adds,
- groups[1].adds.slice(1)
- );
- assert.deepEqual(
- collapsedGroups[2].contextGroups[0].removes,
- groups[1].removes.slice(1)
+ collapsedGroups[2].contextGroups[0].lines,
+ groups[1].lines.slice(1)
);
assert.equal(
@@ -181,6 +220,54 @@
assert.deepEqual(collapsedGroups[3].lines, groups[2].lines.slice(1));
});
+ test('splits partially hidden common delta groups', () => {
+ const collapsedGroups = hideInContextControl(
+ groupsWithWhiteSpaceOnlyChange,
+ 4,
+ 8
+ );
+ assert.equal(collapsedGroups.length, 4);
+ assert.equal(collapsedGroups[0], groupsWithWhiteSpaceOnlyChange[0]);
+
+ assert.equal(collapsedGroups[1].type, GrDiffGroupType.DELTA);
+ assert.deepEqual(collapsedGroups[1].adds, [
+ groupsWithWhiteSpaceOnlyChange[1].adds[0],
+ ]);
+ assert.deepEqual(collapsedGroups[1].removes, [
+ groupsWithWhiteSpaceOnlyChange[1].removes[0],
+ ]);
+
+ assert.equal(collapsedGroups[2].type, GrDiffGroupType.CONTEXT_CONTROL);
+ assert.equal(collapsedGroups[2].contextGroups.length, 2);
+
+ assert.equal(
+ collapsedGroups[2].contextGroups[0].type,
+ GrDiffGroupType.DELTA
+ );
+ assert.deepEqual(
+ collapsedGroups[2].contextGroups[0].adds,
+ groupsWithWhiteSpaceOnlyChange[1].adds.slice(1)
+ );
+ assert.deepEqual(
+ collapsedGroups[2].contextGroups[0].removes,
+ groupsWithWhiteSpaceOnlyChange[1].removes.slice(1)
+ );
+
+ assert.equal(
+ collapsedGroups[2].contextGroups[1].type,
+ GrDiffGroupType.BOTH
+ );
+ assert.deepEqual(collapsedGroups[2].contextGroups[1].lines, [
+ groupsWithWhiteSpaceOnlyChange[2].lines[0],
+ ]);
+
+ assert.equal(collapsedGroups[3].type, GrDiffGroupType.BOTH);
+ assert.deepEqual(
+ collapsedGroups[3].lines,
+ groupsWithWhiteSpaceOnlyChange[2].lines.slice(1)
+ );
+ });
+
suite('with skip chunks', () => {
setup(() => {
const skipGroup = new GrDiffGroup({
diff --git a/polygerrit-ui/app/test/test-data-generators.ts b/polygerrit-ui/app/test/test-data-generators.ts
index 22dbe7c1..2309977 100644
--- a/polygerrit-ui/app/test/test-data-generators.ts
+++ b/polygerrit-ui/app/test/test-data-generators.ts
@@ -688,6 +688,27 @@
});
}
+export function createContextGroupWithDelta() {
+ return new GrDiffGroup({
+ type: GrDiffGroupType.CONTEXT_CONTROL,
+ contextGroups: [
+ new GrDiffGroup({
+ type: GrDiffGroupType.DELTA,
+ lines: [
+ new GrDiffLine(GrDiffLineType.REMOVE, 8),
+ new GrDiffLine(GrDiffLineType.ADD, 0, 10),
+ new GrDiffLine(GrDiffLineType.REMOVE, 9),
+ new GrDiffLine(GrDiffLineType.ADD, 0, 11),
+ new GrDiffLine(GrDiffLineType.REMOVE, 10),
+ new GrDiffLine(GrDiffLineType.ADD, 0, 12),
+ new GrDiffLine(GrDiffLineType.REMOVE, 11),
+ new GrDiffLine(GrDiffLineType.ADD, 0, 13),
+ ],
+ }),
+ ],
+ });
+}
+
export function createBlame(): BlameInfo {
return {
author: 'test-author',
