Skip to content

Commit a0e6126

Browse files
committed
libutil-tests: cover deletePath's read-only handling
Review feedback from xokdvium on NixOS#16359. Adds a DeletePathTest fixture, modelled on MovePathTest: - readOnlyFile / treeOfReadOnlyFiles pin the behaviour the Windows walk needs clearReadOnly for. `nix::chmod(f, 0444)` is `::_wchmod` there, which sets FILE_ATTRIBUTE_READONLY and blocks deletion outright. Verified they catch it: stubbing clearReadOnly to a no-op fails exactly these two under Wine and leaves the other three passing. - nonWritableDirectory is the Unix counterpart, where the walk has to add write permission to the directory before unlinking its contents. Guarded to Unix because the read-only attribute is not honoured on Windows directories. - reportsBytesFreed, nonexistentIsNoop cover the accounting and the already-gone early return on both platforms. - emptyPathIsNoop is Windows-only. The Unix side asserts is_absolute() instead, so on a debug build the same call aborts rather than returning; that asymmetry predates this change and the test pins the side that guarantees it. Also drops the doxygen comment above the shared deletePath wrapper: it is a definition in a .cc, so it never reaches the docs, and the wrapper is self-evident.
1 parent 8578932 commit a0e6126

2 files changed

Lines changed: 107 additions & 4 deletions

File tree

src/libutil-tests/file-system.cc

Lines changed: 107 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -437,4 +437,111 @@ TEST_F(MovePathTest, symlinkReplacesRegular)
437437

438438
#endif
439439

440+
441+
/* ----------------------------------------------------------------------------
442+
* deletePath
443+
* --------------------------------------------------------------------------*/
444+
445+
class DeletePathTest : public ::testing::Test
446+
{
447+
protected:
448+
std::filesystem::path tmpDir;
449+
nix::AutoDelete delTmpDir;
450+
451+
private:
452+
void SetUp() override
453+
{
454+
tmpDir = createTempDir();
455+
delTmpDir = {tmpDir, /*recursive=*/true};
456+
}
457+
458+
void TearDown() override
459+
{
460+
delTmpDir.deletePath();
461+
}
462+
};
463+
464+
TEST_F(DeletePathTest, readOnlyFile)
465+
{
466+
auto file = tmpDir / OS_STR("read-only");
467+
writeFile(file, "contents");
468+
/* On Windows this sets `FILE_ATTRIBUTE_READONLY`, which blocks deletion
469+
outright until it is cleared. On Unix it only drops the write bit, which
470+
does not, because unlinking needs write permission on the directory
471+
rather than on the file. Either way the file has to go. */
472+
nix::chmod(file, 0444);
473+
474+
deletePath(file);
475+
ASSERT_FALSE(pathExists(file));
476+
}
477+
478+
TEST_F(DeletePathTest, treeOfReadOnlyFiles)
479+
{
480+
/* Read-only entries at three depths, so the recursive walk has to relax
481+
each one as it reaches it rather than only the top. */
482+
auto root = tmpDir / OS_STR("tree");
483+
auto mid = root / OS_STR("a");
484+
auto leaf = mid / OS_STR("b");
485+
createDirs(leaf);
486+
for (auto & file : {root / OS_STR("f0"), mid / OS_STR("f1"), leaf / OS_STR("f2")}) {
487+
writeFile(file, "contents");
488+
nix::chmod(file, 0444);
489+
}
490+
491+
deletePath(root);
492+
ASSERT_FALSE(pathExists(root));
493+
}
494+
495+
TEST_F(DeletePathTest, reportsBytesFreed)
496+
{
497+
auto file = tmpDir / OS_STR("sized");
498+
std::string contents(4096, 'x');
499+
writeFile(file, contents);
500+
501+
uint64_t bytesFreed = 0;
502+
deletePath(file, bytesFreed);
503+
ASSERT_FALSE(pathExists(file));
504+
ASSERT_EQ(bytesFreed, contents.size());
505+
}
506+
507+
TEST_F(DeletePathTest, nonexistentIsNoop)
508+
{
509+
ASSERT_NO_THROW(deletePath(tmpDir / OS_STR("nonexistent")));
510+
}
511+
512+
#ifdef _WIN32
513+
514+
TEST_F(DeletePathTest, emptyPathIsNoop)
515+
{
516+
/* The `std::filesystem::remove_all` this replaced treated an empty path as
517+
a no-op, and callers still depend on that -- `nix-fetchers-tests` reaches
518+
here with one while tearing down a skipped test.
519+
520+
Windows-only, because the Unix implementation asserts `is_absolute()`
521+
instead, so on a debug build the same call aborts rather than returning.
522+
That asymmetry predates this change; the test pins the behaviour on the
523+
side that guarantees it. */
524+
ASSERT_NO_THROW(deletePath(std::filesystem::path{}));
525+
}
526+
527+
#endif
528+
529+
#ifndef _WIN32
530+
531+
TEST_F(DeletePathTest, nonWritableDirectory)
532+
{
533+
/* The walk has to add write permission to the directory before it can
534+
unlink what is inside it. `FILE_ATTRIBUTE_READONLY` is not honoured on
535+
Windows directories, so this case is Unix-only. */
536+
auto dir = tmpDir / "locked";
537+
createDir(dir, 0755);
538+
writeFile(dir / "file", "contents");
539+
nix::chmod(dir, 0500);
540+
541+
deletePath(dir);
542+
ASSERT_FALSE(pathExists(dir));
543+
}
544+
545+
#endif
546+
440547
} // namespace nix

src/libutil/file-system.cc

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -408,10 +408,6 @@ AutoDelete::AutoDelete(const std::filesystem::path & p, bool recursive)
408408
{
409409
}
410410

411-
/**
412-
* Both platforms implement the accounting overload; this wrapper is the same
413-
* either way, so it lives here rather than being duplicated in each.
414-
*/
415411
void deletePath(const std::filesystem::path & path)
416412
{
417413
uint64_t dummy;

0 commit comments

Comments
 (0)