snapshots: no second copy of a save already kept

e2e/baseline_test.go

@@ -0,0 +1,118 @@
+package e2e
+
+import (
+	"net/http"
+	"os"
+	"path/filepath"
+	"testing"
+	"time"
+
+	"github.com/opensave/opensave/testutil"
+)
+
+// A newly tracked game's save is recorded as its baseline, so a file event
+// that changes nothing synced takes no second snapshot of it.
+//
+// It had none: the first event of any kind after tracking snapshotted the
+// unchanged save again, and so did the first after the game's exclusions
+// changed. A game writing a log into a folder excluded from syncing did that
+// straight away. With cloud sync on, the copy was then the device's newest
+// save, and a newer one another device had put in the cloud was no longer
+// offered — TestIgnored_ASaveFromTheCloudLeavesAnIgnoredFileAlone failed that
+// way whenever the extra snapshot came after the other device's.
+
+func TestBaseline_AFileInAnExcludedFolderTakesNoSnapshot(t *testing.T) {
+	d := testutil.NewTestDaemon(t, "BaselineExcluded")
+	d.WriteSave("settings.ini", "graphics")
+	d.WriteSave("slot1.sav", "act 1")
+	game := d.TrackGame("Baseline Game")
+	waitForWatch(t, d, game)
+	if !testutil.WaitFor(30*time.Second, func() bool { return countSnapshots(d, game) == 1 }) {
+		t.Fatalf("the initial snapshot was not taken: %d snapshots", countSnapshots(d, game))
+	}
+	d.API(http.MethodPatch, "/api/games/"+game, map[string]any{"syncIgnore": "settings.ini\nlogs/"}, nil)
+
+	d.WriteSave("logs/session.log", "started")
+	d.WriteSave("settings.ini", "other graphics")
+	time.Sleep(8 * time.Second) // well past the watcher's debounce
+	if n := countSnapshots(d, game); n != 1 {
+		t.Errorf("%d snapshots after only excluded files changed, want the initial one alone", n)
+	}
+
+	// A real change is still taken.
+	d.WriteSave("slot1.sav", "act 2")
+	if !testutil.WaitFor(30*time.Second, func() bool { return countSnapshots(d, game) == 2 }) {
+		t.Errorf("a change to the save was not snapshotted: %d snapshots", countSnapshots(d, game))
+	}
+}
+
+// A snapshot taken by hand is the baseline from then on: the change it kept is
+// not snapshotted a second time when the watcher gets to it.
+func TestBaseline_ASnapshotByHandIsNotTakenAgain(t *testing.T) {
+	d := testutil.NewTestDaemon(t, "BaselineManual")
+	d.WriteSave("slot1.sav", "act 1")
+	game := d.TrackGame("Baseline Manual Game")
+	waitForWatch(t, d, game)
+	if !testutil.WaitFor(30*time.Second, func() bool { return countSnapshots(d, game) == 1 }) {
+		t.Fatalf("the initial snapshot was not taken: %d snapshots", countSnapshots(d, game))
+	}
+	d.WriteSave("slot1.sav", "act 2")
+	d.API(http.MethodPost, "/api/games/"+game+"/snapshot", map[string]string{"comment": "before the boss"}, nil)
+	time.Sleep(8 * time.Second) // well past the watcher's debounce
+	if n := countSnapshots(d, game); n != 2 {
+		t.Errorf("%d snapshots, want the initial one and the one taken by hand", n)
+	}
+}
+
+// The first real change after tracking is snapshotted as before.
+func TestBaseline_TheFirstChangeAfterTrackingIsSnapshotted(t *testing.T) {
+	d := testutil.NewTestDaemon(t, "BaselineFirst")
+	d.WriteSave("slot1.sav", "act 1")
+	game := d.TrackGame("Baseline First Game")
+	waitForWatch(t, d, game)
+	if !testutil.WaitFor(30*time.Second, func() bool { return countSnapshots(d, game) == 1 }) {
+		t.Fatalf("the initial snapshot was not taken: %d snapshots", countSnapshots(d, game))
+	}
+	d.WriteSave("slot1.sav", "act 2")
+	if !testutil.WaitFor(30*time.Second, func() bool { return countSnapshots(d, game) == 2 }) {
+		t.Errorf("the first change after tracking was not snapshotted: %d snapshots", countSnapshots(d, game))
+	}
+}
+
+// Adding a save location changes no file, so it takes no snapshot; one that
+// brings files with it is snapshotted, as the backup should hold them.
+//
+// The hash the watcher compares against covers every location, so adding one
+// changed it, and the catch-up a watch runs when it starts snapshotted a save
+// already kept.
+func TestBaseline_AddingASaveLocationTakesNoSnapshotUnlessItBringsFiles(t *testing.T) {
+	d := testutil.NewTestDaemon(t, "BaselineLocation")
+	d.WriteSave("slot1.sav", "act 1")
+	game := d.TrackGame("Baseline Location Game")
+	waitForWatch(t, d, game)
+	if !testutil.WaitFor(30*time.Second, func() bool { return countSnapshots(d, game) == 1 }) {
+		t.Fatalf("the initial snapshot was not taken: %d snapshots", countSnapshots(d, game))
+	}
+
+	empty := filepath.Join(testutil.TempDir(t), "Config")
+	if err := os.MkdirAll(empty, 0o777); err != nil {
+		t.Fatal(err)
+	}
+	d.API(http.MethodPost, "/api/games/"+game+"/roots", map[string]string{"name": "config", "path": empty}, nil)
+	time.Sleep(8 * time.Second) // past the debounce and the catch-up
+	if n := countSnapshots(d, game); n != 1 {
+		t.Errorf("%d snapshots after adding an empty location, want the initial one alone", n)
+	}
+
+	full := filepath.Join(testutil.TempDir(t), "Mods")
+	if err := os.MkdirAll(full, 0o777); err != nil {
+		t.Fatal(err)
+	}
+	if err := os.WriteFile(filepath.Join(full, "mod.cfg"), []byte("on"), 0o666); err != nil {
+		t.Fatal(err)
+	}
+	d.API(http.MethodPost, "/api/games/"+game+"/roots", map[string]string{"name": "mods", "path": full}, nil)
+	if !testutil.WaitFor(30*time.Second, func() bool { return countSnapshots(d, game) == 2 }) {
+		t.Errorf("a location that brought files was not snapshotted: %d snapshots", countSnapshots(d, game))
+	}
+}

internal/api/routes.go

@@ -442,14 +442,15 @@ func (s *Server) handleUpdateGame(w http.ResponseWriter, r *http.Request) {
 			s.Daemon.Log.Log("warn", fmt.Sprintf(
 				"could not reset sync agreement for %q after its exclusions changed: %v", game.Name, err))
 		}
-		// The last-snapshot hash goes too, for exactly the same reason: it was
+		// The last-snapshot hash too, for exactly the same reason: it was
 		// recorded over the old view, and the sync engine compares against it
 		// to decide whether the save holds changes a pull would overwrite. A
 		// value that can never match again makes that check fire on every
-		// pull. Clearing it disables the check until the next snapshot
-		// re-records it, which is the safe direction — it can cost one
-		// unnecessary prompt, never a silent overwrite.
-		game.LastManifestHash = ""
+		// pull. Restated in the new view when the save provably has not
+		// changed since that snapshot, and cleared otherwise, which disables
+		// the check until the next snapshot re-records it — the safe
+		// direction (daemon.LastSnapshotHashUnder).
+		game.LastManifestHash = s.Daemon.LastSnapshotHashUnder(game, oldIgnore)
 	}
 
 	if err := s.Daemon.Store.UpdateGame(game); err != nil {
@@ -609,7 +610,7 @@ func (s *Server) handleCreateSnapshot(w http.ResponseWriter, r *http.Request) {
 	}
 	_ = readJSON(r, &body) // empty body is fine
 
-	snap, err := s.Daemon.Snapshots.Create(gameID, body.Comment, false)
+	snap, err := s.Daemon.SnapshotWithBaseline(gameID, body.Comment, false)
 	if err != nil {
 		writeError(w, notFoundToStatus(err), err.Error())
 		return

internal/api/routes_roots.go

@@ -72,10 +72,12 @@ func (s *Server) handleAddGameRoot(w http.ResponseWriter, r *http.Request) {
 	// Rejections here are the overlap and naming rules, which are the user's
 	// to hear about rather than a server fault: "that folder is already
 	// covered by the main save" is a sentence, not a 500.
+	oldRoots, _ := s.Daemon.Store.GameRootPaths(gameID)
 	if err := s.Daemon.Store.AddGameRoot(gameID, body.Name, path); err != nil {
 		writeError(w, http.StatusBadRequest, err.Error())
 		return
 	}
+	s.Daemon.LocationsChanged(gameID, oldRoots)
 	// Start watching it now, not at the next restart.
 	s.Daemon.RewatchGame(gameID)
 	s.BroadcastGamesUpdate()
@@ -90,10 +92,12 @@ func (s *Server) handleRemoveGameRoot(w http.ResponseWriter, r *http.Request) {
 		writeError(w, http.StatusBadRequest, "invalid location name")
 		return
 	}
+	oldRoots, _ := s.Daemon.Store.GameRootPaths(gameID)
 	if err := s.Daemon.Store.RemoveGameRoot(gameID, name); err != nil {
 		writeError(w, http.StatusInternalServerError, err.Error())
 		return
 	}
+	s.Daemon.LocationsChanged(gameID, oldRoots)
 	// Stop watching it now, or it keeps waking the game up over a folder
 	// nobody is covering any more.
 	s.Daemon.RewatchGame(gameID)

internal/cliapp/cliapp.go

@@ -847,7 +847,7 @@ func cmdSnapshot(d *daemon.Daemon, args []string) int {
 	}
 	comment := strings.Join(words, " ")
 
-	snap, err := d.Snapshots.Create(args[0], comment, false)
+	snap, err := d.SnapshotWithBaseline(args[0], comment, false)
 	if err != nil {
 		fmt.Fprintf(os.Stderr, "error: %v\n", err)
 		return 1

internal/cliapp/commands_exclude_game.go

@@ -119,6 +119,7 @@ func saveIgnore(d *daemon.Daemon, asJSON bool, gameID, gameName string, lines []
 	if err != nil {
 		return fail(asJSON, err)
 	}
+	oldIgnore := game.SyncIgnore
 	game.SyncIgnore = strings.Join(lines, "\n")
 
 	// Same reset the API performs: the recorded agreement was measured over a
@@ -129,7 +130,7 @@ func saveIgnore(d *daemon.Daemon, asJSON bool, gameID, gameName string, lines []
 		d.P2P.Sync.FilteredContentHash(gameID, game)); err != nil {
 		d.Log.Log("warn", "could not reset sync agreement after an exclusion change: "+err.Error())
 	}
-	game.LastManifestHash = ""
+	game.LastManifestHash = d.LastSnapshotHashUnder(game, oldIgnore)
 
 	if err := d.Store.UpdateGame(game); err != nil {
 		return fail(asJSON, err)

internal/cliapp/commands_locations.go

@@ -29,9 +29,11 @@ func cmdLocations(d *daemon.Daemon, args []string) int {
 		if err != nil {
 			return fail(asJSON, err)
 		}
+		oldRoots, _ := d.Store.GameRootPaths(gameID)
 		if err := d.Store.AddGameRoot(gameID, name, abs); err != nil {
 			return fail(asJSON, err)
 		}
+		d.LocationsChanged(gameID, oldRoots)
 		if asJSON {
 			return emitJSON(map[string]any{"game": gameID, "location": name, "path": abs})
 		}
@@ -44,9 +46,11 @@ func cmdLocations(d *daemon.Daemon, args []string) int {
 			fmt.Fprintln(os.Stderr, locationsUsage)
 			return 1
 		}
+		oldRoots, _ := d.Store.GameRootPaths(args[1])
 		if err := d.Store.RemoveGameRoot(args[1], args[2]); err != nil {
 			return fail(asJSON, err)
 		}
+		d.LocationsChanged(args[1], oldRoots)
 		if asJSON {
 			return emitJSON(map[string]any{"removed": args[2]})
 		}

internal/daemon/autotrack.go

@@ -41,7 +41,7 @@ func (d *Daemon) adoptAutoTracked(game store.Game) {
 			readDone, err := d.P2P.Sync.Reading(ctx, game.ID)
 			cancel()
 			if err == nil {
-				_, err = d.Snapshots.Create(game.ID, "Initial snapshot", true)
+				_, err = d.SnapshotWithBaseline(game.ID, "Initial snapshot", true)
 				readDone()
 			}
 			if err != nil {