Merge "Document how to get pretty-printed JSON for REST calls"
diff --git a/java/com/google/gerrit/server/patch/ComparisonType.java b/java/com/google/gerrit/server/patch/ComparisonType.java
index 260c507..eca2658 100644
--- a/java/com/google/gerrit/server/patch/ComparisonType.java
+++ b/java/com/google/gerrit/server/patch/ComparisonType.java
@@ -16,34 +16,40 @@
import static com.google.gerrit.server.ioutil.BasicSerialization.readVarInt32;
import static com.google.gerrit.server.ioutil.BasicSerialization.writeVarInt32;
-import static java.util.Objects.requireNonNull;
+import com.google.auto.value.AutoValue;
+import com.google.gerrit.server.cache.proto.Cache.FileDiffOutputProto;
import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;
+import java.util.Optional;
-public class ComparisonType {
+/** Relation between the old and new commits used in the diff. */
+@AutoValue
+public abstract class ComparisonType {
- /** 1-based parent */
- private final Integer parentNum;
+ /**
+ * 1-based parent. Available if the old commit is the parent of the new commit and old commit is
+ * not the auto-merge.
+ */
+ abstract Optional<Integer> parentNum();
- private final boolean autoMerge;
+ abstract boolean autoMerge();
public static ComparisonType againstOtherPatchSet() {
- return new ComparisonType(null, false);
+ return new AutoValue_ComparisonType(Optional.empty(), false);
}
public static ComparisonType againstParent(int parentNum) {
- return new ComparisonType(parentNum, false);
+ return new AutoValue_ComparisonType(Optional.of(parentNum), false);
}
public static ComparisonType againstAutoMerge() {
- return new ComparisonType(null, true);
+ return new AutoValue_ComparisonType(Optional.empty(), true);
}
- private ComparisonType(Integer parentNum, boolean autoMerge) {
- this.parentNum = parentNum;
- this.autoMerge = autoMerge;
+ private static ComparisonType create(Optional<Integer> parent, boolean automerge) {
+ return new AutoValue_ComparisonType(parent, automerge);
}
public boolean isAgainstParentOrAutoMerge() {
@@ -51,27 +57,43 @@
}
public boolean isAgainstParent() {
- return parentNum != null;
+ return parentNum().isPresent();
}
public boolean isAgainstAutoMerge() {
- return autoMerge;
+ return autoMerge();
}
- public int getParentNum() {
- requireNonNull(parentNum);
- return parentNum;
+ public Optional<Integer> getParentNum() {
+ return parentNum();
}
void writeTo(OutputStream out) throws IOException {
- writeVarInt32(out, parentNum != null ? parentNum : 0);
- writeVarInt32(out, autoMerge ? 1 : 0);
+ writeVarInt32(out, isAgainstParent() ? parentNum().get() : 0);
+ writeVarInt32(out, autoMerge() ? 1 : 0);
}
static ComparisonType readFrom(InputStream in) throws IOException {
int p = readVarInt32(in);
- Integer parentNum = p > 0 ? p : null;
+ Optional<Integer> parentNum = p > 0 ? Optional.of(p) : Optional.empty();
boolean autoMerge = readVarInt32(in) != 0;
- return new ComparisonType(parentNum, autoMerge);
+ return create(parentNum, autoMerge);
+ }
+
+ public FileDiffOutputProto.ComparisonType toProto() {
+ FileDiffOutputProto.ComparisonType.Builder builder =
+ FileDiffOutputProto.ComparisonType.newBuilder().setAutoMerge(autoMerge());
+ if (parentNum().isPresent()) {
+ builder.setParentNum(parentNum().get());
+ }
+ return builder.build();
+ }
+
+ public static ComparisonType fromProto(FileDiffOutputProto.ComparisonType proto) {
+ Optional<Integer> parentNum = Optional.empty();
+ if (proto.hasField(FileDiffOutputProto.ComparisonType.getDescriptor().findFieldByNumber(1))) {
+ parentNum = Optional.of(proto.getParentNum());
+ }
+ return create(parentNum, proto.getAutoMerge());
}
}
diff --git a/java/com/google/gerrit/server/patch/DiffOperations.java b/java/com/google/gerrit/server/patch/DiffOperations.java
index 8b90531..93aefff 100644
--- a/java/com/google/gerrit/server/patch/DiffOperations.java
+++ b/java/com/google/gerrit/server/patch/DiffOperations.java
@@ -74,8 +74,9 @@
/**
* Returns the diff for a single file between a patchset commit against its parent or the
* auto-merge commit. For deleted files, the {@code fileName} parameter should contain the old
- * name of the file. This method will return {@link FileDiffOutput#empty(String)} if the requested
- * file identified by {@code fileName} has unchanged content or does not exist at both commits.
+ * name of the file. This method will return {@link FileDiffOutput#empty(String, ObjectId,
+ * ObjectId)} if the requested file identified by {@code fileName} has unchanged content or does
+ * not exist at both commits.
*
* @param project a project name representing a git repository.
* @param newCommit 20 bytes SHA-1 of the new commit used in the diff.
@@ -98,8 +99,8 @@
/**
* Returns the diff for a single file between two patchset commits. For deleted files, the {@code
* fileName} parameter should contain the old name of the file. This method will return {@link
- * FileDiffOutput#empty(String)} if the requested file identified by {@code fileName} has
- * unchanged content or does not exist at both commits.
+ * FileDiffOutput#empty(String, ObjectId, ObjectId)} if the requested file identified by {@code
+ * fileName} has unchanged content or does not exist at both commits.
*
* @param project a project name representing a git repository.
* @param oldCommit 20 bytes SHA-1 of the old commit used in the diff.
diff --git a/java/com/google/gerrit/server/patch/DiffOperationsImpl.java b/java/com/google/gerrit/server/patch/DiffOperationsImpl.java
index 16bd135..efb64bc 100644
--- a/java/com/google/gerrit/server/patch/DiffOperationsImpl.java
+++ b/java/com/google/gerrit/server/patch/DiffOperationsImpl.java
@@ -125,7 +125,9 @@
FileDiffCacheKey key =
createFileDiffCacheKey(project, diffParams.baseCommit(), newCommit, fileName, whitespace);
Map<String, FileDiffOutput> result = getModifiedFilesForKeys(ImmutableList.of(key));
- return result.containsKey(fileName) ? result.get(fileName) : FileDiffOutput.empty(fileName);
+ return result.containsKey(fileName)
+ ? result.get(fileName)
+ : FileDiffOutput.empty(fileName, key.oldCommit(), key.newCommit());
} catch (IOException e) {
throw new DiffNotAvailableException(
"Failed to evaluate the parent/base commit for commit " + newCommit, e);
@@ -143,7 +145,9 @@
FileDiffCacheKey key =
createFileDiffCacheKey(project, oldCommit, newCommit, fileName, whitespace);
Map<String, FileDiffOutput> result = getModifiedFilesForKeys(ImmutableList.of(key));
- return result.containsKey(fileName) ? result.get(fileName) : FileDiffOutput.empty(fileName);
+ return result.containsKey(fileName)
+ ? result.get(fileName)
+ : FileDiffOutput.empty(fileName, oldCommit, newCommit);
}
private Map<String, FileDiffOutput> getModifiedFiles(
diff --git a/java/com/google/gerrit/server/patch/MagicFile.java b/java/com/google/gerrit/server/patch/MagicFile.java
index aa6b11f..e42dd8c 100644
--- a/java/com/google/gerrit/server/patch/MagicFile.java
+++ b/java/com/google/gerrit/server/patch/MagicFile.java
@@ -93,7 +93,7 @@
}
default:
int uninterestingParent =
- comparisonType.isAgainstParent() ? comparisonType.getParentNum() : 1;
+ comparisonType.isAgainstParent() ? comparisonType.getParentNum().get() : 1;
b.append("Merge List:\n\n");
for (RevCommit commit : MergeListBuilder.build(rw, c, uninterestingParent)) {
diff --git a/java/com/google/gerrit/server/patch/filediff/FileDiffCacheImpl.java b/java/com/google/gerrit/server/patch/filediff/FileDiffCacheImpl.java
index 63f311b..1bb407d 100644
--- a/java/com/google/gerrit/server/patch/filediff/FileDiffCacheImpl.java
+++ b/java/com/google/gerrit/server/patch/filediff/FileDiffCacheImpl.java
@@ -93,6 +93,7 @@
persist(DIFF, FileDiffCacheKey.class, FileDiffOutput.class)
.maximumWeight(10 << 20)
.weigher(FileDiffWeigher.class)
+ .version(2)
.keySerializer(FileDiffCacheKey.Serializer.INSTANCE)
.valueSerializer(FileDiffOutput.Serializer.INSTANCE)
.loader(FileDiffLoader.class);
@@ -219,19 +220,16 @@
RawTextComparator cmp = comparatorFor(key.whitespace());
ComparisonType comparisonType =
getComparisonType(rw, reader, key.oldCommit(), key.newCommit());
- RevCommit aCommit =
- comparisonType.isAgainstParentOrAutoMerge()
- ? null
- : DiffUtil.getRevCommit(rw, key.oldCommit());
+ RevCommit aCommit = DiffUtil.getRevCommit(rw, key.oldCommit());
RevCommit bCommit = DiffUtil.getRevCommit(rw, key.newCommit());
return magicPath == MagicPath.COMMIT
- ? createCommitEntry(reader, aCommit, bCommit, cmp, key.diffAlgorithm())
+ ? createCommitEntry(reader, aCommit, bCommit, comparisonType, cmp, key.diffAlgorithm())
: createMergeListEntry(
reader, aCommit, bCommit, comparisonType, cmp, key.diffAlgorithm());
} catch (IOException e) {
logger.atWarning().log("Failed to compute commit entry for key %s", key);
}
- return FileDiffOutput.empty(key.newFilePath());
+ return FileDiffOutput.empty(key.newFilePath(), key.oldCommit(), key.newCommit());
}
private static RawTextComparator comparatorFor(Whitespace ws) {
@@ -255,13 +253,24 @@
ObjectReader reader,
RevCommit oldCommit,
RevCommit newCommit,
+ ComparisonType comparisonType,
RawTextComparator rawTextComparator,
GitFileDiffCacheImpl.DiffAlgorithm diffAlgorithm)
throws IOException {
- Text aText = oldCommit != null ? Text.forCommit(reader, oldCommit) : Text.EMPTY;
+ Text aText =
+ comparisonType.isAgainstParentOrAutoMerge()
+ ? Text.EMPTY
+ : Text.forCommit(reader, oldCommit);
Text bText = Text.forCommit(reader, newCommit);
return createMagicFileDiffOutput(
- rawTextComparator, oldCommit, aText, bText, Patch.COMMIT_MSG, diffAlgorithm);
+ oldCommit,
+ newCommit,
+ comparisonType,
+ rawTextComparator,
+ aText,
+ bText,
+ Patch.COMMIT_MSG,
+ diffAlgorithm);
}
private FileDiffOutput createMergeListEntry(
@@ -273,20 +282,31 @@
GitFileDiffCacheImpl.DiffAlgorithm diffAlgorithm)
throws IOException {
Text aText =
- oldCommit != null ? Text.forMergeList(comparisonType, reader, oldCommit) : Text.EMPTY;
+ comparisonType.isAgainstParentOrAutoMerge()
+ ? Text.EMPTY
+ : Text.forMergeList(comparisonType, reader, oldCommit);
Text bText = Text.forMergeList(comparisonType, reader, newCommit);
return createMagicFileDiffOutput(
- rawTextComparator, oldCommit, aText, bText, Patch.MERGE_LIST, diffAlgorithm);
+ oldCommit,
+ newCommit,
+ comparisonType,
+ rawTextComparator,
+ aText,
+ bText,
+ Patch.MERGE_LIST,
+ diffAlgorithm);
}
private static FileDiffOutput createMagicFileDiffOutput(
+ ObjectId oldCommit,
+ ObjectId newCommit,
+ ComparisonType comparisonType,
RawTextComparator rawTextComparator,
- RevCommit aCommit,
Text aText,
Text bText,
String fileName,
GitFileDiffCacheImpl.DiffAlgorithm diffAlgorithm) {
- byte[] rawHdr = getRawHeader(aCommit != null, fileName);
+ byte[] rawHdr = getRawHeader(!comparisonType.isAgainstParentOrAutoMerge(), fileName);
byte[] aContent = aText.getContent();
byte[] bContent = bText.getContent();
long size = bContent.length;
@@ -298,6 +318,9 @@
FileHeader fileHeader = new FileHeader(rawHdr, edits, PatchType.UNIFIED);
Patch.ChangeType changeType = FileHeaderUtil.getChangeType(fileHeader);
return FileDiffOutput.builder()
+ .oldCommitId(oldCommit)
+ .newCommitId(newCommit)
+ .comparisonType(comparisonType)
.oldPath(FileHeaderUtil.getOldPath(fileHeader))
.newPath(FileHeaderUtil.getNewPath(fileHeader))
.changeType(changeType)
@@ -370,8 +393,13 @@
mainGitDiff.newPath().get())
: 0;
+ ObjectId oldCommit = augmentedKey.key().oldCommit();
+ ObjectId newCommit = augmentedKey.key().newCommit();
FileDiffOutput fileDiff =
FileDiffOutput.builder()
+ .oldCommitId(oldCommit)
+ .newCommitId(newCommit)
+ .comparisonType(getComparisonType(rw, reader, oldCommit, newCommit))
.changeType(mainGitDiff.changeType())
.patchType(mainGitDiff.patchType())
.oldPath(mainGitDiff.oldPath())
diff --git a/java/com/google/gerrit/server/patch/filediff/FileDiffOutput.java b/java/com/google/gerrit/server/patch/filediff/FileDiffOutput.java
index 3348033..e7f47ef 100644
--- a/java/com/google/gerrit/server/patch/filediff/FileDiffOutput.java
+++ b/java/com/google/gerrit/server/patch/filediff/FileDiffOutput.java
@@ -24,16 +24,28 @@
import com.google.gerrit.proto.Protos;
import com.google.gerrit.server.cache.proto.Cache.FileDiffOutputProto;
import com.google.gerrit.server.cache.serialize.CacheSerializer;
+import com.google.gerrit.server.cache.serialize.ObjectIdConverter;
+import com.google.gerrit.server.patch.ComparisonType;
import com.google.protobuf.Descriptors.FieldDescriptor;
import java.io.Serializable;
import java.util.Optional;
import java.util.stream.Collectors;
+import org.eclipse.jgit.lib.ObjectId;
/** File diff for a single file path. Produced as output of the {@link FileDiffCache}. */
@AutoValue
public abstract class FileDiffOutput implements Serializable {
private static final long serialVersionUID = 1L;
+ /** The 20 bytes SHA-1 object ID of the old git commit used in the diff. */
+ public abstract ObjectId oldCommitId();
+
+ /** The 20 bytes SHA-1 object ID of the new git commit used in the diff. */
+ public abstract ObjectId newCommitId();
+
+ /** Comparison type of old and new commits: against another patchset, parent or auto-merge. */
+ public abstract ComparisonType comparisonType();
+
/**
* The file path at the old commit. Returns an empty Optional if {@link #changeType()} is equal to
* {@link ChangeType#ADDED}.
@@ -95,8 +107,11 @@
}
/** Returns an entity representing an unchanged file between two commits. */
- public static FileDiffOutput empty(String filePath) {
+ public static FileDiffOutput empty(String filePath, ObjectId oldCommitId, ObjectId newCommitId) {
return builder()
+ .oldCommitId(oldCommitId)
+ .newCommitId(newCommitId)
+ .comparisonType(ComparisonType.againstOtherPatchSet()) // not important
.oldPath(Optional.empty())
.newPath(Optional.of(filePath))
.changeType(ChangeType.MODIFIED)
@@ -124,6 +139,8 @@
if (newPath().isPresent()) {
result += stringSize(newPath().get());
}
+ result += 20 + 20; // old and new commit IDs
+ result += 4; // comparison type
result += 4; // changeType
if (patchType().isPresent()) {
result += 4;
@@ -140,6 +157,12 @@
@AutoValue.Builder
public abstract static class Builder {
+ public abstract Builder oldCommitId(ObjectId value);
+
+ public abstract Builder newCommitId(ObjectId value);
+
+ public abstract Builder comparisonType(ComparisonType value);
+
public abstract Builder oldPath(Optional<String> value);
public abstract Builder newPath(Optional<String> value);
@@ -173,8 +196,12 @@
@Override
public byte[] serialize(FileDiffOutput fileDiff) {
+ ObjectIdConverter idConverter = ObjectIdConverter.create();
FileDiffOutputProto.Builder builder =
FileDiffOutputProto.newBuilder()
+ .setOldCommit(idConverter.toByteString(fileDiff.oldCommitId().toObjectId()))
+ .setNewCommit(idConverter.toByteString(fileDiff.newCommitId().toObjectId()))
+ .setComparisonType(fileDiff.comparisonType().toProto())
.setSize(fileDiff.size())
.setSizeDelta(fileDiff.sizeDelta())
.addAllHeaderLines(fileDiff.headerLines())
@@ -212,9 +239,13 @@
@Override
public FileDiffOutput deserialize(byte[] in) {
+ ObjectIdConverter idConverter = ObjectIdConverter.create();
FileDiffOutputProto proto = Protos.parseUnchecked(FileDiffOutputProto.parser(), in);
FileDiffOutput.Builder builder = FileDiffOutput.builder();
builder
+ .oldCommitId(idConverter.fromByteString(proto.getOldCommit()))
+ .newCommitId(idConverter.fromByteString(proto.getNewCommit()))
+ .comparisonType(ComparisonType.fromProto(proto.getComparisonType()))
.size(proto.getSize())
.sizeDelta(proto.getSizeDelta())
.headerLines(proto.getHeaderLinesList().stream().collect(ImmutableList.toImmutableList()))
diff --git a/javatests/com/google/gerrit/server/cache/serialize/entities/FileDiffOutputSerializerTest.java b/javatests/com/google/gerrit/server/cache/serialize/entities/FileDiffOutputSerializerTest.java
index 44ea55a..17fd959 100644
--- a/javatests/com/google/gerrit/server/cache/serialize/entities/FileDiffOutputSerializerTest.java
+++ b/javatests/com/google/gerrit/server/cache/serialize/entities/FileDiffOutputSerializerTest.java
@@ -19,10 +19,12 @@
import com.google.common.collect.ImmutableList;
import com.google.gerrit.entities.Patch.ChangeType;
import com.google.gerrit.entities.Patch.PatchType;
+import com.google.gerrit.server.patch.ComparisonType;
import com.google.gerrit.server.patch.filediff.Edit;
import com.google.gerrit.server.patch.filediff.FileDiffOutput;
import com.google.gerrit.server.patch.filediff.TaggedEdit;
import java.util.Optional;
+import org.eclipse.jgit.lib.ObjectId;
import org.junit.Test;
public class FileDiffOutputSerializerTest {
@@ -35,6 +37,9 @@
FileDiffOutput fileDiff =
FileDiffOutput.builder()
+ .oldCommitId(ObjectId.fromString("dd4d2a1498870ca5fe415b33f65d052d69d9eaf5"))
+ .newCommitId(ObjectId.fromString("0cfaab3f2ba76f71798da0a2651f41be8d45f842"))
+ .comparisonType(ComparisonType.againstOtherPatchSet())
.oldPath(Optional.of("old_file_path.txt"))
.newPath(Optional.empty())
.changeType(ChangeType.DELETED)
diff --git a/polygerrit-ui/app/elements/change/gr-confirm-cherrypick-dialog/gr-confirm-cherrypick-dialog.ts b/polygerrit-ui/app/elements/change/gr-confirm-cherrypick-dialog/gr-confirm-cherrypick-dialog.ts
index 225e3e9..ebcabbf 100644
--- a/polygerrit-ui/app/elements/change/gr-confirm-cherrypick-dialog/gr-confirm-cherrypick-dialog.ts
+++ b/polygerrit-ui/app/elements/change/gr-confirm-cherrypick-dialog/gr-confirm-cherrypick-dialog.ts
@@ -38,6 +38,7 @@
import {AutocompleteSuggestion} from '../../shared/gr-autocomplete/gr-autocomplete';
import {HttpMethod, ChangeStatus} from '../../../constants/constants';
import {dom, EventApi} from '@polymer/polymer/lib/legacy/polymer.dom';
+import {fireEvent} from '../../../utils/event-util';
const SUGGESTIONS_LIMIT = 15;
const CHANGE_SUBJECT_LIMIT = 50;
@@ -264,10 +265,12 @@
_handlecherryPickSingleChangeClicked() {
this._cherryPickType = CherryPickType.SINGLE_CHANGE;
+ fireEvent(this, 'iron-resize');
}
_handlecherryPickTopicClicked() {
this._cherryPickType = CherryPickType.TOPIC;
+ fireEvent(this, 'iron-resize');
}
@observe('changeStatus', 'commitNum', 'commitMessage')
diff --git a/polygerrit-ui/app/elements/change/gr-confirm-cherrypick-dialog/gr-confirm-cherrypick-dialog_html.ts b/polygerrit-ui/app/elements/change/gr-confirm-cherrypick-dialog/gr-confirm-cherrypick-dialog_html.ts
index 4de395c..6c0099e 100644
--- a/polygerrit-ui/app/elements/change/gr-confirm-cherrypick-dialog/gr-confirm-cherrypick-dialog_html.ts
+++ b/polygerrit-ui/app/elements/change/gr-confirm-cherrypick-dialog/gr-confirm-cherrypick-dialog_html.ts
@@ -46,11 +46,12 @@
}
.cherryPickTopicLayout {
display: flex;
+ align-items: center;
+ margin-bottom: var(--spacing-m);
}
.cherryPickSingleChange,
.cherryPickTopic {
margin-left: var(--spacing-m);
- margin-bottom: var(--spacing-m);
}
.cherry-pick-topic-message {
margin-bottom: var(--spacing-m);
diff --git a/polygerrit-ui/app/elements/checks/gr-checks-tab.ts b/polygerrit-ui/app/elements/checks/gr-checks-tab.ts
index e5386f4..9376a03 100644
--- a/polygerrit-ui/app/elements/checks/gr-checks-tab.ts
+++ b/polygerrit-ui/app/elements/checks/gr-checks-tab.ts
@@ -28,18 +28,23 @@
allActions$,
allResults$,
allRuns$,
+ checksPatchsetNumber$,
someProvidersAreLoading$,
} from '../../services/checks/checks-model';
import './gr-checks-runs';
import './gr-checks-results';
import {sharedStyles} from '../../styles/shared-styles';
-import {changeNum$, currentPatchNum$} from '../../services/change/change-model';
-import {NumericChangeId, PatchSetNum} from '../../types/common';
+import {changeNum$, latestPatchNum$} from '../../services/change/change-model';
+import {NumericChangeId, PatchSetNumber} from '../../types/common';
import {
ActionTriggeredEvent,
fireActionTriggered,
} from '../../services/checks/checks-util';
-import {checkRequiredProperty} from '../../utils/common-util';
+import {
+ assertIsDefined,
+ check,
+ checkRequiredProperty,
+} from '../../utils/common-util';
import {RunSelectedEvent} from './gr-checks-runs';
import {ChecksTabState} from '../../types/events';
import {fireAlert} from '../../utils/event-util';
@@ -64,7 +69,10 @@
tabState?: ChecksTabState;
@property()
- currentPatchNum: PatchSetNum | undefined = undefined;
+ checksPatchsetNumber: PatchSetNumber | undefined = undefined;
+
+ @property()
+ latestPatchsetNumber: PatchSetNumber | undefined = undefined;
@property()
changeNum: NumericChangeId | undefined = undefined;
@@ -82,7 +90,8 @@
this.subscribe('runs', allRuns$);
this.subscribe('actions', allActions$);
this.subscribe('results', allResults$);
- this.subscribe('currentPatchNum', currentPatchNum$);
+ this.subscribe('checksPatchsetNumber', checksPatchsetNumber$);
+ this.subscribe('latestPatchsetNumber', latestPatchNum$);
this.subscribe('changeNum', changeNum$);
this.subscribe('someProvidersAreLoading', someProvidersAreLoading$);
@@ -124,7 +133,6 @@
}
render() {
- const ps = `Patchset ${this.currentPatchNum} (Latest)`;
const filteredRuns = this.runs.filter(
r =>
this.selectedRuns.length === 0 ||
@@ -134,13 +142,9 @@
<div class="header">
<div class="left">
<gr-dropdown-list
- value="${ps}"
- .items="${[
- {
- value: `${ps}`,
- text: `${ps}`,
- },
- ]}"
+ value="${this.checksPatchsetNumber}"
+ .items="${this.createPatchsetDropdownItems()}"
+ @value-change="${this.onPatchsetSelected}"
></gr-dropdown-list>
<span ?hidden="${!this.someProvidersAreLoading}">Loading...</span>
</div>
@@ -163,6 +167,24 @@
`;
}
+ private onPatchsetSelected(e: CustomEvent<{value: string}>) {
+ const patchset = Number(e.detail.value);
+ check(!isNaN(patchset), 'selected patchset must be a number');
+ this.checksService.setPatchset(patchset as PatchSetNumber);
+ }
+
+ private createPatchsetDropdownItems() {
+ return Array.from(Array(this.latestPatchsetNumber), (_, i) => {
+ assertIsDefined(this.latestPatchsetNumber, 'latestPatchsetNumber');
+ const index = this.latestPatchsetNumber - i;
+ const postfix = index === this.latestPatchsetNumber ? ' (latest)' : '';
+ return {
+ value: `${index}`,
+ text: `Patchset ${index}${postfix}`,
+ };
+ });
+ }
+
protected updated(changedProperties: PropertyValues) {
super.updated(changedProperties);
if (changedProperties.has('tabState')) {
@@ -181,10 +203,10 @@
handleActionTriggered(action: Action, run?: CheckRun) {
if (!this.changeNum) return;
- if (!this.currentPatchNum) return;
+ if (!this.checksPatchsetNumber) return;
const promise = action.callback(
this.changeNum,
- this.currentPatchNum as number,
+ this.checksPatchsetNumber,
run?.attempt,
run?.externalId,
run?.checkName,
diff --git a/polygerrit-ui/app/services/change/change-model.ts b/polygerrit-ui/app/services/change/change-model.ts
index b2bdcfe..e7472de 100644
--- a/polygerrit-ui/app/services/change/change-model.ts
+++ b/polygerrit-ui/app/services/change/change-model.ts
@@ -46,10 +46,18 @@
// Must only be used by the change service or whatever is in control of this
// model.
export function updateState(change?: ParsedChangeInfo) {
- privateState$.next({
- ...privateState$.getValue(),
- change,
- });
+ const current = privateState$.getValue();
+ // We want to make it easy for subscribers to react to change changes, so we
+ // are explicitly emitting and additional `undefined` when the change number
+ // changes. So if you are subscribed to the latestPatchsetNumber for example,
+ // then you can rely on emissions even if the old and the new change have the
+ // same latestPatchsetNumber.
+ if (change !== undefined && current.change !== undefined) {
+ if (change._number !== current.change._number) {
+ privateState$.next({...current, change: undefined});
+ }
+ }
+ privateState$.next({...current, change});
}
/**
@@ -91,9 +99,6 @@
*
* Note that this selector can emit a patchNum without the change being
* available!
- *
- * TODO: It would be good to assert/enforce somehow that currentPatchNum$ cannot
- * emit 'PARENT'.
*/
export const currentPatchNum$: Observable<
PatchSetNum | undefined
diff --git a/polygerrit-ui/app/services/change/change-service.ts b/polygerrit-ui/app/services/change/change-service.ts
index 4524813..c292fb5 100644
--- a/polygerrit-ui/app/services/change/change-service.ts
+++ b/polygerrit-ui/app/services/change/change-service.ts
@@ -16,21 +16,16 @@
*/
import {routerChangeNum$} from '../router/router-model';
import {updateState} from './change-model';
-import {tap} from 'rxjs/operators';
import {ParsedChangeInfo} from '../../types/types';
export class ChangeService {
- // TODO: In the future we will want to make restApiService.getChangeDetail()
- // calls from a switchMap() here. For now just make sure to invalidate the
- // change when no changeNum is set.
- private routerChangeNumEffect = routerChangeNum$.pipe(
- tap(changeNum => {
- if (!changeNum) updateState(undefined);
- })
- );
-
constructor() {
- this.routerChangeNumEffect.subscribe();
+ // TODO: In the future we will want to make restApiService.getChangeDetail()
+ // calls from a switchMap() here. For now just make sure to invalidate the
+ // change when no changeNum is set.
+ routerChangeNum$.subscribe(changeNum => {
+ if (!changeNum) updateState(undefined);
+ });
}
/**
diff --git a/polygerrit-ui/app/services/checks/checks-model.ts b/polygerrit-ui/app/services/checks/checks-model.ts
index d8dc688..1c5b862 100644
--- a/polygerrit-ui/app/services/checks/checks-model.ts
+++ b/polygerrit-ui/app/services/checks/checks-model.ts
@@ -26,6 +26,7 @@
RunStatus,
} from '../../api/checks';
import {distinctUntilChanged, map} from 'rxjs/operators';
+import {PatchSetNumber} from '../../types/common';
// This is a convenience type for working with results, because when working
// with a bunch of results you will typically also want to know about the run
@@ -41,29 +42,44 @@
}
interface ChecksState {
- [name: string]: ChecksProviderState;
+ patchsetNumber?: PatchSetNumber;
+ providerNameToState: {
+ [name: string]: ChecksProviderState;
+ };
}
-const initialState: ChecksState = {};
+const initialState: ChecksState = {
+ providerNameToState: {},
+};
const privateState$ = new BehaviorSubject(initialState);
// Re-exporting as Observable so that you can only subscribe, but not emit.
export const checksState$: Observable<ChecksState> = privateState$;
-export const aPluginHasRegistered$ = checksState$.pipe(
+export const checksPatchsetNumber$ = checksState$.pipe(
+ map(state => state.patchsetNumber),
+ distinctUntilChanged()
+);
+
+export const checksProviderState$ = checksState$.pipe(
+ map(state => state.providerNameToState),
+ distinctUntilChanged()
+);
+
+export const aPluginHasRegistered$ = checksProviderState$.pipe(
map(state => Object.keys(state).length > 0),
distinctUntilChanged()
);
-export const someProvidersAreLoading$ = checksState$.pipe(
+export const someProvidersAreLoading$ = checksProviderState$.pipe(
map(state => {
return Object.values(state).some(providerState => providerState.loading);
}),
distinctUntilChanged()
);
-export const allActions$ = checksState$.pipe(
+export const allActions$ = checksProviderState$.pipe(
map(state => {
return Object.values(state).reduce(
(allActions: Action[], providerState: ChecksProviderState) => [
@@ -75,7 +91,7 @@
})
);
-export const allRuns$ = checksState$.pipe(
+export const allRuns$ = checksProviderState$.pipe(
map(state => {
return Object.values(state).reduce(
(allRuns: CheckRun[], providerState: ChecksProviderState) => [
@@ -87,7 +103,7 @@
})
);
-export const checkToPluginMap$ = checksState$.pipe(
+export const checkToPluginMap$ = checksProviderState$.pipe(
map(state => {
const map = new Map<string, string>();
for (const [pluginName, providerState] of Object.entries(state)) {
@@ -99,7 +115,7 @@
})
);
-export const allResults$ = checksState$.pipe(
+export const allResults$ = checksProviderState$.pipe(
map(state => {
return Object.values(state)
.reduce(
@@ -124,7 +140,8 @@
config?: ChecksApiConfig
) {
const nextState = {...privateState$.getValue()};
- nextState[pluginName] = {
+ nextState.providerNameToState = {...nextState.providerNameToState};
+ nextState.providerNameToState[pluginName] = {
pluginName,
loading: false,
config,
@@ -200,8 +217,9 @@
export function updateStateSetLoading(pluginName: string) {
const nextState = {...privateState$.getValue()};
- nextState[pluginName] = {
- ...nextState[pluginName],
+ nextState.providerNameToState = {...nextState.providerNameToState};
+ nextState.providerNameToState[pluginName] = {
+ ...nextState.providerNameToState[pluginName],
loading: true,
};
privateState$.next(nextState);
@@ -213,11 +231,18 @@
actions: Action[] = []
) {
const nextState = {...privateState$.getValue()};
- nextState[pluginName] = {
- ...nextState[pluginName],
+ nextState.providerNameToState = {...nextState.providerNameToState};
+ nextState.providerNameToState[pluginName] = {
+ ...nextState.providerNameToState[pluginName],
loading: false,
runs: [...runs],
actions: [...actions],
};
privateState$.next(nextState);
}
+
+export function updateStateSetPatchset(patchsetNumber?: PatchSetNumber) {
+ const nextState = {...privateState$.getValue()};
+ nextState.patchsetNumber = patchsetNumber;
+ privateState$.next(nextState);
+}
diff --git a/polygerrit-ui/app/services/checks/checks-service.ts b/polygerrit-ui/app/services/checks/checks-service.ts
index d2a4775..fd1f810 100644
--- a/polygerrit-ui/app/services/checks/checks-service.ts
+++ b/polygerrit-ui/app/services/checks/checks-service.ts
@@ -16,9 +16,9 @@
*/
import {
+ filter,
switchMap,
takeWhile,
- tap,
throttleTime,
withLatestFrom,
} from 'rxjs/operators';
@@ -29,12 +29,14 @@
FetchResponse,
ResponseCode,
} from '../../api/checks';
-import {change$, currentPatchNum$} from '../change/change-model';
+import {change$, changeNum$, latestPatchNum$} from '../change/change-model';
import {
updateStateSetLoading,
checkToPluginMap$,
updateStateSetProvider,
updateStateSetResults,
+ checksPatchsetNumber$,
+ updateStateSetPatchset,
} from './checks-model';
import {
BehaviorSubject,
@@ -43,19 +45,33 @@
Observable,
of,
Subject,
+ timer,
} from 'rxjs';
+import {PatchSetNumber} from '../../types/common';
export class ChecksService {
private readonly providers: {[name: string]: ChecksProvider} = {};
private readonly reloadSubjects: {[name: string]: Subject<void>} = {};
- private changeAndPatchNum$ = change$.pipe(withLatestFrom(currentPatchNum$));
-
private checkToPluginMap = new Map<string, string>();
+ private readonly documentVisibilityChange$ = new BehaviorSubject(undefined);
+
constructor() {
- checkToPluginMap$.subscribe(x => (this.checkToPluginMap = x));
+ checkToPluginMap$.subscribe(map => {
+ this.checkToPluginMap = map;
+ });
+ latestPatchNum$.subscribe(num => {
+ updateStateSetPatchset(num);
+ });
+ document.addEventListener('visibilitychange', () => {
+ this.documentVisibilityChange$.next(undefined);
+ });
+ }
+
+ setPatchset(num: PatchSetNumber) {
+ updateStateSetPatchset(num);
}
reload(pluginName: string) {
@@ -80,39 +96,52 @@
this.providers[pluginName] = provider;
this.reloadSubjects[pluginName] = new BehaviorSubject<void>(undefined);
updateStateSetProvider(pluginName, config);
- // Both, changed numbers and and announceUpdate request should trigger.
+ const pollIntervalMs = (config?.fetchPollingIntervalSeconds ?? 60) * 1000;
+ // Various events should trigger fetching checks from the provider:
+ // 1. Change number and patchset number changes.
+ // 2. Specific reload requests.
+ // 3. Regular polling starting with an initial fetch right now.
+ // 4. A hidden Gerrit tab becoming visible.
combineLatest([
- this.changeAndPatchNum$,
+ changeNum$,
+ checksPatchsetNumber$,
this.reloadSubjects[pluginName].pipe(throttleTime(1000)),
+ timer(0, pollIntervalMs),
+ this.documentVisibilityChange$,
])
.pipe(
takeWhile(_ => !!this.providers[pluginName]),
+ filter(_ => document.visibilityState !== 'hidden'),
+ withLatestFrom(change$),
switchMap(
- ([[change, patchNum], _]): Observable<FetchResponse> => {
- if (!change || !patchNum || typeof patchNum !== 'number') {
+ ([[changeNum, patchNum], change]): Observable<FetchResponse> => {
+ if (
+ !change ||
+ !changeNum ||
+ !patchNum ||
+ typeof patchNum !== 'number'
+ ) {
return of({
responseCode: ResponseCode.OK,
runs: [],
});
}
const data: ChangeData = {
- changeNumber: change._number,
+ changeNumber: changeNum,
patchsetNumber: patchNum,
repo: change.project,
};
updateStateSetLoading(pluginName);
return from(this.providers[pluginName].fetch(data));
}
- ),
- tap(response => {
- updateStateSetResults(
- pluginName,
- response.runs ?? [],
- response.actions
- );
- })
+ )
)
- .subscribe(() => {});
- this.reload(pluginName);
+ .subscribe(response => {
+ updateStateSetResults(
+ pluginName,
+ response.runs ?? [],
+ response.actions
+ );
+ });
}
}
diff --git a/polygerrit-ui/app/types/common.ts b/polygerrit-ui/app/types/common.ts
index 711b11b..1bfabf8 100644
--- a/polygerrit-ui/app/types/common.ts
+++ b/polygerrit-ui/app/types/common.ts
@@ -76,6 +76,7 @@
export type ParsedJSON = BrandType<unknown, '_parsedJSON'>;
export type PatchSetNum = BrandType<'PARENT' | 'edit' | number, '_patchSet'>;
+export type PatchSetNumber = BrandType<number, '_patchSet'>;
export const EditPatchSetNum = 'edit' as PatchSetNum;
// TODO(TS): This is not correct, it is better to have a separate ApiPatchSetNum
diff --git a/polygerrit-ui/app/utils/patch-set-util.ts b/polygerrit-ui/app/utils/patch-set-util.ts
index 40e3eef..af56798 100644
--- a/polygerrit-ui/app/utils/patch-set-util.ts
+++ b/polygerrit-ui/app/utils/patch-set-util.ts
@@ -3,11 +3,12 @@
ChangeInfo,
PatchSetNum,
EditPatchSetNum,
- BrandType,
ParentPatchSetNum,
+ PatchSetNumber,
} from '../types/common';
import {RestApiService} from '../services/gr-rest-api/gr-rest-api';
import {EditRevisionInfo, ParsedChangeInfo} from '../types/types';
+import {check} from './common-util';
/**
* @license
@@ -82,9 +83,7 @@
return patchset as PatchSetNum;
}
-export function isNumber(
- psn: PatchSetNum
-): psn is BrandType<number, '_patchSet'> {
+export function isNumber(psn: PatchSetNum): psn is PatchSetNumber {
return typeof psn === 'number';
}
@@ -250,14 +249,16 @@
export function computeLatestPatchNum(
allPatchSets?: PatchSet[]
-): PatchSetNum | undefined {
+): PatchSetNumber | undefined {
if (!allPatchSets || !allPatchSets.length) {
return undefined;
}
- if (allPatchSets[0].num === EditPatchSetNum) {
- return allPatchSets[1].num;
+ let latest = allPatchSets[0].num;
+ if (latest === EditPatchSetNum) {
+ latest = allPatchSets[1].num;
}
- return allPatchSets[0].num;
+ check(isNumber(latest), 'Latest patchset cannot be EDIT or PARENT.');
+ return latest;
}
export function computePredecessor(
diff --git a/proto/cache.proto b/proto/cache.proto
index 4fd037d..a610e49 100644
--- a/proto/cache.proto
+++ b/proto/cache.proto
@@ -617,7 +617,7 @@
// Serialized form of
// com.google.gerrit.server.patch.filediff.FileDiffOutput
-// Next ID: 9
+// Next ID: 12
message FileDiffOutputProto {
// Next ID: 5
message Edit {
@@ -632,6 +632,11 @@
Edit edit = 1;
bool due_to_rebase = 2;
}
+ // Next ID: 3
+ message ComparisonType {
+ int32 parent_num = 1;
+ bool auto_merge = 2;
+ }
string old_path = 1;
string new_path = 2;
string change_type = 3;
@@ -640,4 +645,7 @@
int64 size = 6;
int64 size_delta = 7;
repeated TaggedEdit edits = 8;
+ bytes old_commit = 9;
+ bytes new_commit = 10;
+ ComparisonType comparison_type = 11;
}
