Close bank-export review findings: name-cap underflow, double overwrite prompt, test scope
Clamps insertSuffix's underflow, floors uniqueEntryName's validity guard, suppresses the redundant overwrite confirm via a picker out-param, adds a PayloadBuffer high-water mark, and corrects stale CLAUDE.md/CMake claims.
This commit is contained in:
@@ -164,7 +164,8 @@ void doBankPackageExport(const ReaSamplerSession& session, const std::string& ba
|
||||
const std::string suggested =
|
||||
projectDir + "/" + capture::sanitizeStem(bankName) + ".rsbank";
|
||||
std::string dest;
|
||||
if (!pickPackageSavePath(suggested, dest)) return; // user cancelled the picker
|
||||
bool appended = false;
|
||||
if (!pickPackageSavePath(suggested, dest, &appended)) return; // user cancelled the picker
|
||||
|
||||
ExportRequest req;
|
||||
req.projectDir = projectDir;
|
||||
@@ -172,6 +173,11 @@ void doBankPackageExport(const ReaSamplerSession& session, const std::string& ba
|
||||
req.destAbsPath = dest;
|
||||
req.exportTimestamp = static_cast<std::int64_t>(std::time(nullptr));
|
||||
req.allowIncomplete = allowIncomplete;
|
||||
// The dialog's own overwrite confirm covered exactly this path when the picker
|
||||
// did not need to append `.rsbank` to reach it — asking again would be a second
|
||||
// prompt for the same consent. An appended path is one the dialog never saw, so
|
||||
// that case still falls through to exportBank's own refusal and the confirm below.
|
||||
req.allowOverwrite = !appended;
|
||||
|
||||
ExportOutcome out = exportBank(session, req);
|
||||
if (out.status == ExportStatus::RefusedDestinationExists) {
|
||||
|
||||
+19
-19
@@ -5,12 +5,12 @@
|
||||
The filesystem and dialog acts behind bank-package export/import: streaming package
|
||||
file I/O plus the file-status and exclusive-create acts (`package_io`), the
|
||||
UTF-8 path conversion every one of them goes through (`package_path`), the landed-file
|
||||
journal and its rollback delete (`package_rollback`), and the two file pickers
|
||||
(`package_pickers`). This seam is bytes-only — the package format (magic, manifest,
|
||||
entry layout) is `core/package`'s business, and the export/import verbs that
|
||||
orchestrate both do not live here yet. No REAPER project state is touched in this
|
||||
directory: no ext-state read or write, no undo block, no generation bump — those
|
||||
belong to the verbs.
|
||||
journal and its rollback delete (`package_rollback`), the two file pickers
|
||||
(`package_pickers`), and the promptless export verb (`export_bank`) — the import verb
|
||||
does not live here yet. The package format itself (magic, manifest, entry layout)
|
||||
stays `core/package`'s business. No REAPER project state is touched in this directory:
|
||||
no ext-state read or write, no undo block, no generation bump — those belong to the
|
||||
prompting skin (`shell/actions/package_export_action`), not this seam.
|
||||
|
||||
## Invariants
|
||||
|
||||
@@ -47,17 +47,17 @@ belong to the verbs.
|
||||
writer could win the race against. Collision handling (auto-rename) remains the
|
||||
import plan's job upstream. The package writer itself DOES replace an existing
|
||||
destination — the export save dialog's own overwrite confirm is the consent — and
|
||||
that asymmetry is deliberate. **Known gap, obligation on the export verb:**
|
||||
`pickPackageSavePath`'s own `.rsbank` re-append (see its Gotcha below) can turn a
|
||||
confirmed path `X` into a write target `X.rsbank` that the dialog never asked about.
|
||||
The export verb MUST re-check `fileStatus()` on the path actually handed to
|
||||
`PackageFileWriter` — after any extension append — and get its own consent if that
|
||||
re-checked path is `Present`; the dialog's confirm only ever covered the pre-append
|
||||
path. Not fixed at this seam: prompting is verb-level UX, and `pickPackageSavePath`
|
||||
has no caller yet, so the gap is latent, not live. **Closed on the export side:**
|
||||
`exportBank` re-checks `fileStatus()` on the post-append path and refuses
|
||||
`RefusedDestinationExists` until the caller sets `allowOverwrite`, which
|
||||
`package_export_action` does only after its own confirm naming that exact path.
|
||||
that asymmetry is deliberate. **Closed, both halves.** `pickPackageSavePath`'s own
|
||||
`.rsbank` re-append (see its Gotcha below) can turn a confirmed path `X` into a write
|
||||
target `X.rsbank` that the dialog never asked about, so `exportBank` re-checks
|
||||
`fileStatus()` on the path actually handed to `PackageFileWriter` — after any
|
||||
extension append — and refuses `RefusedDestinationExists` until the caller sets
|
||||
`allowOverwrite`. The caller does not always re-prompt to get there:
|
||||
`pickPackageSavePath` reports whether it appended (`outAppended`), and
|
||||
`package_export_action` pre-grants `allowOverwrite` whenever it did NOT — an
|
||||
unappended path is exactly what the dialog's own confirm already covered, so asking
|
||||
again would be a second prompt for the same consent. Only an appended path, one the
|
||||
dialog never saw, still costs the verb's own confirm naming that exact path.
|
||||
- **The rollback delete is prune's ONE carve-out, and only HALF of it is structural.**
|
||||
The citation and the full discriminator live at `package_rollback.cpp`'s header.
|
||||
"Did this call create it" is structural: only exclusively-created paths are
|
||||
@@ -79,8 +79,8 @@ belong to the verbs.
|
||||
- `package_path` — header-only; the ONE UTF-8-narrow → `fs::path` conversion, so the encoding contract has a single enforcement point.
|
||||
- `package_io` — every filesystem act the verbs need: `PayloadBuffer` (move-only payload + the `alive()` seam counter), `PackageFileWriter` (append-only temp+atomic-rename writer), `PackageFileReader` (seek-and-read one range per call, range-checked against the real file size), `readFilePayload` (one source file as one entry's payload), `fileStatus` (Present/Absent/Unreadable — export's refusal message must distinguish the last two, and an empty payload cannot), `writeFileExclusive` (exclusive create + write, self-cleaning on a partial write), and `listFolderFileNames` (bare UTF-8 names, sorted, non-recursive, non-throwing). REAPER-free; tested without a DAW.
|
||||
- `package_rollback` — `LandedFileJournal`: `writeLandedFile` (exclusive-create land, path resolved absolute, recorded on success only), `markIndexCommitted` (disarms the journal), and `rollback` (deletes exactly the recorded set, hard unlink, tolerating a vanished file; refuses once disarmed). REAPER-free; tested without a DAW.
|
||||
- `package_pickers` — `pickPackageForImport` and `pickPackageSavePath`, both `GetUserFileName`. Compile-only until the verbs land; nothing here can be exercised in a unit test.
|
||||
- `export_bank` — the promptless export verb, in three composable public steps: `surveyBankExport` (the read-only plan, report-before-acting), `digestSources` (measures each entry's length + `hashBytes` digest, one payload at a time), and `writePackageFile` (prefix, then each payload re-read and re-verified against that digest before it is appended, then commit). `exportBank` composes the three and gates on the plan verdict, the incomplete confirm and the destination confirm. The session arrives **const** — every mutator on it is non-const, so "an export writes no ext state, opens no undo point and never bumps the generation" is enforced by the type rather than remembered. Reads the session through inline accessors only, which is why its tests link and run without a DAW.
|
||||
- `package_pickers` — `pickPackageForImport` and `pickPackageSavePath`, both `GetUserFileName`; `pickPackageSavePath` also reports whether it appended `.rsbank` (`outAppended`), the signal `package_export_action` uses to skip a redundant overwrite confirm. `pickPackageForImport` stays compile-only until the import verb lands; neither picker can be exercised in a unit test.
|
||||
- `export_bank` — the promptless export verb, in three composable public steps: `surveyBankExport` (the read-only plan, report-before-acting), `digestSources` (measures each entry's length + `hashBytes` digest, one payload at a time), and `writePackageFile` (prefix, then each payload re-read and re-verified against that digest before it is appended, then commit). `exportBank` composes the three and gates on the plan verdict, the incomplete confirm and the destination confirm. The session arrives **const**: `saveToActiveProject`, `bumpBankGeneration` and `writeAssignmentRequest` are the session's only non-const acts, so a const session cannot reach them and "an export writes no ext state, opens no undo point and never bumps the generation" holds by the type rather than by memory (`pruneReclaim`, the sole file-deletion path, is const too and sits outside this claim). Reads the session through inline accessors only, which is why its tests link and run without a DAW.
|
||||
|
||||
## Gotchas
|
||||
|
||||
|
||||
@@ -1,7 +1,8 @@
|
||||
# The filesystem + dialog seam for bank packages. package_io / package_rollback are
|
||||
# REAPER-free (standard filesystem only), so the pure-library/test helpers fit and
|
||||
# their tests run without a DAW. The export/import verbs that drive all three targets
|
||||
# are not in this directory yet.
|
||||
# their tests run without a DAW. export_bank, the promptless export verb, lives here
|
||||
# too for the same reason (ReaSamplerSession's inline accessors keep it REAPER-free);
|
||||
# the import verb does not live here yet.
|
||||
|
||||
reasampler_pure_library(package_io SOURCES package_io.cpp)
|
||||
reasampler_test(package_io LINK package_io)
|
||||
@@ -11,9 +12,15 @@ reasampler_test(package_rollback LINK package_rollback)
|
||||
|
||||
# export_bank reads the live session through ReaSamplerSession's INLINE accessors only,
|
||||
# so it pulls in no REAPER-facing TU and its tests link (and run) without a DAW.
|
||||
# bank_book / tail_control / origin_ledger / tracking_authority / prune_reconcile /
|
||||
# app_version / view_mode_model are session.h's own transitive includes (BankBook::bank()
|
||||
# in particular is out-of-line, in bank_book.cpp) — declared here, on the library that
|
||||
# actually needs them, rather than left for every consumer to enumerate.
|
||||
reasampler_pure_library(export_bank
|
||||
SOURCES export_bank.cpp
|
||||
LINK PUBLIC export_plan bank_package package_io PRIVATE capture_paths wav_codec)
|
||||
LINK PUBLIC export_plan bank_package package_io
|
||||
PRIVATE capture_paths wav_codec bank_book tail_control origin_ledger
|
||||
tracking_authority prune_reconcile app_version view_mode_model)
|
||||
reasampler_test(export_bank
|
||||
LINK export_bank bank_book slot_map view_mode_model tail_control origin_ledger
|
||||
tracking_authority prune_reconcile app_version capture_paths)
|
||||
|
||||
@@ -2,8 +2,12 @@
|
||||
// stream, commit. No prompts and no message boxes (shell/actions/
|
||||
// package_export_action is the skin). The session arrives CONST, which is how "an
|
||||
// export writes no ext state, opens no undo point and never bumps the bank
|
||||
// generation" is enforced rather than remembered — every mutator on the session is
|
||||
// non-const. Blocking I/O: UI-thread actions only.
|
||||
// generation" is enforced rather than remembered — those three acts
|
||||
// (saveToActiveProject, bumpBankGeneration, writeAssignmentRequest) are exactly the
|
||||
// session members that are non-const (session.h:113,108,147). Constness does not
|
||||
// block every mutation, though: pruneReclaim (session.h:140-141) is const and is
|
||||
// the system's sole file-deletion path — irrelevant to export, but not something a
|
||||
// const session forbids in general. Blocking I/O: UI-thread actions only.
|
||||
|
||||
#pragma once
|
||||
|
||||
|
||||
@@ -29,6 +29,7 @@ namespace fs = std::filesystem;
|
||||
|
||||
namespace {
|
||||
std::atomic<int> g_alivePayloads{0};
|
||||
std::atomic<int> g_peakAlivePayloads{0};
|
||||
}
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
@@ -36,7 +37,13 @@ std::atomic<int> g_alivePayloads{0};
|
||||
|
||||
PayloadBuffer::PayloadBuffer(std::vector<std::uint8_t> bytes)
|
||||
: bytes_(std::move(bytes)), counted_(!bytes_.empty()) {
|
||||
if (counted_) g_alivePayloads.fetch_add(1, std::memory_order_relaxed);
|
||||
if (counted_) {
|
||||
const int now = g_alivePayloads.fetch_add(1, std::memory_order_relaxed) + 1;
|
||||
int peak = g_peakAlivePayloads.load(std::memory_order_relaxed);
|
||||
while (now > peak && !g_peakAlivePayloads.compare_exchange_weak(
|
||||
peak, now, std::memory_order_relaxed)) {
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
PayloadBuffer::~PayloadBuffer() { release(); }
|
||||
@@ -60,6 +67,7 @@ PayloadBuffer& PayloadBuffer::operator=(PayloadBuffer&& other) noexcept {
|
||||
}
|
||||
|
||||
int PayloadBuffer::alive() { return g_alivePayloads.load(std::memory_order_relaxed); }
|
||||
int PayloadBuffer::highWaterMark() { return g_peakAlivePayloads.load(std::memory_order_relaxed); }
|
||||
|
||||
void PayloadBuffer::release() {
|
||||
if (counted_) g_alivePayloads.fetch_sub(1, std::memory_order_relaxed);
|
||||
|
||||
@@ -33,6 +33,11 @@ public:
|
||||
|
||||
// Buffers currently holding at least one byte, process-wide.
|
||||
static int alive();
|
||||
// The largest alive() has ever been, process-wide. A point-in-time alive() == 0
|
||||
// check after a call returns cannot fail on a whole-package-in-memory shape that
|
||||
// allocated N buffers and freed them all one at a time — highWaterMark() can,
|
||||
// since it is never reset.
|
||||
static int highWaterMark();
|
||||
|
||||
private:
|
||||
void release();
|
||||
|
||||
@@ -47,7 +47,8 @@ bool pickPackageForImport(std::string& outAbsPath) {
|
||||
return runPicker(1, "Import bank package", "", outAbsPath);
|
||||
}
|
||||
|
||||
bool pickPackageSavePath(const std::string& suggestedPath, std::string& outAbsPath) {
|
||||
bool pickPackageSavePath(const std::string& suggestedPath, std::string& outAbsPath,
|
||||
bool* outAppended) {
|
||||
// [verify — DAW] GetUserFileName takes no owner window, so the dialog's parenting
|
||||
// is REAPER's to do; the previous Win32 path passed GetMainHwnd() explicitly.
|
||||
if (!runPicker(0, "Export bank package", suggestedPath.c_str(), outAbsPath)) {
|
||||
@@ -56,7 +57,9 @@ bool pickPackageSavePath(const std::string& suggestedPath, std::string& outAbsPa
|
||||
// GetUserFileName has no lpstrDefExt equivalent (the old Win32 picker's
|
||||
// ofn.lpstrDefExt = L"rsbank"); whether mode 0 appends one itself from
|
||||
// kExtList is [verify — DAW], so append it ourselves whenever it's missing.
|
||||
if (!hasCaseInsensitiveSuffix(outAbsPath, ".rsbank")) outAbsPath += ".rsbank";
|
||||
const bool appended = !hasCaseInsensitiveSuffix(outAbsPath, ".rsbank");
|
||||
if (appended) outAbsPath += ".rsbank";
|
||||
if (outAppended) *outAppended = appended;
|
||||
return true;
|
||||
}
|
||||
|
||||
|
||||
@@ -18,7 +18,12 @@ bool pickPackageForImport(std::string& outAbsPath);
|
||||
// suggestedPath is a bare file name ("MyBank.rsbank") or a full path — a full one
|
||||
// also seeds the dialog's starting directory, which is how a caller keeps the picker
|
||||
// off REAPER's process working directory. True with outAbsPath set iff the user chose
|
||||
// a destination; the dialog's own overwrite confirm has already run by then.
|
||||
bool pickPackageSavePath(const std::string& suggestedPath, std::string& outAbsPath);
|
||||
// a destination; the dialog's own overwrite confirm has already run by then, against
|
||||
// the path the user actually chose — NOT necessarily outAbsPath, if the `.rsbank`
|
||||
// re-append below fires. outAppended, when non-null, is set to whether it fired: the
|
||||
// caller's signal that its own overwrite consent may not cover the returned path
|
||||
// (see this directory's CLAUDE.md).
|
||||
bool pickPackageSavePath(const std::string& suggestedPath, std::string& outAbsPath,
|
||||
bool* outAppended = nullptr);
|
||||
|
||||
} // namespace reasampler
|
||||
|
||||
@@ -251,7 +251,7 @@ enum : unsigned int {
|
||||
} // namespace
|
||||
|
||||
// Shows the right-click context menu for a named-bank TAB: activate / rename / delete
|
||||
// / evacuate that bank, plus a create entry. Drives the id-keyed ops.
|
||||
// / evacuate / export that bank, plus a create entry. Drives the id-keyed ops.
|
||||
void showTabMenu(int screenX, int screenY, const std::string& bankId) {
|
||||
if (!book()) return;
|
||||
const Bank* bk = book()->bank(bankId);
|
||||
|
||||
Reference in New Issue
Block a user