#100346 Support merge in synchronizer and add tests

src/vs/platform/userDataSync/common/abstractSynchronizer.ts

@@ -63,6 +63,7 @@ export interface IResourcePreview extends IBaseResourcePreview {
 	readonly remoteContent: string | null;
 	readonly localContent: string | null;
 	readonly previewContent: string | null;
+	readonly hasConflicts: boolean;
 }
 
 export abstract class AbstractSynchroniser extends Disposable {
@@ -359,7 +360,9 @@ export abstract class AbstractSynchroniser extends Disposable {
 			}
 
 			if (apply) {
-				return await this.apply(remoteUserData, lastSyncUserData, false);
+				const preview = await this.syncPreviewPromise;
+				const newConflicts = preview.resourcePreviews.filter(({ hasConflicts }) => hasConflicts);
+				return await this.updateConflictsAndApply(newConflicts, false);
 			} else {
 				return SyncStatus.Syncing;
 			}
@@ -392,7 +395,41 @@ export abstract class AbstractSynchroniser extends Disposable {
 				return newPreview;
 			});
 
-			const status = await this.apply(preview.remoteUserData, preview.lastSyncUserData, force);
+			return this.merge(resource, force, headers);
+		} finally {
+			this.syncHeaders = {};
+		}
+	}
+
+	async merge(resource: URI, force: boolean, headers: IHeaders = {}): Promise<ISyncResourcePreview | null> {
+		if (!this.syncPreviewPromise) {
+			return null;
+		}
+
+		try {
+			this.syncHeaders = { ...headers };
+			const preview = await this.syncPreviewPromise;
+			const resourcePreview = preview.resourcePreviews.find(({ localResource, remoteResource, previewResource }) =>
+				isEqual(localResource, resource) || isEqual(remoteResource, resource) || isEqual(previewResource, resource));
+			if (!resourcePreview) {
+				return preview;
+			}
+
+			/* Add or remove the preview from conflicts */
+			const newConflicts = [...this._conflicts];
+			const index = newConflicts.findIndex(({ localResource, remoteResource, previewResource }) =>
+				isEqual(localResource, resource) || isEqual(remoteResource, resource) || isEqual(previewResource, resource));
+			if (resourcePreview.hasConflicts) {
+				if (newConflicts.indexOf(resourcePreview) === -1) {
+					newConflicts.push(resourcePreview);
+				}
+			} else {
+				if (index !== -1) {
+					newConflicts.splice(index, 1);
+				}
+			}
+
+			const status = await this.updateConflictsAndApply(newConflicts, force);
 			this.setStatus(status);
 			return this.syncPreviewPromise;
 
@@ -401,7 +438,7 @@ export abstract class AbstractSynchroniser extends Disposable {
 		}
 	}
 
-	private async apply(remoteUserData: IRemoteUserData, lastSyncUserData: IRemoteUserData | null, force: boolean): Promise<SyncStatus> {
+	private async updateConflictsAndApply(conflicts: IResourcePreview[], force: boolean): Promise<SyncStatus> {
 		if (!this.syncPreviewPromise) {
 			return SyncStatus.Idle;
 		}
@@ -409,13 +446,13 @@ export abstract class AbstractSynchroniser extends Disposable {
 		const preview = await this.syncPreviewPromise;
 
 		// update conflicts
-		this.updateConflicts();
+		this.updateConflicts(conflicts);
 		if (this._conflicts.length) {
 			return SyncStatus.HasConflicts;
 		}
 
 		// apply preview
-		await this.applyPreview(remoteUserData, lastSyncUserData, preview.resourcePreviews, force);
+		await this.applyPreview(preview.remoteUserData, preview.lastSyncUserData, preview.resourcePreviews, force);
 
 		// reset preview
 		this.syncPreviewPromise = null;
@@ -466,12 +503,10 @@ export abstract class AbstractSynchroniser extends Disposable {
 		}
 	}
 
-	private updateConflicts(): void {
-		const oldConflicts = this._conflicts;
-		const newConflicts = this._resourcePreviews.filter(({ hasConflicts }) => hasConflicts);
-		if (!equals(oldConflicts, newConflicts, (a, b) => isEqual(a.previewResource, b.previewResource))) {
-			this._conflicts = newConflicts;
-			this._onDidChangeConflicts.fire(newConflicts);
+	private updateConflicts(conflicts: IResourcePreview[]): void {
+		if (!equals(this._conflicts, conflicts, (a, b) => isEqual(a.previewResource, b.previewResource))) {
+			this._conflicts = conflicts;
+			this._onDidChangeConflicts.fire(conflicts);
 		}
 	}
 
@@ -645,7 +680,7 @@ export abstract class AbstractSynchroniser extends Disposable {
 		}
 
 		await this.updateResourcePreviews([], CancellationToken.None);
-		this.updateConflicts();
+		this.updateConflicts([]);
 
 		this.setStatus(SyncStatus.Idle);
 		this.logService.info(`${this.syncResourceLogLabel}: Stopped synchronizing ${this.resource.toLowerCase()}.`);

src/vs/platform/userDataSync/common/userDataSync.ts

@@ -304,7 +304,7 @@ export interface IResourcePreview {
 	readonly previewResource: URI;
 	readonly localChange: Change;
 	readonly remoteChange: Change;
-	readonly hasConflicts: boolean;
+
 }
 
 export interface ISyncResourcePreview {
@@ -336,6 +336,7 @@ export interface IUserDataSynchroniser {
 
 	resolveContent(resource: URI): Promise<string | null>;
 	acceptPreviewContent(resource: URI, content: string, force: boolean, headers: IHeaders): Promise<ISyncResourcePreview | null>;
+	merge(resource: URI, force: boolean, headers: IHeaders): Promise<ISyncResourcePreview | null>;
 
 	getRemoteSyncResourceHandles(): Promise<ISyncResourceHandle[]>;
 	getLocalSyncResourceHandles(): Promise<ISyncResourceHandle[]>;

src/vs/platform/userDataSync/common/userDataSyncService.ts

@@ -502,11 +502,7 @@ class ManualSyncTask implements IManualSyncTask {
 			const synchroniser = this.synchronisers.find(s => s.resource === syncResource)!;
 			let newPreview: ISyncResourcePreview | null = null;
 			for (const resourcePreview of preview.resourcePreviews) {
-				/* merge only if there are no conflicts */
-				if (!resourcePreview.hasConflicts) {
-					const content = await synchroniser.resolveContent(resourcePreview.previewResource) || '';
-					newPreview = await synchroniser.acceptPreviewContent(resourcePreview.previewResource, content, false, this.syncHeaders);
-				}
+				newPreview = await synchroniser.merge(resourcePreview.previewResource, false, this.syncHeaders);
 			}
 			if (newPreview) {
 				previews.push(this.toSyncResourcePreview(syncResource, newPreview));
@@ -594,6 +590,5 @@ function toStrictResourcePreview(resourcePreview: IResourcePreview): IResourcePr
 		remoteResource: resourcePreview.remoteResource,
 		localChange: resourcePreview.localChange,
 		remoteChange: resourcePreview.remoteChange,
-		hasConflicts: resourcePreview.hasConflicts,
 	};
 }

src/vs/platform/userDataSync/test/common/synchronizer.test.ts

@@ -7,16 +7,12 @@ import * as assert from 'assert';
 import { IUserDataSyncStoreService, SyncResource, SyncStatus, IUserDataSyncResourceEnablementService, IRemoteUserData, ISyncData, Change, USER_DATA_SYNC_SCHEME, IUserDataManifest } from 'vs/platform/userDataSync/common/userDataSync';
 import { UserDataSyncClient, UserDataSyncTestServer } from 'vs/platform/userDataSync/test/common/userDataSyncClient';
 import { DisposableStore, toDisposable } from 'vs/base/common/lifecycle';
-import { AbstractSynchroniser, ISyncResourcePreview, IResourcePreview } from 'vs/platform/userDataSync/common/abstractSynchronizer';
+import { AbstractSynchroniser, IResourcePreview } from 'vs/platform/userDataSync/common/abstractSynchronizer';
 import { Barrier } from 'vs/base/common/async';
 import { Emitter, Event } from 'vs/base/common/event';
 import { CancellationToken } from 'vs/base/common/cancellation';
 import { URI } from 'vs/base/common/uri';
 
-interface ITestResourcePreview extends IResourcePreview {
-	ref?: string;
-}
-
 const resource = URI.from({ scheme: USER_DATA_SYNC_SCHEME, authority: 'testResource', path: `/current.json` });
 
 class TestSynchroniser extends AbstractSynchroniser {
@@ -50,32 +46,28 @@ class TestSynchroniser extends AbstractSynchroniser {
 		return super.doSync(remoteUserData, lastSyncUserData, apply);
 	}
 
-	protected async generatePullPreview(remoteUserData: IRemoteUserData, lastSyncUserData: IRemoteUserData | null, token: CancellationToken): Promise<ITestResourcePreview[]> {
-		return [{ localContent: null, localResource: resource, remoteContent: null, remoteResource: resource, previewContent: null, previewResource: resource, localChange: Change.None, remoteChange: Change.None, hasConflicts: this.syncResult.hasConflicts }];
+	protected async generatePullPreview(remoteUserData: IRemoteUserData, lastSyncUserData: IRemoteUserData | null, token: CancellationToken): Promise<IResourcePreview[]> {
+		return [{ localContent: null, localResource: resource, remoteContent: null, remoteResource: resource, previewContent: remoteUserData.ref, previewResource: resource, localChange: Change.Modified, remoteChange: Change.None, hasConflicts: this.syncResult.hasConflicts }];
 	}
 
-	protected async generatePushPreview(remoteUserData: IRemoteUserData, lastSyncUserData: IRemoteUserData | null, token: CancellationToken): Promise<ITestResourcePreview[]> {
-		return [{ localContent: null, localResource: resource, remoteContent: null, remoteResource: resource, previewContent: null, previewResource: resource, localChange: Change.None, remoteChange: Change.None, hasConflicts: this.syncResult.hasConflicts }];
+	protected async generatePushPreview(remoteUserData: IRemoteUserData, lastSyncUserData: IRemoteUserData | null, token: CancellationToken): Promise<IResourcePreview[]> {
+		return [{ localContent: null, localResource: resource, remoteContent: null, remoteResource: resource, previewContent: remoteUserData.ref, previewResource: resource, localChange: Change.Modified, remoteChange: Change.None, hasConflicts: this.syncResult.hasConflicts }];
 	}
 
-	protected async generateReplacePreview(syncData: ISyncData, remoteUserData: IRemoteUserData, lastSyncUserData: IRemoteUserData | null): Promise<ITestResourcePreview[]> {
-		return [{ localContent: null, localResource: resource, remoteContent: null, remoteResource: resource, previewContent: null, previewResource: resource, localChange: Change.None, remoteChange: Change.None, hasConflicts: this.syncResult.hasConflicts }];
+	protected async generateReplacePreview(syncData: ISyncData, remoteUserData: IRemoteUserData, lastSyncUserData: IRemoteUserData | null): Promise<IResourcePreview[]> {
+		return [{ localContent: null, localResource: resource, remoteContent: null, remoteResource: resource, previewContent: remoteUserData.ref, previewResource: resource, localChange: Change.Modified, remoteChange: Change.None, hasConflicts: this.syncResult.hasConflicts }];
 	}
 
-	protected async generateSyncPreview(remoteUserData: IRemoteUserData, lastSyncUserData: IRemoteUserData | null, token: CancellationToken): Promise<ITestResourcePreview[]> {
+	protected async generateSyncPreview(remoteUserData: IRemoteUserData, lastSyncUserData: IRemoteUserData | null, token: CancellationToken): Promise<IResourcePreview[]> {
 		if (this.syncResult.hasError) {
 			throw new Error('failed');
 		}
-		return [{ localContent: null, localResource: resource, remoteContent: null, remoteResource: resource, previewContent: null, previewResource: resource, localChange: Change.None, remoteChange: Change.None, hasConflicts: this.syncResult.hasConflicts, ref: remoteUserData.ref }];
+		return [{ localContent: null, localResource: resource, remoteContent: null, remoteResource: resource, previewContent: remoteUserData.ref, previewResource: resource, localChange: Change.Modified, remoteChange: Change.None, hasConflicts: this.syncResult.hasConflicts }];
 	}
 
-	protected async updatePreviewWithConflict(preview: ISyncResourcePreview, conflictResource: URI, conflictContent: string): Promise<ISyncResourcePreview> {
-		return preview;
-	}
-
-	protected async applyPreview(remoteUserData: IRemoteUserData, lastSyncUserData: IRemoteUserData | null, preview: ITestResourcePreview[], forcePush: boolean): Promise<void> {
-		if (preview[0]?.ref) {
-			await this.applyRef(preview[0].ref);
+	protected async applyPreview(remoteUserData: IRemoteUserData, lastSyncUserData: IRemoteUserData | null, preview: IResourcePreview[], forcePush: boolean): Promise<void> {
+		if (preview[0]?.previewContent) {
+			await this.applyRef(preview[0].previewContent);
 		}
 	}
 
@@ -172,16 +164,7 @@ suite('TestSynchronizer', () => {
 		await testObject.sync(await client.manifest());
 
 		assert.deepEqual(testObject.status, SyncStatus.HasConflicts);
-	});
-
-	test('status is set to syncing when asked for preview if there are conflicts', async () => {
-		const testObject: TestSynchroniser = client.instantiationService.createInstance(TestSynchroniser, SyncResource.Settings);
-		testObject.syncResult = { hasConflicts: true, hasError: false };
-		testObject.syncBarrier.open();
-
-		await testObject.preview(await client.manifest());
-
-		assert.deepEqual(testObject.status, SyncStatus.Syncing);
+		assertConflicts(testObject.conflicts, [testObject.resourcePreviews[0].previewResource]);
 	});
 
 	test('sync should not run if syncing already', async () => {
@@ -281,5 +264,102 @@ suite('TestSynchronizer', () => {
 		assert.equal(testObject.status, SyncStatus.Idle);
 	});
 
+	test('preview: status is set to syncing when asked for preview if there are no conflicts', async () => {
+		const testObject: TestSynchroniser = client.instantiationService.createInstance(TestSynchroniser, SyncResource.Settings);
+		testObject.syncResult = { hasConflicts: false, hasError: false };
+		testObject.syncBarrier.open();
+
+		const preview = await testObject.preview(await client.manifest());
+
+		assert.deepEqual(testObject.status, SyncStatus.Syncing);
+		assertPreviews(preview!.resourcePreviews, [resource]);
+		assertConflicts(testObject.conflicts, []);
+	});
+
+	test('preview: status is set to idle after merging if there are no conflicts', async () => {
+		const testObject: TestSynchroniser = client.instantiationService.createInstance(TestSynchroniser, SyncResource.Settings);
+		testObject.syncResult = { hasConflicts: false, hasError: false };
+		testObject.syncBarrier.open();
+
+		let preview = await testObject.preview(await client.manifest());
+		preview = await testObject.merge(preview!.resourcePreviews[0].previewResource, false);
+
+		assert.deepEqual(testObject.status, SyncStatus.Idle);
+		assert.equal(preview, null);
+		assertConflicts(testObject.conflicts, []);
+	});
+
+	test('preview: status is set to idle and sync is applied after accepting when there are no conflicts before merging', async () => {
+		const testObject: TestSynchroniser = client.instantiationService.createInstance(TestSynchroniser, SyncResource.Settings);
+		testObject.syncResult = { hasConflicts: false, hasError: false };
+		testObject.syncBarrier.open();
+
+		let preview = await testObject.preview(await client.manifest());
+		preview = await testObject.acceptPreviewContent(preview!.resourcePreviews[0].previewResource, preview!.resourcePreviews[0].previewContent!, false);
+
+		assert.deepEqual(testObject.status, SyncStatus.Idle);
+		assert.equal(preview, null);
+		assertConflicts(testObject.conflicts, []);
+	});
+
+	test('preview: status is set to syncing when asked for preview if there are conflicts', async () => {
+		const testObject: TestSynchroniser = client.instantiationService.createInstance(TestSynchroniser, SyncResource.Settings);
+		testObject.syncResult = { hasConflicts: true, hasError: false };
+		testObject.syncBarrier.open();
+
+		const preview = await testObject.preview(await client.manifest());
+
+		assert.deepEqual(testObject.status, SyncStatus.Syncing);
+		assertPreviews(preview!.resourcePreviews, [resource]);
+		assertConflicts(testObject.conflicts, []);
+	});
+
+	test('preview: status is set to hasConflicts after merging', async () => {
+		const testObject: TestSynchroniser = client.instantiationService.createInstance(TestSynchroniser, SyncResource.Settings);
+		testObject.syncResult = { hasConflicts: true, hasError: false };
+		testObject.syncBarrier.open();
+
+		const preview = await testObject.preview(await client.manifest());
+		await testObject.merge(preview!.resourcePreviews[0].previewResource, false);
+
+		assert.deepEqual(testObject.status, SyncStatus.HasConflicts);
+		assertPreviews(preview!.resourcePreviews, [resource]);
+		assertConflicts(testObject.conflicts, [preview!.resourcePreviews[0].previewResource]);
+	});
+
+	test('preview: status is set to idle and sync is applied after accepting when there are conflicts', async () => {
+		const testObject: TestSynchroniser = client.instantiationService.createInstance(TestSynchroniser, SyncResource.Settings);
+		testObject.syncResult = { hasConflicts: true, hasError: false };
+		testObject.syncBarrier.open();
+
+		let preview = await testObject.preview(await client.manifest());
+		await testObject.merge(preview!.resourcePreviews[0].previewResource, false);
+		preview = await testObject.acceptPreviewContent(preview!.resourcePreviews[0].previewResource, preview!.resourcePreviews[0].previewContent!, false);
+
+		assert.deepEqual(testObject.status, SyncStatus.Idle);
+		assert.equal(preview, null);
+		assertConflicts(testObject.conflicts, []);
+	});
+
+	test('preview: status is set to idle and sync is applied after accepting when there are conflicts before merging', async () => {
+		const testObject: TestSynchroniser = client.instantiationService.createInstance(TestSynchroniser, SyncResource.Settings);
+		testObject.syncResult = { hasConflicts: true, hasError: false };
+		testObject.syncBarrier.open();
+
+		let preview = await testObject.preview(await client.manifest());
+		preview = await testObject.acceptPreviewContent(preview!.resourcePreviews[0].previewResource, preview!.resourcePreviews[0].previewContent!, false);
+
+		assert.deepEqual(testObject.status, SyncStatus.Idle);
+		assert.equal(preview, null);
+		assertConflicts(testObject.conflicts, []);
+	});
+
+	function assertConflicts(actual: IResourcePreview[], expected: URI[]) {
+		assert.deepEqual(actual.map(({ previewResource }) => previewResource.toString()), expected.map(uri => uri.toString()));
+	}
+
+	function assertPreviews(actual: IResourcePreview[], expected: URI[]) {
+		assert.deepEqual(actual.map(({ previewResource }) => previewResource.toString()), expected.map(uri => uri.toString()));
+	}
 
 });