From a744e2303fccd6a158aa2e0ce79521890ae45e16 Mon Sep 17 00:00:00 2001 From: Wyatt Allen Date: Wed, 8 Nov 2017 15:33:48 -0800 Subject: [PATCH] Support for parent diff bases in merge changes With this change, in merge changes, patch ranges can diff against a specific parent (as indicated by a negative parent index) similarly to the GWT UI. - Parent options appear in patch range selectors when available. - The router no longer redirects parent indexed patch ranges now that they are supported. - The RevisionInfo class is added to house revision related functions in a form that's easily passed between components. - On merge changes the default patch range base is labeled as "AutoMerge" rather than "Base". Feature: Issue 4760 Change-Id: I221ba97e28be52f225f7d90f5f8c5a0f17ddb8c2 --- .../gr-change-view/gr-change-view_test.html | 14 +-- .../gr-file-list-header.html | 2 + .../gr-file-list-header.js | 8 ++ .../app/elements/core/gr-router/gr-router.js | 8 +- .../core/gr-router/gr-router_test.html | 10 --- .../diff/gr-diff-view/gr-diff-view.html | 2 + .../diff/gr-diff-view/gr-diff-view.js | 8 ++ .../diff/gr-diff-view/gr-diff-view_test.html | 16 ++-- .../gr-patch-range-select.js | 40 ++++++++- .../gr-patch-range-select_test.html | 68 ++++++++++----- .../gr-rest-api-interface.js | 1 + .../shared/revision-info/revision-info.html | 79 +++++++++++++++++ .../revision-info/revision-info_test.html | 85 +++++++++++++++++++ polygerrit-ui/app/test/index.html | 1 + 14 files changed, 285 insertions(+), 57 deletions(-) create mode 100644 polygerrit-ui/app/elements/shared/revision-info/revision-info.html create mode 100644 polygerrit-ui/app/elements/shared/revision-info/revision-info_test.html diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.html b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.html index f04f6643c6..cb7da081f3 100644 --- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.html +++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.html @@ -226,7 +226,7 @@ limitations under the License. change_id: 'Iad9dc96274af6946f3632be53b106ef80f7ba6ca', _number: 42, revisions: { - rev1: {_number: 1}, + rev1: {_number: 1, commit: {parents: []}}, }, current_revision: 'rev1', status: 'NEW', @@ -408,10 +408,10 @@ limitations under the License. element._change = { change_id: 'Iad9dc96274af6946f3632be53b106ef80f7ba6ca', revisions: { - rev2: {_number: 2}, - rev1: {_number: 1}, - rev13: {_number: 13}, - rev3: {_number: 3}, + rev2: {_number: 2, commit: {parents: []}}, + rev1: {_number: 1, commit: {parents: []}}, + rev13: {_number: 13, commit: {parents: []}}, + rev3: {_number: 3, commit: {parents: []}}, }, current_revision: 'rev3', status: 'NEW', @@ -914,8 +914,8 @@ limitations under the License. element._change = { change_id: 'Iad9dc96274af6946f3632be53b106ef80f7ba6ca', revisions: { - rev1: {_number: 1}, - rev2: {_number: 2}, + rev1: {_number: 1, commit: {parents: []}}, + rev2: {_number: 2, commit: {parents: []}}, }, current_revision: 'rev1', status: element.ChangeStatus.MERGED, diff --git a/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header.html b/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header.html index f46af09f05..4b158d3744 100644 --- a/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header.html +++ b/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header.html @@ -26,6 +26,7 @@ limitations under the License. + @@ -155,6 +156,7 @@ limitations under the License. base-patch-num="[[basePatchNum]]" available-patches="[[allPatchSets]]" revisions="[[change.revisions]]" + revision-info="[[_revisionInfo]]" on-patch-range-change="_handlePatchChange"> diff --git a/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header.js b/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header.js index 123b15649b..9db173e871 100644 --- a/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header.js +++ b/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header.js @@ -65,6 +65,10 @@ UNIFIED: 'UNIFIED_DIFF', }, }, + _revisionInfo: { + type: Object, + computed: '_getRevisionInfo(change)', + }, }, behaviors: [ @@ -212,5 +216,9 @@ } return 'patchInfoOldPatchSet'; }, + + _getRevisionInfo(change) { + return new Gerrit.RevisionInfo(change); + }, }); })(); diff --git a/polygerrit-ui/app/elements/core/gr-router/gr-router.js b/polygerrit-ui/app/elements/core/gr-router/gr-router.js index c31855a21b..ff353cc201 100644 --- a/polygerrit-ui/app/elements/core/gr-router/gr-router.js +++ b/polygerrit-ui/app/elements/core/gr-router/gr-router.js @@ -412,14 +412,8 @@ // Diffing a patch against itself is invalid, so if the base and revision // patches are equal clear the base. - // NOTE: while selecting numbered parents of a merge is not yet - // implemented, normalize parent base patches to be un-selected parents in - // the same way. - // TODO(issue 4760): Remove the isMergeParent check when PG supports - // diffing against numbered parents of a merge. if (hasBasePatchNum && - (this.patchNumEquals(params.basePatchNum, params.patchNum) || - this.isMergeParent(params.basePatchNum))) { + this.patchNumEquals(params.basePatchNum, params.patchNum)) { needsRedirect = true; params.basePatchNum = null; } else if (hasBasePatchNum && !hasPatchNum) { diff --git a/polygerrit-ui/app/elements/core/gr-router/gr-router_test.html b/polygerrit-ui/app/elements/core/gr-router/gr-router_test.html index 2e86f73804..7821db4ed9 100644 --- a/polygerrit-ui/app/elements/core/gr-router/gr-router_test.html +++ b/polygerrit-ui/app/elements/core/gr-router/gr-router_test.html @@ -493,16 +493,6 @@ limitations under the License. assert.isNotOk(params.basePatchNum); assert.equal(params.patchNum, 'edit'); }); - - // TODO(issue 4760): Remove when PG supports diffing against numbered - // parents of a merge. - test('range -n..m normalizes to m', () => { - const params = {basePatchNum: -2, patchNum: 4}; - const needsRedirect = element._normalizePatchRangeParams(params); - assert.isTrue(needsRedirect); - assert.isNotOk(params.basePatchNum); - assert.equal(params.patchNum, 4); - }); }); }); diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.html b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.html index 48b6964772..39ff9bb9ef 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.html +++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.html @@ -26,6 +26,7 @@ limitations under the License. + @@ -227,6 +228,7 @@ limitations under the License. files-weblinks="[[_filesWeblinks]]" available-patches="[[_allPatchSets]]" revisions="[[_change.revisions]]" + revision-info="[[_revisionInfo]]" on-patch-range-change="_handlePatchChange"> diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.js b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.js index 953ae1a57c..f0b70c173e 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.js +++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.js @@ -147,6 +147,10 @@ type: Array, computed: 'computeAllPatchSets(_change, _change.revisions.*)', }, + _revisionInfo: { + type: Object, + computed: '_getRevisionInfo(_change)', + }, }, behaviors: [ @@ -869,5 +873,9 @@ _computeBlameLoaderClass(isImageDiff, supported) { return !isImageDiff && supported ? 'show' : ''; }, + + _getRevisionInfo(change) { + return new Gerrit.RevisionInfo(change); + }, }); })(); diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.html b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.html index 5e23ea362a..ccf986e923 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.html +++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.html @@ -85,7 +85,7 @@ limitations under the License. element._change = { _number: 42, revisions: { - a: {_number: 10}, + a: {_number: 10, commit: {parents: []}}, }, }; element._fileList = ['chell.go', 'glados.txt', 'wheatley.md']; @@ -170,8 +170,8 @@ limitations under the License. element._change = { _number: 42, revisions: { - a: {_number: 10}, - b: {_number: 5}, + a: {_number: 10, commit: {parents: []}}, + b: {_number: 5, commit: {parents: []}}, }, }; element._fileList = ['chell.go', 'glados.txt', 'wheatley.md']; @@ -234,8 +234,8 @@ limitations under the License. element._change = { _number: 42, revisions: { - a: {_number: 1}, - b: {_number: 2}, + a: {_number: 1, commit: {parents: []}}, + b: {_number: 2, commit: {parents: []}}, }, }; element._fileList = ['chell.go', 'glados.txt', 'wheatley.md']; @@ -407,7 +407,7 @@ limitations under the License. element._change = { _number: 42, revisions: { - a: {_number: 10}, + a: {_number: 10, commit: {parents: []}}, }, }; element._fileList = ['chell.go', 'glados.txt', 'wheatley.md']; @@ -448,8 +448,8 @@ limitations under the License. element._change = { _number: 42, revisions: { - a: {_number: 5}, - b: {_number: 10}, + a: {_number: 5, commit: {parents: []}}, + b: {_number: 10, commit: {parents: []}}, }, }; element._fileList = ['chell.go', 'glados.txt', 'wheatley.md']; diff --git a/polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select.js b/polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select.js index 9ac44517e8..341cb554cc 100644 --- a/polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select.js +++ b/polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select.js @@ -34,7 +34,7 @@ _baseDropdownContent: { type: Object, computed: '_computeBaseDropdownContent(availablePatches, patchNum,' + - '_sortedRevisions, changeComments)', + '_sortedRevisions, changeComments, revisionInfo)', }, _patchDropdownContent: { type: Object, @@ -48,6 +48,7 @@ patchNum: String, basePatchNum: String, revisions: Object, + revisionInfo: Object, _sortedRevisions: Array, }, @@ -58,7 +59,13 @@ behaviors: [Gerrit.PatchSetBehavior], _computeBaseDropdownContent(availablePatches, patchNum, _sortedRevisions, - changeComments) { + changeComments, revisionInfo) { + const parentCounts = revisionInfo.getParentCountMap(); + const currentParentCount = parentCounts.hasOwnProperty(patchNum) ? + parentCounts[patchNum] : 1; + const maxParents = revisionInfo.getMaxParents(); + const isMerge = currentParentCount > 1; + const dropdownContent = []; for (const basePatch of availablePatches) { const basePatchNum = basePatch.num; @@ -75,10 +82,22 @@ value: basePatch.num, }); } + dropdownContent.push({ - text: 'Base', + text: isMerge ? 'Auto Merge' : 'Base', value: 'PARENT', }); + + for (let idx = 0; isMerge && idx < maxParents; idx++) { + dropdownContent.push({ + disabled: idx >= currentParentCount, + triggerText: `Parent ${idx + 1}`, + text: `Parent ${idx + 1}`, + mobileText: `Parent ${idx + 1}`, + value: -(idx + 1), + }); + } + return dropdownContent; }, @@ -133,14 +152,27 @@ * The basePatchNum should always be <= patchNum -- because sortedRevisions * is sorted in reverse order (higher patchset nums first), invalid patch * nums have an index greater than the index of basePatchNum. + * * In addition, if the current basePatchNum is 'PARENT', all patchNums are * valid. + * + * If the curent basePatchNum is a parent index, then only patches that have + * at least that many parents are valid. + * * @param {number|string} basePatchNum The current selected base patch num. * @param {number|string} patchNum The possible patch num. * @param {!Array} sortedRevisions + * @return {boolean} */ _computeRightDisabled(basePatchNum, patchNum, sortedRevisions) { - if (basePatchNum === 'PARENT') { return false; } + if (this.patchNumEquals(basePatchNum, 'PARENT')) { return false; } + + if (this.isMergeParent(basePatchNum)) { + // Note: parent indices use 1-offset. + return this.revisionInfo.getParentCount(patchNum) < + this.getParentIndex(basePatchNum); + } + return this.findSortedIndex(basePatchNum, sortedRevisions) <= this.findSortedIndex(patchNum, sortedRevisions); }, diff --git a/polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select_test.html b/polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select_test.html index 04f83bc6da..a48396e9a8 100644 --- a/polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select_test.html +++ b/polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select_test.html @@ -25,6 +25,8 @@ limitations under the License. + + @@ -50,6 +52,14 @@ limitations under the License. let sandbox; let commentApiWrapper; + function getInfo(revisions) { + const revisionObj = {}; + for (let i = 0; i < revisions.length; i++) { + revisionObj[i] = revisions[i]; + } + return new Gerrit.RevisionInfo({revisions: revisionObj}); + } + setup(() => { sandbox = sinon.sandbox.create(); @@ -112,6 +122,17 @@ limitations under the License. {num: 2}, {num: 1}, ]; + const revisions = [ + { + commit: {parents: []}, + _number: 2, + description: 'description', + }, + {commit: {parents: []}}, + {commit: {parents: []}}, + {commit: {parents: []}}, + ]; + element.revisionInfo = getInfo(revisions); const patchNum = 1; const sortedRevisions = [ {_number: 3}, @@ -158,17 +179,19 @@ limitations under the License. }, ]; assert.deepEqual(element._computeBaseDropdownContent(availablePatches, - patchNum, sortedRevisions, element.changeComments), + patchNum, sortedRevisions, element.changeComments, + element.revisionInfo), expectedResult); }); test('_computeBaseDropdownContent called when patchNum updates', () => { element.revisions = [ - {commit: {}}, - {commit: {}}, - {commit: {}}, - {commit: {}}, + {commit: {parents: []}}, + {commit: {parents: []}}, + {commit: {parents: []}}, + {commit: {parents: []}}, ]; + element.revisionInfo = getInfo(element.revisions); element.availablePatches = [ {num: 1}, {num: 2}, @@ -189,16 +212,17 @@ limitations under the License. test('_computeBaseDropdownContent called when changeComments update', done => { element.revisions = [ - {commit: {}}, - {commit: {}}, - {commit: {}}, - {commit: {}}, + {commit: {parents: []}}, + {commit: {parents: []}}, + {commit: {parents: []}}, + {commit: {parents: []}}, ]; + element.revisionInfo = getInfo(element.revisions); element.availablePatches = [ - {num: 'edit'}, - {num: 3}, - {num: 2}, - {num: 1}, + {num: 'edit'}, + {num: 3}, + {num: 2}, + {num: 1}, ]; element.patchNum = 2; element.basePatchNum = 'PARENT'; @@ -215,11 +239,12 @@ limitations under the License. test('_computePatchDropdownContent called when basePatchNum updates', () => { element.revisions = [ - {commit: {}}, - {commit: {}}, - {commit: {}}, - {commit: {}}, + {commit: {parents: []}}, + {commit: {parents: []}}, + {commit: {parents: []}}, + {commit: {parents: []}}, ]; + element.revisionInfo = getInfo(element.revisions); element.availablePatches = [ {num: 1}, {num: 2}, @@ -238,11 +263,12 @@ limitations under the License. test('_computePatchDropdownContent called when comments update', done => { element.revisions = [ - {commit: {}}, - {commit: {}}, - {commit: {}}, - {commit: {}}, + {commit: {parents: []}}, + {commit: {parents: []}}, + {commit: {parents: []}}, + {commit: {parents: []}}, ]; + element.revisionInfo = getInfo(element.revisions); element.availablePatches = [ {num: 1}, {num: 2}, diff --git a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js index 45def55441..10032706ea 100644 --- a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js +++ b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js @@ -861,6 +861,7 @@ */ getDiffChangeDetail(changeNum, opt_errFn, opt_cancelCondition) { const params = this.listChangesOptionsToHex( + this.ListChangesOption.ALL_COMMITS, this.ListChangesOption.ALL_REVISIONS ); return this._getChangeDetail(changeNum, params, opt_errFn, diff --git a/polygerrit-ui/app/elements/shared/revision-info/revision-info.html b/polygerrit-ui/app/elements/shared/revision-info/revision-info.html new file mode 100644 index 0000000000..5669143182 --- /dev/null +++ b/polygerrit-ui/app/elements/shared/revision-info/revision-info.html @@ -0,0 +1,79 @@ + + + diff --git a/polygerrit-ui/app/elements/shared/revision-info/revision-info_test.html b/polygerrit-ui/app/elements/shared/revision-info/revision-info_test.html new file mode 100644 index 0000000000..27233eb66f --- /dev/null +++ b/polygerrit-ui/app/elements/shared/revision-info/revision-info_test.html @@ -0,0 +1,85 @@ + + + + +revision-info + + + + + + + diff --git a/polygerrit-ui/app/test/index.html b/polygerrit-ui/app/test/index.html index ed608a2093..68dbfd62b0 100644 --- a/polygerrit-ui/app/test/index.html +++ b/polygerrit-ui/app/test/index.html @@ -161,6 +161,7 @@ limitations under the License. 'shared/gr-textarea/gr-textarea_test.html', 'shared/gr-tooltip-content/gr-tooltip-content_test.html', 'shared/gr-tooltip/gr-tooltip_test.html', + 'shared/revision-info/revision-info_test.html', ]; /* eslint-enable max-len */ for (let file of elements) {