Properly normalise paths on Windows #7676

RayyanAnsari · 2023-06-25T16:40:58Z

codecov-commenter · 2023-06-25T16:52:05Z

Codecov Report

Merging #7676 (7e0cdd7) into master (3495144) will decrease coverage by 0.12%.
The diff coverage is 33.33%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##           master    #7676      +/-   ##
==========================================
- Coverage   83.87%   83.76%   -0.12%     
==========================================
  Files          66       66              
  Lines       11873    11897      +24     
  Branches     2149     2155       +6     
==========================================
+ Hits         9958     9965       +7     
- Misses       1346     1359      +13     
- Partials      569      573       +4

Impacted Files	Coverage Δ
src/borg/helpers/fs.py	`83.27% <33.33%> (-1.26%)`	⬇️

... and 5 files with indirect coverage changes

ThomasWaldmann · 2023-06-25T18:34:52Z

src/borg/helpers/fs.py

+    if is_win32:
+        path = os.path.splitdrive(path)[1]


what i already wondered: is borg already getting paths with slashes (and not backslashes)?

added some logging, it looks like it is:

$ pytest -s -k "test_archived_paths and not remote and not binary" =========================================================================================== test session starts ============================================================================================ platform win32 -- Python 3.10.9, pytest-7.2.0, pluggy-1.0.0 benchmark: 4.0.0 (defaults: timer=time.perf_counter disable_gc=False min_rounds=5 min_time=0.000005 max_time=1.0 calibration_precision=10 warmup=False warmup_iterations=100000) Tests enabled: root, hardlinks, modes Tests disabled: BSD flags, fuse2, fuse3, symlinks, atime/mtime rootdir: C:/msys64/home/Rayyan/borg, configfile: setup.cfg plugins: benchmark-4.0.0, cov-4.0.0, forked-1.4.0, xdist-3.1.0 collected 1746 items / 1745 deselected / 1 selected src/borg/testsuite/archiver/create_cmd.py self.cmd output START self.cmd output END self.cmd output START remove_dotdot_prefixes path = input remove_dotdot_prefixes path = input/test remove_dotdot_prefixes path = C:/Users/Rayyan/AppData/Local/Temp/tmpz08pkzkq/input/test self.cmd output END self.cmd output START input input/test Users/Rayyan/AppData/Local/Temp/tmpz08pkzkq/input/test self.cmd output END self.cmd output START {"flags": 0, "gid": 0, "group": "root", "healthy": true, "hlid": "", "mode": "drwxrwxrwx", "mtime": "2023-06-25T21:38:16.244628+01:00", "path": "input", "size": 0, "target": "", "type": "d", "uid": 0, "user": "root"} {"flags": 0, "gid": 0, "group": "root", "healthy": true, "hlid": "", "mode": "-rw-rw-rw-", "mtime": "2023-06-25T21:38:16.244628+01:00", "path": "input/test", "size": 0, "target": "", "type": "-", "uid": 0, "user": "root"} {"flags": 0, "gid": 0, "group": "root", "healthy": true, "hlid": "", "mode": "-rw-rw-rw-", "mtime": "2023-06-25T21:38:16.244628+01:00", "path": "Users/Rayyan/AppData/Local/Temp/tmpz08pkzkq/input/test", "size": 0, "target": "", "type": "-", "uid": 0, "user": "root"} self.cmd output END . ==================================================================================== 1 passed, 1745 deselected in 1.63s ====================================================================================

Do you know where exactly the usual backslashes on windows are transformed to slashes?

just tested making an archive manually with borg.exe, looks like the paths are still backslashes...
strange. will look into this more.

could be that the test is passing it a forward slash path but at the terminal it's getting passed a backward slash path

appears to work fine now but i don't know if posixpath.normpath is needed in other places as well

ams-tschoening · 2023-06-27T10:08:43Z

There was a discussion on the mailing list about not taking drive letters into account on Windows, which prevents using files of multiple drives in the same archive. That feels a bit strange, especially as some softwares like Cygwin simply use the drive letter character as part of some path and Borg might do the same. The following is an example:

C:\windows\system32\something
D:\windows\foobar

vs.

C/windows/system32/something
D/windows/foobar

ThomasWaldmann · 2023-06-27T10:18:04Z

@ams-tschoening makes sense.

Just one thing might complicate this a bit:
guess we don't always have drive letters, so we would only include them if they were part of the recursion root?

E.g.

borg create ARCHIVE c:\windows Documents

would we back up this like?:

c/windows/...
...
Documents/example.doc
...

ams-tschoening · 2023-06-27T11:21:58Z

c/windows/...
...
Documents/example.doc
...

In my opinion yes and should be in-line with what Borg documents about paths in other OS as well: They are used as is. If one wants relative paths, one needs to cd into parents (Documents), otherwise the whole path prefix is used (C:\Windows). Borg doesn't calculate some path prefix for other relative paths on other OS as well, so don't need to here. The only question is if to remove C:\ and make things special this way, which doesn't seem necessary. The only special thing here is that C:\ should become part of the path using non-Windows syntax as C/[...] and making paths behave mostly the same this way across different OS.

tschoening@tschoening-nb MINGW64 ~/Documents
$ pwd
/c/Users/tschoening/Documents

ThomasWaldmann · 2023-06-27T12:16:37Z

Note: lower case "c" on mingw64 (not uppercase).

ams-tschoening · 2023-06-27T16:27:59Z

Note: lower case "c" on mingw64 (not uppercase).

Should you care about that? Borg doesn't normalize upper-lower case in any other path or does it on Windows? In my opinion things should be forwarded 1:1.

RayyanAnsari · 2023-06-27T17:41:38Z

There was a discussion on the mailing list about not taking drive letters into account on Windows, which prevents using files of multiple drives in the same archive. That feels a bit strange, especially as some softwares like Cygwin simply use the drive letter character as part of some path and Borg might do the same. The following is an example:

Do you have a link to this discussion?

ams-tschoening · 2023-06-28T06:27:14Z

https://mail.python.org/pipermail/borgbackup/2023q2/002269.html

An answer is missing, because I've sent privately by accident first.

src/borg/helpers/fs.py

Use forward slashes and integrate the drive letter into the path.

ThomasWaldmann

LGTM.

RayyanAnsari · 2023-07-06T22:32:49Z

anything stopping this from being merged?

ThomasWaldmann · 2023-07-06T22:41:57Z

No. :)

ThomasWaldmann reviewed Jun 25, 2023

View reviewed changes

RayyanAnsari force-pushed the paths-win branch from 00af2f6 to 5f16a66 Compare June 26, 2023 22:12

RayyanAnsari force-pushed the paths-win branch 2 times, most recently from e327a24 to ebbf8b0 Compare July 2, 2023 21:59

ThomasWaldmann requested changes Jul 2, 2023

View reviewed changes

src/borg/helpers/fs.py Outdated Show resolved Hide resolved

src/borg/helpers/fs.py Outdated Show resolved Hide resolved

RayyanAnsari force-pushed the paths-win branch from ebbf8b0 to bef3148 Compare July 4, 2023 18:42

RayyanAnsari added 2 commits July 4, 2023 19:44

fs: Properly normalise paths on Windows

54b654e

Use forward slashes and integrate the drive letter into the path.

testsuite: Enable test_archived_paths on Windows

7e0cdd7

RayyanAnsari force-pushed the paths-win branch from bef3148 to 7e0cdd7 Compare July 4, 2023 18:46

ThomasWaldmann approved these changes Jul 4, 2023

View reviewed changes

RayyanAnsari marked this pull request as ready for review July 4, 2023 22:21

ThomasWaldmann merged commit cfbfe24 into borgbackup:master Jul 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Properly normalise paths on Windows #7676

Properly normalise paths on Windows #7676

RayyanAnsari commented Jun 25, 2023

codecov-commenter commented Jun 25, 2023 •

edited

Loading

ThomasWaldmann Jun 25, 2023

RayyanAnsari Jun 25, 2023

ThomasWaldmann Jun 26, 2023

RayyanAnsari Jun 26, 2023 •

edited

Loading

RayyanAnsari Jun 26, 2023

ams-tschoening commented Jun 27, 2023

ThomasWaldmann commented Jun 27, 2023

ams-tschoening commented Jun 27, 2023 •

edited

Loading

ThomasWaldmann commented Jun 27, 2023

ams-tschoening commented Jun 27, 2023

RayyanAnsari commented Jun 27, 2023

ams-tschoening commented Jun 28, 2023

ThomasWaldmann left a comment

RayyanAnsari commented Jul 6, 2023

ThomasWaldmann commented Jul 6, 2023

Properly normalise paths on Windows #7676

Properly normalise paths on Windows #7676

Conversation

RayyanAnsari commented Jun 25, 2023

codecov-commenter commented Jun 25, 2023 • edited Loading

Codecov Report

ThomasWaldmann Jun 25, 2023

Choose a reason for hiding this comment

RayyanAnsari Jun 25, 2023

Choose a reason for hiding this comment

ThomasWaldmann Jun 26, 2023

Choose a reason for hiding this comment

RayyanAnsari Jun 26, 2023 • edited Loading

Choose a reason for hiding this comment

RayyanAnsari Jun 26, 2023

Choose a reason for hiding this comment

ams-tschoening commented Jun 27, 2023

ThomasWaldmann commented Jun 27, 2023

ams-tschoening commented Jun 27, 2023 • edited Loading

ThomasWaldmann commented Jun 27, 2023

ams-tschoening commented Jun 27, 2023

RayyanAnsari commented Jun 27, 2023

ams-tschoening commented Jun 28, 2023

ThomasWaldmann left a comment

Choose a reason for hiding this comment

RayyanAnsari commented Jul 6, 2023

ThomasWaldmann commented Jul 6, 2023

codecov-commenter commented Jun 25, 2023 •

edited

Loading

RayyanAnsari Jun 26, 2023 •

edited

Loading

ams-tschoening commented Jun 27, 2023 •

edited

Loading