Vet, lint and race-detect in CI

.github/workflows/build.yml

@@ -48,13 +48,38 @@ jobs:
         working-directory: frontend
         run: npm test
 
+      - name: Vet
+        # Both builds (see internal/features): the personal tag compiles the
+        # other half of a few files.
+        run: |
+          go vet ./...
+          if ($LASTEXITCODE -ne 0) { exit $LASTEXITCODE }
+          go vet -tags personal ./...
+
+      - name: Lint
+        uses: golangci/golangci-lint-action@ba0d7d2ec06a0ea1cb5fa41b2e4a3ab91d21278a # v9.3.0
+        with:
+          version: v2.15.0 # the version to run locally too (CONTRIBUTING.md)
+
+      - name: Lint (personal build)
+        # The action above put golangci-lint on PATH.
+        run: golangci-lint run --build-tags personal ./...
+
       - name: Test
         run: go test ./...
 
       - name: Test (personal build)
         # The personal build is tested too (see internal/features).
         run: go test -tags personal ./...
 
+      - name: Test with the race detector
+        # The sync code is concurrent; -short keeps the randomized crash test
+        # in internal/accounts small (the full run is above).
+        run: |
+          go test -race -short ./...
+          if ($LASTEXITCODE -ne 0) { exit $LASTEXITCODE }
+          go test -race -short -tags personal ./...
+
       - name: Version
         # Release builds carry their tag (shown in the app, used by the update
         # check) and put it in the exe's file properties; others are "dev".

.golangci.yml

@@ -0,0 +1,21 @@
+version: "2"
+
+linters:
+  # The standard set: govet, ineffassign, staticcheck, unused (errcheck is in
+  # it too, see below). CI pins the golangci-lint version in
+  # .github/workflows/build.yml; run the same one locally.
+  default: standard
+  disable:
+    # Not enabled yet: it finds about 50 unchecked Close/Remove calls that need
+    # fixing one by one before it can gate CI.
+    - errcheck
+  settings:
+    staticcheck:
+      checks:
+        - all
+        # Error strings are shown to the user as they are, so they are
+        # sentences: capitalized and with a full stop.
+        - -ST1005
+        # Doc comments on vars and consts here often read "Name, when set, ..."
+        # or "Name: ..."; the name still comes first, which is what matters.
+        - -ST1022

CONTRIBUTING.md

@@ -16,7 +16,9 @@ Thanks for helping. Syncer is a one-person project, so small, focused changes ge
 
    ```bash
    go vet ./... && go vet -tags personal ./...
+   golangci-lint run ./... && golangci-lint run --build-tags personal ./...
    go test ./... && go test -tags personal ./...
+   go test -race -short ./... && go test -race -short -tags personal ./...
    cd frontend && npm ci && npm run check && npx vitest run
    cd .. && wails build
    ```
@@ -26,7 +28,7 @@ Thanks for helping. Syncer is a one-person project, so small, focused changes ge
 
 ## Build and layout
 
-Requirements: Go 1.26+, Node 22+, [Wails v2](https://wails.io) (`go install github.com/wailsapp/wails/v2/cmd/wails@v2.16.0`).
+Requirements: Go 1.26+, Node 22+, [Wails v2](https://wails.io) (`go install github.com/wailsapp/wails/v2/cmd/wails@v2.16.0`), [golangci-lint](https://golangci-lint.run) at the version pinned in `.github/workflows/build.yml` (`go install github.com/golangci/golangci-lint/v2/cmd/golangci-lint@v2.15.0`), and a C compiler such as mingw gcc for `go test -race`.
 
 ```bash
 wails dev          # live-reloading app

internal/accounts/ops.go

@@ -949,7 +949,7 @@ func swap(ctx context.Context, env Env, j *Journal, m *Move) error {
 		}
 	}
 	if m.Step == swapHold {
-		if !(exists(m.Hold) && !exists(m.Live)) {
+		if !exists(m.Hold) || exists(m.Live) {
 			if err := os.Rename(m.Live, m.Hold); err != nil {
 				return back(fmt.Errorf("could not move the current saves (is the game running?): %w", err))
 			}
@@ -975,7 +975,7 @@ func swap(ctx context.Context, env Env, j *Journal, m *Move) error {
 				return j.failAt(err)
 			}
 		}
-		if !(exists(m.Live) && !exists(m.In)) {
+		if !exists(m.Live) || exists(m.In) {
 			if err := moveDir(m.In, m.Live); err != nil {
 				return back(fmt.Errorf("could not bring in the other saves: %w", err))
 			}
@@ -995,7 +995,7 @@ func swap(ctx context.Context, env Env, j *Journal, m *Move) error {
 		}
 	}
 	if m.Step == swapOut {
-		if !(exists(m.Out) && !exists(m.Hold)) {
+		if !exists(m.Out) || exists(m.Hold) {
 			if exists(m.Out) {
 				if _, err := retire(m.Out); err != nil {
 					return j.failAt(err)

internal/app/app.go

@@ -1,3 +1,6 @@
+// Package app is the desktop app: the Wails App whose methods the frontend
+// calls, the tray, the background and scheduled runs, the local launcher API
+// and the glue that wires the internal packages to the UI.
 package app
 
 import (
@@ -201,13 +204,13 @@ type Overview struct {
 	Target     string           `json:"target"`
 	LastBackup *store.BackupRun `json:"lastBackup"` // the last attempt, failed or not
 	// LastSuccess is when a backup last finished without errors; zero if never.
-	LastSuccess time.Time `json:"lastSuccess"`
-	BackingUp  bool             `json:"backingUp"`
-	Gaming     bool             `json:"gaming"` // automatic backups are holding off right now
-	Paused     bool             `json:"paused"` // syncing and automatic backups are paused (until settings.pausedUntil)
-	Settings   store.Settings   `json:"settings"`
-	Version    string           `json:"version"`
-	Update     *UpdateInfo      `json:"update"` // a newer release, if one is out
+	LastSuccess time.Time      `json:"lastSuccess"`
+	BackingUp   bool           `json:"backingUp"`
+	Gaming      bool           `json:"gaming"` // automatic backups are holding off right now
+	Paused      bool           `json:"paused"` // syncing and automatic backups are paused (until settings.pausedUntil)
+	Settings    store.Settings `json:"settings"`
+	Version     string         `json:"version"`
+	Update      *UpdateInfo    `json:"update"` // a newer release, if one is out
 	// VersionGaps: linked PCs running an older or newer Syncer than this one.
 	VersionGaps []VersionGap `json:"versionGaps"`
 	// Waiting: folders from other PCs that wait for the user's OK.
@@ -490,7 +493,7 @@ func normalizeID(id string) (string, error) {
 		return "", errors.New("that doesn't look like a device ID (expected 56 letters/digits)")
 	}
 	for _, ch := range raw {
-		if !(ch >= 'A' && ch <= 'Z' || ch >= '2' && ch <= '7') {
+		if (ch < 'A' || ch > 'Z') && (ch < '2' || ch > '7') {
 			return "", errors.New("device IDs only contain letters A–Z and digits 2–7")
 		}
 	}
@@ -909,7 +912,7 @@ func applySettingsInput(s *store.Settings, in, old store.Settings) {
 // modsAutoOn reports whether adding mod folders automatically just started
 // (or its size limit changed), so it should run now.
 func modsAutoOn(old, s store.Settings) bool {
-	return features.Mods && s.FindMods && s.AutoAddMods && (!(old.FindMods && old.AutoAddMods) || s.ModsMaxGB != old.ModsMaxGB)
+	return features.Mods && s.FindMods && s.AutoAddMods && (!old.FindMods || !old.AutoAddMods || s.ModsMaxGB != old.ModsMaxGB)
 }
 
 // publishSettings shares a change of the mod options with the other PCs now.

internal/backup/backup.go

@@ -275,7 +275,7 @@ func mirror(ctx context.Context, f Folder, opts Options, stamp string, p *Progre
 		}
 		if m.Ignored(rel) || sc.skip(rel, d.IsDir()) {
 			// A negation can reach into an ignored folder.
-			if d.IsDir() && !(m.HasNegation() && !sc.on) {
+			if d.IsDir() && (!m.HasNegation() || sc.on) {
 				return filepath.SkipDir
 			}
 			return nil

internal/meta/meta.go

@@ -1062,15 +1062,15 @@ func ModVersionsDir(id string) string {
 	return filepath.Join(paths.Root(paths.Local), "Syncer", "mod-versions", id)
 }
 
+// HoldNewMod, when set, says whether a new mod folder must start paused
+// (its mod manager is open) and remembers to resume it.
+var HoldNewMod func(id string) bool
+
 // ModSpec is how a mod folder of kind is added. A staging folder uses
 // Vortex's own marker as Syncthing's, so no .stfolder appears among the mods
 // and the folder stops, rather than looking emptied, if its drive is
 // missing. Replaced files are kept for a week. A PC receiving deployed mods
 // only takes them from the source, and stays paused between audited updates.
-// HoldNewMod, when set, says whether a new mod folder must start paused
-// (its mod manager is open) and remembers to resume it.
-var HoldNewMod func(id string) bool
-
 func ModSpec(id, label, path, kind string, source bool) FolderSpec {
 	v := syncthing.StaggeredVersioning()
 	v["params"] = map[string]string{"maxAge": "604800", "cleanInterval": "3600"}

internal/meta/modsettings_test.go

@@ -79,3 +79,19 @@ func TestModSettingsNewestWins(t *testing.T) {
 		t.Fatal("mod options stayed on without finding mods")
 	}
 }
+
+// writtenBySelf records PC device's file, as it is now, as last written by
+// that PC, as a reconcile finds out from Syncthing.
+func writtenBySelf(t *testing.T, device string) {
+	t.Helper()
+	sum, ok := fileSum(device)
+	if !ok {
+		t.Fatalf("no file of %s", device)
+	}
+	writers.Lock()
+	defer writers.Unlock()
+	if writers.m == nil {
+		writers.m = map[string]writerCheck{}
+	}
+	writers.m[device] = writerCheck{sum, true}
+}

internal/meta/owners.go

@@ -322,7 +322,7 @@ func noIntroducers(ctx context.Context, c *syncthing.Client, ds []syncthing.Devi
 	for _, d := range ds {
 		id := d.DeviceID
 		_, introduced := st.Introduced[id]
-		if !d.Introducer || !(pcs[id] || d.IntroducedBy != "" || st.JoinRequests[id] || st.IgnoredPCs[id] || introduced || ours[id]) {
+		if !d.Introducer || (!pcs[id] && d.IntroducedBy == "" && !st.JoinRequests[id] && !st.IgnoredPCs[id] && !introduced && !ours[id]) {
 			continue
 		}
 		if err := c.PatchDevice(ctx, id, map[string]any{"introducer": false}); err != nil {
@@ -465,7 +465,7 @@ func validDeviceID(id string) bool {
 			if ch != '-' {
 				return false
 			}
-		} else if !(ch >= 'A' && ch <= 'Z' || ch >= '2' && ch <= '7') {
+		} else if (ch < 'A' || ch > 'Z') && (ch < '2' || ch > '7') {
 			return false
 		}
 	}

internal/meta/owners_test.go

@@ -177,22 +177,6 @@ func TestWriteVortexListsLeavesAnUnchangedFileAlone(t *testing.T) {
 	}
 }
 
-// writtenBySelf records PC device's file, as it is now, as last written by
-// that PC, as a reconcile finds out from Syncthing.
-func writtenBySelf(t *testing.T, device string) {
-	t.Helper()
-	sum, ok := fileSum(device)
-	if !ok {
-		t.Fatalf("no file of %s", device)
-	}
-	writers.Lock()
-	defer writers.Unlock()
-	if writers.m == nil {
-		writers.m = map[string]writerCheck{}
-	}
-	writers.m[device] = writerCheck{sum, true}
-}
-
 // fakeSyncthing is a Syncthing that, besides Syncer's PCs, syncs a notes
 // vault with a server and a laptop, as one paired before Syncer was fixed:
 // every folder offered to every device.

internal/steam/steam.go

@@ -421,22 +421,3 @@ func isFile(p string) bool {
 	return err == nil && fi.Mode().IsRegular()
 }
 
-func clean(p string) string {
-	p = strings.TrimSpace(p)
-	if p == "" {
-		return ""
-	}
-	return filepath.Clean(filepath.FromSlash(p))
-}
-
-// within reports whether child is parent or lies below it (case-insensitive).
-func within(parent, child string) bool {
-	parent, child = strings.ToLower(filepath.Clean(parent)), strings.ToLower(filepath.Clean(child))
-	if parent == child {
-		return true
-	}
-	if !strings.HasSuffix(parent, `\`) && !strings.HasSuffix(parent, "/") {
-		parent += string(filepath.Separator)
-	}
-	return strings.HasPrefix(child, parent)
-}

internal/store/store.go

@@ -648,7 +648,7 @@ var rename = os.Rename
 func renameRetry(from, to string) error {
 	var err error
 	for i, wait := 0, 10*time.Millisecond; i < 6; i, wait = i+1, wait*2 {
-		if err = rename(from, to); err == nil || !(errors.Is(err, windows.ERROR_ACCESS_DENIED) || errors.Is(err, windows.ERROR_SHARING_VIOLATION)) {
+		if err = rename(from, to); err == nil || (!errors.Is(err, windows.ERROR_ACCESS_DENIED) && !errors.Is(err, windows.ERROR_SHARING_VIOLATION)) {
 			return err
 		}
 		time.Sleep(wait)