Deconflict local renames

frontend/sync-client/src/file-operations/file-operations.test.ts

@@ -0,0 +1,102 @@
+import { FileSystemOperations } from "sync-client";
+import type { RelativePath } from "../persistence/database";
+import { FileOperations } from "./file-operations";
+import { Logger } from "../tracing/logger";
+import assert from "assert";
+
+describe("File operations", () => {
+	class FakeFileSystemOperations implements FileSystemOperations {
+		public readonly names = new Set<string>();
+
+		public listAllFiles(): Promise<RelativePath[]> {
+			throw new Error("Method not implemented.");
+		}
+		public read(path: RelativePath): Promise<Uint8Array> {
+			throw new Error("Method not implemented.");
+		}
+		public async write(
+			path: RelativePath,
+			_content: Uint8Array
+		): Promise<void> {
+			this.names.add(path);
+		}
+		public atomicUpdateText(
+			path: RelativePath,
+			updater: (currentContent: string) => string
+		): Promise<string> {
+			throw new Error("Method not implemented.");
+		}
+		public getFileSize(path: RelativePath): Promise<number> {
+			throw new Error("Method not implemented.");
+		}
+		public getModificationTime(path: RelativePath): Promise<Date> {
+			throw new Error("Method not implemented.");
+		}
+		public async exists(path: RelativePath): Promise<boolean> {
+			return this.names.has(path);
+		}
+		public async createDirectory(path: RelativePath): Promise<void> {}
+		public delete(path: RelativePath): Promise<void> {
+			throw new Error("Method not implemented.");
+		}
+		public async rename(
+			oldPath: RelativePath,
+			newPath: RelativePath
+		): Promise<void> {
+			this.names.delete(oldPath);
+			this.names.add(newPath);
+		}
+	}
+
+	test("should deconflict renames", async () => {
+		let fs = new FakeFileSystemOperations();
+		let fileOperations = new FileOperations(new Logger(), fs);
+
+		await fileOperations.create("a", new Uint8Array());
+		assertSetOnlyContains(fs.names, "a");
+		await fileOperations.move("a", "b");
+		assertSetOnlyContains(fs.names, "b");
+
+		await fileOperations.create("c", new Uint8Array());
+		assertSetOnlyContains(fs.names, "b", "c");
+
+		await fileOperations.move("c", "b");
+		assertSetOnlyContains(fs.names, "b", "b (1)");
+
+		await fileOperations.create("c", new Uint8Array());
+		await fileOperations.move("c", "b");
+		assertSetOnlyContains(fs.names, "b", "b (1)", "b (2)");
+	});
+
+	test("should deconflict renames with file extension", async () => {
+		let fs = new FakeFileSystemOperations();
+		let fileOperations = new FileOperations(new Logger(), fs);
+
+		await fileOperations.create("b.md", new Uint8Array());
+		await fileOperations.create("c.md", new Uint8Array());
+		await fileOperations.move("c.md", "b.md");
+		assertSetOnlyContains(fs.names, "b.md", "b (1).md");
+	});
+
+	test("should deconflict renames with paths", async () => {
+		let fs = new FakeFileSystemOperations();
+		let fileOperations = new FileOperations(new Logger(), fs);
+
+		await fileOperations.create("a/b.c/d", new Uint8Array());
+		await fileOperations.create("a/b.c/e", new Uint8Array());
+		await fileOperations.move("a/b.c/d", "a/b.c/e");
+		assertSetOnlyContains(fs.names, "a/b.c/e", "a/b.c/e (1)");
+	});
+});
+
+function assertSetOnlyContains<T>(set: Set<T>, ...values: T[]) {
+	assert(
+		set.size === values.length &&
+			Array.from(set).every((value) => values.includes(value)),
+		`Expected set to contain only ${values.map((v) => '"' + v + '"').join(", ")}, but it contained ${Array.from(
+			set
+		)
+			.map((v) => '"' + v + '"')
+			.join(", ")}`
+	);
+}

frontend/sync-client/src/file-operations/file-operations.ts

@@ -21,7 +21,6 @@ export class FileOperations {
 	}
 
 	public async read(path: RelativePath): Promise<Uint8Array> {
-		this.logger.debug(`Reading file: ${path}`);
 		const content = await this.fs.read(path);
 
 		if (isBinary(content)) {
@@ -38,17 +37,14 @@ export class FileOperations {
 	}
 
 	public async getFileSize(path: RelativePath): Promise<number> {
-		this.logger.debug(`Getting file size: ${path}`);
 		return this.fs.getFileSize(path);
 	}
 
 	public async getModificationTime(path: RelativePath): Promise<Date> {
-		this.logger.debug(`Getting modification time: ${path}`);
 		return this.fs.getModificationTime(path);
 	}
 
 	public async exists(path: RelativePath): Promise<boolean> {
-		this.logger.debug(`Checking existence of ${path}`);
 		return this.fs.exists(path);
 	}
 
@@ -58,7 +54,6 @@ export class FileOperations {
 		path: RelativePath,
 		newContent: Uint8Array
 	): Promise<void> {
-		this.logger.debug(`Creating file: ${path}`);
 		if (await this.fs.exists(path)) {
 			this.logger.debug(
 				`Didn't expect ${path} to exist, when trying to create it, merging instead`
@@ -79,7 +74,6 @@ export class FileOperations {
 		expectedContent: Uint8Array,
 		newContent: Uint8Array
 	): Promise<Uint8Array> {
-		this.logger.debug(`Writing file: ${path}`);
 		if (!(await this.fs.exists(path))) {
 			this.logger.debug(
 				`The caller assumed ${path} exists, but it no longer, so we wont recreate it`
@@ -133,24 +127,25 @@ export class FileOperations {
 		oldPath: RelativePath,
 		newPath: RelativePath
 	): Promise<void> {
-		this.logger.debug(`Moving file: ${oldPath} -> ${newPath}`);
-
 		if (oldPath === newPath) {
 			return;
 		}
 
-		await this.createParentDirectories(newPath);
+		if (await this.fs.exists(newPath)) {
+			const deconflictedPath = await this.deconflictPath(newPath);
+			this.logger.debug(
+				`Conflict when moving '${oldPath}' to '${newPath}', '${newPath}' already exists, deconflicting by moving it to '${deconflictedPath}'`
+			);
+			this.fs.rename(newPath, deconflictedPath);
+		} else {
+			await this.createParentDirectories(newPath);
+		}
+
 		await this.fs.rename(oldPath, newPath);
 	}
 
-	public isFileEligibleForSync(_path: RelativePath): boolean {
+	public isFileEligibleForSync(path: RelativePath): boolean {
-		return true;
+		return isFileTypeMergable(path);
-		// TODO: figure this out
-		// if (Platform.isDesktopApp) {
-		// 	return true;
-		// }
-
-		// return isFileTypeMergable(path);
 	}
 
 	private async createParentDirectories(path: string): Promise<void> {
@@ -165,4 +160,34 @@ export class FileOperations {
 			}
 		}
 	}
+
+	private async deconflictPath(path: RelativePath): Promise<RelativePath> {
+		const pathParts = path.split("/");
+		const fileName = pathParts.pop()!;
+
+		let directory = pathParts.join("/");
+		if (directory) {
+			directory += "/";
+		}
+
+		const nameParts = fileName.split(".");
+		const extension =
+			nameParts.length > 1 ? "." + nameParts[nameParts.length - 1] : "";
+		const stem = extension ? nameParts.slice(0, -1).join(".") : fileName;
+		let currentCount = Number.parseInt(
+			/ \((\d+)\)$/.exec(stem)?.groups?.[0] ?? "0"
+		);
+
+		while (true) {
+			const newName =
+				currentCount === 0
+					? `${directory}${stem}${extension}`
+					: `${directory}${stem} (${currentCount})${extension}`;
+			if (await this.fs.exists(newName)) {
+				currentCount++;
+			} else {
+				return newName;
+			}
+		}
+	}
 }