Repository: syncthing/syncthing (language: go)

Issue:
spurious log file in $XDG_CONFIG_HOME
In default configuration

According to [spec](https://specifications.freedesktop.org/basedir-spec/basedir-spec-latest.html)

> $XDG_CONFIG_HOME defines the base directory relative to which user-specific configuration files should be stored

So this file should not be there: `~/.config/syncthing/syncthing.log`

This is more suitable:

>  The $XDG_STATE_HOME contains state data that should persist between (application) restarts, but that is not important or portable enough to the user that it should be stored in $XDG_DATA_HOME. It may contain: 
> - actions history (logs, history, recently used files, …)


Discussion / hints:
(And I would add most other files in `~/.config/syncthing` should probably rather end up in `~/.local/share/syncthing`)
As described in #9178, I think even `$XDG_DATA_HOME` would not be the ideal placement.  Not authoritative, but https://wiki.archlinux.org/title/XDG_Base_Directory#User_directories describes it as analogous to `/usr/share`, which as the name implies, could be shared between devices.  Sharing the configuration files, e.g. device ID certificate, would be problematic though.
So `$XDG_STATE_HOME` is more appropriate then?
If there's a log file there it's because you manually asked for it, we don't default to using any log file on unixes.
@calmh the XDG anti-pattern is a technical debt. I suspect we will continue to see these kind of issues reported from ST users until the issue is fixed.

There are indeed log files stored in the default installation.

![image](https://github.com/syncthing/syncthing/assets/6200749/b3d024ee-a2ed-42e9-96bb-0c4dcfd62ead)

> As described in #9178, I think even `$XDG_DATA_HOME` would not be the ideal placement. Not authoritative, but https://wiki.archlinux.org/title/XDG_Base_Directory#User_directories describes it as analogous to `/usr/share`, which as the name implies, could be shared between devices. Sharing the configuration files, e.g. device ID certificate, would be problematic though.

That is why there is a /usr/**local**/share on your system. XDG has a ~/.local too.
That's an implementation detail of the leveldb database and not what the OP is talking about.  
> That is why there is a /usr/local/share on your system. XDG has a ~/.local too.

The XDG Base Directory Specification is not really related to the LSB Filesystem Hierarchy Standard (FHS).  In contrast to the latter, it is very vague on what goes where and what could possibly be shared between different machines.  The name `.local` MAY imply that it should be kept local to the machine, but the standard makes no such statement.  And the `share` part does suggest to me that it is intended to be shared.

Regarding the FHS and the `/usr/local/` hierarchy, the distinction is basically between stuff managed by the distribution through its package system (`/usr/`) and stuff added by the local administrator.  It has nothing to do with `~/.local/`.
@calmh 

> If there's a log file there it's because you manually asked for it, we don't default to using any log file on unixes.

I didn't ask for this path specifically, the command run by `syncthing-start.desktop` is `syncthing […] --logfile=default`, so it **is** syncthing that selects this bogus path, not me.
Indeed, that's being set in our own `etc/linux-desktop/syncthing-start.desktop` file.  Should we treat it as a bug to be fixed in that file?

Anyhow, when fixing #9178, the default log file location should be moved as well.
LOL I called it 

<img width="816" alt="Screenshot 2023-10-22 at 21 42 42" src="https://github.com/syncthing/syncthing/assets/125426/75b9a387-39e9-4031-8251-3b634569c9a2">

For what it's worth, the current "default" apparently maps into XDG_DATA_HOME if set, so 🤷 (and this stuff predates the existence of the XDG state stuff)

Fix patch:
diff --git a/lib/locations/locations.go b/lib/locations/locations.go
index 0fa488e7e23..157a6342efd 100644
--- a/lib/locations/locations.go
+++ b/lib/locations/locations.go
@@ -10,7 +10,6 @@ import (
 	"fmt"
 	"os"
 	"path/filepath"
-	"runtime"
 	"strings"
 	"time"
 
@@ -40,13 +39,18 @@ const (
 type BaseDirEnum string
 
 const (
-	// Overridden by --home flag
+	// Overridden by --home flag, $STHOMEDIR, --config flag, or $STCONFDIR
 	ConfigBaseDir BaseDirEnum = "config"
-	DataBaseDir   BaseDirEnum = "data"
+	// Overridden by --home flag, $STHOMEDIR, --data flag, or $STDATADIR
+	DataBaseDir BaseDirEnum = "data"
+
 	// User's home directory, *not* --home flag
 	UserHomeBaseDir BaseDirEnum = "userHome"
 
-	LevelDBDir = "index-v0.14.0.db"
+	LevelDBDir          = "index-v0.14.0.db"
+	configFileName      = "config.xml"
+	defaultStateDir     = ".local/state/syncthing"
+	oldDefaultConfigDir = ".config/syncthing"
 )
 
 // Platform dependent directories
@@ -55,12 +59,13 @@ var baseDirs = make(map[BaseDirEnum]string, 3)
 func init() {
 	userHome := userHomeDir()
 	config := defaultConfigDir(userHome)
+	data := defaultDataDir(userHome, config)
+
 	baseDirs[UserHomeBaseDir] = userHome
 	baseDirs[ConfigBaseDir] = config
-	baseDirs[DataBaseDir] = defaultDataDir(userHome, config)
+	baseDirs[DataBaseDir] = data
 
-	err := expandLocations()
-	if err != nil {
+	if err := expandLocations(); err != nil {
 		fmt.Println(err)
 		panic("Failed to expand locations at init time")
 	}
@@ -92,8 +97,7 @@ func SetBaseDir(baseDirName BaseDirEnum, path string) error {
 			return err
 		}
 	}
-	_, ok := baseDirs[baseDirName]
-	if !ok {
+	if _, ok := baseDirs[baseDirName]; !ok {
 		return fmt.Errorf("unknown base dir: %s", baseDirName)
 	}
 	baseDirs[baseDirName] = filepath.Clean(path)
@@ -131,9 +135,9 @@ var locations = make(map[LocationEnum]string)
 func expandLocations() error {
 	newLocations := make(map[LocationEnum]string)
 	for key, dir := range locationTemplates {
-		for varName, value := range baseDirs {
-			dir = strings.ReplaceAll(dir, "${"+string(varName)+"}", value)
-		}
+		dir = os.Expand(dir, func(s string) string {
+			return baseDirs[BaseDirEnum(s)]
+		})
 		var err error
 		dir, err = fs.ExpandTilde(dir)
 		if err != nil {
@@ -175,49 +179,99 @@ func PrettyPaths() string {
 // out by various the environment variables present on each platform, or dies
 // trying.
 func defaultConfigDir(userHome string) string {
-	switch runtime.GOOS {
-	case build.Windows:
-		if p := os.Getenv("LocalAppData"); p != "" {
-			return filepath.Join(p, "Syncthing")
-		}
-		return filepath.Join(os.Getenv("AppData"), "Syncthing")
+	switch {
+	case build.IsWindows:
+		return windowsConfigDataDir()
 
-	case build.Darwin:
-		return filepath.Join(userHome, "Library/Application Support/Syncthing")
+	case build.IsDarwin:
+		return darwinConfigDataDir(userHome)
 
 	default:
-		if xdgCfg := os.Getenv("XDG_CONFIG_HOME"); xdgCfg != "" {
-			return filepath.Join(xdgCfg, "syncthing")
-		}
-		return filepath.Join(userHome, ".config/syncthing")
+		return unixConfigDir(userHome, os.Getenv("XDG_CONFIG_HOME"), os.Getenv("XDG_STATE_HOME"), fileExists)
 	}
 }
 
-// defaultDataDir returns the default data directory, which usually is the
-// config directory but might be something else.
-func defaultDataDir(userHome, config string) string {
+// defaultDataDir returns the default data directory, where we store the
+// database, log files, etc.
+func defaultDataDir(userHome, configDir string) string {
 	if build.IsWindows || build.IsDarwin {
-		return config
+		return configDir
 	}
 
-	// If a database exists at the "normal" location, use that anyway.
-	if _, err := os.Lstat(filepath.Join(config, LevelDBDir)); err == nil {
-		return config
+	return unixDataDir(userHome, configDir, os.Getenv("XDG_DATA_HOME"), os.Getenv("XDG_STATE_HOME"), fileExists)
+}
+
+func windowsConfigDataDir() string {
+	if p := os.Getenv("LocalAppData"); p != "" {
+		return filepath.Join(p, "Syncthing")
 	}
-	// Always use this env var, as it's explicitly set by the user
-	if xdgHome := os.Getenv("XDG_DATA_HOME"); xdgHome != "" {
-		return filepath.Join(xdgHome, "syncthing")
+	return filepath.Join(os.Getenv("AppData"), "Syncthing")
+}
+
+func darwinConfigDataDir(userHome string) string {
+	return filepath.Join(userHome, "Library/Application Support/Syncthing")
+}
+
+func unixConfigDir(userHome, xdgConfigHome, xdgStateHome string, fileExists func(string) bool) string {
+	// Legacy: if our config exists under $XDG_CONFIG_HOME/syncthing,
+	// use that. The variable should be set to an absolute path or be
+	// ignored, but that's not what we did previously, so we retain the
+	// old behavior.
+	if xdgConfigHome != "" {
+		candidate := filepath.Join(xdgConfigHome, "syncthing")
+		if fileExists(filepath.Join(candidate, configFileName)) {
+			return candidate
+		}
 	}
-	// Only use the XDG default, if a syncthing specific dir already
-	// exists. Existence of ~/.local/share is not deemed enough, as
-	// it may also exist erroneously on non-XDG systems.
-	xdgDefault := filepath.Join(userHome, ".local/share/syncthing")
-	if _, err := os.Lstat(xdgDefault); err == nil {
-		return xdgDefault
+
+	// Legacy: if our config exists under ~/.config/syncthing, use that
+	candidate := filepath.Join(userHome, oldDefaultConfigDir)
+	if fileExists(filepath.Join(candidate, configFileName)) {
+		return candidate
 	}
-	// FYI: XDG_DATA_DIRS is not relevant, as it is for system-wide
-	// data dirs, not user specific ones.
-	return config
+
+	// If XDG_STATE_HOME is set to an absolute path, use that
+	if filepath.IsAbs(xdgStateHome) {
+		return filepath.Join(xdgStateHome, "syncthing")
+	}
+
+	// Use our default
+	return filepath.Join(userHome, defaultStateDir)
+}
+
+// unixDataDir returns the default data directory, where we store the
+// database, log files, etc, on Unix-like systems.
+func unixDataDir(userHome, configDir, xdgDataHome, xdgStateHome string, fileExists func(string) bool) string {
+	// If a database exists at the config location, use that. This is the
+	// most common case for both legacy (~/.config/syncthing) and current
+	// (~/.local/state/syncthing) setups.
+	if fileExists(filepath.Join(configDir, LevelDBDir)) {
+		return configDir
+	}
+
+	// Legacy: if a database exists under $XDG_DATA_HOME/syncthing, use
+	// that. The variable should be set to an absolute path or be ignored,
+	// but that's not what we did previously, so we retain the old behavior.
+	if xdgDataHome != "" {
+		candidate := filepath.Join(xdgDataHome, "syncthing")
+		if fileExists(filepath.Join(candidate, LevelDBDir)) {
+			return candidate
+		}
+	}
+
+	// Legacy: if a database exists under ~/.config/syncthing, use that
+	candidate := filepath.Join(userHome, oldDefaultConfigDir)
+	if fileExists(filepath.Join(candidate, LevelDBDir)) {
+		return candidate
+	}
+
+	// If XDG_STATE_HOME is set to an absolute path, use that
+	if filepath.IsAbs(xdgStateHome) {
+		return filepath.Join(xdgStateHome, "syncthing")
+	}
+
+	// Use our default
+	return filepath.Join(userHome, defaultStateDir)
 }
 
 // userHomeDir returns the user's home directory, or dies trying.
@@ -240,3 +294,8 @@ func GetTimestamped(key LocationEnum) string {
 	now := time.Now().Format("20060102-150405")
 	return strings.ReplaceAll(tpl, "${timestamp}", now)
 }
+
+func fileExists(path string) bool {
+	_, err := os.Lstat(path)
+	return err == nil
+}


Test patch:
diff --git a/lib/locations/locations_test.go b/lib/locations/locations_test.go
new file mode 100644
index 00000000000..610e0cf6a84
--- /dev/null
+++ b/lib/locations/locations_test.go
@@ -0,0 +1,102 @@
+// Copyright (C) 2023 The Syncthing Authors.
+//
+// This Source Code Form is subject to the terms of the Mozilla Public
+// License, v. 2.0. If a copy of the MPL was not distributed with this file,
+// You can obtain one at https://mozilla.org/MPL/2.0/.
+
+//go:build !windows
+
+package locations
+
+import (
+	"testing"
+
+	"golang.org/x/exp/slices"
+)
+
+func TestUnixConfigDir(t *testing.T) {
+	t.Parallel()
+
+	cases := []struct {
+		userHome      string
+		xdgConfigHome string
+		xdgStateHome  string
+		filesExist    []string
+		expected      string
+	}{
+		// First some "new installations", no files exist previously.
+
+		// No variables set, use our current default
+		{"/home/user", "", "", nil, "/home/user/.local/state/syncthing"},
+		// Config home set, doesn't matter
+		{"/home/user", "/somewhere/else", "", nil, "/home/user/.local/state/syncthing"},
+		// State home set, use that
+		{"/home/user", "", "/var/state", nil, "/var/state/syncthing"},
+		// State home set, again config home doesn't matter
+		{"/home/user", "/somewhere/else", "/var/state", nil, "/var/state/syncthing"},
+
+		// Now some "upgrades", where we have files in the old locations.
+
+		// Config home set, a file exists in the default location
+		{"/home/user", "/somewhere/else", "", []string{"/home/user/.config/syncthing/config.xml"}, "/home/user/.config/syncthing"},
+		// State home set, a file exists in the default location
+		{"/home/user", "", "/var/state", []string{"/home/user/.config/syncthing/config.xml"}, "/home/user/.config/syncthing"},
+		// Both config home and state home set, a file exists in the default location
+		{"/home/user", "/somewhere/else", "/var/state", []string{"/home/user/.config/syncthing/config.xml"}, "/home/user/.config/syncthing"},
+
+		// Config home set, and a file exists at that place
+		{"/home/user", "/somewhere/else", "", []string{"/somewhere/else/syncthing/config.xml"}, "/somewhere/else/syncthing"},
+		// Config home and state home set, and a file exists in config home
+		{"/home/user", "/somewhere/else", "/var/state", []string{"/somewhere/else/syncthing/config.xml"}, "/somewhere/else/syncthing"},
+	}
+
+	for _, c := range cases {
+		fileExists := func(path string) bool { return slices.Contains(c.filesExist, path) }
+		actual := unixConfigDir(c.userHome, c.xdgConfigHome, c.xdgStateHome, fileExists)
+		if actual != c.expected {
+			t.Errorf("unixConfigDir(%q, %q, %q) == %q, expected %q", c.userHome, c.xdgConfigHome, c.xdgStateHome, actual, c.expected)
+		}
+	}
+}
+
+func TestUnixDataDir(t *testing.T) {
+	t.Parallel()
+
+	cases := []struct {
+		userHome     string
+		configDir    string
+		xdgDataHome  string
+		xdgStateHome string
+		filesExist   []string
+		expected     string
+	}{
+		// First some "new installations", no files exist previously.
+
+		// No variables set, use our current default
+		{"/home/user", "", "", "", nil, "/home/user/.local/state/syncthing"},
+		// Data home set, doesn't matter
+		{"/home/user", "", "/somewhere/else", "", nil, "/home/user/.local/state/syncthing"},
+		// State home set, use that
+		{"/home/user", "", "", "/var/state", nil, "/var/state/syncthing"},
+
+		// Now some "upgrades", where we have files in the old locations.
+
+		// A database exists in the old default location, use that
+		{"/home/user", "", "", "", []string{"/home/user/.config/syncthing/index-v0.14.0.db"}, "/home/user/.config/syncthing"},
+		{"/home/user", "/config/dir", "/xdg/data/home", "/xdg/state/home", []string{"/home/user/.config/syncthing/index-v0.14.0.db"}, "/home/user/.config/syncthing"},
+
+		// A database exists in the config dir, use that
+		{"/home/user", "/config/dir", "/xdg/data/home", "/xdg/state/home", []string{"/config/dir/index-v0.14.0.db"}, "/config/dir"},
+
+		// A database exists in the old xdg data home, use that
+		{"/home/user", "/config/dir", "/xdg/data/home", "/xdg/state/home", []string{"/xdg/data/home/syncthing/index-v0.14.0.db"}, "/xdg/data/home/syncthing"},
+	}
+
+	for _, c := range cases {
+		fileExists := func(path string) bool { return slices.Contains(c.filesExist, path) }
+		actual := unixDataDir(c.userHome, c.configDir, c.xdgDataHome, c.xdgStateHome, fileExists)
+		if actual != c.expected {
+			t.Errorf("unixDataDir(%q, %q, %q, %q) == %q, expected %q", c.userHome, c.configDir, c.xdgDataHome, c.xdgStateHome, actual, c.expected)
+		}
+	}
+}