Skip to content

Commit

Permalink
feat(merge-lane)!: change no-snap to avoid fast-forward (#9159)
Browse files Browse the repository at this point in the history
Until now, in case lane-b is merged into lane-a and a component (e.g.
comp-a) is ahead in lane-b, we simply updated lane-a with the new snap
of lane-b. (this behavior is similar to the "fast-forward" in Git).
This was true regardless of `--no-snap` flag. The `--no-snap` flag was
only for cases when comp-a is diverged in history and no conflicts
found, which was auto-snapping the component and saving two parents, the
head of lane-a and the head of lane-b.

In come cases, the end result of such a merge is not desirable. Some
changes might be done to this comp-a in lane-b which I would like to
investigate and test before I decide to get them in lane-a. Although
comp-a wasn't changed/snapped in my lane-a, still, I prefer to get the
changes of comp-a as modified, so then `bit diff` will show the new
changes I'm getting and then I can decide later to snap these changes.
This PR makes this possible by using the `--no-snap` flag. In practice,
it considers this state of target-ahead (lane-b) as diverged, which, as
a result - doesn't update lane-a, only the component-files and the
unmerged files with the heads data.

A new flag `--no-auto-snap` was introduced to keep the previous behavior
of `--no-snap` in case users need them.
  • Loading branch information
davidfirst authored Aug 30, 2024
1 parent 99390f3 commit 3ab2db4
Show file tree
Hide file tree
Showing 8 changed files with 122 additions and 37 deletions.
4 changes: 2 additions & 2 deletions e2e/harmony/lanes/bit-remove-on-lanes.e2e.ts
Original file line number Diff line number Diff line change
Expand Up @@ -324,10 +324,10 @@ describe('bit lane command', function () {
expect(staged).to.include(`${helper.scopes.remote}/comp2`);
});
});
describe('when merging with --no-snap', () => {
describe('when merging with --no-auto-snap', () => {
before(() => {
helper.scopeHelper.getClonedLocalScope(beforeMerge);
output = helper.command.mergeLane('lane-a', '-x --no-snap');
output = helper.command.mergeLane('lane-a', '-x --no-auto-snap');
});
it('bit status should not show the component as soft-removed from remote', () => {
const status = helper.command.statusJson();
Expand Down
77 changes: 68 additions & 9 deletions e2e/harmony/lanes/merge-lanes.e2e.ts
Original file line number Diff line number Diff line change
Expand Up @@ -523,7 +523,7 @@ describe('merge lanes', function () {
describe('merge the lane without snapping', () => {
before(() => {
helper.scopeHelper.getClonedLocalScope(beforeMerge);
helper.command.mergeLane('main', '--auto-merge-resolve theirs --no-snap -x');
helper.command.mergeLane('main', '--auto-merge-resolve theirs --no-auto-snap -x');
});
it('should show the during-merge as modified', () => {
const status = helper.command.statusJson();
Expand Down Expand Up @@ -664,12 +664,12 @@ describe('merge lanes', function () {
expect(() => helper.command.export()).to.not.throw();
});
});
describe('bit lane merge with --resolve-unrelated and --no-snap', () => {
describe('bit lane merge with --resolve-unrelated and --no-auto-snap', () => {
before(() => {
helper.scopeHelper.getClonedRemoteScope(remoteScopeAfterExport);
helper.scopeHelper.getClonedLocalScope(afterLaneExport);
helper.command.import();
helper.command.mergeLane('main', '--resolve-unrelated --no-snap');
helper.command.mergeLane('main', '--resolve-unrelated --no-auto-snap');
});
it('bit status should show the component as during-merge and staged and not everywhere else', () => {
helper.command.expectStatusToBeClean(['componentsDuringMergeState', 'stagedComponents']);
Expand Down Expand Up @@ -701,7 +701,7 @@ describe('merge lanes', function () {
helper.scopeHelper.getClonedRemoteScope(remoteScopeAfterExport);
helper.scopeHelper.getClonedLocalScope(afterLaneExport);
helper.command.import();
helper.command.mergeLane('main', '--resolve-unrelated theirs --no-snap');
helper.command.mergeLane('main', '--resolve-unrelated theirs --no-auto-snap');
});
it('bit status should show the component as during-merge and staged and not everywhere else', () => {
helper.command.expectStatusToBeClean(['componentsDuringMergeState', 'stagedComponents']);
Expand Down Expand Up @@ -1063,7 +1063,7 @@ describe('merge lanes', function () {
});
describe('when the lane is merged to main, so currently on the FS the file exits', () => {
before(() => {
helper.command.mergeLane('dev', '--no-squash --no-snap -x');
helper.command.mergeLane('dev', '--no-squash --no-auto-snap -x');
});
// previously the file was removed
it('should not remove the file', () => {
Expand All @@ -1074,7 +1074,7 @@ describe('merge lanes', function () {
before(() => {
helper.scopeHelper.getClonedLocalScope(beforeMerge);
helper.command.switchLocalLane('dev', '-x');
helper.command.mergeLane('main', '--no-snap -x');
helper.command.mergeLane('main', '--no-auto-snap -x');
});
// previously it was in "remain-deleted" state and the file was not created
it('should add the file', () => {
Expand Down Expand Up @@ -1329,7 +1329,7 @@ describe('merge lanes', function () {
helper.command.snapAllComponentsWithoutBuild();
helper.command.export();
helper.command.switchLocalLane('main', '-x');
mergeOutput = helper.command.mergeLane('lane-a', '-x --no-snap');
mergeOutput = helper.command.mergeLane('lane-a', '-x --no-auto-snap');
});
it('should indicate that this file was removed in the output', () => {
expect(mergeOutput).to.have.string('removed foo.js');
Expand All @@ -1353,7 +1353,7 @@ describe('merge lanes', function () {
helper.command.switchLocalLane('main', '-x');
helper.command.tagAllWithoutBuild('--unmodified');
helper.command.export();
mergeOutput = helper.command.mergeLane('lane-a', '-x --no-snap --no-squash');
mergeOutput = helper.command.mergeLane('lane-a', '-x --no-auto-snap --no-squash');
});
it('should indicate that this file was removed in the output', () => {
expect(mergeOutput).to.have.string('removed foo.js');
Expand Down Expand Up @@ -1409,7 +1409,7 @@ describe('merge lanes', function () {
});
// previously it was throwing the "unrelated" error
it('bit-lane-merge should not throw', () => {
expect(() => helper.command.mergeLane('main', '-x --no-snap')).to.not.throw();
expect(() => helper.command.mergeLane('main', '-x --no-auto-snap')).to.not.throw();
});
});
describe('renaming files from uppercase to lowercase', () => {
Expand Down Expand Up @@ -1629,4 +1629,63 @@ describe('merge lanes', function () {
expect(comp1.head).to.equal(oldSnapComp1);
});
});
describe('--no-snap vs --no-auto-snap', () => {
let beforeMerge: string;
let snapLaneA: string;
let snapLaneB: string;
before(() => {
helper.scopeHelper.setNewLocalAndRemoteScopes();
helper.command.createLane('lane-a');
helper.fixtures.populateComponents(1);
helper.command.snapAllComponentsWithoutBuild();
snapLaneA = helper.command.getHeadOfLane('lane-a', 'comp1');
helper.command.export();
helper.command.createLane('lane-b');
helper.fixtures.populateComponents(1, undefined, 'from-lane-b');
helper.command.snapAllComponentsWithoutBuild();
snapLaneB = helper.command.getHeadOfLane('lane-b', 'comp1');
helper.command.export();
helper.command.switchLocalLane('lane-a', '-x');
beforeMerge = helper.scopeHelper.cloneLocalScope();
});
describe('with --no-auto-snap', () => {
before(() => {
helper.command.mergeLane('lane-b', '--no-auto-snap -x');
});
it('should update current lane according to the merged one', () => {
const snap = helper.command.getHeadOfLane('lane-a', 'comp1');
expect(snap).to.equal(snapLaneB);
});
it('should not leave the components as modified', () => {
const status = helper.command.statusJson();
expect(status.modifiedComponents).to.have.lengthOf(0);
});
});
describe('with --no-snap', () => {
before(() => {
helper.scopeHelper.getClonedLocalScope(beforeMerge);
helper.command.mergeLane('lane-b', '--no-snap -x');
});
it('should not update current lane according to the merged one', () => {
const snap = helper.command.getHeadOfLane('lane-a', 'comp1');
expect(snap).to.not.equal(snapLaneB);
expect(snap).to.equal(snapLaneA);
});
it('should leave the components as modified', () => {
const status = helper.command.statusJson();
expect(status.modifiedComponents).to.have.lengthOf(1);
});
describe('after snapping', () => {
before(() => {
helper.command.snapAllComponentsWithoutBuild();
});
it('should save two parents, from the current lane and from the merged lane', () => {
const versionObj = helper.command.catComponent('comp1@latest');
expect(versionObj.parents).to.have.lengthOf(2);
expect(versionObj.parents[0]).to.equal(snapLaneA);
expect(versionObj.parents[1]).to.equal(snapLaneB);
});
});
});
});
});
31 changes: 17 additions & 14 deletions e2e/harmony/merge-config.e2e.ts
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,10 @@ describe('merge config scenarios', function () {
helper.scopeHelper.getClonedLocalScope(mainBeforeDiverge);
helper.command.snapAllComponentsWithoutBuild('--unmodified');
helper.command.export();
helper.command.mergeLane(`${helper.scopes.remote}/dev`, '--no-snap --skip-dependency-installation --no-squash');
helper.command.mergeLane(
`${helper.scopes.remote}/dev`,
'--no-auto-snap --skip-dependency-installation --no-squash'
);
});
it('bit status should not show the component with an issue of MergeConfigHasConflict', () => {
helper.command.expectStatusToNotHaveIssue(IssuesClasses.MergeConfigHasConflict.name);
Expand Down Expand Up @@ -163,7 +166,7 @@ describe('merge config scenarios', function () {
helper.scopeHelper.reInitLocalScope();
helper.scopeHelper.addRemoteScope();
helper.command.importLane('dev');
helper.command.mergeLane('main', '--no-snap --skip-dependency-installation');
helper.command.mergeLane('main', '--no-auto-snap --skip-dependency-installation');
beforeConfigResolved = helper.scopeHelper.cloneLocalScope();
});
it('bit status should show the component with an issue of MergeConfigHasConflict', () => {
Expand Down Expand Up @@ -230,7 +233,7 @@ describe('merge config scenarios', function () {
helper.scopeHelper.addRemoteScope();
helper.command.importLane('dev', '--skip-dependency-installation');
beforeMerge = helper.scopeHelper.cloneLocalScope();
helper.command.mergeLane('main', '--no-snap --skip-dependency-installation');
helper.command.mergeLane('main', '--no-auto-snap --skip-dependency-installation');
beforeConfigResolved = helper.scopeHelper.cloneLocalScope();
});
it('bit status should show the component with an issue of MergeConfigHasConflict', () => {
Expand Down Expand Up @@ -267,7 +270,7 @@ describe('merge config scenarios', function () {
describe('merging with --auto-merge-resolve ours', () => {
before(() => {
helper.scopeHelper.getClonedLocalScope(beforeMerge);
helper.command.mergeLane('main', '--no-snap --skip-dependency-installation --auto-merge-resolve=ours');
helper.command.mergeLane('main', '--no-auto-snap --skip-dependency-installation --auto-merge-resolve=ours');
});
it('should not generate the config-merge file', () => {
const configMerge = helper.general.getConfigMergePath();
Expand All @@ -283,7 +286,7 @@ describe('merge config scenarios', function () {
describe('merging with --auto-merge-resolve theirs', () => {
before(() => {
helper.scopeHelper.getClonedLocalScope(beforeMerge);
helper.command.mergeLane('main', '--no-snap --skip-dependency-installation --auto-merge-resolve=theirs');
helper.command.mergeLane('main', '--no-auto-snap --skip-dependency-installation --auto-merge-resolve=theirs');
});
it('should not generate the config-merge file', () => {
const configMerge = helper.general.getConfigMergePath();
Expand Down Expand Up @@ -315,7 +318,7 @@ describe('merge config scenarios', function () {
helper.command.snapAllComponentsWithoutBuild('--unmodified');
helper.command.export();

helper.command.mergeLane('dev', '--no-snap -x --no-squash');
helper.command.mergeLane('dev', '--no-auto-snap -x --no-squash');
});
// previously, it was ignoring the previous config and only adding "ramda".
it('should not remove the packages it had previously via deps set', () => {
Expand Down Expand Up @@ -350,7 +353,7 @@ describe('merge config scenarios', function () {
helper.scopeHelper.reInitLocalScope();
helper.scopeHelper.addRemoteScope();
helper.command.importLane('dev', '--skip-dependency-installation');
helper.command.mergeLane('main', '--no-snap --skip-dependency-installation --ignore-config-changes');
helper.command.mergeLane('main', '--no-auto-snap --skip-dependency-installation --ignore-config-changes');
beforeConfigResolved = helper.scopeHelper.cloneLocalScope();
});
it('bit status should show the component with an issue of MergeConfigHasConflict', () => {
Expand Down Expand Up @@ -417,7 +420,7 @@ describe('merge config scenarios', function () {
helper.scopeHelper.reInitLocalScope();
helper.scopeHelper.addRemoteScope();
helper.command.importLane('dev', '--skip-dependency-installation');
helper.command.mergeLane('main', '--no-snap --skip-dependency-installation');
helper.command.mergeLane('main', '--no-auto-snap --skip-dependency-installation');
beforeConfigResolved = helper.scopeHelper.cloneLocalScope();
});
it('bit status should show the component with an issue of MergeConfigHasConflict', () => {
Expand Down Expand Up @@ -537,7 +540,7 @@ describe('merge config scenarios', function () {
describe('when the dep was updated on the lane only, not on main', () => {
describe('when the dep is in workspace.jsonc', () => {
before(() => {
helper.command.mergeLane(`${helper.scopes.remote}/dev --no-squash --no-snap`);
helper.command.mergeLane(`${helper.scopes.remote}/dev --no-squash --no-auto-snap`);
});
it('should change workspace.jsonc with the updated dependency', () => {
const policy = helper.workspaceJsonc.getPolicyFromDependencyResolver();
Expand All @@ -548,7 +551,7 @@ describe('merge config scenarios', function () {
before(() => {
helper.scopeHelper.getClonedLocalScope(beforeMerges);
helper.workspaceJsonc.addPolicyToDependencyResolver({ dependencies: {} });
helper.command.mergeLane(`${helper.scopes.remote}/dev --no-squash --no-snap`);
helper.command.mergeLane(`${helper.scopes.remote}/dev --no-squash --no-auto-snap`);
});
it('should auto-update the dependency according to the lane, because only there it was changed', () => {
const deps = helper.command.getCompDepsIdsFromData('comp1');
Expand Down Expand Up @@ -583,7 +586,7 @@ describe('merge config scenarios', function () {
});
describe('when the dep is in workspace.jsonc', () => {
before(() => {
helper.command.mergeLane(`${helper.scopes.remote}/dev --no-squash --no-snap`);
helper.command.mergeLane(`${helper.scopes.remote}/dev --no-squash --no-auto-snap`);
});
it('should not write config-merge file', () => {
const conflictFile = helper.general.getConfigMergePath();
Expand All @@ -605,7 +608,7 @@ describe('merge config scenarios', function () {
before(() => {
helper.scopeHelper.getClonedLocalScope(afterExport);
helper.workspaceJsonc.addPolicyToDependencyResolver({ dependencies: {} });
helper.command.mergeLane(`${helper.scopes.remote}/dev --no-squash --no-snap`);
helper.command.mergeLane(`${helper.scopes.remote}/dev --no-squash --no-auto-snap`);
});
it('bit status should not show a workspace issue', () => {
const status = helper.command.statusJson();
Expand Down Expand Up @@ -681,7 +684,7 @@ describe('merge config scenarios', function () {
let mergeOutput: string;
before(() => {
helper.scopeHelper.getClonedLocalScope(laneWs);
mergeOutput = helper.command.mergeLane(`main --no-snap -x`);
mergeOutput = helper.command.mergeLane(`main --no-auto-snap -x`);
});
it('should not update workspace.jsonc', () => {
const policy = helper.workspaceJsonc.getPolicyFromDependencyResolver();
Expand Down Expand Up @@ -723,7 +726,7 @@ describe('merge config scenarios', function () {
helper.scopeHelper.reInitLocalScope();
helper.scopeHelper.addRemoteScope();
helper.command.importLane('dev', '--skip-dependency-installation');
helper.command.mergeLane('main', '--no-snap --skip-dependency-installation --ignore-config-changes');
helper.command.mergeLane('main', '--no-auto-snap --skip-dependency-installation --ignore-config-changes');
});
it('should not delete the previously deps set', () => {
const deps = helper.command.getCompDepsIdsFromData('comp1');
Expand Down
2 changes: 1 addition & 1 deletion e2e/harmony/updates-from-main-and-lane.e2e.ts
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ describe('updates from main and lane', function () {
before(() => {
mergeOutput = helper.command.mergeLane(
'main',
'--skip-dependency-installation --no-snap --include-non-lane-comps'
'--skip-dependency-installation --no-auto-snap --include-non-lane-comps'
);
});
it('should update not only components belong to the main but also components that are available on the workspace and have updates from main', () => {
Expand Down
11 changes: 8 additions & 3 deletions scopes/component/merging/merge-status-provider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ export type MergeStatusProviderOptions = {
mergeStrategy: MergeStrategy;
ignoreConfigChanges?: boolean;
shouldSquash?: boolean;
handleTargetAheadAsDiverged?: boolean;
};

export const compIsAlreadyMergedMsg = 'component is already merged';
Expand Down Expand Up @@ -305,7 +306,12 @@ other: ${otherLaneHead.toString()}`);
// component is ahead nothing to merge.
return this.returnUnmerged(id, compIsAlreadyMergedMsg, true);
}
if (divergeData.isTargetAhead()) {
if (!divergeData.isTargetAhead()) {
// we know that localHead and remoteHead are set, so if none of them is ahead they must be equal
return this.returnUnmerged(id, compIsAlreadyMergedMsg, true);
}
// target is ahead.
if (!this.options.handleTargetAheadAsDiverged) {
// just override with the model data
return {
...componentStatus,
Expand All @@ -314,8 +320,7 @@ other: ${otherLaneHead.toString()}`);
divergeData,
};
}
// we know that localHead and remoteHead are set, so if none of them is ahead they must be equal
return this.returnUnmerged(id, compIsAlreadyMergedMsg, true);
// target is ahead and we want to treat it as diverged, continue.
}

// it's diverged and needs merge operation
Expand Down
12 changes: 7 additions & 5 deletions scopes/component/merging/merging.main.runtime.ts
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ export class MergingMain {
mergeStrategy: MergeStrategy,
abort: boolean,
resolve: boolean,
noSnap: boolean,
noAutoSnap: boolean,
message: string,
build: boolean,
skipDependencyInstallation: boolean
Expand All @@ -148,7 +148,7 @@ export class MergingMain {
consumer,
bitIds,
mergeStrategy,
noSnap,
noAutoSnap,
message,
build,
skipDependencyInstallation
Expand All @@ -166,7 +166,7 @@ export class MergingMain {
consumer: Consumer,
bitIds: ComponentID[],
mergeStrategy: MergeStrategy,
noSnap: boolean,
noAutoSnap: boolean,
snapMessage: string,
build: boolean,
skipDependencyInstallation: boolean
Expand Down Expand Up @@ -195,7 +195,7 @@ export class MergingMain {
allComponentsStatus,
otherLaneId: currentLaneId,
currentLane: currentLaneObject,
noSnap,
noAutoSnap: noAutoSnap,
snapMessage,
build,
skipDependencyInstallation,
Expand All @@ -210,6 +210,7 @@ export class MergingMain {
allComponentsStatus,
otherLaneId,
currentLane,
noAutoSnap,
noSnap,
tag,
snapMessage,
Expand All @@ -220,6 +221,7 @@ export class MergingMain {
allComponentsStatus: ComponentMergeStatus[];
otherLaneId: LaneId;
currentLane?: Lane;
noAutoSnap?: boolean;
noSnap?: boolean;
tag?: boolean;
snapMessage?: string;
Expand Down Expand Up @@ -309,7 +311,7 @@ export class MergingMain {
const getSnapOrTagResults = async (): Promise<MergeSnapResults> => {
// if one of the component has conflict, don't snap-merge. otherwise, some of the components would be snap-merged
// and some not. besides the fact that it could by mistake tag dependent, it's a confusing state. better not snap.
if (noSnap || leftUnresolvedConflicts || componentsHasConfigMergeConflicts) {
if (noAutoSnap || noSnap || leftUnresolvedConflicts || componentsHasConfigMergeConflicts) {
return null;
}
if (tag) {
Expand Down
Loading

0 comments on commit 3ab2db4

Please sign in to comment.