opendev/gerrit
Code Issues Proposed changes

Support for parent diff bases in merge changes

Browse Source 
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
This commit is contained in:
Wyatt Allen
2017-11-08 15:33:48 -08:00
parent 3d97b49cdd
commit a744e2303f
14 changed files with 285 additions and 57 deletions
Download Patch File Download Diff File Expand all files Collapse all files

14
polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.html
View File

@@ -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,

2
polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header.html
View File

@@ -26,6 +26,7 @@ limitations under the License.
<link rel="import" href="../../shared/gr-select/gr-select.html">
<link rel="import" href="../../shared/gr-button/gr-button.html">
<link rel="import" href="../../shared/gr-icons/gr-icons.html">
<link rel="import" href="../../shared/revision-info/revision-info.html">
<link rel="import" href="../gr-file-list-constants.html">
<dom-module id="gr-file-list-header">
@@ -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">
</gr-patch-range-select>
<span class="separator"></span>

8
polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header.js
View File

@@ -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);
},
});
})();

8
polygerrit-ui/app/elements/core/gr-router/gr-router.js
View File

@@ -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) {

10
polygerrit-ui/app/elements/core/gr-router/gr-router_test.html
View File

@@ -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);
});
});
});

2
polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.html
View File

@@ -26,6 +26,7 @@ limitations under the License.
<link rel="import" href="../../shared/gr-fixed-panel/gr-fixed-panel.html">
<link rel="import" href="../../shared/gr-rest-api-interface/gr-rest-api-interface.html">
<link rel="import" href="../../shared/gr-select/gr-select.html">
<link rel="import" href="../../shared/revision-info/revision-info.html">
<link rel="import" href="../gr-comment-api/gr-comment-api.html">
<link rel="import" href="../gr-diff-cursor/gr-diff-cursor.html">
<link rel="import" href="../gr-diff-preferences/gr-diff-preferences.html">
@@ -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">
</gr-patch-range-select>
<span class="download desktop">

8
polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.js
View File

@@ -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);
},
});
})();

16
polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.html
View File

@@ -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'];

40
polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select.js
View File

@@ -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);
},

68
polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select_test.html
View File

@@ -25,6 +25,8 @@ limitations under the License.
<link rel="import" href="../../diff/gr-comment-api/gr-comment-api.html">
<link rel="import" href="../../shared/gr-rest-api-interface/mock-diff-response_test.html">
<link rel="import" href="../../shared/revision-info/revision-info.html">
<link rel="import" href="gr-patch-range-select.html">
<script>void(0);</script>
@@ -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},

1
polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js
View File

@@ -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,

79
polygerrit-ui/app/elements/shared/revision-info/revision-info.html Normal file
View File

@@ -0,0 +1,79 @@
<!--
Copyright (C) 2017 The Android Open Source Project
Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
-->
<link rel="import" href="../../../behaviors/gr-patch-set-behavior/gr-patch-set-behavior.html">
<script>
(function() {
'use strict';
/**
* @param {Object} change A change object resulting from a change detail
* call that includes revision information.
*/
function RevisionInfo(change) {
this._change = change;
}
/**
* Get the largest number of parents of the commit in any revision. For
* example, with normal changes this will always return 1. For merge changes
* wherein the revisions are merge commits this will return 2 or potentially
* more.
* @return {Number}
*/
RevisionInfo.prototype.getMaxParents = function() {
return Object.values(this._change.revisions)
.reduce((acc, rev) => Math.max(rev.commit.parents.length, acc), 0);
};
/**
* Get an object that maps revision numbers to the number of parents of the
* commit of that revision.
* @return {!Object}
*/
RevisionInfo.prototype.getParentCountMap = function() {
const result = {};
Object.values(this._change.revisions)
.forEach(rev => { result[rev._number] = rev.commit.parents.length; });
return result;
};
/**
* @param {number|string} patchNum
* @return {number}
*/
RevisionInfo.prototype.getParentCount = function(patchNum) {
return this.getParentCountMap()[patchNum];
};
/**
* Get the commit ID of the (0-offset) indexed parent in the given revision
* number.
* @param {number|string} patchNum
* @param {number} parentIndex (0-offset)
* @return {string}
*/
RevisionInfo.prototype.getParentId = function(patchNum, parentIndex) {
const rev = Object.values(this._change.revisions).find(rev =>
Gerrit.PatchSetBehavior.patchNumEquals(rev._number, patchNum));
return rev.commit.parents[parentIndex].commit;
};
if (!window.Gerrit) {
window.Gerrit = {};
}
window.Gerrit.RevisionInfo = RevisionInfo;
})();
</script>

85
polygerrit-ui/app/elements/shared/revision-info/revision-info_test.html Normal file
View File

@@ -0,0 +1,85 @@
<!DOCTYPE html>
<!--
Copyright (C) 2017 The Android Open Source Project
Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
-->
<meta name="viewport" content="width=device-width, minimum-scale=1.0, initial-scale=1.0, user-scalable=yes">
<title>revision-info</title>
<script src="../../../bower_components/webcomponentsjs/webcomponents-lite.min.js"></script>
<script src="../../../bower_components/web-component-tester/browser.js"></script>
<link rel="import" href="../../../test/common-test-setup.html"/>
<link rel="import" href="revision-info.html">
<script>
suite('revision-info tests', () => {
let mockChange;
setup(() => {
mockChange = {
revisions: {
r1: {_number: 1, commit: {parents: [
{commit: 'p1'},
{commit: 'p2'},
{commit: 'p3'},
]}},
r2: {_number: 2, commit: {parents: [
{commit: 'p1'},
{commit: 'p4'},
]}},
r3: {_number: 3, commit: {parents: [{commit: 'p5'}]}},
r4: {_number: 4, commit: {parents: [
{commit: 'p2'},
{commit: 'p3'},
]}},
r5: {_number: 5, commit: {parents: [
{commit: 'p5'},
{commit: 'p2'},
{commit: 'p3'},
]}},
},
};
});
test('getMaxParents', () => {
const ri = new window.Gerrit.RevisionInfo(mockChange);
assert.equal(ri.getMaxParents(), 3);
});
test('getParentCountMap', () => {
const ri = new window.Gerrit.RevisionInfo(mockChange);
assert.deepEqual(ri.getParentCountMap(), {1: 3, 2: 2, 3: 1, 4: 2, 5: 3});
});
test('getParentCount', () => {
const ri = new window.Gerrit.RevisionInfo(mockChange);
assert.deepEqual(ri.getParentCount(1), 3);
assert.deepEqual(ri.getParentCount(3), 1);
});
test('getParentCount', () => {
const ri = new window.Gerrit.RevisionInfo(mockChange);
assert.deepEqual(ri.getParentCount(1), 3);
assert.deepEqual(ri.getParentCount(3), 1);
});
test('getParentId', () => {
const ri = new window.Gerrit.RevisionInfo(mockChange);
assert.deepEqual(ri.getParentId(1, 2), 'p3');
assert.deepEqual(ri.getParentId(2, 1), 'p4');
assert.deepEqual(ri.getParentId(3, 0), 'p5');
});
});
</script>

1
polygerrit-ui/app/test/index.html
View File

@@ -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) {