From 7155fc2b9db940b2610628bea924699f1e74aa6f Mon Sep 17 00:00:00 2001 From: Andrew Dupont Date: Sun, 18 Oct 2026 12:42:55 -0700 Subject: [PATCH] =?UTF-8?q?Make=20`resurrect`=20idempotent=20in=20more=20s?= =?UTF-8?q?cenarios=E2=80=A6?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit …so that it's safer to call when you don't know much about the buffer. Also ensure we clear a conflicted state whenever a deleted file is restored with the same base text that the buffer expected. --- spec/text-buffer-io-spec.js | 232 +++++++++++++++++++++++++++++++++++- src/text-buffer.js | 84 ++++++++----- 2 files changed, 283 insertions(+), 33 deletions(-) diff --git a/spec/text-buffer-io-spec.js b/spec/text-buffer-io-spec.js index 4f0d28853..2eed718b5 100644 --- a/spec/text-buffer-io-spec.js +++ b/spec/text-buffer-io-spec.js @@ -2,7 +2,7 @@ const fs = require('fs-plus') const path = require('path') const {Writable, Transform} = require('stream') const temp = require('temp') -const {Disposable} = require('event-kit') +const {Disposable, Emitter} = require('event-kit') const Point = require('../src/point') const Range = require('../src/range') const TextBuffer = require('../src/text-buffer') @@ -842,18 +842,34 @@ describe('TextBuffer IO', () => { expect(buffer.getText()).toBe(`lorem ipsum dolor`) }) - it('stays in conflict if it was already in conflict before the file was deleted', async () => { + it('clears an earlier conflict if the file is restored with the contents it had before', async () => { const conflicted = new Promise((resolve) => buffer.onDidConflict(resolve)) fs.writeFileSync(filePath, `dolor sit amet`) await conflicted expect(buffer.isInConflict()).toBe(true) - // Even a file restored with the buffer's base text doesn't resolve - // a conflict that predates the deletion. - await deleteAndResurrect(`lorem ipsum`) + // The conflict predates the deletion, but the file came back with + // exactly what our changes are based on, so there's no longer + // anything to conflict with. + const events = await deleteAndResurrect(`lorem ipsum`) + expect(events.conflictedStatusChanges).toEqual([false]) + expect(buffer.isInConflict()).toBe(false) + expect(buffer.isModified()).toBe(true) + expect(buffer.isDeleted()).toBe(false) + expect(buffer.getText()).toBe(`lorem ipsum dolor`) + }) + + it('stays in conflict if the file is restored with the contents that caused the conflict', async () => { + const conflicted = new Promise((resolve) => buffer.onDidConflict(resolve)) + fs.writeFileSync(filePath, `dolor sit amet`) + await conflicted + expect(buffer.isInConflict()).toBe(true) + + await deleteAndResurrect(`dolor sit amet`) expect(buffer.isInConflict()).toBe(true) expect(buffer.isModified()).toBe(true) expect(buffer.isDeleted()).toBe(false) + expect(buffer.getText()).toBe(`lorem ipsum dolor`) }) }) @@ -1442,8 +1458,214 @@ describe('TextBuffer IO', () => { fs.writeFileSync(filePath, 'moo') }) }) + + describe('.resurrect', () => { + let filePath + + beforeEach(() => { + filePath = path.join(temp.mkdirSync(), 'file-to-resurrect') + }) + + function recordEvents () { + const events = [] + buffer.onDidReload(() => events.push('did-reload')) + buffer.onDidConflict(() => events.push('did-conflict')) + buffer.onDidChangeDeleted((status) => events.push(`did-change-deleted:${status}`)) + return events + } + + async function loadAndDelete (options) { + fs.writeFileSync(filePath, 'abc') + buffer = await TextBuffer.load(filePath, options) + const deleted = new Promise((resolve) => buffer.onDidDelete(resolve)) + fs.unlinkSync(filePath) + await deleted + } + + it('does nothing to a buffer that has never had a file', async () => { + buffer = new TextBuffer('abc') + const events = recordEvents() + await buffer.resurrect() + expect(events).toEqual([]) + expect(buffer.getText()).toBe('abc') + expect(buffer.isDeleted()).toBe(false) + }) + + it('does nothing to a buffer whose file has never existed on disk', async () => { + buffer = new TextBuffer('abc') + buffer.setPath(filePath) + const events = recordEvents() + await buffer.resurrect() + + // Not even if a file shows up at that path later. + fs.writeFileSync(filePath, 'xyz') + await buffer.resurrect() + + expect(events).toEqual([]) + expect(buffer.getText()).toBe('abc') + expect(buffer.isDeleted()).toBe(false) + }) + + it('does nothing to a buffer that was destroyed when its file was deleted', async () => { + fs.writeFileSync(filePath, 'abc') + buffer = await TextBuffer.load(filePath, {shouldDestroyOnFileDelete: () => true}) + const destroyed = new Promise((resolve) => buffer.onDidDestroy(resolve)) + fs.unlinkSync(filePath) + await destroyed + + fs.writeFileSync(filePath, 'abc') + spyOn(buffer, 'subscribeToFile') + await buffer.resurrect() + expect(buffer.subscribeToFile).not.toHaveBeenCalled() + }) + + it('does nothing to a buffer that is not deleted', async () => { + fs.writeFileSync(filePath, 'abc') + buffer = await TextBuffer.load(filePath) + buffer.append('def') + const events = recordEvents() + spyOn(buffer, 'subscribeToFile') + await buffer.resurrect() + expect(events).toEqual([]) + expect(buffer.subscribeToFile).not.toHaveBeenCalled() + expect(buffer.getText()).toBe('abcdef') + expect(buffer.isInConflict()).toBe(false) + }) + + it('does nothing to a buffer whose file is still missing', async () => { + await loadAndDelete({shouldDestroyOnFileDelete: () => false}) + const events = recordEvents() + await buffer.resurrect() + expect(events).toEqual([]) + expect(buffer.isDeleted()).toBe(true) + }) + + it('resurrects a buffer only once when called repeatedly', async () => { + await loadAndDelete({shouldDestroyOnFileDelete: () => false}) + fs.writeFileSync(filePath, 'xyz') + const events = recordEvents() + await Promise.all([buffer.resurrect(), buffer.resurrect()]) + await buffer.resurrect() + expect(events).toEqual(['did-change-deleted:false', 'did-reload']) + expect(buffer.getText()).toBe('xyz') + expect(buffer.isDeleted()).toBe(false) + }) + + describe('when a deleted buffer is given a new path', () => { + let newPath + + beforeEach(async () => { + await loadAndDelete({shouldDestroyOnFileDelete: () => false}) + buffer.append('def') + newPath = path.join(path.dirname(filePath), 'new-file') + }) + + it('leaves the deleted state if nothing exists at the new path', async () => { + const events = recordEvents() + buffer.setPath(newPath) + expect(events).toEqual(['did-change-deleted:false']) + expect(buffer.isDeleted()).toBe(false) + + // Nor does a file appearing at that path later make it resurrectable. + fs.writeFileSync(newPath, 'xyz') + await buffer.resurrect() + expect(events).toEqual(['did-change-deleted:false']) + expect(buffer.getText()).toBe('abcdef') + }) + + it('leaves the deleted state if a file exists at the new path', async () => { + fs.writeFileSync(newPath, 'xyz') + const events = recordEvents() + buffer.setPath(newPath) + expect(events).toEqual(['did-change-deleted:false']) + expect(buffer.isDeleted()).toBe(false) + + // There's nothing left to resurrect. + await buffer.resurrect() + expect(events).toEqual(['did-change-deleted:false']) + expect(buffer.getText()).toBe('abcdef') + }) + }) + + describe('when the file does not report its own deletion', () => { + let file + + beforeEach(async () => { + fs.writeFileSync(filePath, 'abc') + file = new ManualFile(filePath) + buffer = await TextBuffer.load(file) + buffer.append('def') + fs.unlinkSync(filePath) + }) + + it('puts the buffer into the deleted state', async () => { + const events = recordEvents() + await buffer.resurrect() + expect(events).toEqual(['did-change-deleted:true']) + expect(buffer.isDeleted()).toBe(true) + expect(buffer.isModified()).toBe(true) + + // Once the file comes back, the buffer can be resurrected as usual. + fs.writeFileSync(filePath, 'abc') + await buffer.resurrect() + expect(events).toEqual(['did-change-deleted:true', 'did-change-deleted:false']) + expect(buffer.isDeleted()).toBe(false) + expect(buffer.isInConflict()).toBe(false) + expect(buffer.getText()).toBe('abcdef') + }) + + it('is resurrected when the file reports a change after it reappears', async () => { + await buffer.resurrect() + expect(buffer.isDeleted()).toBe(true) + + fs.writeFileSync(filePath, 'xyz') + spyOn(buffer, 'resurrect').and.callThrough() + const conflicted = new Promise((resolve) => buffer.onDidConflict(resolve)) + file.emitChange() + await conflicted + + expect(buffer.resurrect).toHaveBeenCalled() + expect(buffer.isDeleted()).toBe(false) + expect(buffer.isInConflict()).toBe(true) + expect(buffer.getText()).toBe('abcdef') + }) + }) + }) }) +// A file whose `did-change` events are emitted on demand, and which never +// reports its own deletion. +class ManualFile { + constructor (path) { + this.path = path + this.emitter = new Emitter() + } + + existsSync () { + return fs.existsSync(this.path) + } + + getPath () { + return this.path + } + + createReadStream () { + return fs.createReadStream(this.path) + } + + createWriteStream () { + return fs.createWriteStream(this.path) + } + + onDidChange (callback) { + return this.emitter.on('did-change', callback) + } + + emitChange () { + this.emitter.emit('did-change') + } +} + class ReverseCaseFile { constructor (path) { this.path = path diff --git a/src/text-buffer.js b/src/text-buffer.js index 403a64d4c..57120db58 100644 --- a/src/text-buffer.js +++ b/src/text-buffer.js @@ -660,6 +660,10 @@ class TextBuffer { this.subscribeToFile() } + // A deleted buffer that moves to a new path isn't necessarily deleted + // anymore. + this.emitDeletedStatusChanged(this.isDeleted()) + this.emitter.emit('did-change-path', this.getPath()) } @@ -2085,7 +2089,23 @@ class TextBuffer { // Instead, this method exists as an easy way for an external file-watcher to // signal to `TextBuffer` that this buffer's backing file has been // resurrected, and that certain events ought to be emitted. + // + // This method is safe to call speculatively. It does nothing unless the + // buffer is in the deleted state and its file exists again — except that, if + // the buffer is _not_ in the deleted state but its file is missing, it puts + // the buffer into the deleted state, since its deletion went unnoticed. async resurrect () { + // There's nothing to resurrect in a destroyed buffer, or in one that has + // never had a file on disk. + if (this.isDestroyed() || !this.file || !this.didHaveFileOnDisk) return + + if (!this.previousDeletedStatus) { + // We don't think we're deleted. If the file is missing anyway, its + // deletion went unreported; otherwise there's nothing to do. + if (this.isDeleted()) this.handleFileDeletion() + return + } + // Either the file came back but was gone again by the time we ran… or // something is calling this method speculatively (perhaps via polling) in // lieu of a file-watcher. @@ -2101,9 +2121,8 @@ class TextBuffer { // A modified buffer won't reload, and a refused load is recorded as a // conflict. That's correct only if the file came back with different - // contents than what our changes are based on — or if we were already in - // conflict before the file was deleted. - if (this.buffer.isModified() && !this.fileHasChangedSinceLastLoad) { + // contents than what our changes are based on. + if (this.buffer.isModified()) { const source = this.file instanceof File ? this.file.getPath() : this.file.createReadStream() @@ -2114,7 +2133,13 @@ class TextBuffer { // If the file vanished again, `load` will handle it. if (error.code !== 'ENOENT') throw error } - if (matches) return + if (matches) { + // The file is back to what our changes are based on, so there's + // nothing to conflict with, whatever happened before it was deleted. + this.fileHasChangedSinceLastLoad = false + this.emitConflictedStatusChanged(false) + return + } } // This is like calling `reload`, but less aggressive; it will preserve @@ -2446,11 +2471,9 @@ class TextBuffer { if (!this.file.existsSync()) return if (this.outstandingSaveCount > 0) return - // The file exists, so the buffer is no longer deleted, whether or not - // anything ever told it so. A file that is deleted and then recreated - // otherwise leaves the deleted status stuck at `true` forever, which - // would swallow the _next_ deletion. - this.emitDeletedStatusChanged(this.isDeleted()) + // The file exists, but we still think it's deleted, so it must have + // come back without anyone telling us. + if (this.previousDeletedStatus) return this.resurrect() // This file has changed since we last loaded it from disk, but that // does not automatically mean there is a conflict. Set the flag, but @@ -2483,25 +2506,7 @@ class TextBuffer { if (this.file.onDidDelete) { this.fileSubscriptions.add(this.file.onDidDelete(() => { - // A `delete` event is proof that a backing file did exist at one - // point! - this.didHaveFileOnDisk = true - // At this point, asking `isModified` of the native buffer will deliver - // an accurate result that does not care about whether the file still - // exists on disk. - const modified = this.buffer.isModified() - this.retainsUnmodifiedTraitAfterDeletion = !modified - this.emitDeletedStatusChanged(true) - if (!modified && this.shouldDestroyOnFileDelete()) { - return this.destroy() - } else { - return this.emitModifiedStatusChanged(this.isModified()) - } - // We don't set `this.file` to `null` because we still have a - // _theoretical_ file, even if it's no longer present on disk. In this - // scenario, we can re-commit the file to disk at its previous path - // with a simple "Save" command. If we nulled out `file`, the editor - // would prompt the user again about a save destination. + return this.handleFileDeletion() })) } @@ -2523,6 +2528,29 @@ class TextBuffer { } } + // Puts the buffer into the deleted state. Called when the file reports its + // own deletion, or when `resurrect` notices a deletion that went unreported. + handleFileDeletion () { + // A `delete` event is proof that a backing file did exist at one point! + this.didHaveFileOnDisk = true + // At this point, asking `isModified` of the native buffer will deliver an + // accurate result that does not care about whether the file still exists + // on disk. + const modified = this.buffer.isModified() + this.retainsUnmodifiedTraitAfterDeletion = !modified + this.emitDeletedStatusChanged(true) + if (!modified && this.shouldDestroyOnFileDelete()) { + return this.destroy() + } else { + return this.emitModifiedStatusChanged(this.isModified()) + } + // We don't set `this.file` to `null` because we still have a + // _theoretical_ file, even if it's no longer present on disk. In this + // scenario, we can re-commit the file to disk at its previous path with a + // simple "Save" command. If we nulled out `file`, the editor would prompt + // the user again about a save destination. + } + createMarkerSnapshot (selectionsMarkerLayer) { const snapshot = {} for (const markerLayerId in this.markerLayers) {