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

Adds Symlink Plugin #84

Closed
wants to merge 1 commit into from
Closed

Conversation

chrisdmacrae
Copy link

No description provided.

@lishid
Copy link
Collaborator

lishid commented Dec 16, 2020

Hmm this is interesting. I'm assuming a plain rename makes symlinks just work? Does this work cross platform?

If it works just like that, then we should probably just support it from the app instead of applying a hack like this. Let me know if you get a chance to test on all platforms.

@chrisdmacrae
Copy link
Author

chrisdmacrae commented Dec 16, 2020

@lishid I tests this on windows 10 and MacOS Catlina and Big Sur, and it worked. My assumption is if it works on Apple's UNIX, it works on UNIX.

@lishid
Copy link
Collaborator

lishid commented Dec 16, 2020

Alright that's good to hear. Give me some time to first see if we can get built in support. If I get stuck I'll pull the plugin into the repo. Is that ok with you?

@pjeby
Copy link
Contributor

pjeby commented Feb 10, 2021

It looks like what this plugin is doing is working around the fact that fs.watch doesn't follow symlinks. But it's doing so by breaking and recreating the symlinks every 3 seconds to force Obsidian to treat the file(s) as changed, or to rescan folder contents.

I could be wrong, but I don't think this will actually generate the correct events when files' contents change outside Obsidian.

ISTM that the way to actually make this work would be to either switch Obsidian to a symlink-supporting watch library (e.g. chokidar), or else change the FileSystemAdapter to add watch paths for symlinks. (Ironically, it looks like Obsidian on Linux might already be close to supporting directory symlinks, since it adds watch paths for folders, and AFAICT that includes symlinked folders.)

I think, however, that symlinks should only be supported for directories, and then only to paths outside the current vault. If you have symlinks pointing within a vault, it seems like it could introduce the possibility for all sorts of new bugs or UX issues. (e.g. silliness like doubled tag counts, duplicate search results, etc.) At the very least, it would mean issuing duplicate fs events for each symlink when a file's contents are changed. (Or for deletes.) And ISTM there are also lots of interesting edge cases (aka latent bugs) for symlinks at the individual file level, whether targeted inside or outside the vault.

I did a quick test on Windows, and if you use mklink /j to make a folder symlink from inside a vault to somewhere outside it, Obsidian will work correctly except for detecting changes made outside Obsidian. But if I then do app.vault.adapter.startWatchPath("SymlinkedFolder", true), then changes made outside Obsidian appear to be detected, whether they are done in the original folder or the symlinked one.

It would probably be worth investigating if this works on OS X and Linux as well, since it basically means with some minor changes to the vault's reconciliation algorithm (to add folder watches if they're symlinked, regardless of platform), then Obsidian could natively support symlinked folders.

@chrisdmacrae
Copy link
Author

It looks like what this plugin is doing is working around the fact that fs.watch doesn't follow symlinks. But it's doing so by breaking and recreating the symlinks every 3 seconds to force Obsidian to treat the file(s) as changed, or to rescan folder contents.

I could be wrong, but I don't think this will actually generate the correct events when files' contents change outside Obsidian.

ISTM that the way to actually make this work would be to either switch Obsidian to a symlink-supporting watch library (e.g. chokidar), or else change the FileSystemAdapter to add watch paths for symlinks. (Ironically, it looks like Obsidian on Linux might already be close to supporting directory symlinks, since it adds watch paths for folders, and AFAICT that includes symlinked folders.)

I think, however, that symlinks should only be supported for directories, and then only to paths outside the current vault. If you have symlinks pointing within a vault, it seems like it could introduce the possibility for all sorts of new bugs or UX issues. (e.g. silliness like doubled tag counts, duplicate search results, etc.) At the very least, it would mean issuing duplicate fs events for each symlink when a file's contents are changed. (Or for deletes.) And ISTM there are also lots of interesting edge cases (aka latent bugs) for symlinks at the individual file level, whether targeted inside or outside the vault.

I did a quick test on Windows, and if you use mklink /j to make a folder symlink from inside a vault to somewhere outside it, Obsidian will work correctly except for detecting changes made outside Obsidian. But if I then do app.vault.adapter.startWatchPath("SymlinkedFolder", true), then changes made outside Obsidian appear to be detected, whether they are done in the original folder or the symlinked one.

It would probably be worth investigating if this works on OS X and Linux as well, since it basically means with some minor changes to the vault's reconciliation algorithm (to add folder watches if they're symlinked, regardless of platform), then Obsidian could natively support symlinked folders.

You are 100% correct -- I view this as a very temporary solution, but a necessary one for people who use mobile markdown clients to send stuff to their obsidian inbox.

@pjeby
Copy link
Contributor

pjeby commented Feb 13, 2021

Some more background: nodejs/node#30646

It seems that on some platforms, Node's readdir does the equivalent of lstat rather than stat, so Obsidian ends up not detecting the symlinks as directories, and ignores them at startup. That's why this plugin works, as does creating the symlink after Obsidian starts: Obsidian normally uses stat() instead of using readdir to find out a file's type, at least once the adapter is finished starting up.

Anyway, the results of readdir's withFileTypes option are extremely platform-specific. On Windows, the dirent for a symlink to a directory will have isSymbolicLink() == true, while on OS X (according to the above bug), it will have isFile()==true. (And I'm not sure what Linux does.)

The tl;dr here seems to be that to avoid this mess, FileSystemAdapter.listRecursiveChild should explicitly call stat() to determine what type of things it's dealing with, instead of relying on withFileTypes.

Based on this information, I've drafted a short plugin that hotfixes FileSystemAdapter's .listRecursiveChild and .reconcileFolderCreation methods to (respectively):

  1. replace the readdir dirent with a proper stat(), and
  2. check for symlink and do a startWatchPath with the correct flags as applicable.

Combined with a refresh of the tree at plugin startup, this seems to be enough to get Obsidian to both notice the symlinks and notice changes made outside Obsidian, without needing to delete and recreate the symlinks every few seconds.

Perhaps if I can get some feedback on how well this plugin works on non-Windows platforms, the approach could be used as a basis for core support of symlinks.

(Note: Currently, the plugin does not attempt to fix FileSystemAdapter.list(), as Obsidian core only uses .list() to access things under .obsidian. But if anybody is using .list for something in a plugin, it won't work properly with symlinks unless .list() is also fixed to stat() its items rather than relying on the dirents returned by readdir being valid stats.)

@lishid
Copy link
Collaborator

lishid commented Feb 13, 2021

How should we deal with recursive symlinks, has anyone tested this?

I can probably port in the lstat code but I want to make sure we can handle all the hairy edge cases first.

@pjeby
Copy link
Contributor

pjeby commented Feb 13, 2021

Do you mean a symlink loop? That could be a problem. (Merely recursive ones, i.e. symlinked dir within a symlinked dir should not be a problem.)

In principle, a loop could be detected by checking the realpath() of a symlink target, and verifying that it does not match the realpath() of any parent folder. That would prevent infinite recursion. A bit of a PITA to do, but possible. Hairy to implement, however, because it needs to be caught by reconcileFile as well as listRecursiveChild, or else creating the symlink after the vault loads will still trigger at least one level of recursion.

(I'm not sure I'm going to add that to the plugin, at least not until I have a chance to think about whether there's an easier way to do it. Node unfortunately doesn't have a convenient way to test if something is the "same" file... though it looks like you can tell stat() to return a bigint for windows inode compatibility, so I suppose one could just check to see if the inode has already been seen in the vault. That would prevent adding multiple paths to the same inode generally, though it would require some additional work to maintain the cache. Ugh. Gonna have to give that one some more thought.)

@pjeby
Copy link
Contributor

pjeby commented Feb 13, 2021

I have now documented the major possible corner cases and issues at https://github.com/pjeby/obsidian-symlinks#limitations , including notes on symlink loops, settings saving, sync conflicts, and other potential challenges.

@lishid
Copy link
Collaborator

lishid commented Feb 13, 2021

Obsidian will natively have symlink support in an upcoming release! Will close this one.

Thanks to all for the discussions and investigations done!

@lishid lishid closed this Feb 13, 2021
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.

3 participants