Name the entry tar could not write, and two CLI answers that were wrong - #109
Merged
Conversation
Issue #64 asked a write failure to name which entry was at fault. zip and 7z do. tar has two write paths, and the one an archive with nothing renamed takes, which on Unix is nearly every archive, still answered in the tar crate's own words: `failed to unpack \`/…/out/a.txt/b.txt\``. No entry named, and the absolute destination loose in the sentence. `unpack_in` itself is untouched. It is the traversal guard tar has always used and its canonicalizing containment check is what stops a write from following a symlink already in the output directory; only its error is dressed. Its error needs unwrapping first, because it is a stack of wrappers each adding the destination again: [0] failed to unpack `/…/out/a.txt/b.txt` [1] failed to unpack `a.txt/b.txt` into `/…/out/a.txt/b.txt` [2] Not a directory (os error 20) Only the last says what went wrong, and it is the register the other two formats answer in. Using level 0 inside an `Entry`, which names the destination itself, printed the path three times. before: failed to unpack `/…/out-tar/a.txt/b.txt` after: cannot write entry "a.txt/b.txt" to /…/out-tar/a.txt/b.txt: Not a directory (os error 20) zip: cannot write entry "a.txt/b.txt" to /…/out-zip/a.txt/b.txt: File exists (os error 17) The test that should have caught this passed for the wrong reason. It asserted `message.contains("a.txt/b.txt")`, which tar satisfied because the entry name is a substring of the destination path, so the guarantee was never checked at all. It now requires the name quoted as an entry, and requires the real cause, and it fails against the old code. Closes #93
…icts -o Two issues in `run_compress`, both decidable long before anything is written. **#67.** The source is canonicalized, for good reasons: `.`, `..` and a trailing slash have to resolve to something with a usable name, and an output that aliases the source can only be spotted against a resolved path. That resolved path then leaked into what the user was told. Type six characters, get an absolute path back: $ collapse compress ./sub/a.txt -f zip before: Created /Users/…/scratch/misc.A4Z7/sub/a.txt.zip after: Created ./sub/a.txt.zip Both spellings are kept now. The resolved one still drives the guards, the engine and the arcname, which has to be the file's real name. The typed one is what gets reported. A source with no name of its own falls back to the resolved path, which is also what keeps a directory's archive beside it rather than inside it, where the guards would then refuse it. **#75.** `--format` won over `-o`'s extension in silence, producing a file whose name lies about its contents and that this same CLI then rejects: $ collapse compress notes.txt -f tar -o mixed.zip Created mixed.zip $ collapse extract mixed.zip -o out error: Could not find EOCD Refused now, before anything is read or written. A warning was the other candidate and is worse: it goes to a terminal nobody reads in a script, and what is left behind is still a broken file with a misleading name. An extension that names no known format is deliberately **not** a contradiction. `-o backup.bin -f 7z` is a choice, and a guard that refused it would be overreaching; a test pins that. Every new test was checked against the unfixed code. Reverting #67 fails two, reverting #75 fails one, and making the guard over-refuse fails two more. Closes #67 Closes #75
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
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.
Fixes #93, #67 and #75. Three small ones, all about what the tool tells you.
#93 — tar did not say which entry failed
Issue #64 asked a write failure to name the entry at fault. zip and 7z do. tar has two write paths, and the one an archive with nothing renamed takes, which on Unix is nearly every archive, still answered in the dependency's words.
unpack_initself is untouched: it is the traversal guard tar has always used, and its canonicalizing containment check is what stops a write from following a symlink already in the output directory. Only its error is dressed.That error needs unwrapping first, because it is a stack of wrappers each adding the destination again:
Only the last says what went wrong. Using level 0 inside an
Entry, which names the destination itself, printed the path three times.The test that should have caught this passed for the wrong reason. It asserted
message.contains("a.txt/b.txt"), which tar satisfied because the entry name is a substring of the destination path, so the guarantee was never checked at all. It now requires the name quoted as an entry.#67 — it answered with a path you never typed
The canonicalization is there for good reasons and stays:
.,..and a trailing slash need to resolve to something with a usable name, and an output aliasing the source can only be caught against a resolved path. Both spellings are kept now, and each is used where it belongs. The arcname deliberately keeps taking the resolved name, since that is what is stored inside the archive.A source with no name of its own falls back to the resolved path, which is also what keeps a directory's archive beside it rather than inside it, where the guards would then refuse it.
#75 —
-fand-ocould contradict each other in silenceRefused now, before anything is read or written:
A warning was the other candidate and is worse: it goes to a terminal nobody reads in a script, and what is left is still a broken file with a misleading name.
An extension naming no known format is deliberately not a contradiction.
-o backup.bin -f 7zis a choice, and a guard that refused it would be overreaching.Tests
Seven new, each checked against the unfixed code:
--formatwins in silence-fOne desktop assertion moves with #93: it pinned the old tar message, and the new one is strictly better, so it now requires the entry named and the real cause, and fails if the dependency's wrapper leaks through again.
620 Rust tests and 116 Vitest, 505 offline.
architecture.md's compression flow updated, since steps 1 to 3 described the old behaviour.