Repository: syncthing/syncthing (language: go)

Issue:
Error for Windows invalid file names should indicate the invalid character or name part
I get the error "name is invalid, contains Windows reserved character (?, *, etc.)"
for this file name:
"Arnold Schoenberg (1874-1951) - Verklärte Nacht (Transfigured Night), Op. 4 (1899).mp3"

The file comes from an android phone and should be synced to Windows 10.
I don't see any problem with the file name. I even checked: I can create a file in Windows with that name.

Version: v1.23.0, Windows 10 (64-bit Intel/AMD)

Looks like a bug to me.



Discussion / hints:
![grafik](https://github.com/syncthing/syncthing/assets/1665799/c8ad3631-8aff-4781-b69b-b6ccdcc95d7b)

![grafik](https://github.com/syncthing/syncthing/assets/1665799/5873b953-2721-4bc6-8625-babf5902b7b3)

Trailing space?
Maybe there is some invisible character in there?

Suggestion:
Improve the error message to tell:
- At which character position the problem is
- And which symbol (also shown as char-number)
> Trailing space?

In deed. There were 2 trailing spaces...

Thanks!

But my previous point stands.
Improved error message would be great.
Maybe even a special one for "trailing spaces".
\+ leading spaces?! (if that's also a problem)

Fix patch:
diff --git a/lib/fs/basicfs.go b/lib/fs/basicfs.go
index 51e598ed2de..58afc0e2797 100644
--- a/lib/fs/basicfs.go
+++ b/lib/fs/basicfs.go
@@ -22,8 +22,8 @@ import (
 var (
 	errInvalidFilenameEmpty               = errors.New("name is invalid, must not be empty")
 	errInvalidFilenameWindowsSpacePeriod  = errors.New("name is invalid, must not end in space or period on Windows")
-	errInvalidFilenameWindowsReservedName = errors.New("name is invalid, contains Windows reserved name (NUL, COM1, etc.)")
-	errInvalidFilenameWindowsReservedChar = errors.New("name is invalid, contains Windows reserved character (?, *, etc.)")
+	errInvalidFilenameWindowsReservedName = errors.New("name is invalid, contains Windows reserved name")
+	errInvalidFilenameWindowsReservedChar = errors.New("name is invalid, contains Windows reserved character")
 )
 
 type OptionJunctionsAsDirs struct{}
diff --git a/lib/fs/util.go b/lib/fs/util.go
index 9e977cb3705..41ee39300cc 100644
--- a/lib/fs/util.go
+++ b/lib/fs/util.go
@@ -54,8 +54,8 @@ const windowsDisallowedCharacters = (`<>:"|?*` +
 
 func WindowsInvalidFilename(name string) error {
 	// The path must not contain any disallowed characters.
-	if strings.ContainsAny(name, windowsDisallowedCharacters) {
-		return errInvalidFilenameWindowsReservedChar
+	if idx := strings.IndexAny(name, windowsDisallowedCharacters); idx != -1 {
+		return fmt.Errorf("%w: %q", errInvalidFilenameWindowsReservedChar, name[idx:idx+1])
 	}
 
 	// None of the path components should end in space or period, or be a
@@ -72,8 +72,8 @@ func WindowsInvalidFilename(name string) error {
 			// Names ending in space or period are not valid.
 			return errInvalidFilenameWindowsSpacePeriod
 		}
-		if windowsIsReserved(part) {
-			return errInvalidFilenameWindowsReservedName
+		if reserved := windowsReservedNamePart(part); reserved != "" {
+			return fmt.Errorf("%w: %q", errInvalidFilenameWindowsReservedName, reserved)
 		}
 	}
 
@@ -117,13 +117,13 @@ func SanitizePath(path string) string {
 	}
 
 	path = strings.TrimSpace(b.String())
-	if windowsIsReserved(path) {
+	if reserved := windowsReservedNamePart(path); reserved != "" {
 		path = "-" + path
 	}
 	return path
 }
 
-func windowsIsReserved(part string) bool {
+func windowsReservedNamePart(part string) string {
 	// nul.txt.jpg is also disallowed.
 	dot := strings.IndexByte(part, '.')
 	if dot != -1 {
@@ -132,7 +132,7 @@ func windowsIsReserved(part string) bool {
 
 	// Check length to skip allocating ToUpper.
 	if len(part) != 3 && len(part) != 4 {
-		return false
+		return ""
 	}
 
 	// COM0 and LPT0 are missing from the Microsoft docs,
@@ -144,9 +144,9 @@ func windowsIsReserved(part string) bool {
 		"COM5", "COM6", "COM7", "COM8", "COM9",
 		"LPT0", "LPT1", "LPT2", "LPT3", "LPT4",
 		"LPT5", "LPT6", "LPT7", "LPT8", "LPT9":
-		return true
+		return part
 	}
-	return false
+	return ""
 }
 
 // IsParent compares paths purely lexicographically, meaning it returns false


Test patch:
diff --git a/lib/fs/util_test.go b/lib/fs/util_test.go
index 1727044d2db..0be64b0f47a 100644
--- a/lib/fs/util_test.go
+++ b/lib/fs/util_test.go
@@ -7,6 +7,7 @@
 package fs
 
 import (
+	"errors"
 	"math/rand"
 	"testing"
 	"unicode"
@@ -69,9 +70,10 @@ func TestWindowsInvalidFilename(t *testing.T) {
 
 	for _, tc := range cases {
 		err := WindowsInvalidFilename(tc.name)
-		if err != tc.err {
+		if !errors.Is(err, tc.err) {
 			t.Errorf("For %q, got %v, expected %v", tc.name, err, tc.err)
 		}
+		t.Logf("%s: %v", tc.name, err)
 	}
 }
 
@@ -124,9 +126,11 @@ func benchmarkWindowsInvalidFilename(b *testing.B, name string) {
 		WindowsInvalidFilename(name)
 	}
 }
+
 func BenchmarkWindowsInvalidFilenameValid(b *testing.B) {
 	benchmarkWindowsInvalidFilename(b, "License.txt.gz")
 }
+
 func BenchmarkWindowsInvalidFilenameNUL(b *testing.B) {
 	benchmarkWindowsInvalidFilename(b, "nul.txt.gz")
 }