From 4305940715b0b66a116c19a07c0a41cd7d027a53 Mon Sep 17 00:00:00 2001 From: Aduneer <249940941+Aduneer@users.noreply.github.com> Date: Thu, 20 Aug 2026 10:20:07 +0200 Subject: [PATCH] Stop opening a deck from rewriting it Studying any deck in examples/ straight from a clone left it modified in git status without a single card having been answered. That looks like the app corrupting its own sample data, and it is an easy way to get surprise diffs. Two causes, both of which had to go. Cards were 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. Deck::ensure_id mints one there instead. The id still exists before the event naming it is written, which is the invariant that matters. Saving then 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. card_to_csv now writes a card only as far as the last column it actually uses. That rule already existed for the audio and image columns for exactly this reason; it now covers the whole row instead of the last two, and the defaults it trims against are the ones card_from_csv fills in for a column that is absent, so the round trip is unchanged. A save that would reproduce the file byte for byte is skipped outright. Every call site saves unconditionally, which is what makes an interrupted session cost nothing; it should not also mean that reading a deck counts as writing it. Six golden transcripts change, all of them cases that browse, tag, list or quit without answering, and all of them in the same direction: the deck fixture stays as it was written. Cases that answer a card are untouched, and still show the deck's id matching the log's card_id. --- CHANGELOG.md | 29 ++++ README.md | 22 ++- src/deck.cpp | 137 ++++++++++++++---- src/deck.h | 30 ++++ src/generate.cpp | 7 +- src/review.cpp | 14 +- tests/golden/cases/eof-mid-session/expected | 2 +- .../golden/cases/export-and-reimport/expected | 4 +- .../cases/manage-list-and-search/expected | 4 +- tests/golden/cases/progress/expected | 4 +- .../golden/cases/review-quit-midway/expected | 2 +- tests/golden/cases/tags/expected | 4 +- tests/tests.cpp | 92 +++++++++++- 13 files changed, 296 insertions(+), 55 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5be6ef6..38a98cd 100644 --- a/CHANGELOG.md +++ b/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 — diff --git a/README.md b/README.md index 206ae55..5a0d94d 100644 --- a/README.md +++ b/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 diff --git a/src/deck.cpp b/src/deck.cpp index 56e1ebf..f9d62c3 100644 --- a/src/deck.cpp +++ b/src/deck.cpp @@ -6,6 +6,7 @@ #include #include #include +#include #include #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 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(index)); diff --git a/src/deck.h b/src/deck.h index 08c628c..95bff8a 100644 --- a/src/deck.h +++ b/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 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 diff --git a/src/generate.cpp b/src/generate.cpp index 5509d46..bd15031 100644 --- a/src/generate.cpp +++ b/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); diff --git a/src/review.cpp b/src/review.cpp index c13dacb..21462b6 100644 --- a/src/review.cpp +++ b/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); diff --git a/tests/golden/cases/eof-mid-session/expected b/tests/golden/cases/eof-mid-session/expected index 35eeff7..38b7be5 100644 --- a/tests/golden/cases/eof-mid-session/expected +++ b/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,,, +Bonjour,Hello,french diff --git a/tests/golden/cases/export-and-reimport/expected b/tests/golden/cases/export-and-reimport/expected index 3593571..e817098 100644 --- a/tests/golden/cases/export-and-reimport/expected +++ b/tests/golden/cases/export-and-reimport/expected @@ -53,5 +53,5 @@ Merci,Thanks|Thank you,french,0,0,1,,, Bonjour,Hello,french;greeting,0,0,1,,, Merci,Thanks|Thank you,french,0,0,1,,, --- file out.csv --- -Bonjour,Hello,french;greeting,0,0,1,,, -Merci,Thanks|Thank you,french,0,0,1,,, +Bonjour,Hello,french;greeting +Merci,Thanks|Thank you,french diff --git a/tests/golden/cases/manage-list-and-search/expected b/tests/golden/cases/manage-list-and-search/expected index 3bc8d3f..d59a07c 100644 --- a/tests/golden/cases/manage-list-and-search/expected +++ b/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,,, -Merci,Thanks|Thank you,french,0,0,1,,, +Bonjour,Hello,french;greeting +Merci,Thanks|Thank you,french diff --git a/tests/golden/cases/progress/expected b/tests/golden/cases/progress/expected index 4d65242..b661f9e 100644 --- a/tests/golden/cases/progress/expected +++ b/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,,, -Merci,Thanks|Thank you,french,0,0,1,,, +Bonjour,Hello,french;greeting +Merci,Thanks|Thank you,french diff --git a/tests/golden/cases/review-quit-midway/expected b/tests/golden/cases/review-quit-midway/expected index 3767b07..7c2bc27 100644 --- a/tests/golden/cases/review-quit-midway/expected +++ b/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,,, +Bonjour,Hello,french diff --git a/tests/golden/cases/tags/expected b/tests/golden/cases/tags/expected index bb051fa..14809b3 100644 --- a/tests/golden/cases/tags/expected +++ b/tests/golden/cases/tags/expected @@ -31,5 +31,5 @@ Loaded 2 flashcards from deck.txt --- exit status --- 0 --- file deck.txt --- -Bonjour,Hello,french;greeting,0,0,1,,, -Merci,Thanks|Thank you,french,0,0,1,,, +Bonjour,Hello,french;greeting +Merci,Thanks|Thank you,french diff --git a/tests/tests.cpp b/tests/tests.cpp index 4e4b8c2..1024a8f 100644 --- a/tests/tests.cpp +++ b/tests/tests.cpp @@ -57,6 +57,16 @@ std::string temp_path(const std::string& name) { return "build/test-" + name; } +// Whole-file, bytes as they are: a test about whether a file was rewritten +// cannot read it a line at a time, since that is exactly what hides a +// difference in trailing newlines. +std::string file_contents(const std::string& path) { + std::ifstream file(path); + std::ostringstream buffer; + buffer << file.rdbuf(); + return buffer.str(); +} + void test_trim_and_case() { EXPECT_EQ(trim(" hello "), std::string("hello")); EXPECT_EQ(trim("\t\r\n"), std::string("")); @@ -1977,7 +1987,9 @@ void test_card_ids() { EXPECT_EQ(log, path + ".log"); - // A deck written before ids existed gets them on load, and they stick. + // A deck written before ids existed keeps its cards id-less on load, because + // reading a deck does not change it. One is minted when something is about + // to be recorded against the card, and then it sticks. { std::ofstream file(path); file << "Q1,A1,tag,3,1,4,2026-08-01,2026-08-15\n" @@ -1986,11 +1998,17 @@ void test_card_ids() { Deck deck(path); EXPECT_TRUE(deck.load()); EXPECT_EQ(deck.size(), size_t{2}); - EXPECT_TRUE(!deck.cards()[0].id.empty()); - EXPECT_TRUE(!deck.cards()[1].id.empty()); - EXPECT_TRUE(deck.cards()[0].id != deck.cards()[1].id); + EXPECT_TRUE(deck.cards()[0].id.empty()); + EXPECT_TRUE(deck.cards()[1].id.empty()); + + const std::string first_id = deck.ensure_id(deck.cards()[0]); + EXPECT_TRUE(!first_id.empty()); + // Asking again returns the same one rather than minting a second. + EXPECT_EQ(deck.ensure_id(deck.cards()[0]), first_id); + // Its neighbour is left alone until it is asked for, and then differs. + EXPECT_TRUE(deck.cards()[1].id.empty()); + EXPECT_TRUE(deck.ensure_id(deck.cards()[1]) != first_id); - const std::string first_id = deck.cards()[0].id; EXPECT_TRUE(deck.save()); Deck reloaded(path); EXPECT_TRUE(reloaded.load()); @@ -2026,6 +2044,69 @@ void test_card_ids() { std::remove(exported.c_str()); } +// Opening a deck and closing it must leave the file exactly as it was. The +// decks in examples/ are the reason: they are checked in, they are written in +// the short three-column form, and studying one used to rewrite every row of +// it into the full nine -- so a fresh clone could not be tried out without the +// repository showing modified files. +void test_save_leaves_an_unchanged_deck_alone() { + const std::string path = temp_path("unchanged.txt"); + std::remove(path.c_str()); + + // The short form, exactly as a deck in examples/ is written. + const std::string original = + "la biblioteca,library,spanish;nouns\n" + "el árbol,tree,spanish;nouns\n"; + { + std::ofstream file(path); + file << original; + } + + Deck deck(path); + EXPECT_TRUE(deck.load()); + EXPECT_EQ(deck.size(), size_t{2}); + // Saving reports success without having touched the file. + EXPECT_TRUE(deck.save()); + EXPECT_EQ(file_contents(path), original); + + // Repeatedly, since every answer and every menu exit saves. + EXPECT_TRUE(deck.save()); + EXPECT_TRUE(deck.save()); + EXPECT_EQ(file_contents(path), original); + + // A real change is still written -- and only as far as the column that + // carries it, so the card that did not change keeps its short row and the + // one that did grows by exactly the three columns it now needs. + deck.cards()[0].leitner_box = 3; + EXPECT_TRUE(deck.save()); + EXPECT_EQ(file_contents(path), + std::string("la biblioteca,library,spanish;nouns,0,0,3\n" + "el árbol,tree,spanish;nouns\n")); + + // ...and having written it, the deck knows it is current again. + const std::string written = file_contents(path); + EXPECT_TRUE(deck.save()); + EXPECT_EQ(file_contents(path), written); + + // A deck file that does not exist yet is created rather than skipped, even + // when the deck is empty and so serialises to nothing at all. The flag and + // an empty string are not the same question. + const std::string fresh = temp_path("unchanged-new.txt"); + std::remove(fresh.c_str()); + Deck new_deck(fresh); + EXPECT_TRUE(!new_deck.load()); // no such file, which is not an error + EXPECT_TRUE(new_deck.save()); + { + std::ifstream check(fresh); + EXPECT_TRUE(check.is_open()); + } + + std::remove(path.c_str()); + std::remove(fresh.c_str()); + std::remove((path + ".log").c_str()); + std::remove((fresh + ".log").c_str()); +} + void test_wrap() { // Short text is one line, and empty text is still one line so a frame drawn // from the result is never zero rows tall. @@ -2291,6 +2372,7 @@ int main() { test_shipped_example_decks(); test_summarize(); test_card_ids(); + test_save_leaves_an_unchanged_deck_alone(); test_wrap(); test_partial_answers(); test_partial_events();