Merge pull request #104927 from microsoft/sandy081/fix-103839

Fix syncing empty keybindings file with only comments

Author: sandy081

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

@@ -28,10 +28,14 @@ interface IMergeResult {
 	conflicts: Set<string>;
 }
 
+export function parseKeybindings(content: string): IUserFriendlyKeybinding[] {
+	return parse(content) || [];
+}
+
 export async function merge(localContent: string, remoteContent: string, baseContent: string | null, formattingOptions: FormattingOptions, userDataSyncUtilService: IUserDataSyncUtilService): Promise<{ mergeContent: string, hasChanges: boolean, hasConflicts: boolean }> {
-	const local = <IUserFriendlyKeybinding[]>parse(localContent);
-	const remote = <IUserFriendlyKeybinding[]>parse(remoteContent);
-	const base = baseContent ? <IUserFriendlyKeybinding[]>parse(baseContent) : null;
+	const local = parseKeybindings(localContent);
+	const remote = parseKeybindings(remoteContent);
+	const base = baseContent ? parseKeybindings(baseContent) : null;
 
 	const userbindings: string[] = [...local, ...remote, ...(base || [])].map(keybinding => keybinding.key);
 	const normalizedKeys = await userDataSyncUtilService.resolveUserBindings(userbindings);
@@ -331,7 +335,7 @@ function addKeybindings(content: string, keybindings: IUserFriendlyKeybinding[],
 }
 
 function removeKeybindings(content: string, command: string, formattingOptions: FormattingOptions): string {
-	const keybindings = <IUserFriendlyKeybinding[]>parse(content);
+	const keybindings = parseKeybindings(content);
 	for (let index = keybindings.length - 1; index >= 0; index--) {
 		if (keybindings[index].command === command || keybindings[index].command === `-${command}`) {
 			content = contentUtil.edit(content, [index], undefined, formattingOptions);
@@ -341,7 +345,7 @@ function removeKeybindings(content: string, command: string, formattingOptions:
 }
 
 function updateKeybindings(content: string, command: string, keybindings: IUserFriendlyKeybinding[], formattingOptions: FormattingOptions): string {
-	const allKeybindings = <IUserFriendlyKeybinding[]>parse(content);
+	const allKeybindings = parseKeybindings(content);
 	const location = findFirstIndex(allKeybindings, keybinding => keybinding.command === command || keybinding.command === `-${command}`);
 	// Remove all entries with this command
 	for (let index = allKeybindings.length - 1; index >= 0; index--) {

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

@@ -209,7 +209,7 @@ export class KeybindingsSynchroniser extends AbstractJsonFileSynchroniser implem
 
 		if (lastSyncUserData?.ref !== remoteUserData.ref) {
 			this.logService.trace(`${this.syncResourceLogLabel}: Updating last synchronized keybindings...`);
-			const lastSyncContent = content !== null ? this.toSyncContent(content, null) : null;
+			const lastSyncContent = content !== null ? this.toSyncContent(content, null) : remoteUserData.syncData?.content;
 			await this.updateLastSyncUserData({
 				ref: remoteUserData.ref,
 				syncData: lastSyncContent ? {

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

@@ -100,6 +100,70 @@ suite('KeybindingsSync', () => {
 		assert.equal((await fileService.readFile(keybindingsResource)).value.toString(), content);
 	});
 
+	test('when keybindings file is empty with comment and remote has no changes', async () => {
+		const fileService = client.instantiationService.get(IFileService);
+		const keybindingsResource = client.instantiationService.get(IEnvironmentService).keybindingsResource;
+		const expectedContent = '// Empty Keybindings';
+		await fileService.writeFile(keybindingsResource, VSBuffer.fromString(expectedContent));
+
+		await testObject.sync(await client.manifest());
+
+		const lastSyncUserData = await testObject.getLastSyncUserData();
+		const remoteUserData = await testObject.getRemoteUserData(null);
+		assert.equal(testObject.getKeybindingsContentFromSyncContent(lastSyncUserData!.syncData!.content!), expectedContent);
+		assert.equal(testObject.getKeybindingsContentFromSyncContent(remoteUserData!.syncData!.content!), expectedContent);
+		assert.equal((await fileService.readFile(keybindingsResource)).value.toString(), expectedContent);
+	});
+
+	test('when keybindings file is empty and remote has keybindings', async () => {
+		const client2 = disposableStore.add(new UserDataSyncClient(server));
+		await client2.setUp(true);
+		const content = JSON.stringify([
+			{
+				'key': 'shift+cmd+w',
+				'command': 'workbench.action.closeAllEditors',
+			}
+		]);
+		await client2.instantiationService.get(IFileService).writeFile(client2.instantiationService.get(IEnvironmentService).keybindingsResource, VSBuffer.fromString(content));
+		await client2.sync();
+
+		const fileService = client.instantiationService.get(IFileService);
+		const keybindingsResource = client.instantiationService.get(IEnvironmentService).keybindingsResource;
+		await fileService.writeFile(keybindingsResource, VSBuffer.fromString('// Empty Keybindings'));
+
+		await testObject.sync(await client.manifest());
+
+		const lastSyncUserData = await testObject.getLastSyncUserData();
+		const remoteUserData = await testObject.getRemoteUserData(null);
+		assert.equal(testObject.getKeybindingsContentFromSyncContent(lastSyncUserData!.syncData!.content!), content);
+		assert.equal(testObject.getKeybindingsContentFromSyncContent(remoteUserData!.syncData!.content!), content);
+		assert.equal((await fileService.readFile(keybindingsResource)).value.toString(), content);
+	});
+
+	test('when keybindings file is empty and remote has empty array', async () => {
+		const client2 = disposableStore.add(new UserDataSyncClient(server));
+		await client2.setUp(true);
+		const content =
+			`// Place your key bindings in this file to override the defaults
+[
+]`;
+		await client2.instantiationService.get(IFileService).writeFile(client2.instantiationService.get(IEnvironmentService).keybindingsResource, VSBuffer.fromString(content));
+		await client2.sync();
+
+		const fileService = client.instantiationService.get(IFileService);
+		const keybindingsResource = client.instantiationService.get(IEnvironmentService).keybindingsResource;
+		const expectedLocalContent = '// Empty Keybindings';
+		await fileService.writeFile(keybindingsResource, VSBuffer.fromString(expectedLocalContent));
+
+		await testObject.sync(await client.manifest());
+
+		const lastSyncUserData = await testObject.getLastSyncUserData();
+		const remoteUserData = await testObject.getRemoteUserData(null);
+		assert.equal(testObject.getKeybindingsContentFromSyncContent(lastSyncUserData!.syncData!.content!), content);
+		assert.equal(testObject.getKeybindingsContentFromSyncContent(remoteUserData!.syncData!.content!), content);
+		assert.equal((await fileService.readFile(keybindingsResource)).value.toString(), expectedLocalContent);
+	});
+
 	test('when keybindings file is created after first sync', async () => {
 		const fileService = client.instantiationService.get(IFileService);
 		const keybindingsResource = client.instantiationService.get(IEnvironmentService).keybindingsResource;