Merge "Add 'has:hashtag' search operator"
diff --git a/java/com/google/gerrit/server/patch/gitfilediff/GitFileDiff.java b/java/com/google/gerrit/server/patch/gitfilediff/GitFileDiff.java
index 580aef5..23b0024 100644
--- a/java/com/google/gerrit/server/patch/gitfilediff/GitFileDiff.java
+++ b/java/com/google/gerrit/server/patch/gitfilediff/GitFileDiff.java
@@ -32,6 +32,7 @@
import com.google.gerrit.server.patch.filediff.Edit;
import com.google.protobuf.Descriptors.FieldDescriptor;
import java.util.Optional;
+import org.eclipse.jgit.attributes.Attribute;
import org.eclipse.jgit.diff.DiffEntry;
import org.eclipse.jgit.lib.AbbreviatedObjectId;
import org.eclipse.jgit.lib.FileMode;
@@ -66,8 +67,17 @@
* parameters.
*/
static GitFileDiff create(DiffEntry diffEntry, FileHeader fileHeader) {
+ Attribute diffAttr = diffEntry.getDiffAttribute();
+ // Treat the file as binary if .gitattributes explicitly unsets diffing (e.g. "-diff"
+ // or the "binary" macro which JGit expands to "-diff -merge -text").
+ boolean isBinary = diffAttr != null && diffAttr.getState() == Attribute.State.UNSET;
+
ImmutableList<Edit> edits =
- fileHeader.toEditList().stream().map(Edit::fromJGitEdit).collect(toImmutableList());
+ isBinary
+ ? ImmutableList.of()
+ : fileHeader.toEditList().stream().map(Edit::fromJGitEdit).collect(toImmutableList());
+
+ PatchType patchType = isBinary ? PatchType.BINARY : FileHeaderUtil.getPatchType(fileHeader);
return builder()
.edits(edits)
@@ -77,7 +87,7 @@
.oldPath(FileHeaderUtil.getOldPath(fileHeader))
.newPath(FileHeaderUtil.getNewPath(fileHeader))
.changeType(FileHeaderUtil.getChangeType(fileHeader))
- .patchType(Optional.of(FileHeaderUtil.getPatchType(fileHeader)))
+ .patchType(Optional.of(patchType))
.oldMode(Optional.of(mapFileMode(diffEntry.getOldMode())))
.newMode(Optional.of(mapFileMode(diffEntry.getNewMode())))
.build();
diff --git a/javatests/com/google/gerrit/server/patch/DiffOperationsTest.java b/javatests/com/google/gerrit/server/patch/DiffOperationsTest.java
index 7c8555e..1234262 100644
--- a/javatests/com/google/gerrit/server/patch/DiffOperationsTest.java
+++ b/javatests/com/google/gerrit/server/patch/DiffOperationsTest.java
@@ -20,6 +20,7 @@
import com.google.common.collect.ImmutableList;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.entities.Patch.ChangeType;
+import com.google.gerrit.entities.Patch.PatchType;
import com.google.gerrit.entities.Project;
import com.google.gerrit.entities.RefNames;
import com.google.gerrit.server.git.GitRepositoryManager;
@@ -423,6 +424,66 @@
}
}
+ @Test
+ public void gitattributesDiffOverride() throws Exception {
+ String jsonFile = "file_2.json";
+ // 1. First, let's create a commit where the file is modified but there is no .gitattributes.
+ // It should be diffed as UNIFIED.
+ ObjectId oldCommitId1 =
+ createCommit(repo, null, ImmutableList.of(new FileEntity(jsonFile, "{}")));
+ ObjectId newCommitId1 =
+ createCommit(
+ repo, oldCommitId1, ImmutableList.of(new FileEntity(jsonFile, "{\"foo\": \"bar\"}")));
+ FileDiffOutput diffOutput1 =
+ diffOperations.getModifiedFileAgainstParent(
+ testProjectName, newCommitId1, 0, jsonFile, null);
+ assertThat(diffOutput1.patchType()).hasValue(PatchType.UNIFIED);
+ assertThat(diffOutput1.edits()).isNotEmpty();
+
+ // 2. Now let's create a commit where .gitattributes specifies "-diff" for the file.
+ // It should be treated as BINARY.
+ ObjectId oldCommitId2 =
+ createCommit(
+ repo,
+ null,
+ ImmutableList.of(
+ new FileEntity(".gitattributes", jsonFile + " -diff"),
+ new FileEntity(jsonFile, "{}")));
+ ObjectId newCommitId2 =
+ createCommit(
+ repo,
+ oldCommitId2,
+ ImmutableList.of(
+ new FileEntity(".gitattributes", jsonFile + " -diff"),
+ new FileEntity(jsonFile, "{\"foo\": \"bar\"}")));
+ FileDiffOutput diffOutput2 =
+ diffOperations.getModifiedFileAgainstParent(
+ testProjectName, newCommitId2, 0, jsonFile, null);
+ assertThat(diffOutput2.patchType()).hasValue(PatchType.BINARY);
+ assertThat(diffOutput2.edits()).isEmpty();
+
+ // 3. Let's also test with "binary" macro attribute.
+ ObjectId oldCommitId3 =
+ createCommit(
+ repo,
+ null,
+ ImmutableList.of(
+ new FileEntity(".gitattributes", jsonFile + " binary"),
+ new FileEntity(jsonFile, "{}")));
+ ObjectId newCommitId3 =
+ createCommit(
+ repo,
+ oldCommitId3,
+ ImmutableList.of(
+ new FileEntity(".gitattributes", jsonFile + " binary"),
+ new FileEntity(jsonFile, "{\"foo\": \"bar\"}")));
+ FileDiffOutput diffOutput3 =
+ diffOperations.getModifiedFileAgainstParent(
+ testProjectName, newCommitId3, 0, jsonFile, null);
+ assertThat(diffOutput3.patchType()).hasValue(PatchType.BINARY);
+ assertThat(diffOutput3.edits()).isEmpty();
+ }
+
static class FileEntity {
String name;
String content;
diff --git a/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread.ts b/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread.ts
index 3020bff..5e8cdd7 100644
--- a/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread.ts
+++ b/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread.ts
@@ -633,6 +633,18 @@
>
`
: nothing}
+ ${this.shouldShowDisagreeButton()
+ ? html`
+ <gr-button
+ id="disagreeBtn"
+ link
+ class="action disagree"
+ ?disabled=${this.saving}
+ @click=${this.handleCommentDisagree}
+ >Disagree</gr-button
+ >
+ `
+ : nothing}
<gr-button
id="ackBtn"
link
@@ -967,6 +979,14 @@
);
}
+ protected handleCommentDisagree() {
+ this.createReplyComment(
+ 'Disagree.',
+ /* userWantsToEdit= */ false,
+ /* unresolved= */ false
+ );
+ }
+
private handleReplyToComment(e: ReplyToCommentEvent) {
const {content, userWantsToEdit, unresolved} = e.detail;
this.createReplyComment(content, userWantsToEdit, unresolved);
@@ -1034,6 +1054,17 @@
return this.isOwner && !hasUserSuggestion(comment);
}
+ protected shouldShowDisagreeButton(): boolean {
+ return !!(
+ this.thread &&
+ this.account &&
+ this.unresolved &&
+ this.thread.comments.length === 1 &&
+ this.isOwner &&
+ this.thread.comments[0]?.is_ai
+ );
+ }
+
private handleAppliedFix(fixSuggestion?: FixSuggestionInfo) {
const message = this.getLastComment()?.message;
assert(!!message, 'empty message');
diff --git a/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread_screenshot_test.ts b/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread_screenshot_test.ts
index 9731c4e8..bf06c20 100644
--- a/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread_screenshot_test.ts
+++ b/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread_screenshot_test.ts
@@ -114,6 +114,15 @@
await visualDiffDarkTheme(element, 'gr-comment-thread-with-ai');
});
+ test('with AI comment author disagree', async () => {
+ element.isOwner = true;
+ element.thread = createThread({...c_ai, unresolved: true});
+ await element.updateComplete;
+
+ await visualDiff(element, 'gr-comment-thread-with-ai-disagree');
+ await visualDiffDarkTheme(element, 'gr-comment-thread-with-ai-disagree');
+ });
+
test('unresolved inline code review', async () => {
// Simulate inline context by setting custom properties
element.style.setProperty('--gr-comment-thread-width', '100%');
diff --git a/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread_test.ts b/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread_test.ts
index 8b5e166..d40490f 100644
--- a/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread_test.ts
+++ b/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread_test.ts
@@ -872,4 +872,52 @@
]);
});
});
+
+ suite('Disagree button', () => {
+ setup(async () => {
+ element.isOwner = true;
+ element.account = createAccountDetailWithId(13);
+ element.thread = createThread({...c1, is_ai: true, unresolved: true});
+ await element.updateComplete;
+ });
+
+ test('renders with unresolved AI comment when owner', async () => {
+ assert.isOk(query(element, '#disagreeBtn'));
+ });
+
+ test('does not show disagree button if comment is not AI', async () => {
+ element.thread = createThread({...c1, is_ai: false, unresolved: true});
+ await element.updateComplete;
+ assert.isNotOk(query(element, '#disagreeBtn'));
+ });
+
+ test('does not show disagree button if user is not change owner', async () => {
+ element.isOwner = false;
+ await element.updateComplete;
+ assert.isNotOk(query(element, '#disagreeBtn'));
+ });
+
+ test('does not show disagree button if thread has more than 1 comment', async () => {
+ element.thread = createThread(
+ {...c1, is_ai: true, unresolved: true},
+ createComment()
+ );
+ await element.updateComplete;
+ assert.isNotOk(query(element, '#disagreeBtn'));
+ });
+
+ test('handleCommentDisagree creates a "Disagree." reply', async () => {
+ const createReplyCommentSpy = sinon.spy(
+ element as unknown as {createReplyComment: () => void},
+ 'createReplyComment'
+ );
+ queryAndAssert<GrButton>(element, '#disagreeBtn').click();
+ assert.isTrue(createReplyCommentSpy.calledOnce);
+ assert.deepEqual(createReplyCommentSpy.firstCall.args, [
+ 'Disagree.',
+ false,
+ false,
+ ]);
+ });
+ });
});
diff --git a/polygerrit-ui/app/models/checks/checks-model.ts b/polygerrit-ui/app/models/checks/checks-model.ts
index c986efe..3fbc552 100644
--- a/polygerrit-ui/app/models/checks/checks-model.ts
+++ b/polygerrit-ui/app/models/checks/checks-model.ts
@@ -321,9 +321,18 @@
);
this.checksLatest$ = select(this.state$, state => state.pluginStateLatest);
this.checksSelected$ = select(
- combineLatest([this.state$, this.changeViewModel.checksPatchset$]),
- ([state, ps]) => {
- const checksPs = ps ? ChecksPatchset.SELECTED : ChecksPatchset.LATEST;
+ combineLatest([
+ this.state$,
+ this.changeViewModel.checksPatchset$,
+ this.changeModel.latestPatchNum$,
+ ]),
+ ([state, ps, latestPs]) => {
+ // When no distinct patchset is selected SELECTED fetch is skipped
+ // (see initFetchingOfData), so fall back to the LATEST state here.
+ const checksPs =
+ ps && ps !== latestPs
+ ? ChecksPatchset.SELECTED
+ : ChecksPatchset.LATEST;
return this.getPluginState(state, checksPs);
}
);
@@ -865,29 +874,54 @@
this.reloadSubjects[pluginName],
pollIntervalMs === 0 ? from([0]) : timer(0, pollIntervalMs),
this.documentVisibilityChange$,
+ // Only the SELECTED subscription needs the latest patchset here, to
+ // detect when it coincides with the latest (see below). The LATEST
+ // subscription already has it as its second element, so feeding it in
+ // again would make combineLatest emit twice per change.
+ patchset === ChecksPatchset.SELECTED
+ ? this.changeModel.latestPatchNum$
+ : of(undefined),
])
.pipe(
takeWhile(_ => !!this.providers[pluginName]),
filter(_ => document.visibilityState !== 'hidden'),
throttleTime(500, undefined, {leading: true, trailing: true}),
- switchMap(([change, patchNum]): Observable<FetchResponse> => {
- if (!change || !patchNum) return of(this.empty());
- if (typeof patchNum !== 'number') return of(this.empty());
- assertIsDefined(change.revisions, 'change.revisions');
- const patchsetSha = getShaByPatchNum(change.revisions, patchNum);
- // Sometimes patchNum is updated earlier than change, so change
- // revisions don't have patchNum yet
- if (!patchsetSha) return of(this.empty());
- const data: ChangeData = {
- changeNumber: change?._number,
- patchsetNumber: patchNum,
- patchsetSha,
- repo: change.project,
- commitMessage: getCurrentRevision(change)?.commit?.message,
- changeInfo: change as ChangeInfo,
- };
- return this.fetchResults(pluginName, data, patchset);
- }),
+ switchMap(
+ ([
+ change,
+ patchNum,
+ ,
+ ,
+ ,
+ latestPatchNum,
+ ]): Observable<FetchResponse> => {
+ if (!change || !patchNum) return of(this.empty());
+ if (typeof patchNum !== 'number') return of(this.empty());
+ // Skip the duplicate fetch when the selected patchset is the
+ // latest: the LATEST subscription already fetches it and
+ // checksSelected$ falls back to that state.
+ if (
+ patchset === ChecksPatchset.SELECTED &&
+ patchNum === latestPatchNum
+ ) {
+ return of(this.empty());
+ }
+ assertIsDefined(change.revisions, 'change.revisions');
+ const patchsetSha = getShaByPatchNum(change.revisions, patchNum);
+ // Sometimes patchNum is updated earlier than change, so change
+ // revisions don't have patchNum yet
+ if (!patchsetSha) return of(this.empty());
+ const data: ChangeData = {
+ changeNumber: change?._number,
+ patchsetNumber: patchNum,
+ patchsetSha,
+ repo: change.project,
+ commitMessage: getCurrentRevision(change)?.commit?.message,
+ changeInfo: change as ChangeInfo,
+ };
+ return this.fetchResults(pluginName, data, patchset);
+ }
+ ),
catchError(e => {
// This should not happen and is really severe, because it means that
// the Observable has terminated and we won't recover from that. No
diff --git a/polygerrit-ui/app/models/checks/checks-model_test.ts b/polygerrit-ui/app/models/checks/checks-model_test.ts
index 94b3d0f..368b7e6 100644
--- a/polygerrit-ui/app/models/checks/checks-model_test.ts
+++ b/polygerrit-ui/app/models/checks/checks-model_test.ts
@@ -17,6 +17,7 @@
import {
Action,
Category,
+ ChangeData,
CheckRun,
ChecksApiConfig,
ChecksProvider,
@@ -27,9 +28,11 @@
import {
createCheckResult,
createParsedChange,
+ createRevisions,
createRun,
+ getCurrentRevision,
} from '../../test/test-data-generators';
-import {waitUntil, waitUntilCalled} from '../../test/test-utils';
+import {waitEventLoop, waitUntil, waitUntilCalled} from '../../test/test-utils';
import {ParsedChangeInfo} from '../../types/types';
import {
changeModelToken,
@@ -38,7 +41,11 @@
import {assert} from '@open-wc/testing';
import {testResolver} from '../../test/common-test-setup';
import {changeViewModelToken} from '../views/change';
-import {NumericChangeId, PatchSetNumber} from '../../api/rest-api';
+import {
+ NumericChangeId,
+ PatchSetNumber,
+ RevisionPatchSetNum,
+} from '../../api/rest-api';
import {pluginLoaderToken} from '../../elements/shared/gr-js-api-interface/gr-plugin-loader';
import {deepEqual} from '../../utils/deep-util';
@@ -79,6 +86,29 @@
};
}
+/**
+ * A provider that echoes back the patchset it was asked to fetch, so that tests
+ * can assert which patchset a tab is populated with.
+ */
+function createPatchsetTaggingProvider(): ChecksProvider {
+ return {
+ fetch: (data: ChangeData) =>
+ Promise.resolve({
+ responseCode: ResponseCode.OK,
+ runs: [createRun({patchset: data.patchsetNumber})],
+ }),
+ };
+}
+
+/** A change with two patchsets, so latest (2) and older (1) are distinct. */
+function createTwoPatchsetChange(): ParsedChangeInfo {
+ return updateRevisionsWithCommitShas({
+ ...createParsedChange(),
+ revisions: createRevisions(2),
+ current_revision: getCurrentRevision(1),
+ })!;
+}
+
suite('checks-model tests', () => {
let model: ChecksModel;
@@ -130,6 +160,130 @@
clock.restore();
});
+ test('no duplicate fetch when viewing latest patchset (no selection)', async () => {
+ const clock = sinon.useFakeTimers({shouldClearNativeTimers: true});
+ let change: ParsedChangeInfo | undefined = undefined;
+ testResolver(changeModelToken).change$.subscribe(c => (change = c));
+ let latestRuns: CheckRun[] = [];
+ let selectedRuns: CheckRun[] = [];
+ model.allRunsLatestPatchset$.subscribe(r => (latestRuns = r));
+ model.allRunsSelectedPatchset$.subscribe(r => (selectedRuns = r));
+ const provider = createPatchsetTaggingProvider();
+ const fetchSpy = sinon.spy(provider, 'fetch');
+
+ model.register({
+ pluginName: PLUGIN_NAME,
+ provider,
+ config: CONFIG_POLLING_NONE,
+ });
+ await waitUntil(() => change === undefined);
+
+ // Viewing the latest patchset (2), no explicit checks patchset override.
+ testResolver(changeViewModelToken).updateState({
+ patchNum: 2 as RevisionPatchSetNum,
+ });
+ const testChange = createTwoPatchsetChange();
+ testResolver(changeModelToken).updateStateChange(testChange);
+ await waitUntil(() => deepEqual(change, testChange));
+
+ // Fire the throttled emission, then flush the fetch promise into state.
+ clock.tick(600);
+ await waitEventLoop();
+
+ // The SELECTED patchset equals LATEST, so only a single fetch is needed.
+ assert.equal(fetchSpy.callCount, 1);
+ // Both tabs show data for the latest patchset (2).
+ assert.isNotEmpty(latestRuns);
+ assert.isNotEmpty(selectedRuns);
+ assert.isTrue(latestRuns.every(r => r.patchset === 2));
+ assert.isTrue(selectedRuns.every(r => r.patchset === 2));
+
+ clock.restore();
+ });
+
+ test('no duplicate fetch when latest patchset is explicitly selected', async () => {
+ const clock = sinon.useFakeTimers({shouldClearNativeTimers: true});
+ let change: ParsedChangeInfo | undefined = undefined;
+ testResolver(changeModelToken).change$.subscribe(c => (change = c));
+ let latestRuns: CheckRun[] = [];
+ let selectedRuns: CheckRun[] = [];
+ model.allRunsLatestPatchset$.subscribe(r => (latestRuns = r));
+ model.allRunsSelectedPatchset$.subscribe(r => (selectedRuns = r));
+ const provider = createPatchsetTaggingProvider();
+ const fetchSpy = sinon.spy(provider, 'fetch');
+
+ model.register({
+ pluginName: PLUGIN_NAME,
+ provider,
+ config: CONFIG_POLLING_NONE,
+ });
+ await waitUntil(() => change === undefined);
+
+ // Viewing patchset 1 but explicitly selecting the latest patchset (2) in the
+ // checks tab. checksPatchset differs from patchNum, so it is not reset.
+ testResolver(changeViewModelToken).updateState({
+ patchNum: 1 as RevisionPatchSetNum,
+ checksPatchset: 2 as PatchSetNumber,
+ });
+ const testChange = createTwoPatchsetChange();
+ testResolver(changeModelToken).updateStateChange(testChange);
+ await waitUntil(() => deepEqual(change, testChange));
+
+ // Fire the throttled emission, then flush the fetch promise into state.
+ clock.tick(600);
+ await waitEventLoop();
+
+ // Selected patchset (2) equals latest, so still only a single fetch.
+ assert.equal(fetchSpy.callCount, 1);
+ assert.isNotEmpty(latestRuns);
+ assert.isNotEmpty(selectedRuns);
+ assert.isTrue(latestRuns.every(r => r.patchset === 2));
+ assert.isTrue(selectedRuns.every(r => r.patchset === 2));
+
+ clock.restore();
+ });
+
+ test('fetches both patchsets when an older one is selected', async () => {
+ const clock = sinon.useFakeTimers({shouldClearNativeTimers: true});
+ let change: ParsedChangeInfo | undefined = undefined;
+ testResolver(changeModelToken).change$.subscribe(c => (change = c));
+ let latestRuns: CheckRun[] = [];
+ let selectedRuns: CheckRun[] = [];
+ model.allRunsLatestPatchset$.subscribe(r => (latestRuns = r));
+ model.allRunsSelectedPatchset$.subscribe(r => (selectedRuns = r));
+ const provider = createPatchsetTaggingProvider();
+ const fetchSpy = sinon.spy(provider, 'fetch');
+
+ model.register({
+ pluginName: PLUGIN_NAME,
+ provider,
+ config: CONFIG_POLLING_NONE,
+ });
+ await waitUntil(() => change === undefined);
+
+ // Explicitly selecting the older patchset (1); latest is 2.
+ testResolver(changeViewModelToken).updateState({
+ checksPatchset: 1 as PatchSetNumber,
+ });
+ const testChange = createTwoPatchsetChange();
+ testResolver(changeModelToken).updateStateChange(testChange);
+ await waitUntil(() => deepEqual(change, testChange));
+
+ // Fire the throttled emission, then flush the fetch promise into state.
+ clock.tick(600);
+ await waitEventLoop();
+
+ // Distinct patchsets require two fetches: one for latest, one for selected.
+ assert.equal(fetchSpy.callCount, 2);
+ // The latest tab shows patchset 2, the selected tab shows the older 1.
+ assert.isNotEmpty(latestRuns);
+ assert.isNotEmpty(selectedRuns);
+ assert.isTrue(latestRuns.every(r => r.patchset === 2));
+ assert.isTrue(selectedRuns.every(r => r.patchset === 1));
+
+ clock.restore();
+ });
+
test('fetch throttle', async () => {
const clock = sinon.useFakeTimers({shouldClearNativeTimers: true});
let change: ParsedChangeInfo | undefined = undefined;
diff --git a/polygerrit-ui/screenshots/Chromium/baseline/gr-comment-thread-with-ai-disagree-dark.png b/polygerrit-ui/screenshots/Chromium/baseline/gr-comment-thread-with-ai-disagree-dark.png
new file mode 100644
index 0000000..025d1be
--- /dev/null
+++ b/polygerrit-ui/screenshots/Chromium/baseline/gr-comment-thread-with-ai-disagree-dark.png
Binary files differ
diff --git a/polygerrit-ui/screenshots/Chromium/baseline/gr-comment-thread-with-ai-disagree.png b/polygerrit-ui/screenshots/Chromium/baseline/gr-comment-thread-with-ai-disagree.png
new file mode 100644
index 0000000..7e1a2af
--- /dev/null
+++ b/polygerrit-ui/screenshots/Chromium/baseline/gr-comment-thread-with-ai-disagree.png
Binary files differ
