{"action":"created","issue":{"url":"https://api.github.com/repos/syncthing/syncthing/issues/5146","repository_url":"https://api.github.com/repos/syncthing/syncthing","labels_url":"https://api.github.com/repos/syncthing/syncthing/issues/5146/labels{/name}","comments_url":"https://api.github.com/repos/syncthing/syncthing/issues/5146/comments","events_url":"https://api.github.com/repos/syncthing/syncthing/issues/5146/events","html_url":"https://github.com/syncthing/syncthing/pull/5146","id":354082101,"node_id":"MDExOlB1bGxSZXF1ZXN0MjEwOTQzNzU0","number":5146,"title":" lib/fs: Don't append path sep to root in rooted (ref #5144) ","user":{"login":"imsodin","id":15955093,"node_id":"MDQ6VXNlcjE1OTU1MDkz","avatar_url":"https://avatars0.githubusercontent.com/u/15955093?v=4","gravatar_id":"","url":"https://api.github.com/users/imsodin","html_url":"https://github.com/imsodin","followers_url":"https://api.github.com/users/imsodin/followers","following_url":"https://api.github.com/users/imsodin/following{/other_user}","gists_url":"https://api.github.com/users/imsodin/gists{/gist_id}","starred_url":"https://api.github.com/users/imsodin/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/imsodin/subscriptions","organizations_url":"https://api.github.com/users/imsodin/orgs","repos_url":"https://api.github.com/users/imsodin/repos","events_url":"https://api.github.com/users/imsodin/events{/privacy}","received_events_url":"https://api.github.com/users/imsodin/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":4,"created_at":"2018-08-26T10:01:24Z","updated_at":"2018-08-26T14:43:59Z","closed_at":null,"author_association":"MEMBER","pull_request":{"url":"https://api.github.com/repos/syncthing/syncthing/pulls/5146","html_url":"https://github.com/syncthing/syncthing/pull/5146","diff_url":"https://github.com/syncthing/syncthing/pull/5146.diff","patch_url":"https://github.com/syncthing/syncthing/pull/5146.patch"},"body":"### Purpose\r\n\r\nBy @calmh in #5144 \r\n\r\n> We take great care to make sure a folder root doesn't end in slash on Windows...\r\n\r\n> Right. But then we do the \"rooted\". This (new) fs test should pass, imho:\r\n> \r\n> ```\r\n> func TestWindowsFolderRoot(t *testing.T) {\r\n> \tfs := BasicFilesystem{root: `c:\\test`}\r\n> \trooted, err := fs.rooted(\".\")\r\n> \tif err != nil {\r\n> \t\tt.Fatal(err)\r\n> \t}\r\n> \tif rooted != `c:\\test` {\r\n> \t\tt.Error(\"Incorrect root:\", rooted)\r\n> \t}\r\n> }\r\n> ```\r\n> \r\n> But it does not:\r\n> \r\n> ```\r\n> basicfs_windows_test.go:101: Incorrect root: c:\\test\\\r\n> ```\r\n> \r\n> So we undo the efforts of our root normalization stuff from earlier...\r\n> \r\n\r\nThis treats the case specially when the  path given to `rooted` is equivalent to `.` (folder root), in that it just returns `BasicFilesystem.root` without modifying it, thus leaving the precautions taken in `newBasicFilesystem` untouched. \r\n\r\n### Testing\r\n\r\nNeeded to adjust and add some test cases to `TestRooted` and added the test proposed by Jakob above."},"comment":{"url":"https://api.github.com/repos/syncthing/syncthing/issues/comments/416043877","html_url":"https://github.com/syncthing/syncthing/pull/5146#issuecomment-416043877","issue_url":"https://api.github.com/repos/syncthing/syncthing/issues/5146","id":416043877,"node_id":"MDEyOklzc3VlQ29tbWVudDQxNjA0Mzg3Nw==","user":{"login":"calmh","id":125426,"node_id":"MDQ6VXNlcjEyNTQyNg==","avatar_url":"https://avatars3.githubusercontent.com/u/125426?v=4","gravatar_id":"","url":"https://api.github.com/users/calmh","html_url":"https://github.com/calmh","followers_url":"https://api.github.com/users/calmh/followers","following_url":"https://api.github.com/users/calmh/following{/other_user}","gists_url":"https://api.github.com/users/calmh/gists{/gist_id}","starred_url":"https://api.github.com/users/calmh/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/calmh/subscriptions","organizations_url":"https://api.github.com/users/calmh/orgs","repos_url":"https://api.github.com/users/calmh/repos","events_url":"https://api.github.com/users/calmh/events{/privacy}","received_events_url":"https://api.github.com/users/calmh/received_events","type":"User","site_admin":false},"created_at":"2018-08-26T14:43:59Z","updated_at":"2018-08-26T14:43:59Z","author_association":"MEMBER","body":"I had a frank discussion with Windows Defender and I think we managed to agree that the build agent artifacts are not to be considered threats henceforth.\r\n\r\nThis LGTM - or rather, it looks like we're removing some safety checks, but the tests that verify them still pass so I'm OK with that. I think my new temporary test should just be folded into the list of paths that we already checkin the other test you tweaked, last among the \"windows special items\" that already checked we handled the `c:\\` root correctly.\r\n\r\nBut that's a nit. 👍 from me"}}
{"actor":{"display_login":"calmh"}}