Skip to content

Commit

Permalink
fix(lane-merge), handle a case when a component is deleted and is als…
Browse files Browse the repository at this point in the history
…o unrelated to the other lane (#7837)

In this case, we set the "--resolve-unrelated" to `theirs`, so then the
history is kept according to the other lane (which is not removed).
  • Loading branch information
davidfirst authored Aug 29, 2023
1 parent 6263e94 commit d007699
Show file tree
Hide file tree
Showing 4 changed files with 84 additions and 8 deletions.
52 changes: 52 additions & 0 deletions e2e/harmony/lanes/unrelated-and-removed.e2e.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
import chai, { expect } from 'chai';
import Helper from '../../../src/e2e-helper/e2e-helper';

chai.use(require('chai-fs'));

describe('current lane a comp is removed, merging a lane that has this comp with different history', function () {
this.timeout(0);
let helper: Helper;
let headOnLaneA: string;
let headOnLaneB: string;
before(() => {
helper = new Helper();
helper.scopeHelper.setNewLocalAndRemoteScopes();
helper.command.createLane('lane-a');
helper.fixtures.populateComponents(1, false, 'lane-a');
helper.command.snapAllComponentsWithoutBuild();
helper.command.export();
headOnLaneA = helper.command.getHeadOfLane('lane-a', 'comp1');

helper.scopeHelper.reInitLocalScope();
helper.scopeHelper.addRemoteScope();
helper.command.createLane('lane-b');
helper.fixtures.populateComponents(1, false, 'lane-b');
helper.command.snapAllComponentsWithoutBuild();
helper.command.export();

helper.command.softRemoveOnLane('comp1');
helper.command.snapAllComponentsWithoutBuild();
helper.command.export();
headOnLaneB = helper.command.getHeadOfLane('lane-b', 'comp1');

helper.command.mergeLane('lane-a', '--resolve-unrelated -x');
});
after(() => {
helper.scopeHelper.destroy();
});
// should default to resolve by "their" because the current is removed
it('should get the file content according to their', () => {
const fileContent = helper.fs.readFile(`${helper.scopes.remote}/comp1/index.js`);
expect(fileContent).to.have.string('lane-a');
expect(fileContent).to.not.have.string('lane-b');
});
it('should populate the unrelated property according to the current head', () => {
const ver = helper.command.catComponent('comp1@latest');
expect(ver.unrelated.head).to.equal(headOnLaneB);
expect(ver.unrelated.laneId.name).to.equal('lane-b');
});
it('should populate the parents according to the other lane', () => {
const ver = helper.command.catComponent('comp1@latest');
expect(ver.parents[0]).to.equal(headOnLaneA);
});
});
22 changes: 18 additions & 4 deletions scopes/component/merging/merge-status-provider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -210,10 +210,21 @@ other: ${otherLaneHead.toString()}`);
// it is possible that it is diverged, in which case, still continue with the merge, and later on, the
// merge-config will show a config conflict of the remove aspect.
// 2. other is not ahead. in this case, just ignore this component, no point to merge it, we want it removed.
// 3. there are errors when calculating the divergeData, e.g. no snap in common. in such cases, we assume
// there are issues with this component, and is better not to merge it.
// 3. there are errors when calculating the divergeData, e.g. no snap in common.
// here we need to differentiate between two cases:
// 3.1. the "otherLane" is main. in this case, we probably noticed that our component by accident got created
// with the same name of an existing component on main and therefore we removed it. so we want to keep this
// component removed during merges and we don't care about merging it.
// 3.2. the "otherLane" is a lane. in this case, it's possible that although it was removed in this lane, it's
// needed and is used by other components of the other other lane, so in order for this merge to work
// (and not throw error about deps from other lane), it needs the option to merge (user can decide whether to
// exclude it or to use --resolve-unrelated)
const divergeData = await getDivergeData({ repo, modelComponent, targetHead: otherLaneHead, throws: false });
if (divergeData.err || !divergeData.isTargetAhead()) {
const isTargetNotAhead = !divergeData.err && !divergeData.isTargetAhead();
const shouldIgnore = this.otherLane
? isTargetNotAhead // options 3.2 above. if they're not related - don't ignore.
: isTargetNotAhead || divergeData.err; // it's main. options 3.1 above. even if they're not related - still ignore.
if (shouldIgnore) {
return this.returnUnmerged(id, `component has been removed`, true);
}
}
Expand Down Expand Up @@ -303,7 +314,10 @@ other: ${otherLaneHead.toString()}`);
componentOnOther?: Version,
divergeData?: SnapsDistance
): Promise<ComponentMergeStatusBeforeMergeAttempt> {
const { resolveUnrelated } = this.options || {};
let { resolveUnrelated } = this.options || {};
if (currentComponent.isRemoved()) {
resolveUnrelated = 'theirs';
}
if (!resolveUnrelated) throw new Error(`handleNoCommonSnap expects resolveUnrelated to be set`);
const consumer = this.workspace.consumer;
const repo = consumer.scope.objects;
Expand Down
12 changes: 11 additions & 1 deletion scopes/lanes/merge-lanes/merge-lane.cmd.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,17 @@ export class MergeLaneCmd implements Command {
to merge the lane from the local scope without updating it first, use "--skip-fetch" flag.
when the current and merge candidate lanes are diverged in history and the files could be merged with no conflicts,
these components will be snap-merged to complete the merge. use "no-snap" to opt-out, or "tag" to tag instead`;
these components will be snap-merged to complete the merge. use "no-snap" to opt-out, or "tag" to tag instead.
in case a component in both ends don't share history (no snap is found in common), the merge will require "--resolve-unrelated" flag.
this flag keeps the history of one end and saves a reference to the other end. the decision of which end to keep is determined by the following:
1. if the component exists on main, then the history linked to main will be kept.
in this case, the strategy of "--resolve-unrelated" only determines which source-code to keep. it's not about the history.
2. if the component doesn't exist on main, then by default, the history of the current lane will be kept.
unless "--resolve-unrelated" is set to "theirs", in which case the history of the other lane will be kept.
2. a. an edge case: if the component is deleted on the current lane, the strategy will always be "theirs".
so then the history (and the source-code) of the other lane will be kept.
`;
arguments = [
{
name: 'lane',
Expand Down
6 changes: 3 additions & 3 deletions src/consumer/component/consumer-component.ts
Original file line number Diff line number Diff line change
Expand Up @@ -273,10 +273,10 @@ export default class Component {
}

/**
* whether the component is soft removed
* whether the component is deleted (soft removed)
*/
isRemoved() {
return this.extensions.findCoreExtension(Extensions.remove)?.config?.removed || this.removed;
isRemoved(): Boolean {
return Boolean(this.extensions.findCoreExtension(Extensions.remove)?.config?.removed || this.removed);
}

setRemoved() {
Expand Down

0 comments on commit d007699

Please sign in to comment.