files - fix issue with etag computation

src/vs/workbench/services/files/common/fileService.ts

@@ -337,7 +337,10 @@ export class FileService extends Disposable implements IFileService {
 		// check for size is a weaker check because it can return a false negative if the file has changed
 		// but to the same length. This is a compromise we take to avoid having to produce checksums of
 		// the file content for comparison which would be much slower to compute.
-		if (options && typeof options.mtime === 'number' && typeof options.etag === 'string' && options.etag !== ETAG_DISABLED && options.mtime < stat.mtime && options.etag !== etag(stat.size, options.mtime)) {
+		if (
+			options && typeof options.mtime === 'number' && typeof options.etag === 'string' &&
+			options.etag !== ETAG_DISABLED && options.mtime < stat.mtime && options.etag !== etag(options.mtime /* not using stat.mtime for a reason, see above */, stat.size)
+		) {
 			throw new FileOperationError(localize('fileModifiedError', "File Modified Since"), FileOperationResult.FILE_MODIFIED_SINCE, options);
 		}
 
@@ -492,8 +495,8 @@ export class FileService extends Disposable implements IFileService {
 			throw new FileOperationError(localize('fileIsDirectoryError', "Expected file {0} is actually a directory", this.resourceForError(resource)), FileOperationResult.FILE_IS_DIRECTORY, options);
 		}
 
-		// Return early if file not modified since
-		if (options && options.etag === stat.etag) {
+		// Return early if file not modified since (unless disabled)
+		if (options && options.etag !== ETAG_DISABLED && options.etag === stat.etag) {
 			throw new FileOperationError(localize('fileNotModifiedError', "File not modified since"), FileOperationResult.FILE_NOT_MODIFIED_SINCE, options);
 		}
 

src/vs/workbench/services/files/test/node/diskFileService.test.ts

@@ -15,12 +15,13 @@ import { getPathFromAmdModule } from 'vs/base/common/amd';
 import { copy, rimraf, symlink, RimRafMode, rimrafSync } from 'vs/base/node/pfs';
 import { URI } from 'vs/base/common/uri';
 import { existsSync, statSync, readdirSync, readFileSync, writeFileSync, renameSync, unlinkSync, mkdirSync } from 'fs';
-import { FileOperation, FileOperationEvent, IFileStat, FileOperationResult, FileSystemProviderCapabilities, FileChangeType, IFileChange, FileChangesEvent, FileOperationError, etag } from 'vs/platform/files/common/files';
+import { FileOperation, FileOperationEvent, IFileStat, FileOperationResult, FileSystemProviderCapabilities, FileChangeType, IFileChange, FileChangesEvent, FileOperationError } from 'vs/platform/files/common/files';
 import { NullLogService } from 'vs/platform/log/common/log';
 import { isLinux, isWindows } from 'vs/base/common/platform';
 import { IDisposable, dispose } from 'vs/base/common/lifecycle';
 import { isEqual } from 'vs/base/common/resources';
 import { VSBuffer, VSBufferReadable } from 'vs/base/common/buffer';
+import { timeout } from 'vs/base/common/async';
 
 function getByName(root: IFileStat, name: string): IFileStat | null {
 	if (root.children === undefined) {
@@ -1336,20 +1337,27 @@ suite('Disk File Service', () => {
 		assert.equal(readFileSync(resource.fsPath), newContent);
 	});
 
-	test('writeFile (error when writing to file that has been updated meanwhile)', async () => {
+	test('writeFile - error when writing to file that has been updated meanwhile', async () => {
 		const resource = URI.file(join(testDir, 'small.txt'));
 
-		const stat = await service.resolve(resource);
+		const statBeforeWrite = await service.resolve(resource);
 
-		const content = readFileSync(resource.fsPath);
+		const content = readFileSync(resource.fsPath).toString();
 		assert.equal(content, 'Small File');
 
+		await timeout(101); // account for mtime precision
+
 		const newContent = 'Updates to the small file';
-		await service.writeFile(resource, VSBuffer.fromString(newContent), { etag: stat.etag, mtime: stat.mtime });
+		const statAfterWrite = await service.writeFile(resource, VSBuffer.fromString(newContent), { etag: statBeforeWrite.etag, mtime: statBeforeWrite.mtime });
+
+		assert.notEqual(statAfterWrite.etag, statBeforeWrite.etag);
+		assert.notEqual(statAfterWrite.mtime, statBeforeWrite.mtime);
+
+		const newContentLeadingToError = newContent + newContent;
 
 		let error: FileOperationError | undefined = undefined;
 		try {
-			await service.writeFile(resource, VSBuffer.fromString(newContent), { etag: etag(0, 0), mtime: 0 });
+			await service.writeFile(resource, VSBuffer.fromString(newContentLeadingToError), { etag: statBeforeWrite.etag, mtime: statBeforeWrite.mtime });
 		} catch (err) {
 			error = err;
 		}
@@ -1359,6 +1367,34 @@ suite('Disk File Service', () => {
 		assert.equal(error!.fileOperationResult, FileOperationResult.FILE_MODIFIED_SINCE);
 	});
 
+	test('writeFile - no error when writing to file where size is the same', async () => {
+		const resource = URI.file(join(testDir, 'small.txt'));
+
+		const statBeforeWrite = await service.resolve(resource);
+
+		const content = readFileSync(resource.fsPath).toString();
+		assert.equal(content, 'Small File');
+
+		await timeout(101); // account for mtime precision
+
+		const newContent = content; // same content
+		const statAfterWrite = await service.writeFile(resource, VSBuffer.fromString(newContent), { etag: statBeforeWrite.etag, mtime: statBeforeWrite.mtime });
+
+		assert.notEqual(statAfterWrite.etag, statBeforeWrite.etag);
+		assert.notEqual(statAfterWrite.mtime, statBeforeWrite.mtime);
+
+		const newContentLeadingToNoError = newContent; // writing the same content should be OK
+
+		let error: FileOperationError | undefined = undefined;
+		try {
+			await service.writeFile(resource, VSBuffer.fromString(newContentLeadingToNoError), { etag: statBeforeWrite.etag, mtime: statBeforeWrite.mtime });
+		} catch (err) {
+			error = err;
+		}
+
+		assert.ok(!error);
+	});
+
 	test('watch - file', done => {
 		const toWatch = URI.file(join(testDir, 'index-watch1.html'));
 		writeFileSync(toWatch.fsPath, 'Init');

src/vs/workbench/services/textfile/common/textFileEditorModel.ts

@@ -301,7 +301,7 @@ export class TextFileEditorModel extends BaseTextEditorModel implements ITextFil
 		// Decide on etag
 		let etag: string | undefined;
 		if (forceReadFromDisk) {
-			etag = undefined; // reset ETag if we enforce to read from disk
+			etag = ETAG_DISABLED; // disable ETag if we enforce to read from disk
 		} else if (this.lastResolvedDiskStat) {
 			etag = this.lastResolvedDiskStat.etag; // otherwise respect etag to support caching
 		}