Report path failures through the return type, and cover path, xdg and error - #31
Merged
Conversation
path::expand() called the throwing overload of std::filesystem::canonical without catching it, so a ~ path whose target did not exist terminated the process rather than returning the std::unexpected its signature promises. Wrap that call, leaving the rest of the function as it was: a path that already resolves still comes back in canonical form. path::mkdir() read a false return from create_directories() as failure, but that overload also returns false when the directory is already there, and leaves the error_code untouched in that case — the filesystem_error built from it printed as "failed: Success". Judge the outcome by the error_code instead, which makes an already-present directory a success and leaves a real failure carrying its own diagnostic code.
These three classes had no test file. The suite reached the type system, environment and TOML, so the classes whose documented behaviour turned out not to match the code were exactly the untested ones. Cover what the fixes now guarantee — mkdir() on an existing directory, on a path whose parent is a regular file, and on an empty string; expand() on a missing target, on a target reached through a symlink, and on a path with no leading tilde — and pin what was left alone: exists() and the default constructor still call the throwing filesystem functions. Each test restores the environment variables it sets, including restoring an unset variable as unset, and works in a directory named after the process rather than a fixed path.
The pages and the header comments were brought in line with the previous behaviour, so they now describe something that is no longer true: that mkdir() reports an already-present directory as a failure carrying a zero code(), and that expand() lets a filesystem_error escape. Rewrite both claims where they appear, and drop the caller-side idiom they required — an else branch guarded on error().code() being nonzero is no longer needed when an existing directory is a success. Note the two things that follow from the new mkdir(): it is closer to "ensure this directory exists" than to a strict create, and it accepts a directory or symbolic link another party left in place, so a caller who cares about ownership has to check for itself. The sentence in each header that grouped expand() with exists() and the default constructor keeps the other two: both still call the throwing std::filesystem functions.
The wording that landed with the fixes said more than what was measured. mkdir() was described as succeeding when the target "is already present", but a regular file already sitting at that path is a failure, and a test pins that. Say "already a directory". The note about accepting what another party left in place said "a directory or symbolic link"; a dangling symbolic link is refused with EEXIST. Say "a symbolic link that resolves to one". The same note told the caller to verify ownership or the link target first, which recommends a check that cannot close the gap it warns about — the check and the use are separate operations — and which this library gives no way to perform. Replace the advice with the reason the gap stays open. expand() was said to convert "any failure", but only what the standard library reports as a filesystem_error is caught. expand()'s own comment never said what a failure is, though it is the call whose failure behaviour changed: canonicalisation needs the target to exist, so a path that has not been created yet comes back as unexpected. Say so, and let the example show the branch. Also note in mkdir() why the return value of create_directories() is discarded, add the POSIX header the environment helpers rely on, and make the non-terminating expand() test assert that home() resolved, so it cannot pass through the other branch without reaching the call it guards.
The note added with the previous change told the caller to create the directory under a parent only they can write to. Read against the sentence before it, that reads as sufficient, and it is not: a writable ancestor can be renamed or replaced, taking the parent with it. Put the condition on the whole chain of ancestors instead. Two more places said more than they should. mkdir() does not roll back the directories it managed to create before a failure, which the caller has to clean up; the standard does not promise either way, so say they may remain rather than that they are left behind. And the example under expand() named a missing target as the reason its else branch runs, when a permission problem, a symlink loop or an unresolvable home directory reach it too.
The note said what closes the gap, and named one thing: a location whose ancestors no other party can write to. That closes the race — nothing can be slipped in while the call runs — but it says nothing about a directory that was already sitting there when the condition came to hold. Its owner, its mode, the links under it: none of that follows from securing the chain afterwards. Say which half each measure covers, and that the call asks nothing about the other. expand()'s example initialised a path from a bare string literal, which is ambiguous between the two converting constructors and does not compile — the same trap the pages document, and which the copy of this example over there already avoids. Name the type, as the other copy does. The string overload sent readers nowhere: every mkdir() call in the pages and headers goes through it, while the failure and safety notes live on the filesystem::path one. Point at it. Cover the symbolic link the note now claims mkdir() accepts.
Four attempts at this paragraph each named a way to be safe, and each was wrong in a different place: check the owner first (the check races), use a parent only you can write to (a writable ancestor takes the parent with it), secure the whole chain of ancestors (it says nothing about a directory that was already there), that removes the opportunity to insert an entry (the directories this call creates itself get the platform's default mode, which can be world-writable). The advice was never what the note was for. Drop it and state what the call does not do — it does not look at who owns the directories on the path, at their permissions, or at where the links under them go, and it does not set the mode of the ones it creates. A caller who needs any of that is on their own, which was the point all along.
…in it The else branch of expand()'s example said the target does not exist yet, so create it with mkdir() and call expand() again. That cannot be done in that order. mkdir() does not expand a tilde, so passing the same string to it makes a directory literally named "~" under the working directory, and expand() will not hand back a path to create until the path already exists. Point at the route that works: build it from home().
Rewriting these lines dropped the qualifier they had carried since the example gained an else branch. A missing target is the case worth showing, but a permission problem, a symlink loop or an unresolvable home directory reach the same branch, and the caveat on the page lists all of them. Frame it as the case being supposed rather than the one that holds.
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.
path::expand()andpath::mkdir()both reported failure in a way theirsignatures do not allow, and the three classes where that went unnoticed had no
tests. This fixes the two calls, covers the classes, and brings the pages back in
line with what the code now does.
This addresses #26, #29 and #30.
expand()
expand()returnsstd::expected<path, std::filesystem::filesystem_error>butcalled the throwing overload of
std::filesystem::canonicalwithout catching it.A
~path whose target did not exist terminated the process — exit status 134,so the caller never got to check the
expectedit was handed. The neighbouringresolve()wrapped the same call and converted the exception, which made the twoasymmetric.
The canonical call is now wrapped. It is not removed: a path that resolves
still comes back in canonical form, with symbolic links resolved, as before. The
consequence is that
expand()still requires the target to exist, and that is nowstated in its own comment rather than left for the reader to discover.
mkdir()
mkdir()read afalsereturn fromcreate_directories()as failure. Thatoverload also returns
falsewhen there was nothing to create, and leaves theerror_codeuntouched in that case, so thefilesystem_errorbuilt from itprinted as
"failed: Success". Callers had to writeif (!r && r.error().code())to tell a real failure from an existing directory.
The outcome is now judged by the
error_code. An already-present directory is asuccess, which is the usual meaning of the operation, and that idiom disappears
from the examples. A regular file already sitting at the path is still a failure,
and so is a dangling symbolic link.
Tests
path,xdganderrorhad no test file. The suite reached the type system,environmentand TOML, and the classes whose documented behaviour turned out notto match the code were exactly the untested ones.
Thirty-five tests cover what the fixes guarantee —
mkdir()on an existingdirectory, on a path whose parent is a regular file, on an empty string, on a
symbolic link that resolves to a directory;
expand()on a missing target, on atarget reached through a symlink, on a path with no leading tilde — and pin what
was deliberately left alone:
exists()and the default constructor still call thethrowing filesystem functions. Canonical comparisons compute both sides rather
than matching a literal, so they hold where
/tmpis itself a link. The errorcases use
ENOTDIRrather than permissions, so they hold when the suite runs asroot. Each test restores the environment variables it sets, including restoring an
unset variable as unset, and works under a directory named after the process.
405 tests before, 440 after; all pass under GCC 13.3.0 and Clang 22.1.8, and under
--gtest_shuffle --gtest_repeat=3.Pages and comments
The pages and the header comments had been brought in line with the previous
behaviour, so they described something that is no longer true. Fourteen places
said that
mkdir()reports an already-present directory as a failure with a zerocode(), or thatexpand()lets afilesystem_errorescape. All of them arerewritten, and the sentence that grouped
expand()withexists()and thedefault constructor keeps the other two, which still throw.
Two notes were added while doing so.
mkdir()is closer to "ensure this directoryexists" than to a strict create, and it is not atomic: directories created before
a failure may remain. And because an already-present directory is accepted without
inspection, one left there by another party is accepted too — the note says what
the call does not check rather than recommending a way to make that safe, since
every such recommendation turned out to have a case it did not cover.
The examples that were edited were extracted and compiled. One of them did not:
pathhas converting constructors from bothstd::stringandstd::filesystem::path, so a bare string literal is ambiguous — a trap the pagesalready document, and which the other copy of that same example already avoided.