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

Handle long filenames on Windows (fixes #1295) #1590

Merged
merged 1 commit into from Apr 8, 2015

Conversation

Projects
None yet
4 participants
@calmh
Copy link
Member

commented Apr 5, 2015

There are many approaches to this... This one works on the assumption that it's safest for most of the code if the change is done as early as possible, i.e. at config loading time, lest we forget some critical place where a conversion might need to be done. And it also assumes that we should not put the \\?\ in the config or show it to the user in the GUI, as they may be scared and confused by it. If we think most Windows users are fine with \\?\ being visible then we can just add it at config loading and not do the dance with RawPath/Path...

I've tested that this starts on Windows and seems to hash files while mentioning paths starting with \\?\ in the debug output... Some more Windows testing would be nice.

@calmh calmh added the pr-enhancement label Apr 5, 2015

// This is intentionally not a pointer method, becuse things like
// cfg.Folders[i].Path() should be valid.

if runtime.GOOS == "windows" && filepath.IsAbs(f.RawPath) && !strings.HasPrefix(f.RawPath, `\\?\`) {

This comment has been minimized.

Copy link
@Zillode

Zillode Apr 5, 2015

Member

I think this should be !strings.HasPrefix(f.RawPath, \) to support network paths
E.g. \\?\\\192.168.2.20 does not work currently

This comment has been minimized.

Copy link
@calmh

calmh Apr 5, 2015

Author Member

Ah

@calmh

This comment has been minimized.

Copy link
Member Author

commented Apr 5, 2015

Does this work as far back as XP, or do we need to exclude that somehow?

@Zillode Zillode referenced this pull request Apr 5, 2015

Closed

request: config.xml paths #118

@AudriusButkevicius

This comment has been minimized.

Copy link
Member

commented Apr 6, 2015

So I don't think we need to compute this every time. We should do it at start up and use that. Also, I think we should expand the ~ and prepend it to that too given we support that.

@calmh

This comment has been minimized.

Copy link
Member Author

commented Apr 6, 2015

How does that look? This time I haven't even tested it at all on Windows. :) Since we do the fixup in prepare() (i.e. at startup) the tests that do manual config juggling need to emulate this...

@AudriusButkevicius

This comment has been minimized.

Copy link
Member

commented Apr 6, 2015

I'm still on the road, but I should be back at a proper keyboard later this evening to test this.

Perhaps we already do the expansion somewhere.

@AudriusButkevicius

This comment has been minimized.

Copy link
Member

commented Apr 7, 2015

So after adding a new folder, all the paths suddenly get unset.
Perhaps we should have a folder.Prepare() and call that in config.Folders() as we are building the map.

Also, perhaps (atleast on windows) we should expand all paths to be absolute, and always apply the magic prefix?

@calmh

This comment has been minimized.

Copy link
Member Author

commented Apr 7, 2015

That's the easiest and safest... Only downside is that it's user visible and we'll get issues for "Syncthing replaced c:\my documents with \\?\c:\my documents?!" and so on...

@Zillode

This comment has been minimized.

Copy link
Member

commented Apr 7, 2015

inotify would benefit from \\?\ prefixes, however syncthing-tray or other tray balloon apps would not.

@calmh

This comment has been minimized.

Copy link
Member Author

commented Apr 7, 2015

We could, optionally, let the path be \\?\-prefixed everywhere and just hide it in the path text field in the GUI. :) Accept it, save it, but strip it before displaying. Those who understand what's going on may look in the config file and be happy, those who don't won't see anything evil, hear anything evil, ...

@AudriusButkevicius

This comment has been minimized.

Copy link
Member

commented Apr 7, 2015

or we could go back to your Path() version, which solves this nicely

I guess I don't want to bleed runtime.OS == "windows" logic into the JS world hence I feel we should solve this internally.

@calmh

This comment has been minimized.

Copy link
Member Author

commented Apr 7, 2015

Or that. :/

@calmh

This comment has been minimized.

Copy link
Member Author

commented Apr 7, 2015

Theoretically that commit should still be somewhere in my reflog...

@calmh

This comment has been minimized.

Copy link
Member Author

commented Apr 7, 2015

So that then... Path() is called fairly seldom so I don't think it's a big deal that it exists... But it may be worth it to move the tilde expansion in there and cache it? Or not cache it. Currently we do the tilde expansion into the config file, so the user says ~/Foo and it becomes /Users/jb/Foo after the mandatory restart....

@calmh

This comment has been minimized.

Copy link
Member Author

commented Apr 8, 2015

I moved some stuff into Path() and attempt to make the path absolute. This may fail; I opted to return the unmodified string rather than pass an annoying error around everywhere... The regular folder checks should catch us if the path does not make sense, I hope...

AudriusButkevicius added a commit that referenced this pull request Apr 8, 2015

Merge pull request #1590 from calmh/long-filenames
Handle long filenames on Windows (fixes #1295)

@AudriusButkevicius AudriusButkevicius merged commit a892f80 into syncthing:master Apr 8, 2015

3 checks passed

authors Present in AUTHORS
Details
gofmt Files formatted correctly
Details
pr-build Merged build finished.
Details

@calmh calmh deleted the calmh:long-filenames branch Apr 8, 2015

@syncthing syncthing locked and limited conversation to collaborators Jun 16, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.