chore: remove the naming machinery v0.10.0 left behind - #116
Merged
Conversation
Release v0.10.0
`NamePlan` has been the identity since extraction stopped renaming entries, so every `extract_*_planned` was its plain counterpart with an extra argument and a lookup that always answered `None`. The type, the three variants and the wrappers that called them are gone; `extract_zip`, `extract_7z` and `extract_tar` are now the functions themselves. The one that mattered is tar. It carried **two** write paths, because `unpack_in` derives the destination from the entry's own name and so cannot write a renamed entry at all — and `unpack_in` is also the traversal guard tar has always used, the canonicalizing containment check that stops a write from following a symlink already sitting in the output directory. The renamed branch therefore had to repeat that check by hand. With nothing renamed, that branch and its hand-rolled containment check are both gone, and every entry goes through `unpack_in` again. No behaviour changes and no test changes: 506 passing before, 506 passing after. That is the point of doing this separately from anything that alters what the code does. The explicit `..` refusal before `unpack_in` stays. It existed because the renamed branch had no other guard, and it is now belt and braces — but it is also what names the offending entry, which `unpack_in`'s own `Ok(false)` does not.
`adjustments`, `Adjustment` and `Outcome::Extracted { adjusted }` are gone. They
reported the names extraction had rewritten to fit the host, and extraction
stopped rewriting names in v0.10.0, so the list was provably empty at the only
place it was built.
**More seriously, the refusal message was lying, and shipped that way.** Two of
its sentences described the old scheme:
Going ahead needs a replacement for '?' (1 entry), and this command cannot
ask for them mid-run without becoming interactive: the Collapse desktop app
asks once per character, checks the answer is writable too, and extracts
with it.
The names above with nothing to replace are adjusted for you once it can go
ahead.
Neither is true. There is no replacement to give, and the desktop dialog it
points at collects answers that reach nothing. A message that sends someone to a
feature that no longer works is worse than one that says less, so it now says
what is actually the case: nothing was extracted, no option would change that,
extract on a system that can hold the names.
`explain` had the same problem in two lines — a trailing run "would be dropped"
and a device name "would be written under an adjusted name". Both described
adjustments that no longer happen.
The characters are still listed with their counts, and deliberately: there is no
answer to give, but one character across forty entries is an archive that is
awkward here, while forty characters is one that does not belong on this machine
at all, and that is the user's call.
The two tests that pinned the old sentences are rewritten rather than deleted —
one of them now asserts the dead pointer is *absent*, so it cannot come back.
506 passing, unchanged.
The whole conversation is gone: `unwritable_names`, the `names` module that shaped its answer for the webview, the sheet in `App.vue` with its fields, validation and adjustment notes, `src/names.js`, and the `replacements` argument `extract_archive` took. It had stopped being a conversation. Extraction refuses a name it cannot write rather than asking what to call it, so the dialog collected answers that reached nothing and then reported a refusal the user could not act on — the worst shape a question can have. `Extraction` goes with it. A naming refusal had a variant of its own so the sheet could stay open on it while everything else went to the error banner; with no sheet there is one destination for both, so `extract_archive` returns `Result<Vec<String>, String>` like every other command. **Four places had to move together**, which is what `tests/ipc.rs` exists to police: the `generate_handler!` list, the command itself, the `invoke` call in `App.vue`, and the stub switch in `tests/App.test.js`. Its `BASELINE` and signature table are updated too, so the five-command surface is now four and nothing can quietly become untyped. Tests: `tests/names.rs` is rewritten rather than deleted, down from eighteen cases to four, because one guarantee in it survives and is worth more now that it is the only one — a refusal must reach the user **with its reason attached**. A NUL prints as nothing, so an entry named without its fault looks perfectly fine, and a user told "extraction failed" would think the archive is broken when it is merely foreign. Ten dialog cases went from `App.test.js`, and the two that remain assert a refusal and an ordinary failure both land in the banner. 101 Rust tests and 43 Vitest cases pass.
With both front ends off it, the rest goes: `Substitutions` and its `FromIterator`, `ExtractOptions::with_replacements` and `replacements`, `NameRules::rewrite`, `rewrite_entry`, `check_replacements` and `check_replacement`, `DEVICE_SUFFIX`, `NameProblem::replaceable`, `NameRules::offending_characters`, and six of `NameError`'s seven variants with the `in_entry` helper that re-pointed them. `NameError` is one variant now, `Unwritable`, which is the only one anything constructed. The other six described a negotiation that no longer happens: no replacement to be missing, unwritable or separator-bearing, no rewrite to make two entries collide, no adjustment to empty a component, no answer key to be more than one character. `NameReport` stays, and so does its `characters` half. It is no longer a questionnaire — nothing can be answered — but it is what the CLI groups its refusal by, and one character across forty entries is a different problem from forty characters. Tests: eleven cases in `apps/core/tests/names.rs` exercised the removed machinery and go with it. `an_entry_splits_the_same_way_on_every_host` goes too, for a different reason — it used `rewrite_entry` only as a way to observe where a name splits, and that guarantee is already held by `the_report_sees_a_colon_in_the_first_component_too` and `only_windows_refuses_a_backslash_inside_a_component`, both of which survive. `the_report_separates_the_questions_from_the_stated_adjustments` is rewritten around what `characters` still means. In `security.rs`, `a_replacement_cannot_carry_an_entry_out_of_the_output_ directory` is replaced by a comment in its place rather than removed silently. It proved a hostile answer like `../../escape` could not move an entry out of the output directory. That hole is now closed by construction instead of by a check — there is no way to supply a replacement, so the code that would fail the test does not compile — and the guarantee is still one this crate makes, so it is worth someone finding the reasoning where the test used to be. 494 passing across the root workspace, down from 506 by exactly the twelve that tested machinery that no longer exists.
`architecture.md` and `desktop.md` described `Substitutions`, `NamePlan`, the `extract_*_planned` variants and the desktop's `unwritable_names` command as inert surface awaiting removal. They have been removed, so the documents now describe what is there. Two facts worth keeping rather than just deleting: `NameError` is down to one variant because the other six described a negotiation that no longer happens, and tar went from two write paths to one — the renamed branch had to repeat by hand the containment check `unpack_in` performs, and both went together. The desktop's command surface is four, not five.
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Takes out the machinery v0.10.0 left behind when extraction stopped renaming
entries to fit the host. Four commits, each one green on its own, because the
first is a pure deletion and the rest change what users see.
What went
NamePlan, the threeextract_*_plannedvariants, tar's second write path,Substitutions,ExtractOptions::with_replacements,NameRules::rewrite/rewrite_entry/check_replacements,DEVICE_SUFFIX, six ofNameError's seven variantsadjustments,Adjustment,Outcome::Extracted { adjusted }unwritable_namescommand,src-tauri/src/names.rs,src/names.js, the naming sheet inApp.vue,extract_archive'sreplacementsargument,ExtractionThe two findings that are not just deletion
The CLI's refusal message was lying, and shipped that way in v0.10.0. It
told users a replacement would unblock the extraction and pointed them at the
desktop dialog as the front end that could ask for one:
There is no replacement to give and that dialog collects answers that reach
nothing.
explainhad the same problem in two more lines. The message now sayswhat is true: nothing was extracted, no option would change that, extract on a
system that can hold the names. A test asserts the dead pointer is absent,
so it cannot come back.
Tar carried two write paths, and the dead one had its own copy of a security
check.
unpack_inderives the destination from the entry's own name, so itcould not write a renamed entry — and
unpack_inis also the traversal guard,the canonicalizing containment check that stops a write following a symlink
already in the output. The renamed branch therefore repeated that check by
hand. Both are gone and every entry goes through
unpack_inagain.Tests
Verified locally at every step, not just at the end —
cargois on this machinenow, so each commit was run before the next was started.
src-tauri).make testis green end to end.The root workspace goes 506 → 494 and the desktop suite 18 → 4 naming cases.
Every one of those tested machinery that no longer exists. Two removals are
recorded rather than done silently:
apps/desktop/src-tauri/tests/names.rskeeps four of eighteen cases, becauseone guarantee survives and matters more now that it stands alone: a refusal
must reach the user with its reason attached. A NUL prints as nothing, so an
entry named without its fault looks fine, and "extraction failed" would read
as a broken archive rather than a foreign one.
a_replacement_cannot_carry_an_entry_out_of_the_output_directoryis replacedby a comment where it stood. It proved an answer like
../../escapecould notmove an entry out of the output directory. That hole is now closed by
construction instead of by a check — there is no way to supply a replacement,
so the code that would fail the test does not compile — and the reasoning is
worth finding where the test used to be.
Docs
architecture.mdanddesktop.mddescribed this surface as inert and awaitingremoval; they now describe what is there. The desktop's command surface is four,
not five.