Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix file watching for symlinked workspace on Linux #19159

Closed
wants to merge 1 commit into from

Conversation

tancop
Copy link

@tancop tancop commented Oct 13, 2024

Release Notes:

  • Fixed file watching for symlinked workspaces on linux

Copy link

cla-bot bot commented Oct 13, 2024

We require contributors to sign our Contributor License Agreement, and we don't have @tancop on file. You can sign our CLA at https://zed.dev/cla. Once you've signed, post a comment here that says '@cla-bot check'.

@tancop
Copy link
Author

tancop commented Oct 13, 2024

@cla-bot check

@cla-bot cla-bot bot added the cla-signed The user has signed the Contributor License Agreement label Oct 13, 2024
Copy link

cla-bot bot commented Oct 13, 2024

The cla-bot has been summoned, and re-checked this pull request!

@tancop tancop force-pushed the fix-symlink-watching branch 3 times, most recently from 65239bd to aab3cfc Compare October 15, 2024 08:27
@tancop
Copy link
Author

tancop commented Oct 15, 2024

@notpeter I rebased and fixed the failing tests, should check for rename events not just delete. Can you run CI again?

@tancop
Copy link
Author

tancop commented Oct 17, 2024

@ConradIrwin

@tancop tancop force-pushed the fix-symlink-watching branch from aab3cfc to d9ab81d Compare October 18, 2024 19:50
@notpeter

This comment was marked as off-topic.

@tancop
Copy link
Author

tancop commented Oct 22, 2024

All tests passed, do you think this is ready for merge?

@tancop
Copy link
Author

tancop commented Nov 2, 2024

@maxdeviant

@ConradIrwin
Copy link
Member

@tancop sorry for the glacial response here!

The change to canonicalize before calling watch makes sense to me; do we also need to canonicalize on every filesystem event? It sounded like from your description on the issue that we were already getting canonical paths in that situation?

@tancop tancop force-pushed the fix-symlink-watching branch from d9ab81d to 0f09217 Compare November 12, 2024 12:53
@tancop tancop force-pushed the fix-symlink-watching branch from 0f09217 to ccc80f3 Compare November 12, 2024 16:55
@tancop
Copy link
Author

tancop commented Nov 12, 2024

@ConradIrwin I rebased on the fs refactor and skipped canonicalize unless the root path is a symlink or the event path we got from notify doesn't match. Not exactly elegant but it works

@ConradIrwin
Copy link
Member

@tancop Thanks!

Why is that needed? It sounded like from your earlier notes that we already got canonical paths from the notify events?

Can you also help me nail down the reproduction steps please: #17161 (comment)

@tancop
Copy link
Author

tancop commented Nov 13, 2024

@ConradIrwin delete and rename events can return non canonical paths because the file doesn't exist anymore so notify can't canonicalize it internally. Same thing happens when the root directory is a descendant of a symlink for whatever reason, I tried to remove that check but it broke.

For the reproduction steps:

  • Create a directory
  • Create a symlink to that directory
  • Open the symlink in Zed
  • Create a file in the original directory outside of Zed (touch, vscode, file manager)
  • Zed doesn't detect the file

@tancop
Copy link
Author

tancop commented Nov 17, 2024

@ConradIrwin do i also need to change repro steps on the issue?

@ConradIrwin
Copy link
Member

@tancop I feel like I'm missing something obvious, but I cannot reproduce this problem as you describe....
Screencast from 2024-11-19 08-46-42.webm

Is there something different about our setups, or is there something else I need to do to get into the broken state?

@tancop
Copy link
Author

tancop commented Nov 20, 2024

@ConradIrwin I tested it with your commands and everything works that way. It breaks when I open the same folder with the recents picker inside zed so the real issue might be in there.

@ConradIrwin
Copy link
Member

Aha!

In that case, I think #21039 is probably a better fix. We can avoid this state entirely by opening the target of the symlink (as we do when you run zed from the command line).

@tancop
Copy link
Author

tancop commented Nov 22, 2024

@ConradIrwin thanks for the link!

Closed in favor of #21039

@tancop tancop closed this Nov 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed The user has signed the Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Symlink as workspace root breaks file watching
4 participants