Skip to content

Fix incorrect isInConflict logic#11

Open
savetheclocktower wants to merge 2 commits intomasterfrom
fix-conflict-after-modification-bug
Open

Fix incorrect isInConflict logic#11
savetheclocktower wants to merge 2 commits intomasterfrom
fix-conflict-after-modification-bug

Conversation

@savetheclocktower
Copy link

This is a blocker for pulsar#1478, which I'd tentatively targeted for version 1.132.0. So it's not a drop-everything-and-review sort of thing, but I'd appreciate if someone took a look at this after we ship 1.131.2.

This is a bit esoteric, but imagine this scenario:

  • You open a TextBuffer backed by a file on disk
  • You make some changes locally that are not yet committed
  • Some other program changes the contents of the file on disk
  • We notice that the file's contents changed, and that they are incompatible with the user's changes; we emit a did-conflict event, and set a flag (fileHasChangedSinceLastLoad) to true to mark the file as conflicted
  • The isInConflict method will thereafter return true, as it will whenever (a) fileHasChangedSinceLastLoad is true and (b) isModified() returns true

Once you save the file again, fileHasChangedSinceLastLoaded is reset to false, since that serves to synchronize the buffer contents and the file contents. Even if you modify the buffer again after saving, isInConflict will correctly return false, since fileHasChangedSinceLastLoaded is still false.

But here's a scenario that slipped through the specs:

  • You open a TextBuffer backed by a file on disk
  • You make some changes locally, then save the file
  • Soon after, we detect the file has changed on disk (never mind that we're the ones who changed it!)
  • fileHasChangedSinceLastLoad flips to true even though the file isn't actually in conflict (and did-conflict was not emitted)
  • Once you modify the buffer again, both halves of the isInConflict method's test are now true, and the buffer is treated as being in conflict even though it isn't

I think the idea was that the other code paths through the onDidChange handler were eventually supposed to result in fileHasChangedSinceLastLoaded flipping back to false… but in practice, we take one of the side doors instead, and return early before the line in finishLoading that actually resets the flag.

In practice, the changes in pulsar#1478 cause the new “this file is in conflict; are you sure you want to save?” dialog to fire much too often… because isInConflict is incorrectly returning true practically all the time after the initial save to disk.

This PR should fix this. There are probably a couple different approaches I could've taken for this, but I went with the one that I knew I could reason about correctly: in the code paths in onDidChange where we know the buffer contents are synchronized with what's on disk, we'll proactively flip fileHasChangedSinceLastLoad back to false.

Testing

The first commit changes an existing spec to demonstrate the problem; if you check out just that commit, the spec will fail.

The next commit makes the failing spec pass again.

…when detected changes to the backing file match what's in the buffer.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant