From 631f61f12a9937d6d66e77b68040db28a2d47acc Mon Sep 17 00:00:00 2001 From: Young Joon Lee Date: Mon, 9 Nov 2026 22:18:24 +0900 Subject: [PATCH 1/3] chore(sync): stop reading the retired mirror_path key ResolveConfig fell back to mirror_path in the workspace store and then in global state before the detected cloud default. The destination is the store's target now, so the fallback goes, with LocalConfig.MirrorPath, UserGsyncState.MirrorPath and every writer of them: `dot sync target` writes the store alone, and under --home it writes that home's store. A store or state file that still sets the old key is not edited quietly. Both write choke points announce the key once and name `dot sync target`, and keepUnknownConfigKeys exempts it, or the next save would carry it back forever. Part of #394. --- internal/cli/sync_cli_test.go | 27 +++---- internal/cli/sync_configure.go | 1 - internal/cli/sync_target_cmd.go | 38 +++------- internal/config/state.go | 65 +++++++++++++++-- internal/config/state_retired_test.go | 54 ++++++++++++++ internal/syncer/local_store.go | 70 ++++++++++++++++-- internal/syncer/local_store_test.go | 100 +++++++++++++++++++++++--- internal/syncer/sync.go | 18 ++--- internal/syncer/sync_test.go | 43 ++++++++--- internal/syncer/target.go | 3 +- 10 files changed, 332 insertions(+), 87 deletions(-) diff --git a/internal/cli/sync_cli_test.go b/internal/cli/sync_cli_test.go index 3bbaf679..aa777dad 100644 --- a/internal/cli/sync_cli_test.go +++ b/internal/cli/sync_cli_test.go @@ -39,7 +39,7 @@ func newSyncCLIFixture(t *testing.T) *syncCLIFixture { if err := os.MkdirAll(filepath.Dir(stateFile), 0o755); err != nil { t.Fatal(err) } - state := "modules:\n gsync:\n local_path: " + local + "\n mirror_path: " + mirror + "\n" + state := "modules:\n gsync:\n local_path: " + local + "\n" if err := os.WriteFile(stateFile, []byte(state), 0o644); err != nil { t.Fatal(err) } @@ -414,33 +414,34 @@ func TestSyncMirrorCLI_PrintAndDryRunAreReadOnly(t *testing.T) { } } -func TestSyncMirrorCLI_HonorsHomeOverride(t *testing.T) { +func TestSyncTargetCLI_HonorsHomeOverride(t *testing.T) { f := newSyncCLIFixture(t) other := t.TempDir() // a different user's home if err := os.MkdirAll(filepath.Join(other, ".config", "dotfiles"), 0o755); err != nil { t.Fatal(err) } - // ~ in the path must expand against --home, and global state must be - // written for that home — not the current HOME. + // ~ in the path must expand against --home, and the store that records it + // must be that home's store — not the current user's. out, errOut, err := runDotForTest("sync", "target", "~/Dropbox/work", "--home", other) if err != nil { - t.Fatalf("gsync mirror --home: %v\nstdout=%s\nstderr=%s", err, out, errOut) + t.Fatalf("sync target --home: %v\nstdout=%s\nstderr=%s", err, out, errOut) } wantMirror := filepath.Join(other, "Dropbox", "work") if !strings.Contains(out, wantMirror) { t.Errorf("~ should expand against --home %q:\n%s", wantMirror, out) } - otherCfg, err := os.ReadFile(filepath.Join(other, ".config", "dotfiles", "config.yaml")) + otherStore := filepath.Join(other, "workspace", "work", ".dotfiles", "sync", "config.yaml") + otherCfg, err := os.ReadFile(otherStore) if err != nil { - t.Fatalf("read --home state: %v", err) + t.Fatalf("read --home store: %v", err) } - if !strings.Contains(string(otherCfg), "mirror_path: "+wantMirror) { - t.Errorf("--home global state missing mirror_path:\n%s", otherCfg) + if !strings.Contains(string(otherCfg), "target: local:"+wantMirror) { + t.Errorf("--home store missing the target:\n%s", otherCfg) } - // The current user's state must NOT have been written with this mirror. - if cur, _ := os.ReadFile(filepath.Join(f.home, ".config", "dotfiles", "config.yaml")); strings.Contains(string(cur), wantMirror) { - t.Errorf("current-user state wrongly written under --home:\n%s", cur) + // The current user's store must NOT have been written with this target. + if cur, _ := os.ReadFile(filepath.Join(f.local, ".dotfiles", "sync", "config.yaml")); strings.Contains(string(cur), wantMirror) { + t.Errorf("current-user store wrongly written under --home:\n%s", cur) } } @@ -580,7 +581,7 @@ func TestSyncPushRefusesLegacyOnlyStore(t *testing.T) { f := newSyncCLIFixture(t) legacy := filepath.Join(f.local, ".dotfiles", "gdrive-sync") writeCLITestFile(t, filepath.Join(legacy, "baseline.manifest"), "docs/a.md\t1\n") - writeCLITestFile(t, filepath.Join(legacy, "config.yaml"), "mirror_path: "+f.mirror+"\n") + writeCLITestFile(t, filepath.Join(legacy, "config.yaml"), "target: local:"+f.mirror+"\n") writeCLITestFile(t, filepath.Join(f.local, "docs", "a.md"), "local edit\n") writeCLITestFile(t, filepath.Join(f.mirror, "docs", "a.md"), "mirror copy\n") before := snapshotTree(t, f.home) diff --git a/internal/cli/sync_configure.go b/internal/cli/sync_configure.go index f55a1c62..1c0a9a9e 100644 --- a/internal/cli/sync_configure.go +++ b/internal/cli/sync_configure.go @@ -59,7 +59,6 @@ func runSyncConfigure(cmd *cobra.Command, _ []string) error { } if target.Kind == syncer.TargetLocal { target.Path = appsettings.ExpandHome(target.Path, homeFor(cmd)) - local.MirrorPath = target.Path } local.Target = target.String() } diff --git a/internal/cli/sync_target_cmd.go b/internal/cli/sync_target_cmd.go index 5c6666d8..e178a4ff 100644 --- a/internal/cli/sync_target_cmd.go +++ b/internal/cli/sync_target_cmd.go @@ -68,39 +68,19 @@ func runSyncTarget(cmd *cobra.Command, args []string) error { p.KV("Target", bs.Config.Target.String()) return nil } - p.Line("[dry-run] would set target to %s (local config%s)", target.String(), - map[bool]string{true: " + global state", false: ""}[target.Kind == syncer.TargetLocal]) + p.Line("[dry-run] would set target to %s (local config)", target.String()) return nil } - homeOverride, _ := cmd.Flags().GetString("home") - - // Local config governs the current workspace (global state is ignored - // once it exists), so write it for immediate effect — but only for the - // current user. Under --home the admin isn't in the target user's - // workspace, so the local config (always current-workspace) doesn't - // apply; only the home-aware global state below is meaningful there. - if homeOverride == "" { - bs, err := syncer.Bootstrap(syncBootstrapOptions(cmd, false)) - if err != nil { - return err - } - if err := syncer.SetLocalTarget(bs.Config, target); err != nil { - return err - } + // The store belongs to the workspace and the workspace belongs to the home: + // Bootstrap resolved both against --home, so an admin running this for + // another user writes that user's store, not the invoking user's. + bs, err := syncer.Bootstrap(syncBootstrapOptions(cmd, false)) + if err != nil { + return err } - - // Global state, home-aware: local targets are inherited by future - // workspaces. SSH targets stay workspace-local. - if target.Kind == syncer.TargetLocal { - state, err := loadStateForCmd(cmd) - if err != nil { - return fmt.Errorf("load global state: %w", err) - } - state.Modules.Gsync.MirrorPath = target.Path - if err := persistUserState(cmd, state); err != nil { - p.Warn("could not update global state: %v", err) - } + if err := syncer.SetLocalTarget(bs.Config, target); err != nil { + return err } p.Line("%s", ui.StyleSuccess.Render("✓ sync target set")) diff --git a/internal/config/state.go b/internal/config/state.go index d2262fa3..820c34d8 100644 --- a/internal/config/state.go +++ b/internal/config/state.go @@ -177,6 +177,37 @@ func sliceHasRetiredCask(tokens []string) bool { return false } +// retiredStateKeys are the state keys no release reads any more, with what +// replaces each one. `modules.gsync.mirror_path` lost its last reader when +// ResolveConfig stopped falling back to it (#394 row 10): the destination is +// the workspace store's `target` now, so the value cannot be carried over and +// is only dropped. +var retiredStateKeys = map[string]string{ + "modules.gsync.mirror_path": "`dot sync` uses the workspace store's `target` (set it with `dot sync target`)", +} + +// detectRetiredKeys records which retired keys the raw state still sets, so +// the next save can say what it dropped. It reads the raw bytes the way +// applyLegacyKeys does -- the field these keys named is gone, so a decode +// cannot see them -- but it runs for every schema version: #373's stamped-file +// rule governs keys whose values are carried, and these have no value left to +// carry. +func detectRetiredKeys(state *UserState, raw []byte) { + var legacy struct { + Modules struct { + Gsync struct { + MirrorPath string `yaml:"mirror_path"` + } `yaml:"gsync"` + } `yaml:"modules"` + } + if err := yaml.Unmarshal(raw, &legacy); err != nil { + return + } + if legacy.Modules.Gsync.MirrorPath != "" { + state.retiredKeys = append(state.retiredKeys, "modules.gsync.mirror_path") + } +} + // warnForwardSchema reports that a state file was written by a newer dot. It // does not fail the load: an additively-forward file still decodes, and an // incompatibly-forward one gets this warning before its decode error so the @@ -210,6 +241,12 @@ type UserState struct { // so yaml.v3 cannot see it: the flag can never be written to disk and // needs no tag. saveStateAt reads it to print one note per converted write. carriedLegacy []string + + // retiredKeys names the keys the file this state was read from still + // carried after they stopped being read, in detection order. Unexported for + // the same reason as carriedLegacy: the save that drops them is the only + // place that may speak of them, and it prints one note, then clears this. + retiredKeys []string } // UserModulesState holds module opt-in/config from user state. @@ -329,21 +366,21 @@ type UserRsyncState struct { // UserGsyncState holds local↔local rsync mirror config from user state. // // It holds the machine-wide defaults `dot sync` seeds a new workspace store -// with (mirror path, max delete, shared excludes) and LocalPath, which locates -// the workspace. Per-workspace settings live in /.dotfiles/sync/. +// with (max delete, shared excludes) and LocalPath, which locates the +// workspace. The destination is not one of them: it lives in the workspace +// store's `target`. Per-workspace settings live in /.dotfiles/sync/. // // Paused defaults to true on a fresh state so the user explicitly opts in by // flipping it to false via `resume`. type UserGsyncState struct { - LocalPath string `yaml:"local_path,omitempty"` // primary tree, default ~/workspace/work - MirrorPath string `yaml:"mirror_path,omitempty"` // mirror tree, default ~/gdrive-workspace/work + LocalPath string `yaml:"local_path,omitempty"` // primary tree, default ~/workspace/work LastPull time.Time `yaml:"last_pull,omitempty"` LastPush time.Time `yaml:"last_push,omitempty"` ConflictDir string `yaml:"conflict_dir,omitempty"` // default /.sync-conflicts Paused bool `yaml:"paused,omitempty"` // gates pull/push/sync; cleared by `resume` MaxDelete int `yaml:"max_delete,omitempty"` // safety cap for push --delete-after, default 1000 Interval int `yaml:"interval,omitempty"` // auto-sync interval in seconds (launchd/systemd), default 300 - SharedExcludes []string `yaml:"shared_excludes,omitempty"` // operator-curated owned-but-shared-out folders, relative to mirror_path + SharedExcludes []string `yaml:"shared_excludes,omitempty"` // operator-curated owned-but-shared-out folders, relative to the sync target } // UserTunnelState holds the Cloudflare Tunnel values managed by `dot tunnel`. @@ -492,12 +529,12 @@ func (s *UserState) Validate() error { } } for _, p := range s.Modules.Gsync.SharedExcludes { - // Paths must be relative to mirror_path. Absolute paths and parent + // Paths must be relative to the sync target. Absolute paths and parent // escapes would let the manual list reach outside the mirror tree // and exclude unrelated content (or be portable across machines // in misleading ways). if strings.HasPrefix(p, "/") { - return fmt.Errorf("gdrive_sync.shared_excludes entry %q must be relative to mirror_path (no leading /)", p) + return fmt.Errorf("gdrive_sync.shared_excludes entry %q must be relative to the sync target (no leading /)", p) } for _, seg := range strings.Split(p, "/") { if seg == ".." { @@ -626,6 +663,11 @@ func readStateFile(path, noun string) (*UserState, int, error) { if version == 0 { applyLegacyKeys(&state, data) } + // Retired keys are the exception to that gate: #373 is about keys whose + // values are carried over, and these have no value to carry. The only + // question left is whether the file still sets one, which the save must + // answer out loud, stamped or not. + detectRetiredKeys(&state, data) rewriteRetiredValues(&state) return &state, version, nil } @@ -750,6 +792,15 @@ func saveStateAt(path string, state *UserState) error { fmt.Fprintf(os.Stderr, "note: migrated legacy state keys to their canonical form: %s\n", strings.Join(state.carriedLegacy, ", ")) } + // A retired key is the other half of the same notice: nothing replaces its + // value, so the note has to name the command that sets the replacement. + // Printed once and cleared, so a run that saves twice says it once. + if len(state.retiredKeys) > 0 { + for _, key := range state.retiredKeys { + fmt.Fprintf(os.Stderr, "note: dropped the retired `%s` key: %s\n", key, retiredStateKeys[key]) + } + state.retiredKeys = nil + } return nil } diff --git a/internal/config/state_retired_test.go b/internal/config/state_retired_test.go index 42fed8f9..79e420d7 100644 --- a/internal/config/state_retired_test.go +++ b/internal/config/state_retired_test.go @@ -65,3 +65,57 @@ func TestLoadState_StampedFileIgnoresStrayLegacyKey(t *testing.T) { t.Fatalf("stray legacy keys were overlaid: ai=%v warp=%v carried=%v", loaded.Modules.AI.Enabled, loaded.Modules.Warp, loaded.carriedLegacy) } } + +// #394 row 10: modules.gsync.mirror_path lost its last reader, so a file that +// still sets it must stop carrying it and say so once. The stamped version is +// the point: #373's gate governs keys whose values are carried over, and this +// one has no value left to carry, so the note has to fire for a canonical file +// too. +func TestLoadState_DropsTheRetiredMirrorPathKeyOnce(t *testing.T) { + path := filepath.Join(t.TempDir(), "config.yaml") + body := "schema_version: 4\nname: Test\nprofile: full\nmodules:\n gsync:\n mirror_path: /legacy/mirror\n" + if err := os.WriteFile(path, []byte(body), 0o644); err != nil { + t.Fatal(err) + } + loaded, err := loadStateAt(path) + if err != nil { + t.Fatalf("load: %v", err) + } + if len(loaded.retiredKeys) != 1 || loaded.retiredKeys[0] != "modules.gsync.mirror_path" { + t.Fatalf("retiredKeys = %v, want [modules.gsync.mirror_path]", loaded.retiredKeys) + } + + out := captureStderr(t, func() { + if err := saveStateAt(path, loaded); err != nil { + t.Fatalf("save: %v", err) + } + }) + if got := strings.Count(out, "note: dropped the retired `modules.gsync.mirror_path` key"); got != 1 { + t.Fatalf("want exactly one retired-key note, got %d in %q", got, out) + } + if !strings.Contains(out, "`dot sync target`") { + t.Errorf("the note does not name the command that sets the replacement: %q", out) + } + + data, _ := os.ReadFile(path) + if strings.Contains(string(data), "mirror_path") { + t.Fatalf("saved file still carries the retired key:\n%s", data) + } + + // One note per run: the save that announced the key clears the flag. + if again := captureStderr(t, func() { + if err := saveStateAt(path, loaded); err != nil { + t.Fatalf("second save: %v", err) + } + }); strings.Contains(again, "retired") { + t.Errorf("second save repeated the note: %q", again) + } + + reloaded, err := loadStateAt(path) + if err != nil { + t.Fatal(err) + } + if len(reloaded.retiredKeys) != 0 { + t.Errorf("rewritten file still reports retired keys: %v", reloaded.retiredKeys) + } +} diff --git a/internal/syncer/local_store.go b/internal/syncer/local_store.go index 2282dfe9..3a4caf7f 100644 --- a/internal/syncer/local_store.go +++ b/internal/syncer/local_store.go @@ -135,7 +135,8 @@ func (p PropagationPolicy) String() string { // settings. Persists to /config.yaml. type LocalConfig struct { // Target is the canonical destination spec ("local:~/Dropbox/work" or - // "ssh:user@host:path"). When empty, MirrorPath (legacy) applies. + // "ssh:user@host:path"). When empty, ResolveConfig uses the detected cloud + // default; it never consults a mirror path from anywhere else. Target string `yaml:"target,omitempty"` // Owner is the hostname allowed to push this profile. A mirror with two // pushers corrupts: both carry different baselines and delete propagation @@ -164,7 +165,6 @@ type LocalConfig struct { // the opposite: "either machine can continue the same work" includes the // uncommitted state inside submodules, which Git by definition has not seen. IncludeSubmodules bool `yaml:"include_submodules,omitempty"` - MirrorPath string `yaml:"mirror_path,omitempty"` FilterMode FilterMode `yaml:"filter_mode,omitempty"` Propagation PropagationPolicy `yaml:"propagation"` MaxDelete int `yaml:"max_delete,omitempty"` @@ -184,6 +184,12 @@ type LocalConfig struct { // ("~/" is the peer's home). Unset, the newest release found there wins; // set it to use a dev build (#176). RemoteDot string `yaml:"remote_dot,omitempty"` + // retiredKeys names the keys the file this config was read from still + // carried after they stopped being read, in load order. Unexported, so it + // can never be written to disk and needs no tag; SaveLocalConfig prints one + // note per retired key and then clears it, which keeps a store that is + // written twice in one run quiet the second time. + retiredKeys []string } // LocalState holds non-config runtime telemetry — the sticky timestamps @@ -471,6 +477,7 @@ func LoadLocalConfig(paths *LocalPaths) (*LocalConfig, bool, error) { if err := yaml.Unmarshal(data, &cfg); err != nil { return nil, false, fmt.Errorf("parsing %s: %w", paths.ConfigFile, err) } + cfg.retiredKeys = retiredLocalConfigKeysIn(data) if err := cfg.Propagation.Validate(); err != nil { // Fall back to defaults when on-disk policy is unusable rather than blocking. fmt.Fprintf(os.Stderr, "warning: %s has invalid propagation (%v); using defaults\n", paths.ConfigFile, err) @@ -518,7 +525,56 @@ func SaveLocalConfig(paths *LocalPaths, cfg *LocalConfig) error { if data, err = keepUnknownConfigKeys(paths.ConfigFile, data); err != nil { return err } - return atomicWrite(paths.ConfigFile, data) + if err := atomicWrite(paths.ConfigFile, data); err != nil { + return err + } + announceRetiredLocalConfigKeys(paths.ConfigFile, cfg) + return nil +} + +// announceRetiredLocalConfigKeys reports the keys this save dropped, once per +// loaded config: a key that no release reads any more disappears here, and +// saying so is the difference between a migration and a silent edit of the +// user's file. Kept at the write choke point so no read path can print it. +func announceRetiredLocalConfigKeys(path string, cfg *LocalConfig) { + if len(cfg.retiredKeys) == 0 { + return + } + fmt.Fprintf(os.Stderr, "note: dropped the retired `%s` key from %s; `dot sync` uses this store's `target` (set it with `dot sync target`)\n", + strings.Join(cfg.retiredKeys, "`, `"), path) + cfg.retiredKeys = nil +} + +// retiredLocalConfigKeys are the config keys this release no longer reads +// (#394 row 10 removed mirror_path in favor of target). A save drops them +// rather than carrying them the way keepUnknownConfigKeys carries a key +// LocalConfig does not own, and LoadLocalConfig records them so the drop is +// announced. +var retiredLocalConfigKeys = []string{"mirror_path"} + +func isRetiredLocalConfigKey(key string) bool { + for _, retired := range retiredLocalConfigKeys { + if key == retired { + return true + } + } + return false +} + +// retiredLocalConfigKeysIn returns the retired keys the given config bytes +// still set to a non-empty value. +func retiredLocalConfigKeysIn(data []byte) []string { + var probe map[string]any + if err := yaml.Unmarshal(data, &probe); err != nil { + return nil + } + var found []string + for _, key := range retiredLocalConfigKeys { + if value, ok := probe[key]; ok && value != nil && value != "" { + found = append(found, key) + } + } + return found } // keepUnknownConfigKeys carries over the top-level keys of the config on @@ -554,7 +610,7 @@ func keepUnknownConfigKeys(path string, data []byte) ([]byte, error) { if err != nil { return nil, fmt.Errorf("refusing to save %s: %w", path, err) } - if known[key.Value] || key.ShortTag() == "!!merge" { + if known[key.Value] || isRetiredLocalConfigKey(key.Value) || key.ShortTag() == "!!merge" { continue } value, err := resolveYAMLAliases(was.Content[0].Content[i+1]) @@ -671,7 +727,6 @@ func UpdateLocalState(paths *LocalPaths, mutate func(*LocalState)) error { func localConfigFromGlobal(globalState *config.UserState) *LocalConfig { gs := globalState.Modules.Gsync return &LocalConfig{ - MirrorPath: gs.MirrorPath, FilterMode: DefaultFilterMode(), Propagation: DefaultPropagationPolicy(), MaxDelete: gs.MaxDelete, @@ -686,8 +741,9 @@ func localConfigFromGlobal(globalState *config.UserState) *LocalConfig { // SeedFromGlobal writes /config.yaml for a workspace that has // none, from the machine-wide defaults in `state.modules.gdrive_sync` -// (mirror path, max delete, shared excludes), which `dot sync target` -// keeps for future workspaces. Returns the new LocalConfig. +// (max delete, shared excludes). The destination is not among them: a store +// starts with an empty `target`, so ResolveConfig uses the detected cloud +// default until `dot sync target` sets one. Returns the new LocalConfig. func SeedFromGlobal(globalState *config.UserState, paths *LocalPaths) (*LocalConfig, error) { cfg := localConfigFromGlobal(globalState) if err := EnsureLocalLayout(paths); err != nil { diff --git a/internal/syncer/local_store_test.go b/internal/syncer/local_store_test.go index eea212e5..5da8aa9f 100644 --- a/internal/syncer/local_store_test.go +++ b/internal/syncer/local_store_test.go @@ -1,6 +1,7 @@ package syncer import ( + "io" "os" "path/filepath" "strings" @@ -204,7 +205,7 @@ func TestLocalConfig_RoundTrip(t *testing.T) { paths := ResolveLocalPaths(tmp) original := &LocalConfig{ - MirrorPath: "/x/mirror", + Target: "local:/x/mirror", FilterMode: FilterModeExclude, Propagation: PropagationPolicy{Create: true, Update: true, Delete: true}, MaxDelete: 500, @@ -226,7 +227,7 @@ func TestLocalConfig_RoundTrip(t *testing.T) { if !ok { t.Fatal("LoadLocalConfig reported missing after save") } - if got.MirrorPath != original.MirrorPath || + if got.Target != original.Target || got.FilterMode != original.FilterMode || got.Propagation != original.Propagation || got.MaxDelete != original.MaxDelete || @@ -410,7 +411,6 @@ func TestSeedFromGlobal_PopulatesLocalLayout(t *testing.T) { paths := ResolveLocalPaths(tmp) state := &config.UserState{} - state.Modules.Gsync.MirrorPath = "/legacy/mirror" state.Modules.Gsync.MaxDelete = 250 state.Modules.Gsync.Interval = 600 state.Modules.Gsync.Paused = true @@ -421,9 +421,13 @@ func TestSeedFromGlobal_PopulatesLocalLayout(t *testing.T) { t.Fatalf("SeedFromGlobal: %v", err) } // Migrated fields: - if cfg.MirrorPath != "/legacy/mirror" || cfg.MaxDelete != 250 || cfg.Interval != 0 || !cfg.Paused { + if cfg.MaxDelete != 250 || cfg.Interval != 0 || !cfg.Paused { t.Errorf("migrated fields wrong: %+v", cfg) } + // The destination is not inherited: only the store's own target sets it. + if cfg.Target != "" { + t.Errorf("seeded Target = %q, want empty (destination is per-store)", cfg.Target) + } if cfg.FilterMode != FilterModeInclude { t.Errorf("migrated FilterMode = %q, want include", cfg.FilterMode) } @@ -451,14 +455,14 @@ func TestLoadOrSeedLocalConfig_MigrationRunsOnce(t *testing.T) { paths := ResolveLocalPaths(tmp) state := &config.UserState{} - state.Modules.Gsync.MirrorPath = "/legacy/mirror" + state.Modules.Gsync.MaxDelete = 777 first, err := LoadOrSeedLocalConfig(state, paths) if err != nil { t.Fatalf("first LoadOrSeedLocalConfig: %v", err) } // Mutate the disk-resident config so we can detect re-migration. - first.MirrorPath = "/edited/by/operator" + first.MaxDelete = 250 if err := SaveLocalConfig(paths, first); err != nil { t.Fatalf("SaveLocalConfig: %v", err) } @@ -468,9 +472,87 @@ func TestLoadOrSeedLocalConfig_MigrationRunsOnce(t *testing.T) { if err != nil { t.Fatalf("second LoadOrSeedLocalConfig: %v", err) } - if second.MirrorPath != "/edited/by/operator" { - t.Errorf("re-migration overwrote local edits: MirrorPath = %q", second.MirrorPath) + if second.MaxDelete != 250 { + t.Errorf("re-migration overwrote local edits: MaxDelete = %d, want 250", second.MaxDelete) + } +} + +// #394 row 10: a store that still carries the retired mirror_path key loads, +// drops it on the next save (keepUnknownConfigKeys must not carry it back a +// third time), and says so once. A second save in the same run stays quiet. +func TestSaveLocalConfig_DropsTheRetiredMirrorPathKeyOnce(t *testing.T) { + tmp := t.TempDir() + paths := ResolveLocalPaths(tmp) + if err := os.MkdirAll(paths.StoreDir, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(paths.ConfigFile, []byte("target: local:/dropbox/work\nmirror_path: /dropbox/work\n"), 0o644); err != nil { + t.Fatal(err) + } + + cfg, ok, err := LoadLocalConfig(paths) + if err != nil || !ok { + t.Fatalf("LoadLocalConfig: ok=%v err=%v", ok, err) + } + if cfg.Target != "local:/dropbox/work" { + t.Fatalf("Target = %q, want the canonical value", cfg.Target) + } + + out := captureSyncerStderr(t, func() { + if err := SaveLocalConfig(paths, cfg); err != nil { + t.Fatalf("SaveLocalConfig: %v", err) + } + }) + if !strings.Contains(out, "note: dropped the retired `mirror_path` key") || !strings.Contains(out, "`dot sync target`") { + t.Errorf("save did not announce the drop once:\n%s", out) + } + saved, err := os.ReadFile(paths.ConfigFile) + if err != nil { + t.Fatal(err) + } + if strings.Contains(string(saved), "mirror_path") { + t.Errorf("saved config still carries the retired key:\n%s", saved) + } + if !strings.Contains(string(saved), "target: local:/dropbox/work") { + t.Errorf("saved config lost the target:\n%s", saved) } + + again, ok, err := LoadLocalConfig(paths) + if err != nil || !ok { + t.Fatalf("second LoadLocalConfig: ok=%v err=%v", ok, err) + } + if quiet := captureSyncerStderr(t, func() { + if err := SaveLocalConfig(paths, again); err != nil { + t.Fatalf("second SaveLocalConfig: %v", err) + } + }); quiet != "" { + t.Errorf("second save spoke again: %q", quiet) + } +} + +// captureSyncerStderr runs fn with os.Stderr redirected to a pipe and returns +// what was written. The syncer's notes go straight to os.Stderr and take no +// injectable writer, so the pipe is the only seam. +func captureSyncerStderr(t *testing.T, fn func()) string { + t.Helper() + r, w, err := os.Pipe() + if err != nil { + t.Fatalf("os.Pipe: %v", err) + } + orig := os.Stderr + os.Stderr = w + done := make(chan string, 1) + go func() { + var buf strings.Builder + _, _ = io.Copy(&buf, r) + done <- buf.String() + }() + fn() + os.Stderr = orig + _ = w.Close() + out := <-done + _ = r.Close() + return out } func TestLocalState_RoundTrip(t *testing.T) { @@ -635,7 +717,7 @@ func TestSaveLocalConfigRefusesRunawayAliases(t *testing.T) { // are exactly what it marshals (an untagged field would be carried twice). func TestLocalConfigKeysMatchWhatItMarshals(t *testing.T) { full := LocalConfig{Target: "t", Owner: "o", OwnerAliases: []string{"a"}, OwnerEpoch: 1, FencePending: true, IncludeSubmodules: true, - MirrorPath: "m", FilterMode: DefaultFilterMode(), MaxDelete: 1, Interval: 1, PullInterval: 1, PushMode: "clean", PullMode: "clean", + FilterMode: DefaultFilterMode(), MaxDelete: 1, Interval: 1, PullInterval: 1, PushMode: "clean", PullMode: "clean", Paused: true, SharedExcludes: []string{"x"}, Hooks: PeerHooks{OnActivate: []string{"a"}}, RemoteDot: "d", HostMerge: map[string][]string{"f": {"k"}}} data, err := yaml.Marshal(&full) if err != nil { diff --git a/internal/syncer/sync.go b/internal/syncer/sync.go index 857e6995..6384a2a4 100644 --- a/internal/syncer/sync.go +++ b/internal/syncer/sync.go @@ -282,8 +282,9 @@ func resolveConfig(state *config.UserState, migrate bool, home, profile string) } } - // Resolve the destination: the canonical `target` spec wins, then the - // legacy mirror_path (local + global), then the detected cloud default. + // Resolve the destination: the store's `target` spec decides it, else the + // detected cloud default. The retired `mirror_path` key is not read at all + // (#394 row 10); a file that still carries one is only noted at save. var target Target mirrorIsDefault := false if spec := strings.TrimSpace(localCfg.Target); spec != "" { @@ -293,15 +294,8 @@ func resolveConfig(state *config.UserState, migrate bool, home, profile string) } target = t } else { - mirrorPath := localCfg.MirrorPath - if mirrorPath == "" { - mirrorPath = gs.MirrorPath - } - mirrorIsDefault = mirrorPath == "" - if mirrorIsDefault { - mirrorPath = defaultMirrorPath(home) - } - target = Target{Kind: TargetLocal, Path: mirrorPath} + mirrorIsDefault = true + target = Target{Kind: TargetLocal, Path: defaultMirrorPath(home)} } mirrorPath := "" if target.Kind == TargetLocal { @@ -639,7 +633,7 @@ func refuseSharedDriveMirror(cfg *Config) error { return fmt.Errorf( "refusing to sync: mirror %q resolves under a Drive 'Shared drives' root.\n"+ "Workspace-authoritative semantics would propagate deletions into a team drive.\n"+ - "Point gsync.mirror_path at a folder under My Drive instead", + "Set the sync target to a folder under My Drive instead (`dot sync target`)", cfg.MirrorPath, ) } diff --git a/internal/syncer/sync_test.go b/internal/syncer/sync_test.go index a1f4d086..4fc0c17c 100644 --- a/internal/syncer/sync_test.go +++ b/internal/syncer/sync_test.go @@ -57,6 +57,20 @@ func newIsolatedState(t *testing.T) *config.UserState { return state } +// writeStoreConfig puts body at /.dotfiles/sync/config.yaml, the +// store ResolveConfig reads. A read-only resolve never creates one, so a test +// that wants a store must write it itself. +func writeStoreConfig(t *testing.T, localPath, body string) { + t.Helper() + dir := filepath.Join(localPath, ".dotfiles", "sync") + if err := os.MkdirAll(dir, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(dir, "config.yaml"), []byte(body), 0o644); err != nil { + t.Fatal(err) + } +} + func TestPullArgs_WorkspaceAuthoritative(t *testing.T) { cfg := newTestConfig(t) conflict := &ConflictDir{Timestamp: "2026-05-01T12-00-00Z"} @@ -353,7 +367,6 @@ func TestResolveConfig_StateOverrides(t *testing.T) { tmp := t.TempDir() state := &config.UserState{} state.Modules.Gsync.LocalPath = tmp - state.Modules.Gsync.MirrorPath = "/custom/mirror" state.Modules.Gsync.MaxDelete = 50 cfg, err := ResolveConfig(state) @@ -363,9 +376,6 @@ func TestResolveConfig_StateOverrides(t *testing.T) { if cfg.LocalPath != tmp+"/" { t.Errorf("LocalPath = %q, want %q", cfg.LocalPath, tmp+"/") } - if cfg.MirrorPath != "/custom/mirror/" { - t.Errorf("MirrorPath = %q, want trailing-slashed /custom/mirror/", cfg.MirrorPath) - } if cfg.MaxDelete != 50 { t.Errorf("MaxDelete = %d, want 50", cfg.MaxDelete) } @@ -413,7 +423,7 @@ func TestResolveConfig_PrefersCloudMirror(t *testing.T) { } } -func TestResolveConfig_LocalMirrorWinsOverCloud(t *testing.T) { +func TestResolveConfig_LocalTargetWinsOverCloud(t *testing.T) { home := t.TempDir() t.Setenv("HOME", home) if err := os.MkdirAll(filepath.Join(home, "Library", "CloudStorage", "Dropbox", "secrets"), 0o755); err != nil { @@ -421,7 +431,7 @@ func TestResolveConfig_LocalMirrorWinsOverCloud(t *testing.T) { } state := &config.UserState{} state.Modules.Gsync.LocalPath = t.TempDir() - state.Modules.Gsync.MirrorPath = "/explicit/mirror" + writeStoreConfig(t, state.Modules.Gsync.LocalPath, "target: local:/explicit/mirror\n") cfg, err := ResolveConfig(state) if err != nil { @@ -432,6 +442,23 @@ func TestResolveConfig_LocalMirrorWinsOverCloud(t *testing.T) { } } +// #394 row 10: the retired mirror_path key decides nothing any more. A store +// that still carries one and no target resolves the detected default, the same +// as a store with an empty target. +func TestResolveConfig_IgnoresTheRetiredMirrorPathKey(t *testing.T) { + state := newIsolatedState(t) + writeStoreConfig(t, state.Modules.Gsync.LocalPath, "mirror_path: /legacy/mirror\n") + + cfg, err := ResolveConfigReadOnly(state) + if err != nil { + t.Fatalf("ResolveConfigReadOnly: %v", err) + } + want := filepath.Join(os.Getenv("HOME"), "gdrive-workspace", "work") + "/" + if cfg.MirrorPath != want { + t.Errorf("MirrorPath = %q, want the detected default %q (mirror_path must not be read)", cfg.MirrorPath, want) + } +} + func TestResolveConfigReadOnlyForHome_UsesGivenHome(t *testing.T) { // os.UserHomeDir() would resolve here — the --home override must not use it. currentHome := t.TempDir() @@ -440,7 +467,7 @@ func TestResolveConfigReadOnlyForHome_UsesGivenHome(t *testing.T) { overrideHome := t.TempDir() state := &config.UserState{} state.Modules.Gsync.LocalPath = t.TempDir() - // MirrorPath empty → default is /gdrive-workspace/work. + // No target in the store → default is /gdrive-workspace/work. cfg, err := ResolveConfigReadOnlyForHomeProfile(state, overrideHome, DefaultProfile) if err != nil { @@ -462,7 +489,7 @@ func TestResolveConfigReadOnlyForHome_ExpandsTildeAgainstHome(t *testing.T) { state := &config.UserState{} state.Modules.Gsync.LocalPath = t.TempDir() - state.Modules.Gsync.MirrorPath = "~/cloud/work" + writeStoreConfig(t, state.Modules.Gsync.LocalPath, "target: local:~/cloud/work\n") cfg, err := ResolveConfigReadOnlyForHomeProfile(state, overrideHome, DefaultProfile) if err != nil { diff --git a/internal/syncer/target.go b/internal/syncer/target.go index 9825e645..34bc4b1c 100644 --- a/internal/syncer/target.go +++ b/internal/syncer/target.go @@ -57,7 +57,8 @@ func ParseTarget(spec string) (Target, error) { } return Target{Kind: TargetSSH, Host: host, Path: path}, nil default: - // Bare paths keep legacy mirror_path values working. + // A bare path is the shorthand `dot sync target ~/Dropbox/work` takes; + // it is not a read of any retired config key. if err := validateTargetField("local path", spec, true); err != nil { return Target{}, err } From 57ccca3848c66100df8178ed0a3752afa995aff6 Mon Sep 17 00:00:00 2001 From: Young Joon Lee Date: Mon, 9 Nov 2026 22:26:20 +0900 Subject: [PATCH 2/3] refuse a retired destination that would move silently A store with no `target` resolves the detected cloud default, so a machine that still names another folder in `mirror_path` would start pushing (create+update+delete) and pulling against a tree nobody chose, with only a save-time note to say so later. The transfer entries now compare the retired value with the destination this config resolved, following symlinks on both sides, and stop when they differ: the error names the stored folder and `dot sync target `. A same-directory match (the peer Mac's ~/Dropbox is a symlink into ~/Library/CloudStorage) proceeds untouched. Resolution deliberately does not refuse: `dot sync target`, the fix the error names, resolves a config of its own and would otherwise fail too. Part of #394. --- internal/config/state.go | 17 +++ internal/config/state_retired_test.go | 8 ++ internal/syncer/intake.go | 3 + internal/syncer/local_store.go | 48 ++++--- internal/syncer/pull.go | 3 + internal/syncer/push_plan.go | 3 + internal/syncer/sync.go | 172 ++++++++++++++++++-------- internal/syncer/sync_test.go | 132 ++++++++++++++++++++ 8 files changed, 321 insertions(+), 65 deletions(-) diff --git a/internal/config/state.go b/internal/config/state.go index 820c34d8..7c4067f6 100644 --- a/internal/config/state.go +++ b/internal/config/state.go @@ -205,9 +205,21 @@ func detectRetiredKeys(state *UserState, raw []byte) { } if legacy.Modules.Gsync.MirrorPath != "" { state.retiredKeys = append(state.retiredKeys, "modules.gsync.mirror_path") + state.retiredMirrorPath = legacy.Modules.Gsync.MirrorPath } } +// RetiredMirrorPath returns the value this state file still sets for the +// retired `modules.gsync.mirror_path` key, or "". The sync engine reads it only +// to refuse a silent destination change on a store that never set `target`: the +// key does not route anything any more (#394 row 10). +func (s *UserState) RetiredMirrorPath() string { + if s == nil { + return "" + } + return s.retiredMirrorPath +} + // warnForwardSchema reports that a state file was written by a newer dot. It // does not fail the load: an additively-forward file still decodes, and an // incompatibly-forward one gets this warning before its decode error so the @@ -247,6 +259,11 @@ type UserState struct { // the same reason as carriedLegacy: the save that drops them is the only // place that may speak of them, and it prints one note, then clears this. retiredKeys []string + + // retiredMirrorPath is the value `modules.gsync.mirror_path` held in that + // same file, kept for the one reader that refuses a destination change + // (RetiredMirrorPath). + retiredMirrorPath string } // UserModulesState holds module opt-in/config from user state. diff --git a/internal/config/state_retired_test.go b/internal/config/state_retired_test.go index 79e420d7..3b666efe 100644 --- a/internal/config/state_retired_test.go +++ b/internal/config/state_retired_test.go @@ -84,6 +84,11 @@ func TestLoadState_DropsTheRetiredMirrorPathKeyOnce(t *testing.T) { if len(loaded.retiredKeys) != 1 || loaded.retiredKeys[0] != "modules.gsync.mirror_path" { t.Fatalf("retiredKeys = %v, want [modules.gsync.mirror_path]", loaded.retiredKeys) } + // The sync engine refuses a silent destination change with this value, so + // the loader has to hand the value on, not only the key's name. + if got := loaded.RetiredMirrorPath(); got != "/legacy/mirror" { + t.Errorf("RetiredMirrorPath = %q, want /legacy/mirror", got) + } out := captureStderr(t, func() { if err := saveStateAt(path, loaded); err != nil { @@ -118,4 +123,7 @@ func TestLoadState_DropsTheRetiredMirrorPathKeyOnce(t *testing.T) { if len(reloaded.retiredKeys) != 0 { t.Errorf("rewritten file still reports retired keys: %v", reloaded.retiredKeys) } + if got := reloaded.RetiredMirrorPath(); got != "" { + t.Errorf("rewritten file still reports a retired value: %q", got) + } } diff --git a/internal/syncer/intake.go b/internal/syncer/intake.go index cc1bb49d..da5ba584 100644 --- a/internal/syncer/intake.go +++ b/internal/syncer/intake.go @@ -70,6 +70,9 @@ func Intake(ctx context.Context, runner *exec.Runner, cfg *Config, opts IntakeOp if err := refuseSharedDriveMirror(cfg); err != nil { return nil, err } + if err := refuseRetiredDestinationChange(cfg); err != nil { + return nil, err + } paths := cfg.LocalPaths mirror := strings.TrimRight(cfg.MirrorPath, "/") diff --git a/internal/syncer/local_store.go b/internal/syncer/local_store.go index 3a4caf7f..6820f208 100644 --- a/internal/syncer/local_store.go +++ b/internal/syncer/local_store.go @@ -184,12 +184,14 @@ type LocalConfig struct { // ("~/" is the peer's home). Unset, the newest release found there wins; // set it to use a dev build (#176). RemoteDot string `yaml:"remote_dot,omitempty"` - // retiredKeys names the keys the file this config was read from still - // carried after they stopped being read, in load order. Unexported, so it - // can never be written to disk and needs no tag; SaveLocalConfig prints one - // note per retired key and then clears it, which keeps a store that is - // written twice in one run quiet the second time. - retiredKeys []string + // retired holds the keys the file this config was read from still set after + // they stopped being read, with the values they held. Unexported, so it can + // never be written to disk and needs no tag; SaveLocalConfig prints one note + // per retired key and then clears it, which keeps a store that is written + // twice in one run quiet the second time. The values are not dead weight: + // a store with no `target` is guarded by the `mirror_path` it still names + // (refuseRetiredDestinationChange). + retired map[string]string } // LocalState holds non-config runtime telemetry — the sticky timestamps @@ -477,7 +479,7 @@ func LoadLocalConfig(paths *LocalPaths) (*LocalConfig, bool, error) { if err := yaml.Unmarshal(data, &cfg); err != nil { return nil, false, fmt.Errorf("parsing %s: %w", paths.ConfigFile, err) } - cfg.retiredKeys = retiredLocalConfigKeysIn(data) + cfg.retired = retiredLocalConfigValues(data) if err := cfg.Propagation.Validate(); err != nil { // Fall back to defaults when on-disk policy is unusable rather than blocking. fmt.Fprintf(os.Stderr, "warning: %s has invalid propagation (%v); using defaults\n", paths.ConfigFile, err) @@ -537,20 +539,31 @@ func SaveLocalConfig(paths *LocalPaths, cfg *LocalConfig) error { // saying so is the difference between a migration and a silent edit of the // user's file. Kept at the write choke point so no read path can print it. func announceRetiredLocalConfigKeys(path string, cfg *LocalConfig) { - if len(cfg.retiredKeys) == 0 { + if len(cfg.retired) == 0 { return } + keys := make([]string, 0, len(cfg.retired)) + for _, key := range retiredLocalConfigKeys { + if _, ok := cfg.retired[key]; ok { + keys = append(keys, key) + } + } fmt.Fprintf(os.Stderr, "note: dropped the retired `%s` key from %s; `dot sync` uses this store's `target` (set it with `dot sync target`)\n", - strings.Join(cfg.retiredKeys, "`, `"), path) - cfg.retiredKeys = nil + strings.Join(keys, "`, `"), path) + cfg.retired = nil } +// retiredMirrorPathKey is the one retired key whose value the sync engine still +// looks at, and only to refuse: a store that sets it and no `target` would +// otherwise move its destination to the detected default without being asked. +const retiredMirrorPathKey = "mirror_path" + // retiredLocalConfigKeys are the config keys this release no longer reads // (#394 row 10 removed mirror_path in favor of target). A save drops them // rather than carrying them the way keepUnknownConfigKeys carries a key // LocalConfig does not own, and LoadLocalConfig records them so the drop is // announced. -var retiredLocalConfigKeys = []string{"mirror_path"} +var retiredLocalConfigKeys = []string{retiredMirrorPathKey} func isRetiredLocalConfigKey(key string) bool { for _, retired := range retiredLocalConfigKeys { @@ -561,17 +574,20 @@ func isRetiredLocalConfigKey(key string) bool { return false } -// retiredLocalConfigKeysIn returns the retired keys the given config bytes -// still set to a non-empty value. -func retiredLocalConfigKeysIn(data []byte) []string { +// retiredLocalConfigValues returns the retired keys the given config bytes +// still set, with the values they hold. +func retiredLocalConfigValues(data []byte) map[string]string { var probe map[string]any if err := yaml.Unmarshal(data, &probe); err != nil { return nil } - var found []string + var found map[string]string for _, key := range retiredLocalConfigKeys { if value, ok := probe[key]; ok && value != nil && value != "" { - found = append(found, key) + if found == nil { + found = map[string]string{} + } + found[key] = fmt.Sprint(value) } } return found diff --git a/internal/syncer/pull.go b/internal/syncer/pull.go index d8e55f77..285567c9 100644 --- a/internal/syncer/pull.go +++ b/internal/syncer/pull.go @@ -62,6 +62,9 @@ func PullTracked(cfg *Config, opts PullOptions) (*PullResult, error) { if err := refuseSharedDriveMirror(cfg); err != nil { return nil, err } + if err := refuseRetiredDestinationChange(cfg); err != nil { + return nil, err + } paths := cfg.LocalPaths local := strings.TrimRight(cfg.LocalPath, "/") diff --git a/internal/syncer/push_plan.go b/internal/syncer/push_plan.go index 7be75c4b..dd996ee4 100644 --- a/internal/syncer/push_plan.go +++ b/internal/syncer/push_plan.go @@ -100,6 +100,9 @@ func PlanPush(cfg *Config) (*PushPlan, error) { if err := refuseSharedDriveMirror(cfg); err != nil { return nil, err } + if err := refuseRetiredDestinationChange(cfg); err != nil { + return nil, err + } local := strings.TrimRight(cfg.LocalPath, "/") mirror := strings.TrimRight(cfg.MirrorPath, "/") diff --git a/internal/syncer/sync.go b/internal/syncer/sync.go index 6384a2a4..72a2e251 100644 --- a/internal/syncer/sync.go +++ b/internal/syncer/sync.go @@ -52,18 +52,26 @@ type Config struct { MirrorPath string // local mirror tree with trailing slash; empty for ssh targets Target Target // parsed destination (local dir or ssh host:path) MirrorIsDefault bool // target came from defaultMirrorPath, not explicit config - FilterMode FilterMode - IncludeFile string // editable include list (under .dotfiles/sync/) - IncludePatterns []string // parsed include list used by Go filters + rsync args - ExcludesFile string // materialized static exclude list (under .dotfiles/sync/) - IgnoreFile string // user-supplied ignore patterns (under .dotfiles/sync/) - AllowFile string // explicit secrets opt-in patterns (under .dotfiles/sync/) - AllowPatterns []string // parsed allow.txt — re-included ahead of the secrets layer - ConfigDir string // workspace-local store dir (.dotfiles/sync/) — dynamic files land here - SharedExcludes []string // operator-curated shared paths (relative to MirrorPath) - LogFile string - LockDir string // copied once from SystemPaths.LockDir at the single resolution point - RsyncPath string // resolved rsync binary; empty if not installed + // retiredMirrorPath is the folder a file this config was resolved from still + // names in the retired `mirror_path` key, expanded and absolute, when the + // store set no target and the destination fell back to the detected default. + // retiredMirrorSource says which file named it. The transfer entries read + // these only to refuse a destination that would move silently + // (refuseRetiredDestinationChange); they never route anything. + retiredMirrorPath string + retiredMirrorSource string + FilterMode FilterMode + IncludeFile string // editable include list (under .dotfiles/sync/) + IncludePatterns []string // parsed include list used by Go filters + rsync args + ExcludesFile string // materialized static exclude list (under .dotfiles/sync/) + IgnoreFile string // user-supplied ignore patterns (under .dotfiles/sync/) + AllowFile string // explicit secrets opt-in patterns (under .dotfiles/sync/) + AllowPatterns []string // parsed allow.txt — re-included ahead of the secrets layer + ConfigDir string // workspace-local store dir (.dotfiles/sync/) — dynamic files land here + SharedExcludes []string // operator-curated shared paths (relative to MirrorPath) + LogFile string + LockDir string // copied once from SystemPaths.LockDir at the single resolution point + RsyncPath string // resolved rsync binary; empty if not installed // RemoteRsyncPath is passed as --rsync-path for ssh targets whose default // rsync cannot serve a modern client (macOS 26 openrsync). Empty = default OK. RemoteRsyncPath string @@ -283,10 +291,15 @@ func resolveConfig(state *config.UserState, migrate bool, home, profile string) } // Resolve the destination: the store's `target` spec decides it, else the - // detected cloud default. The retired `mirror_path` key is not read at all - // (#394 row 10); a file that still carries one is only noted at save. + // detected cloud default. The retired `mirror_path` key is never a route + // (#394 row 10); it is still read here when the store sets no target, + // because a machine that names another folder in it would otherwise change + // destination without being asked. The transfer entries refuse on that + // (refuseRetiredDestinationChange); this resolve does not, so the + // `dot sync target` that fixes it can still run. var target Target mirrorIsDefault := false + retiredMirrorPath, retiredMirrorSource := "", "" if spec := strings.TrimSpace(localCfg.Target); spec != "" { t, err := ParseTarget(spec) if err != nil { @@ -296,6 +309,11 @@ func resolveConfig(state *config.UserState, migrate bool, home, profile string) } else { mirrorIsDefault = true target = Target{Kind: TargetLocal, Path: defaultMirrorPath(home)} + if legacy := strings.TrimSpace(localCfg.retired[retiredMirrorPathKey]); legacy != "" { + retiredMirrorPath, retiredMirrorSource = expandHome(legacy, home), "this store's config" + } else if legacy := strings.TrimSpace(state.RetiredMirrorPath()); legacy != "" { + retiredMirrorPath, retiredMirrorSource = expandHome(legacy, home), "the global state" + } } mirrorPath := "" if target.Kind == TargetLocal { @@ -335,41 +353,43 @@ func resolveConfig(state *config.UserState, migrate bool, home, profile string) } return &Config{ - Profile: profile, - Home: override, - Owner: localCfg.Owner, - HostMerge: localCfg.HostMerge, - OwnerAliases: localCfg.OwnerAliases, - Hooks: localCfg.Hooks, - RemoteDot: localCfg.RemoteDot, - OwnerEpoch: localCfg.OwnerEpoch, - FencePending: localCfg.FencePending, - IncludeSubmodules: localCfg.IncludeSubmodules, - LocalPath: localPath, - MirrorPath: mirrorPath, - Target: target, - MirrorIsDefault: mirrorIsDefault, - FilterMode: filterMode, - IncludeFile: localPaths.IncludeFile, - IncludePatterns: includePatterns, - ExcludesFile: localPaths.ExcludeFile, - IgnoreFile: localPaths.IgnoreFile, - AllowFile: localPaths.AllowFile, - AllowPatterns: allowPatterns, - ConfigDir: localPaths.StoreDir, - SharedExcludes: append([]string(nil), localCfg.SharedExcludes...), - LogFile: localPaths.LogFile, - LockDir: systemPaths.LockDir, - RsyncPath: rsyncPath, - MaxDelete: maxDelete, - Interval: schedule.Interval, - PullInterval: schedule.PullInterval, - PushMode: schedule.PushMode, - PullMode: schedule.PullMode, - Propagation: policy, - Paused: localCfg.Paused, - LocalPaths: localPaths, - SystemPaths: systemPaths, + Profile: profile, + Home: override, + Owner: localCfg.Owner, + HostMerge: localCfg.HostMerge, + OwnerAliases: localCfg.OwnerAliases, + Hooks: localCfg.Hooks, + RemoteDot: localCfg.RemoteDot, + OwnerEpoch: localCfg.OwnerEpoch, + FencePending: localCfg.FencePending, + IncludeSubmodules: localCfg.IncludeSubmodules, + LocalPath: localPath, + MirrorPath: mirrorPath, + Target: target, + MirrorIsDefault: mirrorIsDefault, + retiredMirrorPath: retiredMirrorPath, + retiredMirrorSource: retiredMirrorSource, + FilterMode: filterMode, + IncludeFile: localPaths.IncludeFile, + IncludePatterns: includePatterns, + ExcludesFile: localPaths.ExcludeFile, + IgnoreFile: localPaths.IgnoreFile, + AllowFile: localPaths.AllowFile, + AllowPatterns: allowPatterns, + ConfigDir: localPaths.StoreDir, + SharedExcludes: append([]string(nil), localCfg.SharedExcludes...), + LogFile: localPaths.LogFile, + LockDir: systemPaths.LockDir, + RsyncPath: rsyncPath, + MaxDelete: maxDelete, + Interval: schedule.Interval, + PullInterval: schedule.PullInterval, + PushMode: schedule.PushMode, + PullMode: schedule.PullMode, + Propagation: policy, + Paused: localCfg.Paused, + LocalPaths: localPaths, + SystemPaths: systemPaths, }, nil } @@ -640,6 +660,54 @@ func refuseSharedDriveMirror(cfg *Config) error { return nil } +// refuseRetiredDestinationChange stops a run whose destination would move +// without being asked. A store with no `target` resolves the detected cloud +// default, but the same machine may still name another folder in the retired +// `mirror_path` key, which chose the destination until #394 row 10. Push and +// pull both propagate, so going ahead could delete or overwrite in a tree the +// user never picked, and the save-time note arrives too late to say so. +// +// Reading the key here is a refusal, not a route: the same directory (through +// symlinks too) proceeds with the default this config already resolved, and +// anything else stops before a transfer with the command that settles it. It +// is deliberately not part of resolution: `dot sync target`, the fix the error +// names, resolves a config of its own and would fail with everything else. +func refuseRetiredDestinationChange(cfg *Config) error { + if cfg == nil || cfg.retiredMirrorPath == "" || !cfg.MirrorIsDefault { + return nil + } + if sameDirectory(cfg.retiredMirrorPath, cfg.MirrorPath) { + return nil + } + stored := strings.TrimRight(cfg.retiredMirrorPath, "/") + detected := strings.TrimRight(cfg.MirrorPath, "/") + return fmt.Errorf( + "refusing to sync: %s still sets the retired `mirror_path` to %q and this store has no `target`.\n"+ + "Sync would use %q instead, and push and pull both propagate changes.\n"+ + "Keep the stored folder with `dot sync target %s`, or accept the new one with `dot sync target %s`", + cfg.retiredMirrorSource, stored, detected, stored, detected, + ) +} + +// sameDirectory reports whether two paths name the same directory, following +// symlinks on both sides: the peer Mac's ~/Dropbox is a symlink into +// ~/Library/CloudStorage, so a textual compare would split one tree in two. A +// path that does not resolve (it may not exist yet) makes the comparison +// textual, which leans towards refusing rather than guessing. +func sameDirectory(a, b string) bool { + a, b = strings.TrimSpace(a), strings.TrimSpace(b) + if a == "" || b == "" { + return false + } + ca, cb := filepath.Clean(a), filepath.Clean(b) + ra, errA := filepath.EvalSymlinks(ca) + rb, errB := filepath.EvalSymlinks(cb) + if errA == nil && errB == nil { + return ra == rb + } + return ca == cb +} + // ── pull / push / sync ────────────────────────────────────────────────── // Push local → mirror under cfg.Propagation. The policy maps to rsync flags @@ -679,6 +747,9 @@ func Push(ctx context.Context, runner *exec.Runner, cfg *Config, dryRun bool) er if err := refuseSharedDriveMirror(cfg); err != nil { return err } + if err := refuseRetiredDestinationChange(cfg); err != nil { + return err + } rf, err := prepareRuntimeFilters(cfg, dryRun) defer rf.Cleanup() if err != nil { @@ -958,6 +1029,9 @@ func fetchScopeArgs(entries []fetchEntry) ([]string, error) { // files are never deleted; remote-newer files overwrite local with a backup // under .sync-conflicts/. func PullDirect(ctx context.Context, runner *exec.Runner, cfg *Config, dryRun bool) error { + if err := refuseRetiredDestinationChange(cfg); err != nil { + return err + } if !dryRun { if err := ensureLogDir(cfg.LogFile); err != nil { return err diff --git a/internal/syncer/sync_test.go b/internal/syncer/sync_test.go index 4fc0c17c..fa9f5154 100644 --- a/internal/syncer/sync_test.go +++ b/internal/syncer/sync_test.go @@ -459,6 +459,138 @@ func TestResolveConfig_IgnoresTheRetiredMirrorPathKey(t *testing.T) { } } +// #394 row 10: a store with no `target` resolves the detected default, but the +// folder it still names in `mirror_path` was the destination until this release. +// A different folder has to stop a transfer instead of moving it silently: push +// and pull propagate, so a run could delete in a tree nobody picked. +func TestRefuseRetiredDestinationChange_RefusesAnotherFolder(t *testing.T) { + state := newIsolatedState(t) + writeStoreConfig(t, state.Modules.Gsync.LocalPath, "mirror_path: /legacy/mirror\n") + + // Resolution itself must not refuse: `dot sync target`, the fix the error + // names, resolves a config of its own before it can write the store. + cfg, err := ResolveConfigReadOnly(state) + if err != nil { + t.Fatalf("ResolveConfigReadOnly: %v", err) + } + if cfg.retiredMirrorSource != "this store's config" { + t.Errorf("retiredMirrorSource = %q, want the store", cfg.retiredMirrorSource) + } + + err = refuseRetiredDestinationChange(cfg) + if err == nil { + t.Fatal("a store naming another folder in mirror_path must refuse a transfer") + } + for _, want := range []string{ + "/legacy/mirror", + "dot sync target /legacy/mirror", + strings.TrimRight(cfg.MirrorPath, "/"), + } { + if !strings.Contains(err.Error(), want) { + t.Errorf("refusal does not name %q:\n%v", want, err) + } + } +} + +// The same folder spelled through a symlink is not a destination change: the +// peer Mac's ~/Dropbox is a symlink into ~/Library/CloudStorage/Dropbox. +func TestRefuseRetiredDestinationChange_AllowsALinkedSameFolder(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + cloud := filepath.Join(home, "Library", "CloudStorage", "Dropbox") + for _, dir := range []string{"work", "secrets"} { + if err := os.MkdirAll(filepath.Join(cloud, dir), 0o755); err != nil { + t.Fatal(err) + } + } + if err := os.Symlink(cloud, filepath.Join(home, "Dropbox")); err != nil { + t.Fatal(err) + } + state := &config.UserState{} + state.Modules.Gsync.LocalPath = t.TempDir() + writeStoreConfig(t, state.Modules.Gsync.LocalPath, + "mirror_path: "+filepath.Join(home, "Dropbox", "work")+"\n") + + cfg, err := ResolveConfigReadOnly(state) + if err != nil { + t.Fatalf("ResolveConfigReadOnly: %v", err) + } + want := filepath.Join(cloud, "work") + "/" + if cfg.MirrorPath != want { + t.Fatalf("MirrorPath = %q, want the cloud default %q", cfg.MirrorPath, want) + } + if err := refuseRetiredDestinationChange(cfg); err != nil { + t.Errorf("the same tree through a symlink must sync, got: %v", err) + } +} + +// The global state is the other place the key can still live, and a new +// workspace store is seeded from it. +func TestRefuseRetiredDestinationChange_ReadsTheGlobalState(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + localPath := t.TempDir() + statePath := filepath.Join(t.TempDir(), "config.yaml") + body := "schema_version: 4\nname: Test\nprofile: full\nmodules:\n gsync:\n local_path: " + + localPath + "\n mirror_path: /legacy/mirror\n" + if err := os.WriteFile(statePath, []byte(body), 0o644); err != nil { + t.Fatal(err) + } + state, err := config.LoadStateFrom(statePath) + if err != nil { + t.Fatalf("LoadStateFrom: %v", err) + } + + cfg, err := ResolveConfigReadOnly(state) + if err != nil { + t.Fatalf("ResolveConfigReadOnly: %v", err) + } + if cfg.retiredMirrorSource != "the global state" { + t.Errorf("retiredMirrorSource = %q, want the global state", cfg.retiredMirrorSource) + } + err = refuseRetiredDestinationChange(cfg) + if err == nil { + t.Fatal("an empty target with a global mirror_path must refuse a transfer") + } + if !strings.Contains(err.Error(), "the global state") || !strings.Contains(err.Error(), "/legacy/mirror") { + t.Errorf("refusal does not name the source and the stored value:\n%v", err) + } +} + +// The refusals hang off the transfer entries, not off resolution: a push plan +// built from such a store stops before it walks either tree. +func TestPlanPush_RefusesARetiredDestination(t *testing.T) { + state := newIsolatedState(t) + writeStoreConfig(t, state.Modules.Gsync.LocalPath, "mirror_path: /legacy/mirror\n") + cfg, err := ResolveConfigReadOnly(state) + if err != nil { + t.Fatalf("ResolveConfigReadOnly: %v", err) + } + + _, err = PlanPush(cfg) + if err == nil { + t.Fatal("PlanPush planned a push into a destination nobody chose") + } + if !strings.Contains(err.Error(), "dot sync target /legacy/mirror") { + t.Errorf("PlanPush refusal does not name the fix:\n%v", err) + } +} + +// A store that sets `target` moved its destination deliberately: the retired +// key is irrelevant, whatever it says. +func TestRefuseRetiredDestinationChange_IgnoresAStoreWithTarget(t *testing.T) { + state := newIsolatedState(t) + writeStoreConfig(t, state.Modules.Gsync.LocalPath, "target: local:/chosen/mirror\nmirror_path: /legacy/mirror\n") + + cfg, err := ResolveConfigReadOnly(state) + if err != nil { + t.Fatalf("ResolveConfigReadOnly: %v", err) + } + if err := refuseRetiredDestinationChange(cfg); err != nil { + t.Errorf("an explicit target must not be guarded: %v", err) + } +} + func TestResolveConfigReadOnlyForHome_UsesGivenHome(t *testing.T) { // os.UserHomeDir() would resolve here — the --home override must not use it. currentHome := t.TempDir() From e8973652c74d808ddfeea0d3394c36df9332376a Mon Sep 17 00:00:00 2001 From: Young Joon Lee Date: Mon, 9 Nov 2026 22:43:58 +0900 Subject: [PATCH 3/3] fix(sync): carry a retired destination through unrelated saves The refusal that came with removing the `mirror_path` fallback had one piece of evidence, the retired value itself, and every routine save rewrote the file holding it while `target` was still empty: `dot sync pause`, `resume`, `owner`, `setup` and `shared` all save the store, and `dot apply` or `dot reconfigure` save global state. After one of those the store resolved the detected default, the refusal had nothing left to read, and the next push mirrored, and deleted, in a folder nobody picked. A retired value is evidence now, not a setting. A save writes it back unchanged, and says nothing, while it could still move a destination; it drops it with the existing note only once the value names the folder resolution would use. A store save judges `target` first and then the detected default; a state save judges that home's default, so `--home` writes are decided against the target home. A layout built without a home carries, which is the safe side of a doubt the save cannot settle. `dot sync fetch` copies mirror to workspace, so a destination that moved unasked would refill the workspace from a tree nobody picked and overwrite what is there. It joins the transfer entries that refuse. Also fixes the spelling of the retired state key: detection read `modules.gsync.mirror_path`, and neither the struct tag nor any real file has that block (`gdrive_sync`), so the live machine's key was never detected and the passing test only proved the wrong spelling. --- internal/appsettings/appsettings.go | 12 +++ internal/config/state.go | 135 ++++++++++++++++++++++---- internal/config/state_retired_test.go | 80 ++++++++++++--- internal/fileutil/samedir.go | 26 +++++ internal/fileutil/samedir_test.go | 41 ++++++++ internal/syncer/fetch_test.go | 33 +++++++ internal/syncer/local_store.go | 58 +++++++++-- internal/syncer/local_store_test.go | 68 +++++++++++++ internal/syncer/sync.go | 45 +++------ internal/syncer/sync_test.go | 47 ++++++++- 10 files changed, 474 insertions(+), 71 deletions(-) create mode 100644 internal/fileutil/samedir.go create mode 100644 internal/fileutil/samedir_test.go diff --git a/internal/appsettings/appsettings.go b/internal/appsettings/appsettings.go index 7567023e..16c7400b 100644 --- a/internal/appsettings/appsettings.go +++ b/internal/appsettings/appsettings.go @@ -793,6 +793,18 @@ func DetectCloudCandidate(home string) string { return DetectDriveCandidate(home) } +// DefaultMirrorPath is the folder `dot sync` mirrors into when a workspace +// store sets no target: "/work" on the same cloud as the detected +// backup root, or ~/gdrive-workspace/work when no cloud root qualifies. +func DefaultMirrorPath(home string) string { + if cloud := DetectCloudCandidate(home); cloud != "" { + // cloud is "/secrets/dotfiles-backup"; the mirror lives at + // "/work", two levels up from the secrets marker. + return filepath.Join(filepath.Dir(filepath.Dir(cloud)), "work") + } + return filepath.Join(home, "gdrive-workspace", "work") +} + // detectDropboxCandidate scans for a Dropbox cloud root (under // ~/Library/CloudStorage, then the ~/Dropbox symlink in home) that has a // "secrets" subdirectory marker. CloudStorage is scanned first so the diff --git a/internal/config/state.go b/internal/config/state.go index 7c4067f6..88f6485c 100644 --- a/internal/config/state.go +++ b/internal/config/state.go @@ -10,6 +10,8 @@ import ( "strings" "time" + "github.com/STAIxBWLB/dotfiles/internal/appsettings" + "github.com/STAIxBWLB/dotfiles/internal/fileutil" "github.com/STAIxBWLB/dotfiles/internal/sliceutil" "github.com/STAIxBWLB/dotfiles/internal/tunnel" "gopkg.in/yaml.v3" @@ -178,14 +180,17 @@ func sliceHasRetiredCask(tokens []string) bool { } // retiredStateKeys are the state keys no release reads any more, with what -// replaces each one. `modules.gsync.mirror_path` lost its last reader when -// ResolveConfig stopped falling back to it (#394 row 10): the destination is -// the workspace store's `target` now, so the value cannot be carried over and -// is only dropped. +// replaces each one. `modules.gdrive_sync.mirror_path` lost its last reader +// when ResolveConfig stopped falling back to it (#394 row 10): the destination +// is the workspace store's `target` now, so the value cannot be carried over as +// a setting. It is carried as evidence while it would still move a +// destination (retiredMirrorPathStillMoves), and dropped once it does not. var retiredStateKeys = map[string]string{ - "modules.gsync.mirror_path": "`dot sync` uses the workspace store's `target` (set it with `dot sync target`)", + retiredMirrorPathStateKey: "`dot sync` uses the workspace store's `target` (set it with `dot sync target`)", } +const retiredMirrorPathStateKey = "modules.gdrive_sync.mirror_path" + // detectRetiredKeys records which retired keys the raw state still sets, so // the next save can say what it dropped. It reads the raw bytes the way // applyLegacyKeys does -- the field these keys named is gone, so a decode @@ -197,22 +202,22 @@ func detectRetiredKeys(state *UserState, raw []byte) { Modules struct { Gsync struct { MirrorPath string `yaml:"mirror_path"` - } `yaml:"gsync"` + } `yaml:"gdrive_sync"` } `yaml:"modules"` } if err := yaml.Unmarshal(raw, &legacy); err != nil { return } if legacy.Modules.Gsync.MirrorPath != "" { - state.retiredKeys = append(state.retiredKeys, "modules.gsync.mirror_path") + state.retiredKeys = append(state.retiredKeys, retiredMirrorPathStateKey) state.retiredMirrorPath = legacy.Modules.Gsync.MirrorPath } } // RetiredMirrorPath returns the value this state file still sets for the -// retired `modules.gsync.mirror_path` key, or "". The sync engine reads it only -// to refuse a silent destination change on a store that never set `target`: the -// key does not route anything any more (#394 row 10). +// retired `modules.gdrive_sync.mirror_path` key, or "". The sync engine reads +// it only to refuse a silent destination change on a store that never set +// `target`: the key does not route anything any more (#394 row 10). func (s *UserState) RetiredMirrorPath() string { if s == nil { return "" @@ -220,6 +225,26 @@ func (s *UserState) RetiredMirrorPath() string { return s.retiredMirrorPath } +// retiredMirrorPathStillMoves reports whether this state's retired value has to +// survive a save. A workspace store that sets no target resolves the detected +// cloud default, and `dot sync` refuses exactly that while the retired value +// names a different folder (syncer.refuseRetiredDestinationChange reads it from +// here when the store has none of its own). Dropping it on an unrelated save +// (`dot apply`, `dot reconfigure`) would take that evidence away, and the next +// push would mirror, and delete, in a tree nobody picked. Once the value names +// the folder resolution would use, it decides nothing and goes. +func retiredMirrorPathStillMoves(state *UserState, home string) bool { + value := strings.TrimSpace(state.retiredMirrorPath) + if value == "" { + return false + } + if home == "" { + // No home, so no default to compare with: keep the evidence. + return true + } + return !fileutil.SameDirectory(appsettings.ExpandHome(value, home), appsettings.DefaultMirrorPath(home)) +} + // warnForwardSchema reports that a state file was written by a newer dot. It // does not fail the load: an additively-forward file still decodes, and an // incompatibly-forward one gets this warning before its decode error so the @@ -260,9 +285,10 @@ type UserState struct { // place that may speak of them, and it prints one note, then clears this. retiredKeys []string - // retiredMirrorPath is the value `modules.gsync.mirror_path` held in that - // same file, kept for the one reader that refuses a destination change - // (RetiredMirrorPath). + // retiredMirrorPath is the value `modules.gdrive_sync.mirror_path` held in + // that same file, kept for the one reader that refuses a destination change + // (RetiredMirrorPath), and written back by a save until it names the folder + // resolution would use. retiredMirrorPath string } @@ -681,9 +707,10 @@ func readStateFile(path, noun string) (*UserState, int, error) { applyLegacyKeys(&state, data) } // Retired keys are the exception to that gate: #373 is about keys whose - // values are carried over, and these have no value to carry. The only - // question left is whether the file still sets one, which the save must - // answer out loud, stamped or not. + // values are migrated to a canonical form, and there is no canonical form + // for these. The value is kept only as evidence for the destination refusal + // in internal/syncer, and the save decides whether to keep or drop it, + // stamped or not. detectRetiredKeys(&state, data) rewriteRetiredValues(&state) return &state, version, nil @@ -753,6 +780,14 @@ func MarshalState(state *UserState) ([]byte, error) { // On POSIX filesystems, rename is atomic, so partial writes cannot corrupt // the existing config file. func saveStateAt(path string, state *UserState) error { + home, _ := os.UserHomeDir() + return saveStateAtForHome(path, state, home) +} + +// saveStateAtForHome is saveStateAt for an explicit home: `dot ... --home` +// writes another user's state, and the retired destination is judged against +// that home's detected default, never the invoking user's. +func saveStateAtForHome(path string, state *UserState, home string) error { if err := state.Validate(); err != nil { return fmt.Errorf("refusing to save invalid state: %w", err) } @@ -769,6 +804,15 @@ func saveStateAt(path string, state *UserState) error { if err != nil { return err } + // A retired value that would still move a sync destination is evidence, not + // a setting: it stays in the file until it names the folder resolution would + // use, so an unrelated save cannot quietly hand the next push a new one. + carryRetired := state.retiredMirrorPath != "" && retiredMirrorPathStillMoves(state, home) + if carryRetired { + if data, err = carryRetiredMirrorPath(data, strings.TrimSpace(state.retiredMirrorPath)); err != nil { + return err + } + } // Write to temp file in the same directory (same filesystem → rename is atomic) tmpFile, err := os.CreateTemp(dir, ".config.yaml.*.tmp") @@ -811,16 +855,71 @@ func saveStateAt(path string, state *UserState) error { } // A retired key is the other half of the same notice: nothing replaces its // value, so the note has to name the command that sets the replacement. - // Printed once and cleared, so a run that saves twice says it once. + // Printed once and cleared, so a run that saves twice says it once. A key + // this save carried was not dropped, and says nothing. if len(state.retiredKeys) > 0 { for _, key := range state.retiredKeys { + if carryRetired && key == retiredMirrorPathStateKey { + continue + } fmt.Fprintf(os.Stderr, "note: dropped the retired `%s` key: %s\n", key, retiredStateKeys[key]) } state.retiredKeys = nil } + if !carryRetired { + // The value is gone from the file, so the memory of it goes too: a + // later save in the same run would otherwise write it back. + state.retiredMirrorPath = "" + } return nil } +// carryRetiredMirrorPath writes the retired key back into the marshaled state. +// The struct has no field for it any more, so this is a document edit: find +// (or add) modules.gdrive_sync and append the key. The value is carried as +// evidence for the destination refusal; nothing reads it as a setting. +func carryRetiredMirrorPath(data []byte, value string) ([]byte, error) { + var doc yaml.Node + if err := yaml.Unmarshal(data, &doc); err != nil { + return nil, fmt.Errorf("carrying the retired mirror_path: %w", err) + } + if len(doc.Content) == 0 || doc.Content[0].Kind != yaml.MappingNode { + return nil, fmt.Errorf("carrying the retired mirror_path: the state is not a mapping") + } + node := doc.Content[0] + for _, section := range []string{"modules", "gdrive_sync"} { + node = yamlMappingChild(node, section) + } + node.Content = append(node.Content, + &yaml.Node{Kind: yaml.ScalarNode, Tag: "!!str", Value: "mirror_path"}, + &yaml.Node{Kind: yaml.ScalarNode, Tag: "!!str", Value: value}, + ) + out, err := yaml.Marshal(&doc) + if err != nil { + return nil, fmt.Errorf("carrying the retired mirror_path: %w", err) + } + return out, nil +} + +// yamlMappingChild returns the mapping's value node for key, adding an empty +// mapping when the key is missing or holds something else. The state struct +// always writes both sections, so this is the defensive path. +func yamlMappingChild(mapping *yaml.Node, key string) *yaml.Node { + for i := 0; i+1 < len(mapping.Content); i += 2 { + if mapping.Content[i].Value != key { + continue + } + if mapping.Content[i+1].Kind != yaml.MappingNode { + mapping.Content[i+1] = &yaml.Node{Kind: yaml.MappingNode, Tag: "!!map"} + } + return mapping.Content[i+1] + } + child := &yaml.Node{Kind: yaml.MappingNode, Tag: "!!map"} + mapping.Content = append(mapping.Content, + &yaml.Node{Kind: yaml.ScalarNode, Tag: "!!str", Value: key}, child) + return child +} + // schemaForceEnv overrides the downgrade refusal below. It ships with the name // DEBT-02 gives it. Three naming conventions are in play in this tree and none // of them is this one -- the single existing DOT_* variable is an internal @@ -902,7 +1001,7 @@ func LoadStateForHome(homeDir string) (*UserState, error) { // SaveStateForHome writes user state to a specific home directory atomically. func SaveStateForHome(homeDir string, state *UserState) error { - return saveStateAt(StatePathForHome(homeDir), state) + return saveStateAtForHome(StatePathForHome(homeDir), state, homeDir) } // ApplyStateToConfig merges user state into a Config loaded from a profile. diff --git a/internal/config/state_retired_test.go b/internal/config/state_retired_test.go index 3b666efe..179d62e2 100644 --- a/internal/config/state_retired_test.go +++ b/internal/config/state_retired_test.go @@ -5,6 +5,8 @@ import ( "path/filepath" "strings" "testing" + + "github.com/STAIxBWLB/dotfiles/internal/appsettings" ) // #373 AC1: each retired value loads, is rewritten on the next save and @@ -66,14 +68,17 @@ func TestLoadState_StampedFileIgnoresStrayLegacyKey(t *testing.T) { } } -// #394 row 10: modules.gsync.mirror_path lost its last reader, so a file that -// still sets it must stop carrying it and say so once. The stamped version is -// the point: #373's gate governs keys whose values are carried over, and this -// one has no value left to carry, so the note has to fire for a canonical file -// too. -func TestLoadState_DropsTheRetiredMirrorPathKeyOnce(t *testing.T) { +// #394 row 10: modules.gdrive_sync.mirror_path lost its last reader, so a file +// that still sets it has nothing left to resolve. It does not lose the value +// yet: while it names a folder other than the detected default, a workspace +// store without `target` would silently redirect, and internal/syncer refuses +// exactly that on the strength of this value. Any unrelated save (`dot apply`, +// `dot reconfigure`) must carry it through, silently. +func TestLoadState_CarriesTheRetiredMirrorPathWhileItStillMoves(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) path := filepath.Join(t.TempDir(), "config.yaml") - body := "schema_version: 4\nname: Test\nprofile: full\nmodules:\n gsync:\n mirror_path: /legacy/mirror\n" + body := "schema_version: 4\nname: Test\nprofile: full\nmodules:\n gdrive_sync:\n mirror_path: /legacy/mirror\n" if err := os.WriteFile(path, []byte(body), 0o644); err != nil { t.Fatal(err) } @@ -81,11 +86,9 @@ func TestLoadState_DropsTheRetiredMirrorPathKeyOnce(t *testing.T) { if err != nil { t.Fatalf("load: %v", err) } - if len(loaded.retiredKeys) != 1 || loaded.retiredKeys[0] != "modules.gsync.mirror_path" { - t.Fatalf("retiredKeys = %v, want [modules.gsync.mirror_path]", loaded.retiredKeys) + if len(loaded.retiredKeys) != 1 || loaded.retiredKeys[0] != retiredMirrorPathStateKey { + t.Fatalf("retiredKeys = %v, want [%s]", loaded.retiredKeys, retiredMirrorPathStateKey) } - // The sync engine refuses a silent destination change with this value, so - // the loader has to hand the value on, not only the key's name. if got := loaded.RetiredMirrorPath(); got != "/legacy/mirror" { t.Errorf("RetiredMirrorPath = %q, want /legacy/mirror", got) } @@ -95,7 +98,60 @@ func TestLoadState_DropsTheRetiredMirrorPathKeyOnce(t *testing.T) { t.Fatalf("save: %v", err) } }) - if got := strings.Count(out, "note: dropped the retired `modules.gsync.mirror_path` key"); got != 1 { + if strings.Contains(out, "retired") { + t.Errorf("a carried key was announced as dropped: %q", out) + } + + data, _ := os.ReadFile(path) + if !strings.Contains(string(data), "mirror_path: /legacy/mirror") { + t.Fatalf("the save dropped a value that still moves a destination:\n%s", data) + } + + // Carrying is idempotent: the flag is spent, the value stays. + if again := captureStderr(t, func() { + if err := saveStateAt(path, loaded); err != nil { + t.Fatalf("second save: %v", err) + } + }); strings.Contains(again, "retired") { + t.Errorf("second save repeated the note: %q", again) + } + reloaded, err := loadStateAt(path) + if err != nil { + t.Fatal(err) + } + if got := reloaded.RetiredMirrorPath(); got != "/legacy/mirror" { + t.Errorf("reloaded value = %q, want /legacy/mirror", got) + } +} + +// The other half of the same rule: once the retired value names the folder +// resolution would use, it decides nothing, so the save drops it and says so +// once. The stamped version is the point: #373's gate governs keys whose values +// are migrated, and this one has no canonical form, so the note has to fire for +// a canonical file too. +func TestLoadState_DropsTheRetiredMirrorPathKeyOnceItIsMoot(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + moot := filepath.Join(home, "gdrive-workspace", "work") + if want := appsettings.DefaultMirrorPath(home); moot != want { + t.Fatalf("test default drifted: %q vs %q", moot, want) + } + path := filepath.Join(t.TempDir(), "config.yaml") + body := "schema_version: 4\nname: Test\nprofile: full\nmodules:\n gdrive_sync:\n mirror_path: " + moot + "\n" + if err := os.WriteFile(path, []byte(body), 0o644); err != nil { + t.Fatal(err) + } + loaded, err := loadStateAt(path) + if err != nil { + t.Fatalf("load: %v", err) + } + + out := captureStderr(t, func() { + if err := saveStateAt(path, loaded); err != nil { + t.Fatalf("save: %v", err) + } + }) + if got := strings.Count(out, "note: dropped the retired `"+retiredMirrorPathStateKey+"` key"); got != 1 { t.Fatalf("want exactly one retired-key note, got %d in %q", got, out) } if !strings.Contains(out, "`dot sync target`") { diff --git a/internal/fileutil/samedir.go b/internal/fileutil/samedir.go new file mode 100644 index 00000000..15dcb484 --- /dev/null +++ b/internal/fileutil/samedir.go @@ -0,0 +1,26 @@ +package fileutil + +import ( + "path/filepath" + "strings" +) + +// SameDirectory reports whether two paths name the same directory, following +// symlinks on both sides: a cloud folder is routinely reachable under a +// symlinked spelling (~/Dropbox -> ~/Library/CloudStorage/Dropbox), and a +// textual compare would read one tree as two. A path that does not resolve (it +// may not exist yet) makes the comparison textual, which leans towards +// reporting a difference rather than guessing that two paths are the same. +func SameDirectory(a, b string) bool { + a, b = strings.TrimSpace(a), strings.TrimSpace(b) + if a == "" || b == "" { + return false + } + ca, cb := filepath.Clean(a), filepath.Clean(b) + ra, errA := filepath.EvalSymlinks(ca) + rb, errB := filepath.EvalSymlinks(cb) + if errA == nil && errB == nil { + return ra == rb + } + return ca == cb +} diff --git a/internal/fileutil/samedir_test.go b/internal/fileutil/samedir_test.go new file mode 100644 index 00000000..f40bc49f --- /dev/null +++ b/internal/fileutil/samedir_test.go @@ -0,0 +1,41 @@ +package fileutil + +import ( + "os" + "path/filepath" + "testing" +) + +func TestSameDirectory(t *testing.T) { + tmp := t.TempDir() + real := filepath.Join(tmp, "Dropbox") + if err := os.MkdirAll(real, 0o755); err != nil { + t.Fatal(err) + } + link := filepath.Join(tmp, "Box") + if err := os.Symlink(real, link); err != nil { + t.Fatal(err) + } + + for _, tc := range []struct { + name string + a, b string + want bool + }{ + {"identical", real, real, true}, + {"trailing separator", real, real + string(filepath.Separator), true}, + {"unclean spelling", real, real + string(filepath.Separator) + ".", true}, + {"through a symlink", link, real, true}, + {"different folders", real, filepath.Join(tmp, "other"), false}, + {"one side missing", real, "", false}, + {"both missing", "", "", false}, + {"missing but spelled the same", "/no/such/tree", "/no/such/tree", true}, + {"missing and spelled apart", "/no/such/tree", "/no/such/../such/sibling", false}, + } { + t.Run(tc.name, func(t *testing.T) { + if got := SameDirectory(tc.a, tc.b); got != tc.want { + t.Errorf("SameDirectory(%q, %q) = %v, want %v", tc.a, tc.b, got, tc.want) + } + }) + } +} diff --git a/internal/syncer/fetch_test.go b/internal/syncer/fetch_test.go index 5fcd0bfd..efcd416a 100644 --- a/internal/syncer/fetch_test.go +++ b/internal/syncer/fetch_test.go @@ -12,6 +12,7 @@ import ( "testing" "time" + "github.com/STAIxBWLB/dotfiles/internal/config" "github.com/STAIxBWLB/dotfiles/internal/exec" ) @@ -295,3 +296,35 @@ func TestPullDirect_WithoutBackupIgnoresTheConflictsDir(t *testing.T) { t.Errorf("PullDirect refused although it passes no --backup: %v", err) } } + +// Fetch copies mirror → workspace, so a destination that moved unasked would +// refill the workspace from a tree nobody picked and overwrite what is there. +// It refuses on the same terms as push and pull (#394 row 10) and writes +// nothing. +func TestFetch_RefusesARetiredDestination(t *testing.T) { + f := newIntakeFixture(t) + t.Setenv("HOME", t.TempDir()) + if err := os.WriteFile(f.cfg.LocalPaths.ConfigFile, []byte("mirror_path: /legacy/mirror\n"), 0o644); err != nil { + t.Fatal(err) + } + f.writeMirror("notes/a.md", "mirror copy") + f.writeLocal("notes/a.md", "workspace copy") + + state := &config.UserState{} + state.Modules.Gsync.LocalPath = f.local + cfg, err := ResolveConfigReadOnly(state) + if err != nil { + t.Fatalf("ResolveConfigReadOnly: %v", err) + } + if !cfg.MirrorIsDefault { + t.Fatalf("the store sets no target, so the destination must be the default: %q", cfg.MirrorPath) + } + + if _, err := Fetch(context.Background(), f.runner, cfg, []string{"notes/a.md"}, false); err == nil || + !strings.Contains(err.Error(), "dot sync target /legacy/mirror") { + t.Fatalf("Fetch = %v, want a refusal naming the fix", err) + } + if body, _ := os.ReadFile(filepath.Join(f.local, "notes/a.md")); string(body) != "workspace copy" { + t.Errorf("the workspace copy was overwritten: %q", body) + } +} diff --git a/internal/syncer/local_store.go b/internal/syncer/local_store.go index 6820f208..e5ee0a07 100644 --- a/internal/syncer/local_store.go +++ b/internal/syncer/local_store.go @@ -11,6 +11,7 @@ import ( "gopkg.in/yaml.v3" "github.com/STAIxBWLB/dotfiles/internal/config" + "github.com/STAIxBWLB/dotfiles/internal/fileutil" "github.com/STAIxBWLB/dotfiles/internal/template" ) @@ -242,6 +243,11 @@ type LocalPaths struct { LogFile string WorkspaceRoot string // the local sync tree itself (parent of .dotfiles) WorkspaceIgnore string // /.gitignore (workspace-level) + // Home is the home this layout was resolved for (resolveConfig sets it). + // A save needs it to tell a retired destination from the detected default; + // empty means the caller built the layout without one, so a save cannot + // prove a retired value is moot and carries it. + Home string } // ResolveLocalPaths returns the canonical layout for a given local @@ -524,33 +530,63 @@ func SaveLocalConfig(paths *LocalPaths, cfg *LocalConfig) error { if err != nil { return fmt.Errorf("marshaling local config: %w", err) } - if data, err = keepUnknownConfigKeys(paths.ConfigFile, data); err != nil { + carried := retiredKeysStillInPlay(cfg, paths.Home) + if data, err = keepUnknownConfigKeys(paths.ConfigFile, data, carried); err != nil { return err } if err := atomicWrite(paths.ConfigFile, data); err != nil { return err } - announceRetiredLocalConfigKeys(paths.ConfigFile, cfg) + announceRetiredLocalConfigKeys(paths.ConfigFile, cfg, carried) return nil } +// retiredKeysStillInPlay names the retired keys a save has to carry instead of +// dropping. The only evidence `dot sync` has for refusing a destination that +// would move unasked is the value this store still sets (see +// refuseRetiredDestinationChange), so a save for an unrelated reason +// (`dot sync pause`, `dot sync owner`, `dot sync setup`, `dot sync shared`) +// must not take it away: after one of those, the store would resolve the +// detected default and the next push would mirror, and delete, there. +// +// A store that sets `target` chose its destination itself, and one whose value +// names the folder resolution would land on decides nothing either, so both +// drop. Without a home the save cannot prove the match, and carrying is the +// safe side of that doubt. +func retiredKeysStillInPlay(cfg *LocalConfig, home string) map[string]bool { + stored := strings.TrimSpace(cfg.retired[retiredMirrorPathKey]) + if stored == "" || strings.TrimSpace(cfg.Target) != "" { + return nil + } + if home != "" && fileutil.SameDirectory(expandHome(stored, home), defaultMirrorPath(home)) { + return nil + } + return map[string]bool{retiredMirrorPathKey: true} +} + // announceRetiredLocalConfigKeys reports the keys this save dropped, once per // loaded config: a key that no release reads any more disappears here, and // saying so is the difference between a migration and a silent edit of the -// user's file. Kept at the write choke point so no read path can print it. -func announceRetiredLocalConfigKeys(path string, cfg *LocalConfig) { +// user's file. Kept at the write choke point so no read path can print it. A +// carried key stays in cfg.retired, so a later save in the same run carries it +// again rather than announcing a drop that did not happen. +func announceRetiredLocalConfigKeys(path string, cfg *LocalConfig, carried map[string]bool) { if len(cfg.retired) == 0 { return } keys := make([]string, 0, len(cfg.retired)) for _, key := range retiredLocalConfigKeys { - if _, ok := cfg.retired[key]; ok { - keys = append(keys, key) + if _, ok := cfg.retired[key]; !ok || carried[key] { + continue } + keys = append(keys, key) + delete(cfg.retired, key) + } + if len(keys) == 0 { + return } fmt.Fprintf(os.Stderr, "note: dropped the retired `%s` key from %s; `dot sync` uses this store's `target` (set it with `dot sync target`)\n", strings.Join(keys, "`, `"), path) - cfg.retired = nil } // retiredMirrorPathKey is the one retired key whose value the sync engine still @@ -602,8 +638,12 @@ func retiredLocalConfigValues(data []byte) map[string]string { // aliases resolved (an anchor on a key the struct rewrote is gone), and // the result must load as a config, or the save refuses. // +// retiredCarried names the retired keys this save must carry rather than drop +// (retiredKeysStillInPlay): they are the store's only evidence for refusing a +// destination change, so they follow the same path as any unknown key. +// // ponytail: known ceiling. See docs/CEILINGS.md (unknown config keys, top level only). -func keepUnknownConfigKeys(path string, data []byte) ([]byte, error) { +func keepUnknownConfigKeys(path string, data []byte, retiredCarried map[string]bool) ([]byte, error) { old, err := os.ReadFile(path) if err != nil { return data, nil @@ -626,7 +666,7 @@ func keepUnknownConfigKeys(path string, data []byte) ([]byte, error) { if err != nil { return nil, fmt.Errorf("refusing to save %s: %w", path, err) } - if known[key.Value] || isRetiredLocalConfigKey(key.Value) || key.ShortTag() == "!!merge" { + if known[key.Value] || (isRetiredLocalConfigKey(key.Value) && !retiredCarried[key.Value]) || key.ShortTag() == "!!merge" { continue } value, err := resolveYAMLAliases(was.Content[0].Content[i+1]) diff --git a/internal/syncer/local_store_test.go b/internal/syncer/local_store_test.go index 5da8aa9f..b2463be3 100644 --- a/internal/syncer/local_store_test.go +++ b/internal/syncer/local_store_test.go @@ -530,6 +530,74 @@ func TestSaveLocalConfig_DropsTheRetiredMirrorPathKeyOnce(t *testing.T) { } } +// The mirror of that rule: a store with no target whose retired value names a +// different folder than the detected default is the only thing that stops +// `dot sync` from mirroring into a tree nobody picked. Every routine save +// rewrites the file, so such a save must carry the value and stay quiet; only +// one whose value names the default may drop it, and then it says so. +func TestSaveLocalConfig_CarriesTheRetiredMirrorPathWhileItStillMoves(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + + tmp := t.TempDir() + paths := ResolveLocalPaths(tmp) + paths.Home = home + if err := os.MkdirAll(paths.StoreDir, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(paths.ConfigFile, []byte("mirror_path: /legacy/mirror\n"), 0o644); err != nil { + t.Fatal(err) + } + + cfg, ok, err := LoadLocalConfig(paths) + if err != nil || !ok { + t.Fatalf("LoadLocalConfig: ok=%v err=%v", ok, err) + } + out := captureSyncerStderr(t, func() { + if err := SaveLocalConfig(paths, cfg); err != nil { + t.Fatalf("SaveLocalConfig: %v", err) + } + }) + if strings.Contains(out, "retired") { + t.Errorf("a carried key was announced as dropped: %q", out) + } + saved, err := os.ReadFile(paths.ConfigFile) + if err != nil { + t.Fatal(err) + } + if !strings.Contains(string(saved), "mirror_path: /legacy/mirror") { + t.Fatalf("the save dropped a value that still moves the destination:\n%s", saved) + } + + // The same store, once its value names the folder resolution would use: the + // key decides nothing, so it goes, out loud. + moot := filepath.Join(home, "gdrive-workspace", "work") + if want := defaultMirrorPath(home); moot != want { + t.Fatalf("test default drifted: %q vs %q", moot, want) + } + if err := os.WriteFile(paths.ConfigFile, []byte("mirror_path: "+moot+"\n"), 0o644); err != nil { + t.Fatal(err) + } + mootCfg, ok, err := LoadLocalConfig(paths) + if err != nil || !ok { + t.Fatalf("second LoadLocalConfig: ok=%v err=%v", ok, err) + } + out = captureSyncerStderr(t, func() { + if err := SaveLocalConfig(paths, mootCfg); err != nil { + t.Fatalf("second SaveLocalConfig: %v", err) + } + }) + if !strings.Contains(out, "note: dropped the retired `mirror_path` key") { + t.Errorf("a key that decides nothing was dropped without a note: %q", out) + } + if saved, err = os.ReadFile(paths.ConfigFile); err != nil { + t.Fatal(err) + } + if strings.Contains(string(saved), "mirror_path") { + t.Errorf("saved config still carries the retired key:\n%s", saved) + } +} + // captureSyncerStderr runs fn with os.Stderr redirected to a pipe and returns // what was written. The syncer's notes go straight to os.Stderr and take no // injectable writer, so the pipe is the only seam. diff --git a/internal/syncer/sync.go b/internal/syncer/sync.go index 72a2e251..e49cafac 100644 --- a/internal/syncer/sync.go +++ b/internal/syncer/sync.go @@ -16,25 +16,20 @@ import ( "github.com/STAIxBWLB/dotfiles/internal/appsettings" "github.com/STAIxBWLB/dotfiles/internal/config" "github.com/STAIxBWLB/dotfiles/internal/exec" + "github.com/STAIxBWLB/dotfiles/internal/fileutil" ) -// defaultMirrorPath is the mirror used when neither the local config nor the -// global state set one. It prefers a detected cloud root (Dropbox, then -// Google Drive) so the mirror follows the same cloud the backup root uses; -// absent any cloud root it falls back to ~/gdrive-workspace/work. +// defaultMirrorPath is the mirror used when the store sets no target. It +// prefers a detected cloud root (Dropbox, then Google Drive) so the mirror +// follows the same cloud the backup root uses; absent any cloud root it falls +// back to ~/gdrive-workspace/work. func defaultMirrorPath(home string) string { - if cloud := appsettings.DetectCloudCandidate(home); cloud != "" { - // cloud is "/secrets/dotfiles-backup"; the mirror lives at - // "/work", i.e. two levels up from the secrets marker. - return filepath.Join(filepath.Dir(filepath.Dir(cloud)), "work") - } - return filepath.Join(home, defaultMirrorRel) + return appsettings.DefaultMirrorPath(home) } // Defaults applied when the user state has not specified a value. const ( defaultLocalRel = "workspace/work" - defaultMirrorRel = "gdrive-workspace/work" defaultMaxDelete = 1000 logRotateMaxLines = 2000 logRotateKeepLines = 1000 @@ -252,6 +247,7 @@ func resolveConfig(state *config.UserState, migrate bool, home, profile string) } localPaths := ResolveLocalPathsForProfile(localPath, profile) + localPaths.Home = home var localCfg *LocalConfig if migrate && profile == DefaultProfile { @@ -676,7 +672,7 @@ func refuseRetiredDestinationChange(cfg *Config) error { if cfg == nil || cfg.retiredMirrorPath == "" || !cfg.MirrorIsDefault { return nil } - if sameDirectory(cfg.retiredMirrorPath, cfg.MirrorPath) { + if fileutil.SameDirectory(cfg.retiredMirrorPath, cfg.MirrorPath) { return nil } stored := strings.TrimRight(cfg.retiredMirrorPath, "/") @@ -689,25 +685,6 @@ func refuseRetiredDestinationChange(cfg *Config) error { ) } -// sameDirectory reports whether two paths name the same directory, following -// symlinks on both sides: the peer Mac's ~/Dropbox is a symlink into -// ~/Library/CloudStorage, so a textual compare would split one tree in two. A -// path that does not resolve (it may not exist yet) makes the comparison -// textual, which leans towards refusing rather than guessing. -func sameDirectory(a, b string) bool { - a, b = strings.TrimSpace(a), strings.TrimSpace(b) - if a == "" || b == "" { - return false - } - ca, cb := filepath.Clean(a), filepath.Clean(b) - ra, errA := filepath.EvalSymlinks(ca) - rb, errB := filepath.EvalSymlinks(cb) - if errA == nil && errB == nil { - return ra == rb - } - return ca == cb -} - // ── pull / push / sync ────────────────────────────────────────────────── // Push local → mirror under cfg.Propagation. The policy maps to rsync flags @@ -848,6 +825,12 @@ type FetchResult struct { // (#231). An absolute path is read relative to the workspace when it lies // under it. func Fetch(ctx context.Context, runner *exec.Runner, cfg *Config, rels []string, dryRun bool) (*FetchResult, error) { + // A fetch copies mirror → workspace, so a destination that moved without + // being asked would refill the workspace from a tree nobody picked and + // overwrite what is there. Same refusal as the other transfer entries. + if err := refuseRetiredDestinationChange(cfg); err != nil { + return nil, err + } if !dryRun { if err := ensureLogDir(cfg.LogFile); err != nil { return nil, err diff --git a/internal/syncer/sync_test.go b/internal/syncer/sync_test.go index fa9f5154..153203b9 100644 --- a/internal/syncer/sync_test.go +++ b/internal/syncer/sync_test.go @@ -531,7 +531,9 @@ func TestRefuseRetiredDestinationChange_ReadsTheGlobalState(t *testing.T) { t.Setenv("HOME", home) localPath := t.TempDir() statePath := filepath.Join(t.TempDir(), "config.yaml") - body := "schema_version: 4\nname: Test\nprofile: full\nmodules:\n gsync:\n local_path: " + + // `gdrive_sync` is the tag the struct and every real file use; `gsync` was + // never a key, so a fixture spelled that way proves nothing. + body := "schema_version: 4\nname: Test\nprofile: full\nmodules:\n gdrive_sync:\n local_path: " + localPath + "\n mirror_path: /legacy/mirror\n" if err := os.WriteFile(statePath, []byte(body), 0o644); err != nil { t.Fatal(err) @@ -576,6 +578,49 @@ func TestPlanPush_RefusesARetiredDestination(t *testing.T) { } } +// The refusal must not be one-shot. Its only evidence is the retired value, and +// every routine store save rewrites the file: `dot sync pause`/`resume` (the +// reload the other half of this change tells users to run), `dot sync owner`, +// `dot sync setup`, `dot sync shared` all go through SaveLocalConfig while +// `target` is still empty. A save that dropped the key there would hand the +// next push the detected default, which is exactly the silent redirect this +// guard exists to stop. +func TestPlanPush_SurvivesAnUnrelatedStoreSave(t *testing.T) { + state := newIsolatedState(t) + writeStoreConfig(t, state.Modules.Gsync.LocalPath, "mirror_path: /legacy/mirror\n") + + cfg, err := ResolveConfigReadOnly(state) + if err != nil { + t.Fatalf("ResolveConfigReadOnly: %v", err) + } + out := captureSyncerStderr(t, func() { + if err := SetLocalPaused(cfg, true); err != nil { + t.Fatalf("SetLocalPaused: %v", err) + } + }) + if strings.Contains(out, "retired") { + t.Errorf("an unrelated save announced a drop: %q", out) + } + saved, err := os.ReadFile(cfg.LocalPaths.ConfigFile) + if err != nil { + t.Fatal(err) + } + if !strings.Contains(string(saved), "mirror_path: /legacy/mirror") { + t.Fatalf("the save dropped the only evidence of the destination change:\n%s", saved) + } + + // The next run resolves afresh, as the next `dot sync push` would. + next, err := ResolveConfigReadOnly(state) + if err != nil { + t.Fatalf("ResolveConfigReadOnly after the save: %v", err) + } + if _, err := PlanPush(next); err == nil { + t.Fatal("after an unrelated save the push no longer refuses a destination nobody chose") + } else if !strings.Contains(err.Error(), "dot sync target /legacy/mirror") { + t.Errorf("the refusal after the save does not name the fix:\n%v", err) + } +} + // A store that sets `target` moved its destination deliberately: the retired // key is irrelevant, whatever it says. func TestRefuseRetiredDestinationChange_IgnoresAStoreWithTarget(t *testing.T) {