Use platform agnostic relative paths for workspace file (fix microsoft#148492) (microsoft#157507)

* Use platform agnostic relative paths for workspace file (fix microsoft#148492)

* more fixes

* cleanup

* fix

* rename methods

* always use slash

* cleanup

* fix tests on windows

Author: bpasero

src/vs/platform/workspaces/common/workspaces.ts

@@ -4,13 +4,13 @@
  *--------------------------------------------------------------------------------------------*/
 
 import { Event } from 'vs/base/common/event';
-import { toSlashes } from 'vs/base/common/extpath';
+import { isUNC, toSlashes } from 'vs/base/common/extpath';
 import * as json from 'vs/base/common/json';
 import * as jsonEdit from 'vs/base/common/jsonEdit';
 import { FormattingOptions } from 'vs/base/common/jsonFormatter';
 import { normalizeDriveLetter } from 'vs/base/common/labels';
 import { Schemas } from 'vs/base/common/network';
-import { isAbsolute } from 'vs/base/common/path';
+import { isAbsolute, posix } from 'vs/base/common/path';
 import { isLinux, isMacintosh, isWindows } from 'vs/base/common/platform';
 import { IExtUri, isEqualAuthority } from 'vs/base/common/resources';
 import { URI } from 'vs/base/common/uri';
@@ -124,55 +124,77 @@ export interface IEnterWorkspaceResult {
 }
 
 /**
- * Given a folder URI and the workspace config folder, computes the IStoredWorkspaceFolder using
-* a relative or absolute path or a uri.
- * Undefined is returned if the folderURI and the targetConfigFolderURI don't have the same schema or authority
+ * Given a folder URI and the workspace config folder, computes the `IStoredWorkspaceFolder`
+ * using a relative or absolute path or a uri.
+ * Undefined is returned if the `folderURI` and the `targetConfigFolderURI` don't have the
+ * same schema or authority.
  *
  * @param folderURI a workspace folder
  * @param forceAbsolute if set, keep the path absolute
  * @param folderName a workspace name
  * @param targetConfigFolderURI the folder where the workspace is living in
- * @param useSlashForPath if set, use forward slashes for file paths on windows
  */
-export function getStoredWorkspaceFolder(folderURI: URI, forceAbsolute: boolean, folderName: string | undefined, targetConfigFolderURI: URI, useSlashForPath = !isWindows, extUri: IExtUri): IStoredWorkspaceFolder {
+export function getStoredWorkspaceFolder(folderURI: URI, forceAbsolute: boolean, folderName: string | undefined, targetConfigFolderURI: URI, extUri: IExtUri): IStoredWorkspaceFolder {
+
+	// Scheme mismatch: use full absolute URI as `uri`
 	if (folderURI.scheme !== targetConfigFolderURI.scheme) {
 		return { name: folderName, uri: folderURI.toString(true) };
 	}
 
+	// Always prefer a relative path if possible unless
+	// prevented to make the workspace file shareable
+	// with other users
 	let folderPath = !forceAbsolute ? extUri.relativePath(targetConfigFolderURI, folderURI) : undefined;
 	if (folderPath !== undefined) {
 		if (folderPath.length === 0) {
 			folderPath = '.';
-		} else if (isWindows && folderURI.scheme === Schemas.file && !useSlashForPath) {
-			// Windows gets special treatment:
-			// - use backslahes unless slash is used by other existing folders
-			folderPath = folderPath.replace(/\//g, '\\');
+		} else {
+			if (isWindows) {
+				folderPath = massagePathForWindows(folderPath);
+			}
 		}
-	} else {
+	}
 
-		// use absolute path
+	// We could not resolve a relative path
+	else {
+
+		// Local file: use `fsPath`
 		if (folderURI.scheme === Schemas.file) {
 			folderPath = folderURI.fsPath;
 			if (isWindows) {
-				// Windows gets special treatment:
-				// - normalize all paths to get nice casing of drive letters
-				// - use backslahes unless slash is used by other existing folders
-				folderPath = normalizeDriveLetter(folderPath);
-				if (useSlashForPath) {
-					folderPath = toSlashes(folderPath);
-				}
-			}
-		} else {
-			if (!extUri.isEqualAuthority(folderURI.authority, targetConfigFolderURI.authority)) {
-				return { name: folderName, uri: folderURI.toString(true) };
+				folderPath = massagePathForWindows(folderPath);
 			}
+		}
+
+		// Different authority: use full absolute URI
+		else if (!extUri.isEqualAuthority(folderURI.authority, targetConfigFolderURI.authority)) {
+			return { name: folderName, uri: folderURI.toString(true) };
+		}
+
+		// Non-local file: use `path` of URI
+		else {
 			folderPath = folderURI.path;
 		}
 	}
 
 	return { name: folderName, path: folderPath };
 }
 
+function massagePathForWindows(folderPath: string) {
+
+	// Drive letter should be upper case
+	folderPath = normalizeDriveLetter(folderPath);
+
+	// Always prefer slash over backslash unless
+	// we deal with UNC paths where backslash is
+	// mandatory.
+	if (!isUNC(folderPath)) {
+		folderPath = toSlashes(folderPath);
+	}
+
+	return folderPath;
+}
+
 export function toWorkspaceFolders(configuredFolders: IStoredWorkspaceFolder[], workspaceConfigFile: URI, extUri: IExtUri): WorkspaceFolder[] {
 	const result: WorkspaceFolder[] = [];
 	const seen: Set<string> = new Set();
@@ -187,8 +209,8 @@ export function toWorkspaceFolders(configuredFolders: IStoredWorkspaceFolder[],
 		} else if (isRawUriWorkspaceFolder(configuredFolder)) {
 			try {
 				uri = URI.parse(configuredFolder.uri);
-				if (uri.path[0] !== '/') {
-					uri = uri.with({ path: '/' + uri.path }); // this makes sure all workspace folder are absolute
+				if (uri.path[0] !== posix.sep) {
+					uri = uri.with({ path: posix.sep + uri.path }); // this makes sure all workspace folder are absolute
 				}
 			} catch (e) {
 				console.warn(e); // ignore
@@ -222,7 +244,6 @@ export function rewriteWorkspaceFileForNewLocation(rawWorkspaceContents: string,
 	const targetConfigFolder = extUri.dirname(targetConfigPathURI);
 
 	const rewrittenFolders: IStoredWorkspaceFolder[] = [];
-	const slashForPath = useSlashForPath(storedWorkspace.folders);
 
 	for (const folder of storedWorkspace.folders) {
 		const folderURI = isRawFileWorkspaceFolder(folder) ? extUri.resolvePath(sourceConfigFolder, folder.path) : URI.parse(folder.uri);
@@ -232,7 +253,7 @@ export function rewriteWorkspaceFileForNewLocation(rawWorkspaceContents: string,
 		} else {
 			absolute = !isRawFileWorkspaceFolder(folder) || isAbsolute(folder.path); // for existing workspaces, preserve whether a path was absolute or relative
 		}
-		rewrittenFolders.push(getStoredWorkspaceFolder(folderURI, absolute, folder.name, targetConfigFolder, slashForPath, extUri));
+		rewrittenFolders.push(getStoredWorkspaceFolder(folderURI, absolute, folder.name, targetConfigFolder, extUri));
 	}
 
 	// Preserve as much of the existing workspace as possible by using jsonEdit
@@ -264,14 +285,6 @@ function doParseStoredWorkspace(path: URI, contents: string): IStoredWorkspace {
 	return storedWorkspace;
 }
 
-export function useSlashForPath(storedFolders: IStoredWorkspaceFolder[]): boolean {
-	if (isWindows) {
-		return storedFolders.some(folder => isRawFileWorkspaceFolder(folder) && folder.path.indexOf('/') >= 0);
-	}
-
-	return true;
-}
-
 //#endregion
 
 //#region Workspace Storage

src/vs/platform/workspaces/electron-main/workspacesManagementMainService.ts

@@ -11,7 +11,6 @@ import { mnemonicButtonLabel } from 'vs/base/common/labels';
 import { Disposable } from 'vs/base/common/lifecycle';
 import { Schemas } from 'vs/base/common/network';
 import { dirname, join } from 'vs/base/common/path';
-import { isWindows } from 'vs/base/common/platform';
 import { basename, extUriBiasedIgnorePathCase, joinPath, originalFSPath } from 'vs/base/common/resources';
 import { withNullAsUndefined } from 'vs/base/common/types';
 import { URI } from 'vs/base/common/uri';
@@ -180,7 +179,7 @@ export class WorkspacesManagementMainService extends Disposable implements IWork
 		const storedWorkspaceFolder: IStoredWorkspaceFolder[] = [];
 
 		for (const folder of folders) {
-			storedWorkspaceFolder.push(getStoredWorkspaceFolder(folder.uri, true, folder.name, untitledWorkspaceConfigFolder, !isWindows, extUriBiasedIgnorePathCase));
+			storedWorkspaceFolder.push(getStoredWorkspaceFolder(folder.uri, true, folder.name, untitledWorkspaceConfigFolder, extUriBiasedIgnorePathCase));
 		}
 
 		return {

src/vs/platform/workspaces/test/electron-main/workspacesManagementMainService.test.ts

@@ -6,6 +6,7 @@
 import * as assert from 'assert';
 import * as fs from 'fs';
 import * as os from 'os';
+import { isUNC, toSlashes } from 'vs/base/common/extpath';
 import { normalizeDriveLetter } from 'vs/base/common/labels';
 import * as path from 'vs/base/common/path';
 import { isWindows } from 'vs/base/common/platform';
@@ -126,13 +127,16 @@ flakySuite('WorkspacesManagementMainService', () => {
 		return pfs.Promises.rm(testDir);
 	});
 
-	function assertPathEquals(p1: string, p2: string): void {
+	function assertPathEquals(pathInWorkspaceFile: string, pathOnDisk: string): void {
 		if (isWindows) {
-			p1 = normalizeDriveLetter(p1);
-			p2 = normalizeDriveLetter(p2);
+			pathInWorkspaceFile = normalizeDriveLetter(pathInWorkspaceFile);
+			pathOnDisk = normalizeDriveLetter(pathOnDisk);
+			if (!isUNC(pathOnDisk)) {
+				pathOnDisk = toSlashes(pathOnDisk); // workspace file is using slashes for all paths except where mandatory
+			}
 		}
 
-		assert.strictEqual(p1, p2);
+		assert.strictEqual(pathInWorkspaceFile, pathOnDisk);
 	}
 
 	function assertEqualURI(u1: URI, u2: URI): void {
@@ -335,16 +339,16 @@ flakySuite('WorkspacesManagementMainService', () => {
 		assert.strictEqual(ws.folders.length, 3);
 		assertPathEquals((<IRawFileWorkspaceFolder>ws.folders[0]).path, folder1);
 		assertPathEquals((<IRawFileWorkspaceFolder>ws.folders[1]).path, 'inside');
-		assertPathEquals((<IRawFileWorkspaceFolder>ws.folders[2]).path, isWindows ? 'inside\\somefolder' : 'inside/somefolder');
+		assertPathEquals((<IRawFileWorkspaceFolder>ws.folders[2]).path, 'inside/somefolder');
 
 		origConfigPath = workspaceConfigPath;
 		workspaceConfigPath = URI.file(path.join(tmpDir, 'other', 'myworkspace2.code-workspace'));
 		newContent = rewriteWorkspaceFileForNewLocation(newContent, origConfigPath, false, workspaceConfigPath, extUriBiasedIgnorePathCase);
 		ws = (JSON.parse(newContent) as IStoredWorkspace);
 		assert.strictEqual(ws.folders.length, 3);
 		assertPathEquals((<IRawFileWorkspaceFolder>ws.folders[0]).path, folder1);
-		assertPathEquals((<IRawFileWorkspaceFolder>ws.folders[1]).path, isWindows ? '..\\inside' : '../inside');
-		assertPathEquals((<IRawFileWorkspaceFolder>ws.folders[2]).path, isWindows ? '..\\inside\\somefolder' : '../inside/somefolder');
+		assertPathEquals((<IRawFileWorkspaceFolder>ws.folders[1]).path, '../inside');
+		assertPathEquals((<IRawFileWorkspaceFolder>ws.folders[2]).path, '../inside/somefolder');
 
 		origConfigPath = workspaceConfigPath;
 		workspaceConfigPath = URI.parse('foo://foo/bar/myworkspace2.code-workspace');
@@ -396,7 +400,7 @@ flakySuite('WorkspacesManagementMainService', () => {
 		const ws = (JSON.parse(newContent) as IStoredWorkspace);
 		assertPathEquals((<IRawFileWorkspaceFolder>ws.folders[0]).path, folder1Location);
 		assertPathEquals((<IRawFileWorkspaceFolder>ws.folders[1]).path, folder2Location);
-		assertPathEquals((<IRawFileWorkspaceFolder>ws.folders[2]).path, 'inner\\more');
+		assertPathEquals((<IRawFileWorkspaceFolder>ws.folders[2]).path, 'inner/more');
 
 		service.deleteUntitledWorkspaceSync(workspace);
 	});

src/vs/workbench/services/configuration/browser/configurationService.ts

@@ -18,7 +18,7 @@ import { Configuration } from 'vs/workbench/services/configuration/common/config
 import { FOLDER_CONFIG_FOLDER_NAME, defaultSettingsSchemaId, userSettingsSchemaId, workspaceSettingsSchemaId, folderSettingsSchemaId, IConfigurationCache, machineSettingsSchemaId, LOCAL_MACHINE_SCOPES, IWorkbenchConfigurationService, RestrictedSettings, PROFILE_SCOPES, LOCAL_MACHINE_PROFILE_SCOPES, profileSettingsSchemaId } from 'vs/workbench/services/configuration/common/configuration';
 import { Registry } from 'vs/platform/registry/common/platform';
 import { IConfigurationRegistry, Extensions, allSettings, windowSettings, resourceSettings, applicationSettings, machineSettings, machineOverridableSettings, ConfigurationScope, IConfigurationPropertySchema, keyFromOverrideIdentifiers, OVERRIDE_PROPERTY_PATTERN, resourceLanguageSettingsSchemaId, configurationDefaultsSchemaId } from 'vs/platform/configuration/common/configurationRegistry';
-import { IStoredWorkspaceFolder, isStoredWorkspaceFolder, IWorkspaceFolderCreationData, useSlashForPath, getStoredWorkspaceFolder, toWorkspaceFolders } from 'vs/platform/workspaces/common/workspaces';
+import { IStoredWorkspaceFolder, isStoredWorkspaceFolder, IWorkspaceFolderCreationData, getStoredWorkspaceFolder, toWorkspaceFolders } from 'vs/platform/workspaces/common/workspaces';
 import { IInstantiationService } from 'vs/platform/instantiation/common/instantiation';
 import { ConfigurationEditingService, EditableConfigurationTarget } from 'vs/workbench/services/configuration/common/configurationEditingService';
 import { WorkspaceConfiguration, FolderConfiguration, RemoteUserConfiguration, UserConfiguration, DefaultConfiguration } from 'vs/workbench/services/configuration/browser/configuration';
@@ -254,8 +254,6 @@ export class WorkspaceService extends Disposable implements IWorkbenchConfigurat
 			return !this.contains(foldersToRemove, currentWorkspaceFolders[index].uri); // keep entries which are unrelated
 		});
 
-		const slashForPath = useSlashForPath(newStoredFolders);
-
 		foldersHaveChanged = currentWorkspaceFolders.length !== newStoredFolders.length;
 
 		// Add afterwards (if any)
@@ -280,7 +278,7 @@ export class WorkspaceService extends Disposable implements IWorkbenchConfigurat
 						continue;
 					}
 				} catch (e) { /* Ignore */ }
-				storedFoldersToAdd.push(getStoredWorkspaceFolder(folderURI, false, folderToAdd.name, workspaceConfigFolder, slashForPath, this.uriIdentityService.extUri));
+				storedFoldersToAdd.push(getStoredWorkspaceFolder(folderURI, false, folderToAdd.name, workspaceConfigFolder, this.uriIdentityService.extUri));
 			}
 
 			// Apply to array of newStoredFolders

src/vs/workbench/services/workspaces/browser/workspacesService.ts

@@ -16,7 +16,6 @@ import { IFileService, FileOperationError, FileOperationResult } from 'vs/platfo
 import { IWorkbenchEnvironmentService } from 'vs/workbench/services/environment/common/environmentService';
 import { joinPath } from 'vs/base/common/resources';
 import { VSBuffer } from 'vs/base/common/buffer';
-import { isWindows } from 'vs/base/common/platform';
 import { IUriIdentityService } from 'vs/platform/uriIdentity/common/uriIdentity';
 import { IWorkspaceBackupInfo, IFolderBackupInfo } from 'vs/platform/backup/common/backup';
 import { Schemas } from 'vs/base/common/network';
@@ -179,7 +178,7 @@ export class BrowserWorkspacesService extends Disposable implements IWorkspacesS
 		const storedWorkspaceFolder: IStoredWorkspaceFolder[] = [];
 		if (folders) {
 			for (const folder of folders) {
-				storedWorkspaceFolder.push(getStoredWorkspaceFolder(folder.uri, true, folder.name, this.environmentService.untitledWorkspacesHome, !isWindows, this.uriIdentityService.extUri));
+				storedWorkspaceFolder.push(getStoredWorkspaceFolder(folder.uri, true, folder.name, this.environmentService.untitledWorkspacesHome, this.uriIdentityService.extUri));
 			}
 		}