Further refine the revision parents UI
<gr-revision-parents> is a new (flag protected) component that shows
additional information about the parents (aka bases) when comparing
two patchsets. The component is shown above the file list and below
the patchset picker on the change page. A small amount of information
is also added to the patchset picker (<gr-patch-range-select>).
This is the outcome of discussions with Arnab and Youssef in
go/revision-parents.
This change is still flag protected, so no users are directly
affected.
I would prefer to have detailed UX discussions once this is live in
canary.
It is expected that this still needs some testing and polish.
Example screenshot: https://imgur.com/a/vhR40hj
Release-Notes: skip
Google-Bug-Id: b/289177174
Change-Id: I2dd65f8bde157235875c938fb13bb2b3555f5a41
diff --git a/polygerrit-ui/app/elements/change/gr-commit-info/gr-commit-info.ts b/polygerrit-ui/app/elements/change/gr-commit-info/gr-commit-info.ts
index 73521a6..1285664 100644
--- a/polygerrit-ui/app/elements/change/gr-commit-info/gr-commit-info.ts
+++ b/polygerrit-ui/app/elements/change/gr-commit-info/gr-commit-info.ts
@@ -20,6 +20,7 @@
import {createSearchUrl} from '../../../models/views/search';
import {getBrowseCommitWeblink} from '../../../utils/weblink-util';
import {shorten} from '../../../utils/patch-set-util';
+import {when} from 'lit/directives/when.js';
declare global {
interface HTMLElementTagNameMap {
@@ -31,7 +32,10 @@
export class GrCommitInfo extends LitElement {
// TODO(TS): Maybe limit to StandaloneCommitInfo.
@property({type: Object})
- commitInfo?: CommitInfo;
+ commitInfo?: Partial<CommitInfo>;
+
+ @property({type: Boolean})
+ showCopyButton = true;
@state() serverConfig?: ServerInfo;
@@ -45,6 +49,9 @@
align-items: center;
display: flex;
}
+ gr-weblink {
+ margin-right: 0;
+ }
`,
];
}
@@ -63,13 +70,18 @@
if (!commit) return nothing;
return html` <div class="container">
<gr-weblink imageAndText .info=${this.getWeblink(commit)}></gr-weblink>
- <gr-copy-clipboard
- hastooltip
- .buttonTitle=${'Copy full SHA to clipboard'}
- hideinput
- .text=${commit}
- >
- </gr-copy-clipboard>
+ ${when(
+ this.showCopyButton,
+ () => html`
+ <gr-copy-clipboard
+ hastooltip
+ .buttonTitle=${'Copy full SHA to clipboard'}
+ hideinput
+ .text=${commit}
+ >
+ </gr-copy-clipboard>
+ `
+ )}
</div>`;
}
@@ -86,6 +98,6 @@
this.serverConfig
);
if (primaryLink) return {...primaryLink, name};
- return {name, url: createSearchUrl({query: name})};
+ return {name, url: createSearchUrl({query: commit})};
}
}
diff --git a/polygerrit-ui/app/elements/change/gr-commit-info/gr-commit-info_test.ts b/polygerrit-ui/app/elements/change/gr-commit-info/gr-commit-info_test.ts
index 7bc4e60..3fed767 100644
--- a/polygerrit-ui/app/elements/change/gr-commit-info/gr-commit-info_test.ts
+++ b/polygerrit-ui/app/elements/change/gr-commit-info/gr-commit-info_test.ts
@@ -79,7 +79,11 @@
assert.shadowDom.equal(
weblink,
/* HTML */ `
- <a href="/q/sha4567" rel="noopener noreferrer" target="_blank">
+ <a
+ href="/q/sha45678901234567890"
+ rel="noopener noreferrer"
+ target="_blank"
+ >
<gr-tooltip-content>
<span> sha4567 </span>
</gr-tooltip-content>
diff --git a/polygerrit-ui/app/elements/change/gr-revision-parents/gr-revision-parents.ts b/polygerrit-ui/app/elements/change/gr-revision-parents/gr-revision-parents.ts
index 7cfebf1..72c2e43 100644
--- a/polygerrit-ui/app/elements/change/gr-revision-parents/gr-revision-parents.ts
+++ b/polygerrit-ui/app/elements/change/gr-revision-parents/gr-revision-parents.ts
@@ -4,21 +4,34 @@
* SPDX-License-Identifier: Apache-2.0
*/
import {customElement, state} from 'lit/decorators.js';
-import {css, html, LitElement} from 'lit';
+import {css, html, HTMLTemplateResult, LitElement} from 'lit';
import {resolve} from '../../../models/dependency';
import {subscribe} from '../../lit/subscription-controller';
import {changeModelToken} from '../../../models/change/change-model';
-import {EDIT, RevisionInfo} from '../../../api/rest-api';
+import {
+ CommitId,
+ EDIT,
+ NumericChangeId,
+ ParentInfo,
+ PatchSetNumber,
+ RepoName,
+ RevisionInfo,
+} from '../../../api/rest-api';
import {fontStyles} from '../../../styles/gr-font-styles';
-import {branchName, shorten} from '../../../utils/patch-set-util';
+import {branchName} from '../../../utils/patch-set-util';
import {when} from 'lit/directives/when.js';
+import {createChangeUrl} from '../../../models/views/change';
@customElement('gr-revision-parents')
export class GrRevisionParents extends LitElement {
+ @state() repo?: RepoName;
+
@state() revision?: RevisionInfo;
@state() baseRevision?: RevisionInfo;
+ @state() showDetails = false;
+
private readonly getChangeModel = resolve(this, changeModelToken);
constructor() {
@@ -33,6 +46,11 @@
);
subscribe(
this,
+ () => this.getChangeModel().repo$,
+ x => (this.repo = x)
+ );
+ subscribe(
+ this,
() => this.getChangeModel().baseRevision$,
x => (this.baseRevision = x)
);
@@ -50,7 +68,7 @@
border-top: 1px solid var(--border-color);
background-color: var(--yellow-50);
}
- .flex {
+ .sections {
display: flex;
}
.section {
@@ -63,53 +81,312 @@
.title {
font-weight: var(--font-weight-bold);
}
+ .messageContainer {
+ display: flex;
+ padding: var(--spacing-m) var(--spacing-l);
+ border-top: 1px solid var(--border-color);
+ }
+ .messageContainer.info {
+ background-color: var(--info-background);
+ }
+ .messageContainer.warning {
+ background-color: var(--warning-background);
+ }
+ .messageContainer gr-icon {
+ margin-right: var(--spacing-m);
+ }
+ .messageContainer.info gr-icon {
+ color: var(--info-foreground);
+ }
+ .messageContainer.warning gr-icon {
+ color: var(--warning-foreground);
+ }
+ .messageContainer .text {
+ max-width: 600px;
+ }
+ .messageContainer .text p {
+ margin: 0;
+ }
+ .messageContainer .text gr-button {
+ margin-left: -4px;
+ }
+ gr-commit-info {
+ display: inline-block;
+ }
`,
];
}
override render() {
- // TODO(revision-parents): Figure out what to do about multiple parents.
- const baseParent = this.baseRevision?.parents_data?.[0];
- const parent = this.revision?.parents_data?.[0];
- if (!parent || !baseParent) return;
- // TODO(revision-parents): Design something nicer for the various cases.
+ return html`${this.renderMessage()}${this.renderDetails()}`;
+ }
+
+ private renderMessage() {
+ if (!this.baseRevision || !this.revision) return;
+ // For merges we are only interested in the target branch parent, which is [0].
+ // And for non-merges there is no more than 1 parent, so [0] is the only choice.
+ const parentLeft = this.baseRevision?.parents_data?.[0];
+ const parentRight = this.revision?.parents_data?.[0];
+ // Note that is you diff a patchset against its base, then baseRevision will be
+ // `undefined`. Thus after this line we know that we are dealing with diffs
+ // of the type "patchset x vs patchset y".
+ if (!parentLeft || !parentRight) return;
+
+ const psLeft = this.baseRevision?._number;
+ const psRight = this.revision?._number;
+ const parentCommitLeft = parentLeft.commit_id;
+ const parentCommitRight = parentRight.commit_id;
+ const branchLeft = branchName(parentLeft.branch_name);
+ const branchRight = branchName(parentRight.branch_name);
+ const isMergedLeft = parentLeft.is_merged_in_target_branch;
+ const isMergedRight = parentRight.is_merged_in_target_branch;
+ const changeNumLeft = parentLeft.change_number;
+ const changeNumRight = parentRight.change_number;
+ const changePsLeft = parentLeft.patch_set_number;
+ const changePsRight = parentRight.patch_set_number;
+
+ if (parentCommitLeft === parentCommitRight) return;
+
+ // Subsequently: different commit
+
+ if (branchLeft !== branchRight) {
+ return html`
+ ${this.renderWarning(
+ 'warning',
+ html`
+ Patchset ${psLeft} and ${psRight} are targeting different branches.
+ `
+ )}
+ `;
+ }
+
+ // Subsequently: different commit, same target branch
+
+ // Such a situation is really rare and weird. You have to do something like committing to one
+ // branch and then uploading to another. This warning should actually also be shown, if
+ // you are not comparing PS X and PS Y, because it is generally a weird patchset state.
+ const isWeirdLeft = !isMergedLeft && !changeNumLeft;
+ const isWeirdRight = !isMergedRight && !changeNumRight;
+ if (isWeirdLeft || isWeirdRight) {
+ const weirdPs =
+ isWeirdLeft && isWeirdRight
+ ? `${psLeft} and ${psRight} are`
+ : isWeirdLeft
+ ? `${psLeft} is`
+ : `${psRight} is`;
+ return html`
+ ${this.renderWarning(
+ 'warning',
+ html`
+ Patchset ${weirdPs} based on a commit that neither exists in its
+ target branch, nor is it a commit of another active change.
+ `
+ )}
+ `;
+ }
+
+ if (
+ changeNumLeft &&
+ changeNumRight &&
+ changeNumLeft === changeNumRight &&
+ // This check is probably redundant, because "same change and ps" should mean "same commit".
+ psLeft !== psRight
+ ) {
+ return html`
+ ${this.renderWarning(
+ 'info',
+ html`
+ The change was rebased from patchset
+ ${this.renderPatchsetLink(changeNumLeft, changePsLeft)} onto
+ patchset ${this.renderPatchsetLink(changeNumLeft, changePsRight)} of
+ change ${this.renderChangeLink(changeNumLeft)}
+ ${when(isMergedRight, () => html` (MERGED)`)}.
+ `
+ )}
+ `;
+ }
+
+ // No additional info? Then "different commit" and "same branch" means "standard rebase".
+ if (isMergedLeft && isMergedRight) {
+ return html`
+ ${this.renderWarning(
+ 'info',
+ html`
+ The change was rebased from
+ ${this.renderCommitLink(parentCommitLeft, false)} onto
+ ${this.renderCommitLink(parentCommitRight, false)}.
+ `
+ )}
+ `;
+ }
+
+ // By now we know that we have different commit, same target branch, no weird parent,
+ // and not a standard rebase. So let's spell out what the left and right side are based on.
+ return this.renderWarning(
+ 'warning',
+ html`${this.renderInfo(this.baseRevision)}<br />${this.renderInfo(
+ this.revision
+ )}`
+ );
+ }
+
+ private renderInfo(rev: RevisionInfo) {
+ const parent = rev.parents_data?.[0];
+ if (!parent) return;
+ const ps = rev._number;
+ const isMerged = parent.is_merged_in_target_branch;
+ const changeNum = parent.change_number;
+
+ if (changeNum && !isMerged) {
+ return html`
+ Patchset ${ps} is based on patchset
+ ${this.renderPatchsetLink(changeNum, parent.patch_set_number)} of change
+ ${this.renderChangeLink(changeNum)}.
+ `;
+ } else {
+ return html`
+ Patchset ${ps} is based on commit
+ ${this.renderCommitLink(parent.commit_id, false)} in the target branch
+ (${branchName(parent.branch_name)}).
+ `;
+ }
+ }
+
+ private renderWarning(icon: string, message: HTMLTemplateResult) {
+ const isWarning = icon === 'warning';
+ return html`
+ <div class="messageContainer ${icon}">
+ <div class="icon">
+ <gr-icon icon=${icon}></gr-icon>
+ </div>
+ <div class="text">
+ <p>
+ ${message}${when(
+ isWarning,
+ () => html`
+ <br />
+ The diff below may not be meaningful and may even be hiding
+ relevant changes.
+ `
+ )}
+ </p>
+ ${when(
+ isWarning,
+ () => html`
+ <p>
+ <gr-button
+ link
+ @click=${() => (this.showDetails = !this.showDetails)}
+ >${this.showDetails ? 'Hide' : 'Show'} details</gr-button
+ >
+ </p>
+ `
+ )}
+ </div>
+ </div>
+ `;
+ }
+
+ private renderDetails() {
+ if (!this.showDetails) return;
+ if (!this.baseRevision || !this.revision) return;
+ const parentLeft = this.baseRevision.parents_data?.[0];
+ const parentRight = this.revision.parents_data?.[0];
+ if (!parentRight || !parentLeft) return;
+
return html`
<div class="container">
- <div class="flex">
- <div class="section">
- <h4 class="heading-4">Patchset ${this.baseRevision?._number}</h4>
- <div>Branch: ${branchName(parent.branch_name)}</div>
- <div>Commit: ${shorten(parent.commit_id)}</div>
- <div>Is Merged: ${parent.is_merged_in_target_branch}</div>
- ${when(
- !!parent.change_number,
- () => html` <div>
- Change ID: ${parent.change_id?.substring(0, 10)}
- </div>
- <div>Change Number: ${parent.change_number}</div>
- <div>Patchset Number: ${parent.patch_set_number}</div>
- <div>Change Status: ${parent.change_status}</div>`
- )}
- </div>
- <div class="section">
- <h4 class="heading-4">Patchset ${this.revision?._number}</h4>
- <div>Branch: ${branchName(baseParent.branch_name)}</div>
- <div>Commit: ${shorten(baseParent.commit_id)}</div>
- <div>Is Merged: ${baseParent.is_merged_in_target_branch}</div>
- ${when(
- !!baseParent.change_number,
- () => html`<div>
- Change ID: ${baseParent.change_id?.substring(0, 10)}
- </div>
- <div>Change Number: ${baseParent.change_number}</div>
- <div>Patchset Number: ${baseParent.patch_set_number}</div>
- <div>Change Status: ${baseParent.change_status}</div>`
- )}
- </div>
+ <div class="sections">
+ ${this.renderSection(
+ this.baseRevision,
+ parentLeft,
+ parentLeft.change_number === parentRight.change_number
+ )}
+ ${this.renderSection(
+ this.revision,
+ parentRight,
+ parentLeft.change_number === parentRight.change_number
+ )}
</div>
</div>
`;
}
+
+ private renderCommitLink(commit?: CommitId, showCopyButton = true) {
+ if (!commit) return;
+ return html`<gr-commit-info
+ .commitInfo=${{commit}}
+ .showCopyButton=${showCopyButton}
+ ></gr-commit-info>`;
+ }
+
+ private renderChangeLink(changeNum: NumericChangeId) {
+ return html`
+ <a href=${createChangeUrl({changeNum, repo: this.repo!})}>${changeNum}</a>
+ `;
+ }
+
+ private renderPatchsetLink(
+ changeNum: NumericChangeId,
+ patchNum?: PatchSetNumber
+ ) {
+ if (!patchNum) return;
+ return html`
+ <a
+ href=${createChangeUrl({
+ changeNum,
+ repo: this.repo!,
+ patchNum,
+ })}
+ >${patchNum}</a
+ >
+ `;
+ }
+
+ private renderSection(
+ revision: RevisionInfo,
+ parent: ParentInfo,
+ sameChange: boolean
+ ) {
+ const ps = revision._number;
+ const commit = parent.commit_id;
+ const branch = branchName(parent.branch_name);
+ const isMerged = parent.is_merged_in_target_branch;
+ const changeNum = parent.change_number as NumericChangeId;
+ const changePs = parent.patch_set_number;
+
+ createChangeUrl({changeNum, repo: this.repo!});
+
+ return html`
+ <div class="section">
+ <h4 class="heading-4">Patchset ${ps}</h4>
+ <div>Target branch: ${branch}</div>
+ <div>Base commit: ${this.renderCommitLink(commit)}</div>
+ ${when(
+ !changeNum && !isMerged,
+ () => html`
+ <div>
+ <gr-icon icon="warning"></gr-icon>
+ <span
+ >Warning: The base commit is not known (aka reachable) in the
+ target branch.</span
+ >
+ </div>
+ `
+ )}
+ ${when(
+ changeNum && (sameChange || !isMerged),
+ () => html`
+ <div>
+ Base change: ${this.renderChangeLink(changeNum)}, patchset
+ ${this.renderPatchsetLink(changeNum, changePs)}
+ ${when(isMerged, () => html`(MERGED)`)}
+ </div>
+ `
+ )}
+ </div>
+ `;
+ }
}
declare global {
diff --git a/polygerrit-ui/app/elements/change/gr-revision-parents/gr-revision-parents_test.ts b/polygerrit-ui/app/elements/change/gr-revision-parents/gr-revision-parents_test.ts
index a1b9fdd..7b97550 100644
--- a/polygerrit-ui/app/elements/change/gr-revision-parents/gr-revision-parents_test.ts
+++ b/polygerrit-ui/app/elements/change/gr-revision-parents/gr-revision-parents_test.ts
@@ -13,12 +13,68 @@
ChangeStatus,
CommitId,
NumericChangeId,
+ ParentInfo,
PatchSetNumber,
} from '../../../api/rest-api';
+import {queryAll} from '../../../utils/common-util';
+
+const PARENT_DEFAULT: ParentInfo = {
+ branch_name: 'refs/heads/master',
+ commit_id: '78e52ce873b1c08396422f51ad6aacf77ed95541' as CommitId,
+ is_merged_in_target_branch: true,
+};
+
+const PARENT_REBASED: ParentInfo = {
+ ...PARENT_DEFAULT,
+ commit_id: '00002ce873b1c08396422f51ad6aacf77ed95541' as CommitId,
+};
+
+const PARENT_OTHER_BRANCH: ParentInfo = {
+ ...PARENT_DEFAULT,
+ branch_name: 'refs/heads/otherbranch',
+ commit_id: '11112ce873b1c08396422f51ad6aacf77ed95541' as CommitId,
+};
+
+const PARENT_WEIRD: ParentInfo = {
+ ...PARENT_DEFAULT,
+ commit_id: '22222ce873b1c08396422f51ad6aacf77ed95541' as CommitId,
+ is_merged_in_target_branch: false,
+};
+
+const PARENT_CHANGE_123_1: ParentInfo = {
+ ...PARENT_DEFAULT,
+ commit_id: '12312ce873b1c08396422f51ad6aacf77ed95541' as CommitId,
+ is_merged_in_target_branch: false,
+ change_id: 'Idc69e6d7bba0ce0a9a0bdcd22adb506c0b76e628' as ChangeId,
+ change_number: 123 as NumericChangeId,
+ patch_set_number: 1 as PatchSetNumber,
+ change_status: ChangeStatus.NEW,
+};
+
+const PARENT_CHANGE_123_2: ParentInfo = {
+ ...PARENT_CHANGE_123_1,
+ commit_id: '12322ce873b1c08396422f51ad6aacf77ed95541' as CommitId,
+ patch_set_number: 2 as PatchSetNumber,
+};
suite('gr-revision-parents tests', () => {
let element: GrRevisionParents;
+ const setParents = async (
+ parentLeft: ParentInfo,
+ parentRight: ParentInfo
+ ) => {
+ element.baseRevision = {
+ ...createRevision(1),
+ parents_data: [parentLeft],
+ };
+ element.revision = {
+ ...createRevision(2),
+ parents_data: [parentRight],
+ };
+ await element.updateComplete;
+ };
+
setup(async () => {
element = await fixture(html`<gr-revision-parents></gr-revision-parents>`);
await element.updateComplete;
@@ -28,65 +84,155 @@
assert.shadowDom.equal(element, '');
});
- test('render', async () => {
- element.baseRevision = {
- ...createRevision(1),
- parents_data: [
- {
- branch_name: 'refs/heads/master',
- commit_id: '78e52ce873b1c08396422f51ad6aacf77ed95541' as CommitId,
- is_merged_in_target_branch: false,
- change_id: 'Idc69e6d7bba0ce0a9a0bdcd22adb506c0b76e628' as ChangeId,
- change_number: 1500 as NumericChangeId,
- patch_set_number: 1 as PatchSetNumber,
- change_status: ChangeStatus.NEW,
- },
- ],
- };
- element.revision = {
- ...createRevision(2),
- parents_data: [
- {
- branch_name: 'refs/heads/master',
- commit_id: '78e52ce873b1c08396422f51ad6aacf77ed95541' as CommitId,
- is_merged_in_target_branch: false,
- change_id: 'Idc69e6d7bba0ce0a9a0bdcd22adb506c0b76e628' as ChangeId,
- change_number: 1500 as NumericChangeId,
- patch_set_number: 2 as PatchSetNumber,
- change_status: ChangeStatus.NEW,
- },
- ],
- };
- await element.updateComplete;
-
- assert.shadowDom.equal(
- element,
+ test('render details: PARENT_DEFAULT', async () => {
+ element.showDetails = true;
+ await setParents(PARENT_DEFAULT, PARENT_DEFAULT);
+ assert.dom.equal(
+ queryAll(element, '.section')[0],
/* HTML */ `
- <div class="container">
- <div class="flex">
- <div class="section">
- <h4 class="heading-4">Patchset 1</h4>
- <div>Branch: master</div>
- <div>Commit: 78e52ce</div>
- <div>Is Merged: false</div>
- <div>Change ID: Idc69e6d7b</div>
- <div>Change Number: 1500</div>
- <div>Patchset Number: 2</div>
- <div>Change Status: NEW</div>
- </div>
- <div class="section">
- <h4 class="heading-4">Patchset 2</h4>
- <div>Branch: master</div>
- <div>Commit: 78e52ce</div>
- <div>Is Merged: false</div>
- <div>Change ID: Idc69e6d7b</div>
- <div>Change Number: 1500</div>
- <div>Patchset Number: 1</div>
- <div>Change Status: NEW</div>
- </div>
+ <div class="section">
+ <h4 class="heading-4">Patchset 1</h4>
+ <div>Target branch: master</div>
+ <div>
+ Base commit:
+ <gr-commit-info> </gr-commit-info>
</div>
</div>
`
);
});
+
+ test('render details: PARENT_WEIRD', async () => {
+ element.showDetails = true;
+ await setParents(PARENT_WEIRD, PARENT_WEIRD);
+ assert.dom.equal(
+ queryAll(element, '.section')[0],
+ `
+ <div class="section">
+ <h4 class="heading-4">Patchset 1</h4>
+ <div>Target branch: master</div>
+ <div>
+ Base commit:
+ <gr-commit-info> </gr-commit-info>
+ </div>
+ <div>
+ <gr-icon icon="warning"> </gr-icon>
+ <span>
+ Warning: The base commit is not known (aka reachable) in the
+ target branch.
+ </span>
+ </div>
+ </div>
+ `
+ );
+ });
+
+ test('render details: PARENT_CHANGE_123_1', async () => {
+ element.showDetails = true;
+ await setParents(PARENT_CHANGE_123_1, PARENT_CHANGE_123_2);
+ assert.dom.equal(
+ queryAll(element, '.section')[0],
+ /* HTML */ `
+ <div class="section">
+ <h4 class="heading-4">Patchset 1</h4>
+ <div>Target branch: master</div>
+ <div>
+ Base commit:
+ <gr-commit-info> </gr-commit-info>
+ </div>
+ <div>
+ Base change:
+ <a href="/c/123"> 123 </a>
+ , patchset
+ <a href="/c/123/1"> 1 </a>
+ </div>
+ </div>
+ `
+ );
+ });
+
+ test('render message PARENT_DEFAULT vs PARENT_DEFAULT', async () => {
+ await setParents(PARENT_DEFAULT, PARENT_DEFAULT);
+ assert.shadowDom.equal(element, '');
+ });
+
+ test('render message PARENT_DEFAULT vs PARENT_OTHER_BRANCH', async () => {
+ await setParents(PARENT_DEFAULT, PARENT_OTHER_BRANCH);
+ assert.shadowDom.equal(
+ element,
+ `<div class="messageContainer warning">
+ <div class="icon"><gr-icon icon="warning"></gr-icon></div>
+ <div class="text"><p>
+ Patchset 1 and 2 are targeting different branches.<br/>
+ The diff below may not be meaningful and may even be hiding
+ relevant changes.
+ </p><p><gr-button link="">Show details</gr-button></p></div></div>`
+ );
+ });
+
+ test('render message PARENT_DEFAULT vs PARENT_WEIRD', async () => {
+ await setParents(PARENT_DEFAULT, PARENT_WEIRD);
+ assert.shadowDom.equal(
+ element,
+ `<div class="messageContainer warning">
+ <div class="icon"><gr-icon icon="warning"></gr-icon></div>
+ <div class="text"><p>
+ Patchset 2 is based on a commit that neither exists in its
+ target branch, nor is it a commit of another active change.<br/>
+ The diff below may not be meaningful and may even be hiding
+ relevant changes.
+ </p><p><gr-button link="">Show details</gr-button></p></div></div>`
+ );
+ });
+
+ test('render message PARENT_DEFAULT vs PARENT_REBASED', async () => {
+ await setParents(PARENT_DEFAULT, PARENT_REBASED);
+ assert.shadowDom.equal(
+ element,
+ `<div class="messageContainer info">
+ <div class="icon"><gr-icon icon="info"></gr-icon></div>
+ <div class="text"><p>
+ The change was rebased from <gr-commit-info></gr-commit-info>
+ onto <gr-commit-info></gr-commit-info>.
+ </p></div></div>`
+ );
+ });
+
+ test('render message PARENT_CHANGE_123_1 vs PARENT_CHANGE_123_2', async () => {
+ await setParents(PARENT_CHANGE_123_1, PARENT_CHANGE_123_2);
+ assert.shadowDom.equal(
+ element,
+ `<div class="messageContainer info">
+ <div class="icon"><gr-icon icon="info"></gr-icon></div>
+ <div class="text"><p>
+ The change was rebased from patchset
+ <a href="/c/123/1">1</a> onto
+ patchset
+ <a href="/c/123/2">2</a> of
+ change
+ <a href="/c/123">123</a>.
+ </p></div></div>`
+ );
+ });
+
+ test('render message PARENT_DEFAULT vs PARENT_CHANGE_123_1', async () => {
+ await setParents(PARENT_DEFAULT, PARENT_CHANGE_123_1);
+ assert.shadowDom.equal(
+ element,
+ `<div class="messageContainer warning">
+ <div class="icon"><gr-icon icon="warning"></gr-icon></div>
+ <div class="text"><p>
+ Patchset 1 is based on commit
+ <gr-commit-info></gr-commit-info>
+ in the target branch
+ (master).<br>
+ Patchset 2 is based on patchset
+ <a href="/c/123/1">1</a>
+ of change
+ <a href="/c/123">123</a>.<br>
+ The diff below may not be meaningful and may even be hiding
+ relevant changes.
+ </p><p><gr-button link="">Show details</gr-button></p></div></div>`
+ );
+ });
});
diff --git a/polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select.ts b/polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select.ts
index 6a05d86..6bf1adc 100644
--- a/polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select.ts
+++ b/polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select.ts
@@ -18,6 +18,7 @@
convertToPatchSetNum,
getParentInfoString,
shorten,
+ getParentCommit,
} from '../../../utils/patch-set-util';
import {ReportingService} from '../../../services/gr-reporting/gr-reporting';
import {
@@ -276,7 +277,8 @@
KnownExperimentId.REVISION_PARENTS_DATA
);
dropdownContent.push({
- text: isMerge ? 'Auto Merge' : 'Base',
+ triggerText: isMerge ? 'Auto Merge' : 'Base',
+ text: isMerge ? 'Auto Merge' : `Base | ${getParentCommit(rev, 0)}`,
bottomText:
showParentsData && !isMerge ? getParentInfoString(rev, 0) : undefined,
value: PARENT,
@@ -286,7 +288,7 @@
dropdownContent.push({
disabled: idx >= parentCount,
triggerText: `Parent ${idx + 1}`,
- text: `Parent ${idx + 1}`,
+ text: `Parent ${idx + 1} | ${getParentCommit(rev, idx)}`,
bottomText: showParentsData ? getParentInfoString(rev, idx) : undefined,
mobileText: `Parent ${idx + 1}`,
value: -(idx + 1),
diff --git a/polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select_test.ts b/polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select_test.ts
index 0f2dd1e..e7ed97b 100644
--- a/polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select_test.ts
+++ b/polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select_test.ts
@@ -180,7 +180,8 @@
commentThreads: [],
} as DropdownItem,
{
- text: 'Base',
+ text: 'Base | ',
+ triggerText: 'Base',
value: PARENT,
bottomText: undefined,
} as DropdownItem,
diff --git a/polygerrit-ui/app/utils/patch-set-util.ts b/polygerrit-ui/app/utils/patch-set-util.ts
index dffc091..7a5cd45 100644
--- a/polygerrit-ui/app/utils/patch-set-util.ts
+++ b/polygerrit-ui/app/utils/patch-set-util.ts
@@ -327,24 +327,32 @@
return branch as BranchName;
}
-export function getParentInfoString(
+export function getParentCommit(
rev?: RevisionInfo | EditRevisionInfo,
index?: number
) {
const parents = rev?.parents_data ?? [];
const parent = parents[index ?? 0];
if (!parent) return '';
+ return shorten(parent.commit_id) ?? '';
+}
- let info = '';
- if (parent.change_number) {
- info = `${info}Change ${parent.change_number} at patchset ${parent.patch_set_number}\n`;
- }
+export function getParentInfoString(
+ rev?: RevisionInfo | EditRevisionInfo,
+ index?: number
+) {
+ const parents = rev?.parents_data ?? [];
+ const parent = parents[index ?? 0];
+ if (!parent || parent.is_merged_in_target_branch) return '';
- if (parent.branch_name) {
- const commit = shorten(parent.commit_id) ?? 'unknown';
- info = `${info}Branch ${branchName(
- parent.branch_name
- )} at commit ${commit} `;
+ if (index === 0) {
+ if (parent.change_number) {
+ return `Patchset ${parent.patch_set_number} of Change ${parent.change_number}`;
+ } else {
+ return 'Warning: The base commit is not known (aka reachable) in the target branch.';
+ }
+ } else {
+ // For merge changes the parents with index > 0 are expected to be from a different branch.
+ return 'Other branch';
}
- return info;
}
