-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
lib/fs,model: Clarify errors for Android filenames (fixes #9395) #9397
base: main
Are you sure you want to change the base?
lib/fs,model: Clarify errors for Android filenames (fixes #9395) #9397
Conversation
) Currently, Syncthing tries to sync files to Android devices with no consideration for its filename restrictions (see [1]). This commit makes it so that similarly to Windows, Syncthing relies on a hard-coded list of characters which are invalid on Android and displays a specific error message when user tries to sync a file with one of the characters in its name. [1] https://stackoverflow.com/a/64021421 Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
// even if the filesystem is not FAT. On Windows, they apply to both FAT | ||
// and NTFS. | ||
// Ref: https://cs.android.com/android/platform/superproject/main/+/main:packages/providers/MediaProvider/src/com/android/providers/media/util/FileUtils.java;drc=9d054a9eb6a8d8c3d79e007ea6d2f89c94a52f06;l=485 | ||
const fatDisallowedCharacters = (`/\<>:"|?*` + |
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.
androidDisallowedCharacters?
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.
Isn't fat
better in this case? It's more generic and that's essentially where these characters come from (even though they are applied on a wider scale than just on FAT).
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.
I can imagine someone reading the code later on thinking "what the hell, you can't blindly assume that the user has a fat filesystem. *reads comment* Ah, that's why".
So, Linux/Mac/BSD users on exFAT filesystems, will still see the generic message, yes? |
Yeah, but on those you would need to actually detect the filesystem, while here both Android and Windows are a special case, where the restrictions apply regardless of the filesystem. Just for information, I've changed the PR to a draft, as I need to figure out how to fix the failing tests. |
This change will globally disallow FAT reserved characters on Android. But not all of Android is fat. On my phone, primary internal storage is backed by an ext4 fs, and reserved characters can be used just fine. AFAIK that is not an uncommon arrangement on phones with enough internal storage. The stackoverflow reference also doesn't say fat reserved characters are forbidden on android, they are only forbidden on fat-backed filesystems. Syncthing should be able to just work on ext4-backed storage. There is a bug somewhere preventing that now, but that is a separate issue. A solution based on the filesystem in question would be much better. That could also cover unixes using a fat filesystem. |
I remember discussing this with @tomasz1986 and coming to the conclusion that the overlay filesystem on newer versions of Android blocks this, regardless of whether it's based on ext4 or not. |
Hmm, that would suck when I buy a new phone. But still, fat reserved characters are working (more or less) on my phone right now, and it looks like this change will overzealously block fat reserved characters for me when merged. If I read that right in the code, this change would be a regression for my usage of syncthing if it is not paired with a configuration option or some kind of encoding option. |
lib/fs,model: Clarify errors for Android filenames (fixes #9395)
Currently, Syncthing tries to sync files to Android devices with no
consideration for its filename restrictions (see [1]). This commit makes
it so that similarly to Windows, Syncthing relies on a hard-coded list
of characters which are invalid on Android and displays a specific error
message when user tries to sync a file with one of the characters in its
name.
[1] https://stackoverflow.com/a/64021421
Signed-off-by: Tomasz Wilczyński twilczynski@naver.com