Honor NO_COLOR in the terminal log handler (closes #402)

The TTY log handler always wrote ANSI color, unlike internal/ui, which
disables color when NO_COLOR is set. Add a color flag to TTYHandler,
set from NO_COLOR with the same test internal/ui uses, and route every
color code through a paint helper that returns plain text when color is
off. The README's color sentence now covers log lines too.

Model: deepseek-v4-pro (implementation); opus-5-5 (rebase)

README.md

@@ -831,7 +831,8 @@ Work planned after 1.0. Loosely ordered by priority.
 
 Every command's user-facing output is governed by `internal/ui`, in one of two
 ways. Color is enabled when the stream written to is a TTY and the `NO_COLOR`
-environment variable is unset (https://no-color.org/).
+environment variable is unset (https://no-color.org/). The same rule governs the
+colorized log lines `internal/log` writes to a terminal.
 
 - **Status, progress, warnings, and errors** go through the `internal/ui`
   message methods below: marker-prefixed, colored on a TTY, and — except

internal/cli/app.go

@@ -7,9 +7,11 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"path/filepath"
 	"sync"
 	"time"
 
+	"github.com/adrg/xdg"
 	"github.com/spf13/cobra"
 	"go.uber.org/dig"
 	"go.uber.org/fx"
@@ -349,7 +351,8 @@ func runVaultikApp(
 // only one runs at a time; a read-only command runs without it and is not
 // blocked while a mutator holds the lock (opts.Mode).
 func RunWithApp(ctx context.Context, opts AppOptions) error {
-	release, err := acquireLockIfMutating(opts.Mode, pidlock.Dir())
+	release, err := acquireLockIfMutating(opts.Mode,
+		filepath.Join(xdg.DataHome, "vaultik"))
 	if err != nil {
 		return err
 	}

internal/cli/entry_snapshot_create_own_files_test.go

@@ -1,69 +0,0 @@
-package cli //nolint:testpackage // shares runEntry and the hermetic config helpers
-
-import (
-	"fmt"
-	"os"
-	"path/filepath"
-	"testing"
-
-	"github.com/adrg/xdg"
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
-)
-
-// TestEntrySnapshotCreateSkipsOwnFiles backs up a directory that holds
-// vaultik's index and PID file beside one other file, with no exclude
-// pattern set. The summary counts only the other file. See
-// https://git.eeqj.de/sneak/vaultik/issues/421.
-//
-// Not parallel: it replaces os.Args, os.Stdout, os.Stderr and the xdg
-// globals.
-func TestEntrySnapshotCreateSkipsOwnFiles(t *testing.T) {
-	for _, testCase := range []struct {
-		name string
-		// linked sets index_path to a symlink outside the backed-up
-		// directory that points to the index inside it.
-		linked bool
-	}{
-		{name: "index in the backed-up directory"},
-		{name: "index_path links into the backed-up directory", linked: true},
-	} {
-		t.Run(testCase.name, func(t *testing.T) {
-			dir := t.TempDir()
-			configPath := filepath.Join(dir, "config.yml")
-			sourceDir := filepath.Join(dir, "source")
-			storeDir := filepath.Join(dir, "store")
-
-			indexPath := filepath.Join(sourceDir, "index.sqlite")
-
-			if testCase.linked {
-				link := filepath.Join(dir, "index.sqlite")
-				require.NoError(t, os.Symlink(indexPath, link))
-
-				indexPath = link
-			}
-
-			contents := fmt.Sprintf(hermeticConfig,
-				sourceDir, storeDir, indexPath)
-			require.NoError(t,
-				os.WriteFile(configPath, []byte(contents), configFileMode))
-			require.NoError(t, os.Mkdir(storeDir, 0o750))
-			require.NoError(t, os.Mkdir(sourceDir, 0o750))
-			require.NoError(t, os.WriteFile(
-				filepath.Join(sourceDir, "file.txt"), []byte("hello"), 0o600))
-
-			// The PID file goes in a directory under XDG_DATA_HOME, which
-			// xdg reads at package init.
-			t.Setenv("XDG_DATA_HOME", filepath.Join(sourceDir, "data"))
-			xdg.Reload()
-			t.Cleanup(xdg.Reload)
-
-			code, stdout, _ := runEntry(t,
-				flagConfig, configPath, cmdSnapshot, cmdCreate)
-
-			require.Equal(t, 0, code)
-			assert.Contains(t, stdout,
-				"Files: 1 examined, 1 backed up, 0 unchanged.")
-		})
-	}
-}

internal/database/module.go

@@ -20,17 +20,6 @@ const indexDirPerm = 0o700
 // backed-up path and chunk hash and must stay private.
 const indexFilePerm = 0o600
 
-// IndexFiles returns the path of the index at indexPath and the paths of
-// the side files SQLite keeps beside it while it writes.
-func IndexFiles(indexPath string) []string {
-	return []string{
-		indexPath,
-		indexPath + "-wal",
-		indexPath + "-shm",
-		indexPath + "-journal",
-	}
-}
-
 // ensureIndexFileMode makes the index file owner-only before the SQLite
 // driver opens it: it creates the file 0600 if absent, or chmods an existing
 // one to 0600. Doing this first matters because SQLite creates its -wal and

internal/log/tty_handler.go

@@ -5,6 +5,7 @@ import (
 	"fmt"
 	"io"
 	"log/slog"
+	"os"
 	"strconv"
 	"strings"
 	"sync"
@@ -60,6 +61,12 @@ type TTYHandler struct {
 	mu   *sync.Mutex
 	out  io.Writer
 
+	// color reports whether records are written with ANSI color. It is
+	// false when NO_COLOR is set, matching internal/ui's shouldColor
+	// test; the caller already chose this handler for a terminal, so
+	// TTY-ness is not re-checked here.
+	color bool
+
 	// attrs are the attributes accumulated through WithAttrs, emitted
 	// ahead of each record's own attributes. Their keys already carry
 	// the group path that was open when they were added, so no
@@ -72,16 +79,19 @@ type TTYHandler struct {
 	groups []string
 }
 
-// NewTTYHandler creates a new TTY handler with colored output.
+// NewTTYHandler creates a new TTY handler. Color is enabled unless the
+// NO_COLOR environment variable is set to a non-empty value, the same
+// test internal/ui applies to status lines.
 func NewTTYHandler(out io.Writer, opts *slog.HandlerOptions) *TTYHandler {
 	if opts == nil {
 		opts = &slog.HandlerOptions{}
 	}
 
 	return &TTYHandler{
-		out:  out,
-		opts: *opts,
-		mu:   &sync.Mutex{},
+		out:   out,
+		opts:  *opts,
+		mu:    &sync.Mutex{},
+		color: os.Getenv("NO_COLOR") == "",
 	}
 }
 
@@ -124,10 +134,10 @@ func (h *TTYHandler) Handle(_ context.Context, r slog.Record) error {
 	// are written around it: it can carry text from an untrusted source
 	// (a storage error, for one), and a raw control character would
 	// otherwise reach the terminal.
-	_, _ = fmt.Fprintf(h.out, "%s%s%s %s%s%s %s%s%s",
-		colorGray, timestamp, colorReset,
-		levelColor, level, colorReset,
-		colorBold, sanitize(r.Message), colorReset)
+	_, _ = fmt.Fprintf(h.out, "%s %s %s",
+		h.paint(colorGray, timestamp),
+		h.paint(levelColor, level),
+		h.paint(colorBold, sanitize(r.Message)))
 
 	// Attributes carried by the handler come first, then the record's
 	// own. Handler attributes were qualified when they were added; the
@@ -236,6 +246,7 @@ func (h *TTYHandler) clone() *TTYHandler {
 		opts:   h.opts,
 		mu:     h.mu,
 		out:    h.out,
+		color:  h.color,
 		attrs:  make([]slog.Attr, len(h.attrs), len(h.attrs)+1),
 		groups: make([]string, len(h.groups), len(h.groups)+1),
 	}
@@ -273,9 +284,19 @@ func (h *TTYHandler) writeAttr(a slog.Attr) {
 	// the snapshot database — so a control character in one of them must
 	// be rendered as an escape sequence rather than reaching the terminal,
 	// where it could move the cursor or inject its own colours.
-	_, _ = fmt.Fprintf(h.out, " %s%s%s=%s%s%s",
-		colorCyan, sanitize(a.Key), colorReset,
-		colorBlue, sanitize(value), colorReset)
+	_, _ = fmt.Fprintf(h.out, " %s=%s",
+		h.paint(colorCyan, sanitize(a.Key)),
+		h.paint(colorBlue, sanitize(value)))
+}
+
+// paint wraps s in the given ANSI color when color is enabled, and
+// returns s unchanged otherwise.
+func (h *TTYHandler) paint(color, s string) string {
+	if !h.color {
+		return s
+	}
+
+	return color + s + colorReset
 }
 
 // sanitize returns s unchanged when every rune in it is printable, and a

internal/log/tty_handler_test.go

@@ -421,3 +421,44 @@ func TestTTYHandlerEmptyGroupAndAttrsAreNoOps(t *testing.T) {
 	assert.Contains(t, plain, "kept=yes")
 	assert.NotContains(t, plain, " =")
 }
+
+// TestTTYHandlerRespectsNoColor checks that only a non-empty NO_COLOR
+// turns color off, matching internal/ui's shouldColor test. The handler
+// is only ever chosen for a terminal, so this is the one remaining
+// condition that can still leave color in log output.
+func TestTTYHandlerRespectsNoColor(t *testing.T) {
+	cases := []struct {
+		name    string
+		noColor string
+		colored bool
+	}{
+		{
+			name:    "NO_COLOR turns color off",
+			noColor: "1",
+		},
+		{
+			name:    "empty NO_COLOR keeps color on",
+			noColor: "",
+			colored: true,
+		},
+	}
+
+	for _, testCase := range cases {
+		t.Run(testCase.name, func(t *testing.T) {
+			t.Setenv("NO_COLOR", testCase.noColor)
+
+			var buf bytes.Buffer
+
+			logger := slog.New(log.NewTTYHandler(&buf, debugHandlerOptions()))
+			logger.Info("hello", "key", "value")
+
+			if testCase.colored {
+				assert.Contains(t, buf.String(), "\x1b[",
+					"an empty NO_COLOR must leave ANSI color in log output")
+			} else {
+				assert.NotContains(t, buf.String(), "\x1b[",
+					"NO_COLOR must suppress every ANSI escape in log output")
+			}
+		})
+	}
+}

internal/pidlock/pidlock.go

@@ -11,7 +11,6 @@ import (
 	"strconv"
 	"strings"
 
-	"github.com/adrg/xdg"
 	"golang.org/x/sys/unix"
 )
 
@@ -30,20 +29,8 @@ const (
 	lockDirPerm = 0o700
 	// pidFilePerm is the mode for the PID file (owner-only).
 	pidFilePerm = 0o600
-	// pidFileName is the name of the PID file in the lock directory.
-	pidFileName = "vaultik.pid"
 )
 
-// Dir returns the directory that holds vaultik's PID file.
-func Dir() string {
-	return filepath.Join(xdg.DataHome, "vaultik")
-}
-
-// Path returns the path of the PID file that Acquire uses in lockDir.
-func Path(lockDir string) string {
-	return filepath.Join(lockDir, pidFileName)
-}
-
 // Acquire attempts to acquire a PID lock in the specified directory.
 // If another process holds the lock, it returns ErrAlreadyRunning with
 // that process's PID. On success, it writes the current PID to the lock
@@ -55,7 +42,7 @@ func Acquire(lockDir string) (*Lock, error) {
 		return nil, fmt.Errorf("creating lock directory: %w", err)
 	}
 
-	lockPath := Path(lockDir)
+	lockPath := filepath.Join(lockDir, "vaultik.pid")
 
 	// No O_TRUNC: the file may hold the PID of the process that has the
 	// lock, which the error below reports.

internal/snapshot/module.go

@@ -2,13 +2,11 @@ package snapshot
 
 import (
 	"os"
-	"path/filepath"
 
 	"github.com/spf13/afero"
 	"go.uber.org/fx"
 	"sneak.berlin/go/vaultik/internal/config"
 	"sneak.berlin/go/vaultik/internal/database"
-	"sneak.berlin/go/vaultik/internal/pidlock"
 	"sneak.berlin/go/vaultik/internal/storage"
 	"sneak.berlin/go/vaultik/internal/ui"
 )
@@ -64,51 +62,9 @@ func provideScannerFactory(
 			EnableProgress:     params.EnableProgress,
 			UI:                 params.UI,
 			Exclude:            excludes,
-			OwnFiles:           ownFiles(cfg.IndexPath, pidlock.Dir()),
 			SkipErrors:         params.SkipErrors,
 			PreviousSnapshotID: params.PreviousSnapshotID,
 			DeviceNumber:       params.DeviceNumber,
 		})
 	}
 }
-
-// ownFiles returns the paths of the index at indexPath, its side files
-// and the PID file in lockDir. Symlinks in their directories are resolved,
-// as they are in the snapshot's source paths, so the scan finds them under
-// the same names. If indexPath is a symlink, the file it points to and
-// that file's side files are returned too.
-func ownFiles(indexPath, lockDir string) []string {
-	paths := append(database.IndexFiles(indexPath), pidlock.Path(lockDir))
-	for i, p := range paths {
-		paths[i] = resolveDir(p)
-	}
-
-	// SQLite writes the file a symlinked index points to and keeps the
-	// side files beside that file. The database package creates the index
-	// before the scanner is built, so the link resolves.
-	index := resolveDir(indexPath)
-
-	target, err := filepath.EvalSymlinks(index)
-	if err == nil && target != index {
-		paths = append(paths, database.IndexFiles(target)...)
-	}
-
-	return paths
-}
-
-// resolveDir returns the absolute form of path with the symlinks in its
-// directory resolved. If the directory cannot be resolved, it returns the
-// absolute form unresolved. If path has no absolute form, it returns path.
-func resolveDir(path string) string {
-	abs, err := filepath.Abs(path)
-	if err != nil {
-		return path
-	}
-
-	dir, err := filepath.EvalSymlinks(filepath.Dir(abs))
-	if err != nil {
-		return abs
-	}
-
-	return filepath.Join(dir, filepath.Base(abs))
-}

internal/snapshot/module_test.go

@@ -1,32 +0,0 @@
-//nolint:testpackage // exercises the unexported ownFiles helper
-package snapshot
-
-import (
-	"os"
-	"path/filepath"
-	"testing"
-
-	"github.com/stretchr/testify/require"
-)
-
-// The scan walks source paths with symlinks resolved, so vaultik's own
-// files are named the same way, or a symlinked home would back them up.
-func TestOwnFilesResolvesSymlinkedDirectory(t *testing.T) {
-	t.Parallel()
-
-	realDir, err := filepath.EvalSymlinks(t.TempDir())
-	require.NoError(t, err)
-
-	link := filepath.Join(t.TempDir(), "link")
-	require.NoError(t, os.Symlink(realDir, link))
-
-	paths := ownFiles(filepath.Join(link, "index.sqlite"), link)
-
-	require.Equal(t, []string{
-		filepath.Join(realDir, "index.sqlite"),
-		filepath.Join(realDir, "index.sqlite-wal"),
-		filepath.Join(realDir, "index.sqlite-shm"),
-		filepath.Join(realDir, "index.sqlite-journal"),
-		filepath.Join(realDir, "vaultik.pid"),
-	}, paths)
-}

internal/snapshot/own_files_test.go

@@ -1,65 +0,0 @@
-package snapshot_test
-
-import (
-	"context"
-	"testing"
-
-	"github.com/spf13/afero"
-	"github.com/stretchr/testify/require"
-	"sneak.berlin/go/vaultik/internal/database"
-	"sneak.berlin/go/vaultik/internal/snapshot"
-)
-
-// A backup of the directory that holds vaultik's index and PID file
-// stores neither, with no exclude pattern set, and still stores the
-// files beside them. See https://git.eeqj.de/sneak/vaultik/issues/421.
-func TestScanSkipsOwnFiles(t *testing.T) {
-	t.Parallel()
-
-	const dataDir = "/home/user/.local/share/vaultik"
-
-	ownFiles := append(database.IndexFiles(dataDir+"/index.sqlite"),
-		dataDir+"/vaultik.pid")
-
-	fs := afero.NewMemMapFs()
-	for _, p := range append(ownFiles, dataDir+"/notes.txt") {
-		require.NoError(t, afero.WriteFile(fs, p, []byte("data"), 0o600))
-	}
-
-	db, err := database.NewTestDB()
-	require.NoError(t, err)
-
-	defer func() { _ = db.Close() }()
-
-	repos := database.NewRepositories(db)
-
-	scanner := snapshot.NewScanner(snapshot.ScannerConfig{
-		FS:               fs,
-		ChunkSize:        64 * 1024,
-		Repositories:     repos,
-		MaxBlobSize:      1024 * 1024,
-		CompressionLevel: 3,
-		AgeRecipients: []string{
-			"age1ql3z7hjy54pw3hyww5ayyfg7zqgvc7w3j2elw8zmrj2kg5sfn9aqmcac8p"},
-		OwnFiles: ownFiles,
-	})
-	require.NotNil(t, scanner)
-
-	ctx := context.Background()
-	createSnapshotRecord(ctx, t, repos, "test-snapshot")
-
-	result, err := scanner.Scan(ctx, "/home/user", "test-snapshot")
-	require.NoError(t, err)
-
-	require.Equal(t, 1, result.FilesScanned)
-
-	stored, err := repos.Files.GetByPath(ctx, dataDir+"/notes.txt")
-	require.NoError(t, err)
-	require.NotNil(t, stored, "a file beside the index is stored")
-
-	for _, p := range ownFiles {
-		f, err := repos.Files.GetByPath(ctx, p)
-		require.NoError(t, err)
-		require.Nil(t, f, "%s is not stored", p)
-	}
-}

internal/snapshot/scanner.go

@@ -67,10 +67,7 @@ type Scanner struct {
 	currentSourcePath string
 	exclude           []string          // Glob patterns for files/directories to exclude
 	compiledExclude   []compiledPattern // Compiled glob patterns
-	// ownFiles holds the paths of vaultik's own working files, which the
-	// scan treats as excluded (ScannerConfig.OwnFiles).
-	ownFiles map[string]struct{}
-	progress *ProgressReporter
+	progress          *ProgressReporter
 	// skipErrors skips files that cannot be opened or read (logged loudly);
 	// packer, database, encryption, and upload errors still abort the run.
 	skipErrors bool
@@ -136,11 +133,6 @@ type ScannerConfig struct {
 	EnableProgress   bool       // Enable the live progress reporter (ETAs, throughput)
 	UI               *ui.Writer // Where user-facing scanner messages go; nil = discard
 	Exclude          []string   // Glob patterns for files/directories to exclude
-	// OwnFiles are the absolute paths of vaultik's own working files: the
-	// index, its side files and the PID file. The scan treats them as
-	// excluded, whatever Exclude says, since a copy taken while they are
-	// being written is torn.
-	OwnFiles []string
 	// SkipErrors skips files that cannot be opened or read (log loudly but
 	// continue); packer, database, encryption, and upload errors still abort.
 	SkipErrors bool
@@ -234,11 +226,6 @@ func NewScanner(cfg ScannerConfig) *Scanner {
 		deviceNumber = deviceNumberFromStat
 	}
 
-	ownFiles := make(map[string]struct{}, len(cfg.OwnFiles))
-	for _, p := range cfg.OwnFiles {
-		ownFiles[filepath.Clean(p)] = struct{}{}
-	}
-
 	return &Scanner{
 		fs:                 cfg.FS,
 		chunker:            chunker.NewChunker(cfg.ChunkSize),
@@ -250,7 +237,6 @@ func NewScanner(cfg ScannerConfig) *Scanner {
 		ageRecipient:       strings.Join(cfg.AgeRecipients, ","),
 		exclude:            cfg.Exclude,
 		compiledExclude:    compiledExclude,
-		ownFiles:           ownFiles,
 		progress:           progress,
 		skipErrors:         cfg.SkipErrors,
 		previousSnapshotID: cfg.PreviousSnapshotID,
@@ -2262,9 +2248,8 @@ func (s *Scanner) detectDeletedFiles(
 
 // excludedOrInExcludedDir reports whether filePath, which is at or
 // beneath rootPath, or any directory between rootPath and it, matches an
-// exclude pattern or is one of vaultik's own files. A pattern such as
-// */.local/share/Trash matches the directory but not the files inside it,
-// which the walk skips too.
+// exclude pattern. A pattern such as */.local/share/Trash matches the
+// directory but not the files inside it, which the walk skips too.
 func (s *Scanner) excludedOrInExcludedDir(filePath, rootPath string) bool {
 	for p := filePath; len(p) > len(rootPath); p = filepath.Dir(p) {
 		if s.shouldExclude(p, rootPath) {
@@ -2388,14 +2373,9 @@ func doubleStarVariants(pattern string) []string {
 }
 
 // shouldExclude checks if a path should be excluded based on exclude patterns
-// or because it is one of vaultik's own working files.
 // filePath is the full path to the file
 // rootPath is the root of the backup source directory
 func (s *Scanner) shouldExclude(filePath, rootPath string) bool {
-	if _, own := s.ownFiles[filePath]; own {
-		return true
-	}
-
 	if len(s.compiledExclude) == 0 {
 		return false
 	}