keyboard fetch does not replace a stored definition

`keyboard fetch` wrote the downloaded file with a bare WriteFile, so
running it a second time destroyed a definition the user had edited,
without a word. The file in the per-user directory is the one thing a
user is most likely to have changed by hand, and nothing on disk says
which files those are.

The check is by content rather than by file name, because what
identifies a definition is the vendor and product ID inside it. It runs
before the download, so a user who already has one is told so without
waiting on the network and whether the answer depends on VIA still
carrying the board.

--force is the only way to replace a file. It removes the file it
replaced when the two differ in name, because the vendor issues one
vendor and product ID to two models and a directory can hold both: two
files for one board leave which is read to the order the directory
comes back in. The removal follows the write, so a write that failed
keeps what was there.

cmd/qmk-rgb-tool/definition.go

@@ -31,20 +31,30 @@ var httpGet = func(url string) ([]byte, int, error) {
 	return body, resp.StatusCode, err
 }
 
+// forceFetch replaces a definition that is already stored. It is the only way to
+// do so, because a file in the user directory may be one the user has edited and
+// nothing on disk says which it is.
+var forceFetch bool
+
 // NewKeyboardFetchCmd is `keyboard fetch`. It belongs to the keyboard command
 // because what it fetches belongs to a board: the file is looked up by the
 // board's vendor and product ID, so there is nothing to fetch without one.
 func NewKeyboardFetchCmd() *cobra.Command {
-	return &cobra.Command{
+	cmd := &cobra.Command{
 		Use:   "fetch",
 		Short: "Download the definition file for the connected keyboard",
 		Long: "Identify the connected keyboard, then download its VIA definition into the\n" +
 			"data directory. VIA does not carry a definition for every board, and answers\n" +
 			"an unknown one with its own web page, so a downloaded file is parsed and\n" +
-			"matched against the keyboard before it is stored.",
+			"matched against the keyboard before it is stored.\n\n" +
+			"A definition that is already stored for this keyboard is not replaced, because\n" +
+			"it may be one you have edited. Edit that file, or pass --force to overwrite it.",
 		Args: cobra.NoArgs,
 		RunE: runKeyboardFetch,
 	}
+	cmd.Flags().BoolVar(&forceFetch, "force", false,
+		"Replace a definition that is already stored, discarding whatever that file holds")
+	return cmd
 }
 
 // NewKeyboardDefinitionsCmd is `keyboard definitions`, and the name is the noun
@@ -68,28 +78,81 @@ func runKeyboardFetch(cmd *cobra.Command, args []string) error {
 		return err
 	}
 
-	def, err := fetchDefinition(target.Device.VendorID, target.Device.ProductID)
+	dir, err := ensureDefinitionsDir()
 	if err != nil {
 		return err
 	}
 
-	dir, err := ensureDefinitionsDir()
+	// The check comes before the download, so a user whose definition is already
+	// there is told so without waiting on the network and without the answer
+	// depending on VIA still carrying the board. It matches by content rather
+	// than by file name, because what identifies a definition is the vendor and
+	// product ID inside it and the name of the file is only a label.
+	stored := storedDefinitionsFor(dir, target.Device.VendorID, target.Device.ProductID)
+	if len(stored) > 0 && !forceFetch {
+		return fmt.Errorf("a definition for %s (0x%04X/0x%04X) is already at %s; it may hold your "+
+			"edits, so fetch does not replace it, use --force to overwrite it or edit that file",
+			stored[0].Name, target.Device.VendorID, target.Device.ProductID, describeDataDir(stored[0].Path))
+	}
+
+	def, err := fetchDefinition(target.Device.VendorID, target.Device.ProductID)
 	if err != nil {
 		return err
 	}
+
 	path := filepath.Join(dir, definitionFileName(def))
 	if err := os.WriteFile(path, []byte(def.raw), 0o644); err != nil {
 		return fmt.Errorf("write %s: %w", path, err)
 	}
 
-	fmt.Fprintf(cmd.OutOrStdout(), "Saved definition for %s (0x%04X/0x%04X) to %s\n",
-		def.Definition.Name, def.Definition.VendorID, def.Definition.ProductID, describeDataDir(path))
+	// A definition that was replaced under a different file name is removed
+	// rather than left standing beside the new one: two files describing one
+	// board leave which of them is read to the order the directory happens to
+	// come back in. The removal follows the write, so a write that failed keeps
+	// what was there.
+	for _, old := range stored {
+		if old.Path == path {
+			continue
+		}
+		if err := os.Remove(old.Path); err != nil {
+			return fmt.Errorf("remove replaced %s: %w", old.Path, err)
+		}
+	}
+
+	verb := "Saved"
+	if len(stored) > 0 {
+		verb = "Replaced"
+	}
+	fmt.Fprintf(cmd.OutOrStdout(), "%s definition for %s (0x%04X/0x%04X) to %s\n",
+		verb, def.Definition.Name, def.Definition.VendorID, def.Definition.ProductID, describeDataDir(path))
 	for _, ch := range def.channels {
 		fmt.Fprintf(cmd.OutOrStdout(), "  %-10s %d effects\n", ch.Subsystem(), len(def.Definition.Catalog.Effects(ch)))
 	}
 	return nil
 }
 
+// storedDefinitionsFor returns the definitions the user directory already holds
+// for a board. It is a slice because two files can describe one board: the
+// vendor issues the same vendor and product ID to two models, which
+// definitions/README.md records, and only one of the two files is read.
+//
+// A directory that cannot be read is not an answer. A file in it may be
+// unreadable, and "nothing stored" is what a fetch acts on, so an unreadable
+// directory leaves the fetch to write rather than refuse.
+func storedDefinitionsFor(dir string, vendorID, productID uint16) []*intrgb.Definition {
+	defs, err := intrgb.LoadDefinitionsDir(dir)
+	if err != nil {
+		return nil
+	}
+	var out []*intrgb.Definition
+	for _, def := range defs {
+		if def.Matches(vendorID, productID) {
+			out = append(out, def)
+		}
+	}
+	return out
+}
+
 func runKeyboardDefinitions(cmd *cobra.Command, args []string) error {
 	dir := definitionsPath()
 	// Both sources are listed, not just the directory: the file built into the

cmd/qmk-rgb-tool/definition_test.go

@@ -54,6 +54,179 @@ func TestFetchDefinitionStoresTheFileForTheBoard(t *testing.T) {
 	}
 }
 
+// A definition in the user directory may be one the user has edited, and nothing
+// on disk says which it is. So a second fetch must not replace it, and must say
+// that the file is there rather than write over it silently.
+func TestFetchDoesNotReplaceAStoredDefinition(t *testing.T) {
+	dir := t.TempDir()
+	t.Cleanup(definitionFlagRestore(t))
+	t.Cleanup(forceDefinitionsDir(t, dir))
+	t.Cleanup(forceFetchRestore(t))
+	stubPrepareTarget(t, 0x1234, 0x5678)
+
+	edited := `{"name":"Test Board","vendorProductId":305419896,"menus":[],"note":"hand edited"}`
+	if err := os.WriteFile(filepath.Join(dir, "test_board.json"), []byte(edited), 0o644); err != nil {
+		t.Fatal(err)
+	}
+
+	// No HTTP stub: a request would panic here, which is the point. The user is
+	// told the file exists without waiting on the network.
+	cmd := NewKeyboardFetchCmd()
+	var out, errOut strings.Builder
+	cmd.SetOut(&out)
+	cmd.SetErr(&errOut)
+
+	err := cmd.Execute()
+	if err == nil {
+		t.Fatal("keyboard fetch = nil error, want a refusal to replace a stored definition")
+	}
+	if !strings.Contains(err.Error(), "already at") || !strings.Contains(err.Error(), "--force") {
+		t.Errorf("error = %q, want it to name the file and --force", err)
+	}
+
+	got, readErr := os.ReadFile(filepath.Join(dir, "test_board.json"))
+	if readErr != nil {
+		t.Fatal(readErr)
+	}
+	if string(got) != edited {
+		t.Errorf("stored file = %q, want it untouched (%q)", got, edited)
+	}
+}
+
+// --force is the only way to replace a stored definition, and it has to say that
+// is what happened, because the file it wrote over is gone.
+func TestFetchForceReplacesAStoredDefinition(t *testing.T) {
+	dir := t.TempDir()
+	t.Cleanup(definitionFlagRestore(t))
+	t.Cleanup(forceDefinitionsDir(t, dir))
+	t.Cleanup(forceFetchRestore(t))
+	stubPrepareTarget(t, 0x1234, 0x5678)
+
+	if err := os.WriteFile(filepath.Join(dir, "test_board.json"),
+		[]byte(`{"name":"Test Board","vendorProductId":305419896,"menus":[],"note":"hand edited"}`), 0o644); err != nil {
+		t.Fatal(err)
+	}
+
+	served := `{"name":"Test Board","vendorProductId":305419896,"menus":[]}`
+	stubHTTPGet(t, func(url string) ([]byte, int, error) {
+		if strings.Contains(url, "/v3/") {
+			return []byte(served), http.StatusOK, nil
+		}
+		return nil, http.StatusNotFound, errors.New("404")
+	})
+
+	cmd := NewKeyboardFetchCmd()
+	var out, errOut strings.Builder
+	cmd.SetOut(&out)
+	cmd.SetErr(&errOut)
+	cmd.SetArgs([]string{"--force"})
+
+	if err := cmd.Execute(); err != nil {
+		t.Fatalf("keyboard fetch --force error = %v (stderr %q)", err, errOut.String())
+	}
+	if !strings.Contains(out.String(), "Replaced definition") {
+		t.Errorf("stdout = %q, want it to say the definition was replaced", out.String())
+	}
+
+	entries, err := os.ReadDir(dir)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(entries) != 1 {
+		t.Fatalf("definitions dir = %v, want the one file replaced in place", entries)
+	}
+	got, readErr := os.ReadFile(filepath.Join(dir, entries[0].Name()))
+	if readErr != nil {
+		t.Fatal(readErr)
+	}
+	if string(got) != served {
+		t.Errorf("stored file = %q, want what the server served (%q)", got, served)
+	}
+}
+
+// A definition for a different board is not a reason to refuse: the fetch is for
+// this one, and that one is still worth having alongside.
+func TestFetchStoresAlongsideAnotherBoardsDefinition(t *testing.T) {
+	dir := t.TempDir()
+	t.Cleanup(definitionFlagRestore(t))
+	t.Cleanup(forceDefinitionsDir(t, dir))
+	t.Cleanup(forceFetchRestore(t))
+	stubPrepareTarget(t, 0x1234, 0x5678)
+
+	if err := os.WriteFile(filepath.Join(dir, "other.json"),
+		[]byte(`{"name":"Other","vendorId":"0x1111","productId":"0x2222"}`), 0o644); err != nil {
+		t.Fatal(err)
+	}
+
+	served := `{"name":"Test Board","vendorProductId":305419896,"menus":[]}`
+	stubHTTPGet(t, func(string) ([]byte, int, error) {
+		return []byte(served), http.StatusOK, nil
+	})
+
+	cmd := NewKeyboardFetchCmd()
+	var out, errOut strings.Builder
+	cmd.SetOut(&out)
+	cmd.SetErr(&errOut)
+
+	if err := cmd.Execute(); err != nil {
+		t.Fatalf("keyboard fetch error = %v (stderr %q)", err, errOut.String())
+	}
+
+	entries, err := os.ReadDir(dir)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(entries) != 2 {
+		t.Errorf("definitions dir = %v, want the other board's file kept alongside", entries)
+	}
+}
+
+// The vendor issues one vendor and product ID to two models, so a directory can
+// hold two files describing the same board. --force has to leave one, not two:
+// which of them is read would otherwise depend on the order the directory comes
+// back in.
+func TestFetchForceLeavesOneFileForABoardWithTwo(t *testing.T) {
+	dir := t.TempDir()
+	t.Cleanup(definitionFlagRestore(t))
+	t.Cleanup(forceDefinitionsDir(t, dir))
+	t.Cleanup(forceFetchRestore(t))
+	stubPrepareTarget(t, 0x320F, 0x5055)
+
+	for _, name := range []string{"first.json", "second.json"} {
+		body := `{"name":"Twin","vendorId":"0x320F","productId":"0x5055","menus":[]}`
+		if err := os.WriteFile(filepath.Join(dir, name), []byte(body), 0o644); err != nil {
+			t.Fatal(err)
+		}
+	}
+
+	served := `{"name":"Twin","vendorProductId":839864405,"menus":[]}`
+	stubHTTPGet(t, func(string) ([]byte, int, error) {
+		return []byte(served), http.StatusOK, nil
+	})
+
+	cmd := NewKeyboardFetchCmd()
+	var out, errOut strings.Builder
+	cmd.SetOut(&out)
+	cmd.SetErr(&errOut)
+	cmd.SetArgs([]string{"--force"})
+
+	if err := cmd.Execute(); err != nil {
+		t.Fatalf("keyboard fetch --force error = %v (stderr %q)", err, errOut.String())
+	}
+
+	entries, err := os.ReadDir(dir)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(entries) != 1 {
+		names := make([]string, 0, len(entries))
+		for _, e := range entries {
+			names = append(names, e.Name())
+		}
+		t.Errorf("definitions dir = %v, want exactly one file for the board", names)
+	}
+}
+
 // An unknown board is answered with a web page and a success status, so the
 // fetch must not store it and must say what happened instead.
 func TestFetchDefinitionRejectsAPageThatIsNotADefinition(t *testing.T) {
@@ -210,6 +383,14 @@ func definitionFlagRestore(t *testing.T) func() {
 	return func() { definitionFlag = original }
 }
 
+// forceFetchRestore resets --force, so a test that sets it does not leak it into
+// the next one.
+func forceFetchRestore(t *testing.T) func() {
+	t.Helper()
+	original := forceFetch
+	return func() { forceFetch = original }
+}
+
 // forceDefinitionsDir points the data directory at a test directory. The tool
 // looks for it next to the executable and then in the working directory, so a
 // test that wants its own has to make that lookup find it.