-
Notifications
You must be signed in to change notification settings - Fork 806
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
Feature/sync unix symlinks as links #6205
base: master
Are you sure you want to change the base?
Feature/sync unix symlinks as links #6205
Conversation
AppImage file: nextcloud-PR-6205-000987f9111afe85b85875b83d37ce6b99699017-x86_64.AppImage |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @taminob really appreciate the effort in bringing this forward on both the client and server ends
How do you plan to handle these symlinks on non-Unix systems (mainly Windows)? I think this is the big question as the policy in the client so far has been to prefer restricting filenames and filetypes to those that are universally compatible to avoid issues
@claucambra I planned to mainly ignore non-Unix systems for now and only ensure that it doesn't break the build and current behavior on those systems. I think that for now it's probably best to just not download symlinks from the server if on a Windows system. Could be that Windows will be relatively easy due to NTFS symlinks (I have no experience with them) and we only have to provide custom "symlink" and "readlink" implementations. Also, I'll look into including an option to toggle this feature and (at least for an experimental phase) default it to off so that the current behavior will be kept as-is. |
0e645eb
to
4719069
Compare
b619ba5
to
cfe27b5
Compare
489725a
to
132aa68
Compare
Signed-off-by: Tamino Bauknecht <[email protected]>
Signed-off-by: Tamino Bauknecht <[email protected]>
Signed-off-by: Tamino Bauknecht <[email protected]>
Before, the common assumption was being made that if an item is not a directory, it must be a file (or the other way around). Signed-off-by: Tamino Bauknecht <[email protected]>
Signed-off-by: Tamino Bauknecht <[email protected]>
Signed-off-by: Tamino Bauknecht <[email protected]>
Signed-off-by: Tamino Bauknecht <[email protected]>
Use SymLinkUploadDevice for symlinks in bulk uploads and add the header field X-File-Type to recognize symlinks on the server. Signed-off-by: Tamino Bauknecht <[email protected]>
Signed-off-by: Tamino Bauknecht <[email protected]>
Signed-off-by: Tamino Bauknecht <[email protected]>
Signed-off-by: Tamino Bauknecht <[email protected]>
Signed-off-by: Tamino Bauknecht <[email protected]>
Signed-off-by: Tamino Bauknecht <[email protected]>
Signed-off-by: Tamino Bauknecht <[email protected]>
This is required to be able to calculate the checksum for symlinks without code duplication. The commit reverts some of the removal done in commit dd178f0. Signed-off-by: Tamino Bauknecht <[email protected]>
Signed-off-by: Tamino Bauknecht <[email protected]>
Signed-off-by: Tamino Bauknecht <[email protected]>
Signed-off-by: Tamino Bauknecht <[email protected]>
This commit reverts the effects of commit "libsync: Remove auto-exclude for symlinks" (5812d5c). Signed-off-by: Tamino Bauknecht <[email protected]>
This reverts commit 73063eb. Since the checksum calculation on symlinks is required in multiple places, it is better to move this differentiation to the checksum calculation class. Signed-off-by: Tamino Bauknecht <[email protected]>
…tembase.h Signed-off-by: Tamino Bauknecht <[email protected]>
Signed-off-by: Tamino Bauknecht <[email protected]>
Signed-off-by: Tamino Bauknecht <[email protected]>
Signed-off-by: Tamino Bauknecht <[email protected]>
Signed-off-by: Tamino Bauknecht <[email protected]>
Signed-off-by: Tamino Bauknecht <[email protected]>
Signed-off-by: Tamino Bauknecht <[email protected]>
The issue was introduced in "libsync/common: Store actual type for items". Additionally, the variable/parameter "directoriesToRemove" was renamed to "remoteItemsToRemove" since it could now also be a file or symlink. Signed-off-by: Tamino Bauknecht <[email protected]>
Signed-off-by: Tamino Bauknecht <[email protected]>
Signed-off-by: Tamino Bauknecht <[email protected]>
This commit will preserve the permissions set on the symlink if a new file is downloaded in the same place and the symlink is overwritten. That fixes a bug which caused the downloaded file to become unreadable if the symlink was broken and no permissions could be retrieved. Signed-off-by: Tamino Bauknecht <[email protected]>
…undoing it) Instead of copying the permissions on the symlink, use the default permissions in that case. Otherwise, the newly downloaded files might become executable since symlinks often have with all permissions set. Signed-off-by: Tamino Bauknecht <[email protected]>
Signed-off-by: Tamino Bauknecht <[email protected]>
132aa68
to
38f5dac
Compare
Any way we can help move this PR forward so it lands? |
Since this PR depends on nextcloud/server#41321 and the server maintainers are actively ignoring it, I don't think it makes sense to spend any time on this PR here. |
Hi, Is someone aware of a fork of nextcloud with this feature merged ? If someone would do it and setup some Patreon/donation I would gladly pay for this feature and I am sure many others will |
@taminob - It looks like the server-side portion was quietly approved/merged into 29.0.0 beta 2 |
Thanks for mentioning it @f1d094 I wasn't really sure given the silence from the maintainers side. |
Hi @claucambra, on server side the proposal was rejected on the basis that the desktop app does not support an apps system to make the symlink support optional. However, I think that it should be possible to extend the desktop app such that the symlink support is in the code base, but can be toggled by the user similar to how it is done for the end-to-end encryption support (or also external storage). What do you think? Is this for you actually a major blocker or shouldn't it be possible to find a solution for that? |
@claucambra ping |
@claucambra Claudio, after a few months away I received a few pings from other threads and it was brought to my attention that the core-holdup is here. I would like to share with you a cross-post I made for the server team: First...I should point out that the lack of proper symlink synchronization is already causing a lot of issues and should be considered a bug, not a feature. As currently implemented Nextcloud actively breaks multi-client Linux/Mac Nextcloud setups. Secondly...I'd like to stress that if implemented correctly, it should be invisible to Windows users and transparently supported on Linux. Allow me to make the case:
Why is this at all complicated? Can you provide a use-case where the above would break any given install or cause comlications? Please explain. We, as a team of Mac and Linux users, are incredibly frustrated with Nextcloud because of this very broken aspect of the way Nextcloud handles POSIX filesystems. If you claim to support Linux as a client, symlinks must be properly supported. They are a critcial and integral part of computing-on-Linux. |
Symlinks under Windows are no problem at all with NTFS as the standard file system. |
I have not used windows in a long time but it was my understanding that symlinks were not enabled by default. Nextcloud can neither anticipate nor require that users have them enabled in order for their client to function. Therefore, they should likely only be an option for Windows. Unless of course this has changed and they are enabled by default now on any version of the operating system currently supported. |
Symbolic links under windows have been available and used for a long time. |
Summary
This PR allows the synchronization of Unix symlinks on Unix-like systems.
The symlinks will be synchronized as links.
I try to provide an MVP implementation which, in the future, can be further extended to improve usability and add more features.
TODO
Features:
setModTime
andreadlink
) since Qt handles shortcut files (.lnk
) as Windows symlinks)Known bugs:
Tasks:
Related
Resolves #250 (and probably also #5509).
This change requires nextcloud/server#41321 for the nextcloud server.
Disclaimer
This PR is still in a WIP state, do not test this on a production Nextcloud server.
Also, everything in there is still subject to change and hacky solutions and code full with TODOs will appear in this PR.