os: on Windows, don't fix long paths that aren't long

Notably, don't allocate.

Follow-up to https://golang.org/cl/32451 which added long path
cleaning.

Updates #3358

Change-Id: I89c59cbd660d0a030f31b6acd070fa9f3250683b
Reviewed-on: https://go-review.googlesource.com/32886
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>

diff --git a/src/os/path_windows.go b/src/os/path_windows.go
index 1a4223deab6029532c2d090d687b4a671b80e91c..ccac1c0b64b6ab87cbe4f100a37ac56c026fad49 100644 (file)
--- a/src/os/path_windows.go
+++ b/src/os/path_windows.go
@@ -129,26 +129,40 @@ func dirname(path string) string {
 }
 
 // fixLongPath returns the extended-length (\\?\-prefixed) form of
-// path if possible, in order to avoid the default 260 character file
-// path limit imposed by Windows.  If path is not easily converted to
+// path when needed, in order to avoid the default 260 character file
+// path limit imposed by Windows. If path is not easily converted to
 // the extended-length form (for example, if path is a relative path
-// or contains .. elements), fixLongPath returns path unmodified.
+// or contains .. elements), or is short enough, fixLongPath returns
+// path unmodified.
+//
+// See https://msdn.microsoft.com/en-us/library/windows/desktop/aa365247(v=vs.85).aspx#maxpath
 func fixLongPath(path string) string {
+       // Do nothing (and don't allocate) if the path is "short".
+       // Empirically (at least on the Windows Server 2013 builder),
+       // the kernel is arbitrarily okay with <= 248 bytes. That
+       // matches what the docs above say:
+       // "When using an API to create a directory, the specified
+       // path cannot be so long that you cannot append an 8.3 file
+       // name (that is, the directory name cannot exceed MAX_PATH
+       // minus 12)." Since MAX_PATH is 260, 260 - 12 = 248.
+       if len(path) <= 248 {
+               // Don't fix. (This is how Go 1.7 and earlier worked,
+               // not automatically generating the \\?\ form)
+               return path
+       }
+
        // The extended form begins with \\?\, as in
        // \\?\c:\windows\foo.txt or \\?\UNC\server\share\foo.txt.
        // The extended form disables evaluation of . and .. path
        // elements and disables the interpretation of / as equivalent
-       // to \.  The conversion here rewrites / to \ and elides
+       // to \. The conversion here rewrites / to \ and elides
        // . elements as well as trailing or duplicate separators. For
        // simplicity it avoids the conversion entirely for relative
-       // paths or paths containing .. elements.  For now,
+       // paths or paths containing .. elements. For now,
        // \\server\share paths are not converted to
        // \\?\UNC\server\share paths because the rules for doing so
        // are less well-specified.
-       //
-       // For details of \\?\ paths, see:
-       // https://msdn.microsoft.com/en-us/library/windows/desktop/aa365247(v=vs.85).aspx#maxpath
-       if len(path) == 0 || (len(path) >= 2 && path[:2] == `\\`) {
+       if len(path) >= 2 && path[:2] == `\\` {
                // Don't canonicalize UNC paths.
                return path
        }

diff --git a/src/os/path_windows_test.go b/src/os/path_windows_test.go
index 8fd515728e2662b5afa7774a534d9d257b399bd6..cce0bdd522dc2fc43328a951efda97eaa1225693 100644 (file)
--- a/src/os/path_windows_test.go
+++ b/src/os/path_windows_test.go
@@ -6,23 +6,40 @@ package os_test
 
 import (
        "os"
+       "strings"
        "testing"
 )
 
 func TestFixLongPath(t *testing.T) {
+       // 248 is long enough to trigger the longer-than-248 checks in
+       // fixLongPath, but short enough not to make a path component
+       // longer than 255, which is illegal on Windows. (which
+       // doesn't really matter anyway, since this is purely a string
+       // function we're testing, and it's not actually being used to
+       // do a system call)
+       veryLong := "l" + strings.Repeat("o", 248) + "ng"
        for _, test := range []struct{ in, want string }{
-               {`C:\foo.txt`, `\\?\C:\foo.txt`},
-               {`C:/foo.txt`, `\\?\C:\foo.txt`},
-               {`C:\foo\\bar\.\baz\\`, `\\?\C:\foo\bar\baz`},
-               {`C:\`, `\\?\C:\`}, // drives must have a trailing slash
+               // Short; unchanged:
+               {`C:\short.txt`, `C:\short.txt`},
+               {`C:\`, `C:\`},
+               {`C:`, `C:`},
+               // The "long" substring is replaced by a looooooong
+               // string which triggers the rewriting. Except in the
+               // cases below where it doesn't.
+               {`C:\long\foo.txt`, `\\?\C:\long\foo.txt`},
+               {`C:/long/foo.txt`, `\\?\C:\long\foo.txt`},
+               {`C:\long\foo\\bar\.\baz\\`, `\\?\C:\long\foo\bar\baz`},
                {`\\unc\path`, `\\unc\path`},
-               {`foo.txt`, `foo.txt`},
-               {`C:foo.txt`, `C:foo.txt`},
-               {`c:\foo\..\bar\baz`, `c:\foo\..\bar\baz`},
-               {`\\?\c:\windows\foo.txt`, `\\?\c:\windows\foo.txt`},
-               {`\\?\c:\windows/foo.txt`, `\\?\c:\windows/foo.txt`},
+               {`long.txt`, `long.txt`},
+               {`C:long.txt`, `C:long.txt`},
+               {`c:\long\..\bar\baz`, `c:\long\..\bar\baz`},
+               {`\\?\c:\long\foo.txt`, `\\?\c:\long\foo.txt`},
+               {`\\?\c:\long/foo.txt`, `\\?\c:\long/foo.txt`},
        } {
-               if got := os.FixLongPath(test.in); got != test.want {
+               in := strings.Replace(test.in, "long", veryLong, -1)
+               want := strings.Replace(test.want, "long", veryLong, -1)
+               if got := os.FixLongPath(in); got != want {
+                       got = strings.Replace(got, veryLong, "long", -1)
                        t.Errorf("fixLongPath(%q) = %q; want %q", test.in, got, test.want)
                }
        }