Fix incorrect isInConflict logic

[savetheclocktower]

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.

Buffer conflict happens when

• A buffer is backed by a file on disk
• The user makes changes locally that are not yet committed
• Some other program changes the contents of the file on disk.

If the buffer were unmodified, we'd simply swap in the new contents of the buffer. But when there are pending buffer changes, we can't do that without losing the user's work. Hence the buffer is now in conflict. We have an isInConflict method that returns a boolean, plus an onDidConflict method that notifies when the buffer enters a conflicted state.

The logic of isInConflict is simple:

  isInConflict () {
    return this.isModified() && this.fileHasChangedSinceLastLoad
  }

fileHasChangedSinceLastLoad is flipped to true whenever we detect a filesystem change for the buffer's file. It should flip to false whenever we load from disk and do something with the contents — or whenever we write to disk, since that guarantees synchronization between the buffer contents and the disk contents.

How it should work

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.

How it actually works

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.

[savetheclocktower]

Added some more explanation. It may be useful; it may just be overkill.

[DeeDeeG]

Sorry for the delay, I think this is next on my to-look-at list.

Not sure how confident I'll be looking into it, but that's for another day...

[DeeDeeG]

Code change looks sensible.

The whole thing (inherited from before this PR) gives me a feeling like walking on a rickety bridge, but... Asking to address all that would broaden the scope of the PR too much, and for all I know this approach (again the overall design from before this PR) is the least-bad option somehow...

(Perhaps a better analogy is a Rube Goldberg machine of pipes leaking into other pipes that catch the water, become heavy and pivot such that they leak intricately into other pipes... Or more benignly, perhaps like those drinking bird toys, always teetering on the edge of states...)

I tried the test approach indicated, and thus verified the new assertions are sensitive to the actual code changes, and verified that the code changes cause the new assertions to pass. For what that's worth.

Smaller PR than I expected!

LGTM.