Files incorrectly opened for delete

[allender]

This was originally a bug reported by another dev here on the old fork. I am reposting here since you are actively maintaining this repo now :) Here is a link to the original bug post.

With deleteOnFileDelete:true, performing a sync in the P4V client will cause files that have been removed from the depot to now be marked as Open for Delete.

For example:
User A syncs to head revision on a folder.
User B deletes several files in that folder and then submits those deletes.
With VS Code open, User A now syncs to head revision on that same folder.
The files that User B deleted are now removed from User A's system, but VS Code detects that removal and marks those same files for delete. The files are no longer on disk, but the Default Changelist now has all of those deleted files showing up as open for delete.

I would expect VS Code to detect the removal of those files and then verify whether they have been intentionally removed before doing a second Open for Delete on a file that's already deleted.l,

Versions & Details:

• Perforce extension: 3.6.2
• VSCode version: 1.42.1
• Operating System: Windows 10
• Perforce server details (if known):
• Are you using a multi-root workspace?: No

Additional context

Please open view -> Output, select "Perforce Log" from the dropdown and paste it here, if possible / relevant

[allender]

@mjcrouch I thought about this some more. I actually think that there is a fundamental problem with the plugin. It should not be file watching the folders. It should, rather 'p4 delete' files if they are deleted from VS code explorer. I say this for a couple of reasons:

• perforce operations should be determined directly by action from the user (i.e. adding/deleting a file from VS code explorer) and not some action that happens in the filesystem. Just from the 'use p4v to sync my workspace' scenario, needless 'p4 add's or 'p4 delete's would be issued. Not to mention some other tool that could potentially do work in your workspace tree. The p4vs plugin (from perforce) operates this way I believe. It is not a file watcher
• by adding/deleting files via file watcher, it's more of an onus to keep a p4ignore file working correctly. Today, I can have folders and files that I know will never get checked in because of the way our pipelines and tools work, but if I have VScode start adding files randomly because of file watching, this is problematic. Tools should operate on what they themselves do, not in response to some external event. VScode should never be responsible for adding/deleting files via watcher. That's not it's job. It's job is to work on files that the user is operating on.
• by not watching the files system, we can keep needless p4 operations out of the queue. We only delete or add when we want to. We don't start spamming large amounts of p4 commands that are needless. Think about this situation. I have vscode open and sync a new folder with thosands of files. The vscode plugin now will add (one at a time) files that were just created on the filesystem due to a sync. They were p4 added by someone else, but the plugin will also try to add them (one at a time). It's a similar situation for deletes (and can cause problems like this bug).
• Handling of large number of adds/delete really needs to be batched to prevent swamp in situations like the previous bullet. This is tricky code and can then delay potential adds/deletes to later (which then can run into other problems).

So my suggestion (and I will try and implement it) is to not use file watchers. But on adding a new file via vscode explorer we can 'p4 add' (if the option is checked) and deleting a file out of VS code explorer we will 'p4 delete' if the option is set.

[mjcrouch]

Indeed, this has been on my mind, I think you're right.

The original PR back in 2015 doesn't have any particular justification for using the FileSystem watcher over the internal events (I have no idea if the events like OnDidCreateFiles even existed at that time)

It's hard to be sure whether there is anyone deliberately using this feature via the filesystem watcher. I've seen mentions from the original creator about using external tools to automate things externally, but this isn't really our responsibility as an IDE extension, and I think it is better to be more explicit / consistent* and restrict these automatic actions to things that have happened within the IDE

Probably, the people negatively impacted by using a fs watcher is greater than the people positively impacted by it - and I'm trying not to worry too much about breaking niche use cases as a fork in the relatively early stages

* since edit on file save is already for IDE actions only, as far as I can tell

[allender]

Hello - I have a potential prototype of the work here. I did a small amount of refactoring and some renaming to get some function names consistent (pet peeve). The tests seem to work, although I wasn't entirely sure if the integration tests were fully working or not. Wanted to maybe ask about those first before submitting pull request.

In looking at the command code too, there is some other cleanup work that would be awesome to do (as the original code was using promises before the command limiter code was put in).

Oh, it will require upgrade to vscode 1.42. The create and delete events are not available in 1.40

[mjcrouch]

Looks good so far

The tests seem to work, although I wasn't entirely sure if the integration tests were fully working or not.

Did you get a summary of test results at the end (depending on whether you ran them from npm or the launch config, they would be in the terminal or in the debug console)? IIRC it's around 166 passing and a few pending that I have as placeholders to do something about later. I don't think I've written any tests for the areas you've touched yet. Also it should automatically run the tests on Azure when you create a PR (even a draft one I think) so if you do have trouble we can see how it runs on there anyway

Oh, it will require upgrade to vscode 1.42. The create and delete events are not available in 1.40

Didn't realise that - you also need to change engines/vscode in the package.json as well :)

Also added a comment to one line on the last commit about the revert followed by a delete

In looking at the command code too, there is some other cleanup work that would be awesome to do

As I mentioned in #5 I'm planning to remove all of the calls to things like PerforceService.execute from all over the code and replace them with calls to functions in PerforceApi.ts so there's one stop for all the command line handling (and ultimately, probably merge the PerforceService stuff in to that area so there aren't any weird cyclic dependencies). I prefer the promisified functions but I wrote the command limiter as pretty much a drop-in replacement for the bottleneck package, which was using callbacks.
I'm also generally replacing all the promise.then / promise.catch stuff with async / await / try / catch when appropriate