Improve windows path parsing

[tomgr]

This PR is necessary to fix some bugs with the WinFSP vfs integration in bb-remote-execution (see buildbarn/bb-remote-execution#213).

Firstly it changes the pretty printing of windows paths to append \. as a bare trailing backslash is not a valid pathname component.

Secondly this neatens up the parsing of Windows file paths and adds handling for paths starting with \\.\.

Claude helped write some of this -- particularly the tests (it has more patience than I do at converting escape sequences!).

[aspect-workflows]

Test

3 test targets passed

Targets

//pkg/blobstore/sharding/integration:integration_test [k8-fastbuild]                 75ms
//pkg/filesystem/path:path_test [k8-fastbuild]                                       128ms
//pkg/filesystem:filesystem_test [k8-fastbuild]                                      143ms

Total test execution time was 346ms. 26 tests (89.7%) were fully cached saving 5s.

[EdSchouten]

I just want to get clarified: is this a limitation/feature of Windows, or is it really just WinFSP that freaks out in case symlink targets have trailing slashes? What happens if you create a symlink on NTFS that has a trailing slash?

In the PR description I see you mention that the issue is in FspFileSystemResolveReparsePointsInternal. Maybe instead of fixing this on the Buildbarn side, we should report an issue against WinFSP that they fix this?

[EdSchouten]

Alternatively, what if instead of a trailing slash we append \. ? Is that valid?

[tomgr]

I think the Windows standards require symlink reparse buffers not to have a trailing slash. The symbolic link reparse data buffer docs state that the substitute name has to be a pathname. However, the pathname documentation does not allow an empty final component: each component of the path has to have at least one character.

\. is valid; WinFSP would form \.\ after substitution which does seem to get handled correctly. However I thought that wouldn't be as elegant as that would mean that whenever directory paths were printed with WindowsPathFormatStandard (which is the default) they would have a trailing \.\. There's also a minor concern which is that Windows has a fairly low limit on path sizes, so keeping them short is always a good idea.

[EdSchouten]

I would still have a preference for appending \. instead of removing the suffix entirely. One intentional design goal of the pathname library was that it does not relax paths in any way.

That MS-FSCC link that you shared is interesting. It raises the question: is it ever valid for us to generate Windows pathnames that end with slashes? If not, maybe we should just make a change like this?

diff --git pkg/filesystem/path/builder.go pkg/filesystem/path/builder.go
index 80be90a..a4ff01d 100644
--- pkg/filesystem/path/builder.go
+++ pkg/filesystem/path/builder.go
@@ -116,7 +116,7 @@ func (b *Builder) GetWindowsString(format WindowsPathFormat) (string, error) {
        // backslash.
        suffix := b.suffix
        if suffix == "/" {
-               suffix = "\\"
+               suffix = "\\."
        }
        out.WriteString(suffix)
        return out.String(), nil

[tomgr]

Ok I've done this. I think it should be ok given the definition of pathname. And the winfsp tests all pass in bb-remote-execution with this.

[EdSchouten]

Can you please remove these two comments, and update/augment the larger comment above it to fully describe why the code is structured the way it is?

[tomgr]

sure done!