Stop opening a deck from rewriting it

CHANGELOG.md

@@ -7,6 +7,35 @@ detail, which is where the reasoning lives.
 
 ### Fixed
 
+- **Opening a deck no longer rewrites it.** Studying any deck in `examples/`
+  straight from a clone left it modified in `git status` without a single card
+  having been answered, which looks like the app corrupting its own sample data
+  and is an easy way to get surprise diffs.
+
+  Two separate causes, both of which had to go:
+
+  - Every card was given an `id` when the deck was *loaded*. An id is what a
+    log event names a card by, so a card needs one at the moment something is
+    recorded against it — and a deck that is only read has nothing recorded
+    against it. Ids are now minted at that moment instead. Answered cards are
+    unaffected: the id still exists before the event that names it is written,
+    which is the invariant that matters.
+  - Saving expanded every row to its full width, so a hand-written
+    `question,answer,tags` deck came back as `question,answer,tags,0,0,1,,,id`.
+    Cards are now written only as far as the last column they actually use.
+    That rule already existed for the `audio` and `image` columns, for exactly
+    this reason; it now covers the whole row rather than the last two.
+
+  A save that would reproduce the file byte for byte is also skipped outright.
+  Saving is unconditional at every call site — after every answer, after every
+  edit, and on the way out — which is deliberate and is what makes an
+  interrupted session cost nothing; it just should not mean that reading a deck
+  counts as writing it.
+
+  Nothing about what a deck can contain has changed, and every deck written by
+  an earlier version still loads and still saves identically once a card in it
+  has been reviewed.
+
 - **`FLASHTERM_SEED` now fixes the review order on every platform, not just the
   one you built on.** The shuffle went through `std::shuffle`, whose output the
   standard does not specify — only that the permutation is uniformly random —

README.md

@@ -206,20 +206,28 @@ question,answer,tags,correct,incorrect,box,last_reviewed,due_date,id,audio,image
 Dates are plain `YYYY-MM-DD`, blank when a card has never been reviewed. Answers may list alternatives separated by `|`. Questions and answers containing commas or quotes are quoted normally, so decks stay greppable and editable by hand.
 
 `id` is 16 hex characters identifying the card for the review log. It is filled
-in automatically the first time a deck is loaded, so hand-written decks can
+in automatically the first time the card is answered, so hand-written decks can
 leave it off, and it stays the same when you edit the card — a fixed typo does
-not orphan the card's history.
+not orphan the card's history. Not on load, deliberately: opening a deck does
+not change it, so a deck you only read is a deck your sync client and your
+version control never see move.
 
 `audio` is a recording of the *question*, as a path relative to the deck file —
 so a deck and the audio directory beside it can be moved or synced as one thing.
 
 `image` is a picture for the card, resolved the same way. See [Images](#images).
 
-Both are written only as far as the last column a card actually uses, which
-means a deck with neither comes out byte for byte as earlier versions wrote it,
-and syncing between a machine that has updated and one that has not does not put
-the whole file in conflict. A card with a picture and no recording still writes
-the empty audio column, because position is what names a field in a CSV.
+**Every card is written only as far as the last column it actually uses**, so a
+deck of plain `question,answer,tags` rows — which is what the examples are, and
+what you get writing one by hand — is saved back in that form rather than
+expanded. A card that has never been reviewed says nothing in the six columns
+after its tags, and so writes none of them.
+
+That is also what keeps a deck byte for byte as earlier versions wrote it, so
+syncing between a machine that has updated and one that has not does not put
+the whole file in conflict. Trailing columns only: a card with a picture and no
+recording still writes the empty audio column, because position is what names a
+field in a CSV.
 
 ### Review Log Format
 

src/deck.cpp

@@ -6,6 +6,7 @@
 #include <cstring>
 #include <fstream>
 #include <set>
+#include <sstream>
 #include <utility>
 
 #include "date.h"
@@ -30,28 +31,58 @@ std::string errno_message() {
 }  // namespace
 
 std::string card_to_csv(const Flashcard& card) {
-  const std::string row =
-      escape_csv_field(card.question) + "," + escape_csv_field(card.answer) +
-      "," + escape_csv_field(card.tags_to_string()) + "," +
-      std::to_string(card.times_correct) + "," +
-      std::to_string(card.times_incorrect) + "," +
-      std::to_string(card.leitner_box) + "," +
-      format_date(card.last_reviewed) + "," + format_date(card.due_date) + "," +
-      escape_csv_field(card.id);
-
-  // Written only as far as the last column the card actually uses. A deck with
-  // no audio and no pictures -- which is most decks -- then comes out byte for
-  // byte as every earlier version wrote it. That matters because decks are
-  // synced between machines as plain files: a trailing comma on every line
-  // would put the whole deck in conflict the first time one machine saved it
-  // and the other had not updated yet.
+  const std::string columns[] = {
+      escape_csv_field(card.question),
+      escape_csv_field(card.answer),
+      escape_csv_field(card.tags_to_string()),
+      std::to_string(card.times_correct),
+      std::to_string(card.times_incorrect),
+      std::to_string(card.leitner_box),
+      format_date(card.last_reviewed),
+      format_date(card.due_date),
+      escape_csv_field(card.id),
+      escape_csv_field(card.audio),
+      escape_csv_field(card.image),
+  };
+
+  // What card_from_csv fills in for a column that is not there at all. A
+  // column holding exactly this says nothing the reader would not have assumed
+  // anyway, so writing it is optional -- which is the whole basis of the trim
+  // below, and the reason these two lists have to stay in step.
+  static const char* const kAbsent[] = {"", "", "",  "0", "0", "1",
+                                        "", "", "",  "",  ""};
+  const std::size_t count = sizeof(columns) / sizeof(columns[0]);
+
+  // Written only as far as the last column the card actually uses. A deck of
+  // plain question,answer,tags rows -- which is what every deck in examples/
+  // is, and what anyone writing one by hand produces -- comes back out in that
+  // form rather than being expanded to the full eleven. Without this, opening
+  // such a deck and closing it rewrote every line of it.
   //
-  // A card with a picture and no recording still has to write the empty audio
-  // column, since position is what names a field in a CSV.
-  if (card.audio.empty() && card.image.empty()) return row;
-  const std::string with_audio = row + "," + escape_csv_field(card.audio);
-  if (card.image.empty()) return with_audio;
-  return with_audio + "," + escape_csv_field(card.image);
+  // It is also what keeps a deck byte for byte as an earlier version wrote it,
+  // which matters because decks are synced between machines as plain files: a
+  // trailing comma on every line would put the whole deck in conflict the
+  // first time one machine saved it and the other had not updated yet. The
+  // audio and image columns were the first to need this and are now simply the
+  // last two cases of the general rule.
+  //
+  // Trailing, not any: a card with a picture and no recording still writes the
+  // empty audio column, because position is what names a field in a CSV.
+  //
+  // Question, answer and tags are always written, even when the tags are
+  // empty. Three columns is the documented short form of a deck, and a
+  // two-column line, though it would read back correctly, is not a shape
+  // anything else in the project produces.
+  std::size_t last = 2;
+  for (std::size_t i = 3; i < count; ++i) {
+    if (columns[i] != kAbsent[i]) last = i;
+  }
+
+  std::string row = columns[0];
+  for (std::size_t i = 1; i <= last; ++i) {
+    row += "," + columns[i];
+  }
+  return row;
 }
 
 bool card_from_csv(const std::string& line, Flashcard* out) {
@@ -101,21 +132,57 @@ bool Deck::load() {
 
   std::ifstream file(path_);
   if (!file.is_open()) {
+    on_disk_.clear();
+    on_disk_known_ = false;
     return false;
   }
+
+  // Read whole rather than line by line, and kept, so that save() can answer
+  // "would writing this change the file?" against the actual bytes. Parsing
+  // then runs over the copy. Line-at-a-time reading cannot serve here: it
+  // silently drops blank lines and cannot tell whether the last line ended in
+  // a newline, and both of those are differences a save would write out.
+  std::ostringstream buffer;
+  buffer << file.rdbuf();
+  on_disk_ = buffer.str();
+  on_disk_known_ = true;
+
+  std::istringstream lines(on_disk_);
   std::string line;
-  while (std::getline(file, line)) {
+  while (std::getline(lines, line)) {
     if (trim(line).empty()) continue;
     Flashcard card("", "");
     if (card_from_csv(line, &card)) {
       cards_.push_back(card);
     }
   }
-  ensure_ids();
+
+  // Note that ids are deliberately *not* minted here; see Deck::ensure_id.
+  // Duplicate ids are likewise left alone, because a deck that is only read
+  // never uses them. import_into_deck resolves them, which is where they come
+  // from.
   return true;
 }
 
 bool Deck::save(std::string* error) const {
+  std::string content;
+  for (const auto& card : cards_) {
+    content += card_to_csv(card);
+    content += "\n";
+  }
+
+  // A write that would reproduce the file exactly is not a save. Every call
+  // site saves unconditionally -- after each answer, after each edit, and on
+  // the way out -- which is deliberate and is what makes an interrupted
+  // session cost nothing. The price was that opening a deck and closing it
+  // rewrote it, and for the decks in examples/ that meant a fresh clone could
+  // not be studied without the repository showing modified files.
+  //
+  // Cheaper than it looks, and cheaper than the write it replaces: the
+  // comparison is against a string already in memory, and the saves that do
+  // nothing are exactly the ones where the deck is large and untouched.
+  if (on_disk_known_ && content == on_disk_) return true;
+
   const std::string tmp_path = path_ + ".tmp";
 
   {
@@ -124,9 +191,7 @@ bool Deck::save(std::string* error) const {
       if (error) *error = "cannot write " + tmp_path + ": " + errno_message();
       return false;
     }
-    for (const auto& card : cards_) {
-      file << card_to_csv(card) << "\n";
-    }
+    file << content;
     file.flush();
     if (!file) {
       if (error) *error = "failed writing " + tmp_path;
@@ -141,6 +206,10 @@ bool Deck::save(std::string* error) const {
     std::remove(tmp_path.c_str());
     return false;
   }
+
+  // Only now, because a failed write leaves the file as it was.
+  on_disk_ = content;
+  on_disk_known_ = true;
   return true;
 }
 
@@ -161,6 +230,22 @@ void Deck::ensure_ids() {
   }
 }
 
+const std::string& Deck::ensure_id(Flashcard& card) {
+  if (!card.id.empty()) return card.id;
+
+  // Collected per call rather than kept as a member: this runs when a card is
+  // answered for the first time since the log existed, which is once per card
+  // in the life of a deck and never in a loop.
+  std::set<std::string> taken;
+  for (const auto& other : cards_) {
+    if (!other.id.empty()) taken.insert(other.id);
+  }
+  do {
+    card.id = generate_id();
+  } while (!taken.insert(card.id).second);
+  return card.id;
+}
+
 bool Deck::remove(std::size_t index) {
   if (index >= cards_.size()) return false;
   cards_.erase(cards_.begin() + static_cast<std::ptrdiff_t>(index));

src/deck.h

@@ -47,6 +47,12 @@ class Deck {
 
   // Writes to a temporary file and renames it into place, so an interrupted
   // or failed write can never leave a truncated deck behind.
+  //
+  // A write that would reproduce the file byte for byte is skipped and
+  // reported as success. Saving is unconditional at every call site -- after
+  // every answer, every edit, and on the way out -- which is what makes an
+  // interrupted session cost nothing; the cost is that merely opening a deck
+  // used to rewrite it. See the comment on the check itself.
   bool save(std::string* error = nullptr) const;
 
   const std::string& path() const { return path_; }
@@ -106,12 +112,36 @@ class Deck {
   // log existed, and duplicated ids arrive from re-importing an export into
   // the deck it came from — two cards sharing one history would make the log
   // ambiguous, so the newcomer is renamed rather than the incumbent.
+  //
+  // Deliberately not called by load(): see ensure_id.
   void ensure_ids();
 
+  // Gives one card an id if it has none, unique against the rest of the deck,
+  // and returns it.
+  //
+  // This exists so that load() does not have to mint ids for a whole deck just
+  // to open it. An id is what a log event names a card by, so a card needs one
+  // at the moment something is about to be recorded against it and not before
+  // — and a deck that is only read has nothing recorded against it at all.
+  // Minting on load meant that opening a deck changed it, which made studying
+  // a deck checked into a repository show up as a source change.
+  const std::string& ensure_id(Flashcard& card);
+
  private:
   std::string path_;
   std::vector<Flashcard> cards_;
   EventLog log_;
+
+  // What the file holds as of the last successful read or write, so that
+  // save() can tell a real change from a no-op. Mutable because save() is
+  // const to its callers and stays that way: what it records here is a fact
+  // about the file rather than about the deck.
+  //
+  // The flag is not the same question as an empty string: a deck file that
+  // does not exist yet still has to be created the first time it is saved,
+  // and an empty deck serialises to nothing.
+  mutable std::string on_disk_;
+  mutable bool on_disk_known_ = false;
 };
 
 // Appends the cards in `path` to `deck`, preserving review statistics when

src/generate.cpp

@@ -88,9 +88,10 @@ GenerateResult generate_audio(Deck& deck, const std::string& voice, bool force,
     return result;
   }
 
-  // A card is named by its id, so every card needs one before anything can be
-  // written. Loading a deck normally mints them; a deck loaded and immediately
-  // generated from may not have been through that yet.
+  // A recording is named after the card's id, so every card needs one before
+  // anything can be written. Loading a deck no longer mints them -- reading a
+  // deck leaves it alone, see Deck::ensure_id -- so this is where a whole
+  // deck's worth gets minted, and the deck is about to be written anyway.
   deck.ensure_ids();
 
   const std::string directory = deck.resolve(kAudioDirectory);

src/review.cpp

@@ -568,12 +568,16 @@ std::string log_event(Deck& deck, const ReviewEvent& event, bool* warned) {
   return event.id;
 }
 
-std::string log_answer(Deck& deck, const Flashcard& card, bool reversed,
+// Takes the card by mutable reference because writing an event is the moment
+// the card needs an id: a deck written before the log existed has none, and
+// ensure_id mints one here rather than on load so that reading a deck leaves
+// it alone.
+std::string log_answer(Deck& deck, Flashcard& card, bool reversed,
                        Outcome outcome, const AnswerResult& result,
                        bool* warned) {
   ReviewEvent event;
   event.id = generate_id();
-  event.card_id = card.id;
+  event.card_id = deck.ensure_id(card);
   event.timestamp = now_timestamp();
   event.direction = reversed ? 'r' : 'n';
   event.outcome = outcome;
@@ -582,11 +586,13 @@ std::string log_answer(Deck& deck, const Flashcard& card, bool reversed,
   return log_event(deck, event, warned);
 }
 
-void log_undo(Deck& deck, const Flashcard& card, const std::string& answer_id,
+// The card has an id by now, having just been answered; ensure_id is asked
+// anyway rather than assuming the order of two calls in another function.
+void log_undo(Deck& deck, Flashcard& card, const std::string& answer_id,
               bool* warned) {
   ReviewEvent event;
   event.id = generate_id();
-  event.card_id = card.id;
+  event.card_id = deck.ensure_id(card);
   event.timestamp = now_timestamp();
   event.undoes = answer_id;
   log_event(deck, event, warned);

tests/golden/cases/eof-mid-session/expected

@@ -20,4 +20,4 @@ Input stream closed/EOF. Flashcards saved. Goodbye!
 --- exit status ---
 0
 --- file deck.txt ---
-Bonjour,Hello,french,0,0,1,,,<ID1>
+Bonjour,Hello,french

tests/golden/cases/export-and-reimport/expected

@@ -53,5 +53,5 @@ Merci,Thanks|Thank you,french,0,0,1,,,<ID2>
 Bonjour,Hello,french;greeting,0,0,1,,,<ID3>
 Merci,Thanks|Thank you,french,0,0,1,,,<ID4>
 --- file out.csv ---
-Bonjour,Hello,french;greeting,0,0,1,,,<ID1>
-Merci,Thanks|Thank you,french,0,0,1,,,<ID2>
+Bonjour,Hello,french;greeting
+Merci,Thanks|Thank you,french

tests/golden/cases/manage-list-and-search/expected

@@ -65,5 +65,5 @@ Nothing matches "zzz".
 --- exit status ---
 0
 --- file deck.txt ---
-Bonjour,Hello,french;greeting,0,0,1,,,<ID1>
-Merci,Thanks|Thank you,french,0,0,1,,,<ID2>
+Bonjour,Hello,french;greeting
+Merci,Thanks|Thank you,french

tests/golden/cases/progress/expected

@@ -53,5 +53,5 @@ Merci                                    0         0      0.00    1     new
 --- exit status ---
 0
 --- file deck.txt ---
-Bonjour,Hello,french;greeting,0,0,1,,,<ID1>
-Merci,Thanks|Thank you,french,0,0,1,,,<ID2>
+Bonjour,Hello,french;greeting
+Merci,Thanks|Thank you,french

tests/golden/cases/review-quit-midway/expected

@@ -62,4 +62,4 @@ Your answer: ==================================================
 --- exit status ---
 0
 --- file deck.txt ---
-Bonjour,Hello,french,0,0,1,,,<ID1>
+Bonjour,Hello,french