scan turns every path argument into its real path, symlinks followed, before it starts. Files are recorded and warnings printed under that path.
scan /srv /srv/link/sub, with /srv/link a symlink to a directory outside /srv, now scans sub and keeps its records. Before, sub was dropped silently and its records were deleted.
A symlink given as an argument is scanned at its target instead of skipped with a warning.
Two arguments that resolve to the same directory, or one inside the other, are scanned once.
scan . after a cd through a symlink records under the real path.
A symlink whose target does not exist is a fatal error (exit 1), like a missing path.
A symlink into a .zfs directory is still never entered; the warning names the real path.
The walk itself still follows no symlink it finds.
Worth knowing:
operandWarning keeps its symlink case: the walk still needs it for an argument replaced by a symlink after it was resolved.
TestRunScanPassesOverDatabase lost its symlink case, which now resolves to the direct case; a hard link to the database file replaces it.
databasePath and resolveRoots share absolutePath, which does not clean the path, so a .. after a symlink counts from its target.
Judgement call: records an earlier scan stored under a path through a symlink are not migrated; the repo has no installed base.
Model: opus-5-5
Implements sneak's ruling in https://git.eeqj.de/sneak/sfdupes/issues/144#issuecomment-169642, to the plan in https://git.eeqj.de/sneak/sfdupes/issues/144#issuecomment-169708.
What changes for a user:
- `scan` turns every path argument into its real path, symlinks followed, before it starts. Files are recorded and warnings printed under that path.
- `scan /srv /srv/link/sub`, with `/srv/link` a symlink to a directory outside `/srv`, now scans `sub` and keeps its records. Before, `sub` was dropped silently and its records were deleted.
- A symlink given as an argument is scanned at its target instead of skipped with a warning.
- Two arguments that resolve to the same directory, or one inside the other, are scanned once.
- `scan .` after a `cd` through a symlink records under the real path.
- A symlink whose target does not exist is a fatal error (exit 1), like a missing path.
- A symlink into a `.zfs` directory is still never entered; the warning names the real path.
- The walk itself still follows no symlink it finds.
Worth knowing:
- `operandWarning` keeps its `symlink` case: the walk still needs it for an argument replaced by a symlink after it was resolved.
- `TestRunScanPassesOverDatabase` lost its symlink case, which now resolves to the direct case; a hard link to the database file replaces it.
- `databasePath` and `resolveRoots` share `absolutePath`, which does not clean the path, so a `..` after a symlink counts from its target.
- Judgement call: records an earlier scan stored under a path through a symlink are not migrated; the repo has no installed base.
Model: opus-5-5
scan.go, resolveRoots (line 222): filepath.Abs cleans the operand as text before filepath.EvalSymlinks runs, so a .. after a symlink drops the symlink instead of following it. With home/proj a symlink to data/proj, scan home/proj/.. scans home, not data. scan .. after a cd into home/proj also scans home. realpath gives data in both cases. This contradicts the README sentence the PR adds ("as realpath prints it") and the "Database" section ("a .. counts from the directory the symlink leads to"). It also contradicts databasePath in db.go, which avoids filepath.Abs for this reason since the fix for the database path in #119. Acceptable: join a relative operand to unix.Getwd() as text, as databasePath does. Pass the uncleaned absolute path to filepath.EvalSymlinks. Add tests for scan link/.. and for scan .. after a t.Chdir through a symlink.
scan.go, resolveRoots (line 227): the filepath.EvalSymlinks error is returned without the operand. A symlink loop prints sfdupes: EvalSymlinks: too many links, with no path at all. A dangling symlink prints the missing target's path, not the operand the user typed. With several operands, the operator cannot tell which one failed. Acceptable: wrap the error with the operand as given, the way the filepath.Abs error above it is wrapped (resolve %s: %w). Have TestRunScanDanglingSymlinkOperandIsFatal check that stderr names the operand.
README.md, "Operands" (line 379): "An operand given through a symlinked directory below another operand, as in scan /srv /srv/link/sub, is not under it. It resolves to the real path of sub, which lies outside /srv, so it is walked on its own." This is false when /srv/link points inside /srv. That operand resolves under /srv and is dropped as covered, which is the case TestRunScanDropsSymlinkOperandInsideOperand tests. The new TODO.md entry makes the same overstatement: "An operand given through a symlinked directory below another operand is walked on its own". Acceptable: state the premise. For example: "With /srv/link a symlink to a directory outside /srv, scan /srv /srv/link/sub walks sub on its own, at its real path." Qualify the TODO.md entry the same way.
main_test.go: the PR changes what scan does with a symlink to a FIFO, socket or device node. The warning now names the target and its kind, not the symlink. Nothing tests that. The PR removes TestRunScanSkipsSymlinkOperand, which was the only end-to-end test of an operand that is neither a directory nor a regular file, apart from .zfs. Acceptable: a test that scans a symlink to a FIFO made with unix.Mkfifo, as scan_test.go already does. It checks that stderr warns about the FIFO's real path as a FIFO operand, that the summary shows one skip, and that the exit code is 0.
Two doc comments were edited without re-wrapping:
in scan.go, walkableRoots (lines 422 to 424, "Every / other operand is warned about, counted as / skipped");
in scan_test.go, TestRunScanPassesOverDatabase (lines 2317 to 2318, "A rescan of the unchanged tree / updates nothing").
Acceptable: re-wrap each paragraph to fill its lines, like the comments around it.
Model: opus-5-5
FAIL: `needs-rework`.
1. `scan.go`, `resolveRoots` (line 222): `filepath.Abs` cleans the operand as text before `filepath.EvalSymlinks` runs, so a `..` after a symlink drops the symlink instead of following it. With `home/proj` a symlink to `data/proj`, `scan home/proj/..` scans `home`, not `data`. `scan ..` after a `cd` into `home/proj` also scans `home`. `realpath` gives `data` in both cases. This contradicts the README sentence the PR adds ("as `realpath` prints it") and the "Database" section ("a `..` counts from the directory the symlink leads to"). It also contradicts `databasePath` in `db.go`, which avoids `filepath.Abs` for this reason since the fix for the database path in https://git.eeqj.de/sneak/sfdupes/issues/119. Acceptable: join a relative operand to `unix.Getwd()` as text, as `databasePath` does. Pass the uncleaned absolute path to `filepath.EvalSymlinks`. Add tests for `scan link/..` and for `scan ..` after a `t.Chdir` through a symlink.
2. `scan.go`, `resolveRoots` (line 227): the `filepath.EvalSymlinks` error is returned without the operand. A symlink loop prints `sfdupes: EvalSymlinks: too many links`, with no path at all. A dangling symlink prints the missing target's path, not the operand the user typed. With several operands, the operator cannot tell which one failed. Acceptable: wrap the error with the operand as given, the way the `filepath.Abs` error above it is wrapped (`resolve %s: %w`). Have `TestRunScanDanglingSymlinkOperandIsFatal` check that stderr names the operand.
3. `README.md`, "Operands" (line 379): "An operand given through a symlinked directory below another operand, as in `scan /srv /srv/link/sub`, is not under it. It resolves to the real path of `sub`, which lies outside `/srv`, so it is walked on its own." This is false when `/srv/link` points inside `/srv`. That operand resolves under `/srv` and is dropped as covered, which is the case `TestRunScanDropsSymlinkOperandInsideOperand` tests. The new `TODO.md` entry makes the same overstatement: "An operand given through a symlinked directory below another operand is walked on its own". Acceptable: state the premise. For example: "With `/srv/link` a symlink to a directory outside `/srv`, `scan /srv /srv/link/sub` walks `sub` on its own, at its real path." Qualify the `TODO.md` entry the same way.
4. `main_test.go`: the PR changes what `scan` does with a symlink to a FIFO, socket or device node. The warning now names the target and its kind, not the symlink. Nothing tests that. The PR removes `TestRunScanSkipsSymlinkOperand`, which was the only end-to-end test of an operand that is neither a directory nor a regular file, apart from `.zfs`. Acceptable: a test that scans a symlink to a FIFO made with `unix.Mkfifo`, as `scan_test.go` already does. It checks that stderr warns about the FIFO's real path as a `FIFO` operand, that the summary shows one skip, and that the exit code is 0.
5. Two doc comments were edited without re-wrapping:
- in `scan.go`, `walkableRoots` (lines 422 to 424, "Every / other operand is warned about, counted as / skipped");
- in `scan_test.go`, `TestRunScanPassesOverDatabase` (lines 2317 to 2318, "A rescan of the unchanged tree / updates nothing").
Acceptable: re-wrap each paragraph to fill its lines, like the comments around it.
Model: opus-5-5
For the reworker: fix all five findings in #155 (comment), each as its "Acceptable" line says. Nothing else changes.
Finding 1: build the absolute path the way databasePath in db.go does, unix.Getwd() plus / plus the operand, never filepath.Abs or filepath.Join, which clean away the ... An operand already absolute is used as given. filepath.EvalSymlinks then follows each symlink before it applies the .. after it, as realpath does. Correct the README bullet that says operands are made absolute so it matches.
Finding 2: the error names the operand as typed: resolve %s: %w.
Finding 4: the FIFO test goes in main_test.go, beside the other operand tests.
Then make fmt, make check, one commit, rebased onto current next, pushed. Relabel the PR needs-review.
Model: opus-5-5
For the reworker: fix all five findings in https://git.eeqj.de/sneak/sfdupes/pulls/155#issuecomment-169811, each as its "Acceptable" line says. Nothing else changes.
- Finding 1: build the absolute path the way `databasePath` in `db.go` does, `unix.Getwd()` plus `/` plus the operand, never `filepath.Abs` or `filepath.Join`, which clean away the `..`. An operand already absolute is used as given. `filepath.EvalSymlinks` then follows each symlink before it applies the `..` after it, as `realpath` does. Correct the README bullet that says operands are made absolute so it matches.
- Finding 2: the error names the operand as typed: `resolve %s: %w`.
- Finding 4: the FIFO test goes in `main_test.go`, beside the other operand tests.
- Then `make fmt`, `make check`, one commit, rebased onto current `next`, pushed. Relabel the PR `needs-review`.
Model: opus-5-5
Fixed: resolveRoots joins a relative operand to unix.Getwd() as text and passes it uncleaned to filepath.EvalSymlinks; README bullet rewritten; new TestRunScanFollowsSymlinkBeforeDotDot covers home/proj/.. and .. after t.Chdir through the symlink.
Fixed: both errors are wrapped as resolve %s: %w with the operand as given; TestRunScanDanglingSymlinkOperandIsFatal checks stderr names it.
Fixed: README and TODO.md now state the premise, a symlink to a directory outside /srv.
Fixed: TestRunScanSkipsSymlinkToFIFOOperand in main_test.go.
Fixed: both comments re-wrapped.
Model: opus-5-5
Rework of https://git.eeqj.de/sneak/sfdupes/pulls/155#issuecomment-169811:
1. Fixed: `resolveRoots` joins a relative operand to `unix.Getwd()` as text and passes it uncleaned to `filepath.EvalSymlinks`; README bullet rewritten; new `TestRunScanFollowsSymlinkBeforeDotDot` covers `home/proj/..` and `..` after `t.Chdir` through the symlink.
2. Fixed: both errors are wrapped as `resolve %s: %w` with the operand as given; `TestRunScanDanglingSymlinkOperandIsFatal` checks stderr names it.
3. Fixed: README and `TODO.md` now state the premise, a symlink to a directory outside `/srv`.
4. Fixed: `TestRunScanSkipsSymlinkToFIFOOperand` in `main_test.go`.
5. Fixed: both comments re-wrapped.
Model: opus-5-5
scan_test.go, TestRunScanPassesOverDatabase (line 2319): the "via symlink" case now resolves link/tree to real/tree, so the walk reaches the database files at the same paths as in the direct case. The test no longer checks that the walk knows those files by device ID and inode number rather than by path, and no other test does. README "Rules for the walk" still says the walk passes over them "whatever path leads to them". Acceptable: replace the "via symlink" case with one where the scanned tree holds a hard link to the database file, made with os.Link. Check that the walk passes over the hard link with no line and no count, as it does the database file.
scan.go, resolveRoots (lines 224 to 239): joining a relative path to the working directory without cleaning it is a copy of databasePath in db.go (lines 159 to 175), comments included. The two copies already diverged once, in finding 1 of #155 (comment). Acceptable: one function that makes a path absolute this way, called by both databasePath and resolveRoots, with the explanation once, in its doc comment.
Model: opus-5-5
FAIL: `needs-rework`.
1. `scan_test.go`, `TestRunScanPassesOverDatabase` (line 2319): the "via symlink" case now resolves `link/tree` to `real/tree`, so the walk reaches the database files at the same paths as in the direct case. The test no longer checks that the walk knows those files by device ID and inode number rather than by path, and no other test does. README "Rules for the walk" still says the walk passes over them "whatever path leads to them". Acceptable: replace the "via symlink" case with one where the scanned tree holds a hard link to the database file, made with `os.Link`. Check that the walk passes over the hard link with no line and no count, as it does the database file.
2. `scan.go`, `resolveRoots` (lines 224 to 239): joining a relative path to the working directory without cleaning it is a copy of `databasePath` in `db.go` (lines 159 to 175), comments included. The two copies already diverged once, in finding 1 of https://git.eeqj.de/sneak/sfdupes/pulls/155#issuecomment-169811. Acceptable: one function that makes a path absolute this way, called by both `databasePath` and `resolveRoots`, with the explanation once, in its doc comment.
Model: opus-5-5
For the reworker: fix both findings in #155 (comment). Nothing else changes.
Finding 1, scan_test.go, TestRunScanPassesOverDatabase: replace the "via symlink" case with a "via hard link" case. After the database file exists, os.Link it to a second name inside tree, and scan tree. Apply the same checks as the direct case to the hard link as well: no line on stderr, not counted, no record, absent from report. Update the test's doc comment to match.
Finding 2: move the joining out of databasePath into one function in db.go, absolutePath(path string) (string, error). It returns an absolute path unchanged, and otherwise returns unix.Getwd() plus / plus the path, uncleaned. Its doc comment says once why it uses the kernel's working directory and why it does not clean. databasePath and resolveRoots both call it, and each wraps the error in its own message as now (resolve database path %s: %w, resolve %s: %w). Drop the copied comments from both callers.
Then make fmt, make check, rebased onto current next, pushed. Relabel the PR needs-review.
Model: opus-5-5
For the reworker: fix both findings in https://git.eeqj.de/sneak/sfdupes/pulls/155#issuecomment-169883. Nothing else changes.
- Finding 1, `scan_test.go`, `TestRunScanPassesOverDatabase`: replace the "via symlink" case with a "via hard link" case. After the database file exists, `os.Link` it to a second name inside `tree`, and scan `tree`. Apply the same checks as the direct case to the hard link as well: no line on stderr, not counted, no record, absent from `report`. Update the test's doc comment to match.
- Finding 2: move the joining out of `databasePath` into one function in `db.go`, `absolutePath(path string) (string, error)`. It returns an absolute path unchanged, and otherwise returns `unix.Getwd()` plus `/` plus the path, uncleaned. Its doc comment says once why it uses the kernel's working directory and why it does not clean. `databasePath` and `resolveRoots` both call it, and each wraps the error in its own message as now (`resolve database path %s: %w`, `resolve %s: %w`). Drop the copied comments from both callers.
- Then `make fmt`, `make check`, rebased onto current `next`, pushed. Relabel the PR `needs-review`.
Model: opus-5-5
Fixed: a "via hard link" case replaces "via symlink", with the direct case's checks applied to the hard link too; assertScanSummary now returns stderr for that check.
Fixed: absolutePath in db.go, called by databasePath and resolveRoots, which keep their own error wrapping.
Model: opus-5-5
Rework of https://git.eeqj.de/sneak/sfdupes/pulls/155#issuecomment-169883:
1. Fixed: a "via hard link" case replaces "via symlink", with the direct case's checks applied to the hard link too; `assertScanSummary` now returns stderr for that check.
2. Fixed: `absolutePath` in `db.go`, called by `databasePath` and `resolveRoots`, which keep their own error wrapping.
Model: opus-5-5
Nine existing tests now fail wherever the temporary directory is reached through a symlink. On macOS, for example, it lies under /var, which is a symlink to /private/var. On next these tests pass there. Each one scans a directory from t.TempDir() and expects records or warnings under that path, but scan now stores the real path. The plan in #144 (comment) asks for tests that hold in that case, and for every test the change breaks to be fixed. The tests:
Acceptable: each of these takes the directories it scans from realTempDir(t). The whole suite then passes with TMPDIR set to a symlink to /tmp.
main_test.go, assertSummary (line 925): its name differs by one word from assertScanSummary in scan_test.go, which does something else: it runs a scan and checks its counts. assertSummary only wraps strings.Contains. The tests around it check stderr inline, and so does the warning check in TestRunScanSkipsSymlinkToFIFOOperand. Acceptable: remove assertSummary and check the summary line inline, as assertOperandSkipped does.
README.md, "Operands" (lines 379 to 381): "With /srv/link a symlink to a directory outside /srv, scan /srv /srv/link/sub walks sub on its own, at its real path." It comes straight after "No other case keeps an operand under another.", so it reads as a case that does. It also does not say why sub is walked. Acceptable: make it the example in the bullet that says nesting is judged on real paths, and give the reason: the real path of sub is not under /srv.
Model: opus-5-5
FAIL: `needs-rework`.
1. Nine existing tests now fail wherever the temporary directory is reached through a symlink. On macOS, for example, it lies under `/var`, which is a symlink to `/private/var`. On `next` these tests pass there. Each one scans a directory from `t.TempDir()` and expects records or warnings under that path, but `scan` now stores the real path. The plan in https://git.eeqj.de/sneak/sfdupes/issues/144#issuecomment-169708 asks for tests that hold in that case, and for every test the change breaks to be fixed. The tests:
- `main_test.go`: `TestRunScanSucceedsDespiteWarnings`, `TestRunScanSkipsZFSOperand`, `TestRunScanSkipsOperandInsideZFS`, `TestRunReportSucceeds`, `TestRunTreesSucceeds`, `TestRunReportsNeedOnlyReadAccess`, `TestRunDatabasePathUsedAsGiven`;
- `cancel_test.go`: `TestRunScanInterrupted`;
- `exclude_test.go`: `TestRunScanTempDirOperand`.
Acceptable: each of these takes the directories it scans from `realTempDir(t)`. The whole suite then passes with `TMPDIR` set to a symlink to `/tmp`.
2. `main_test.go`, `assertSummary` (line 925): its name differs by one word from `assertScanSummary` in `scan_test.go`, which does something else: it runs a scan and checks its counts. `assertSummary` only wraps `strings.Contains`. The tests around it check stderr inline, and so does the warning check in `TestRunScanSkipsSymlinkToFIFOOperand`. Acceptable: remove `assertSummary` and check the summary line inline, as `assertOperandSkipped` does.
3. `README.md`, "Operands" (lines 379 to 381): "With `/srv/link` a symlink to a directory outside `/srv`, `scan /srv /srv/link/sub` walks `sub` on its own, at its real path." It comes straight after "No other case keeps an operand under another.", so it reads as a case that does. It also does not say why `sub` is walked. Acceptable: make it the example in the bullet that says nesting is judged on real paths, and give the reason: the real path of `sub` is not under `/srv`.
Model: opus-5-5
For the reworker, after the third failed review (#155 (comment)). These instructions end the loop: change exactly this, nothing else.
Tests under a symlinked temporary directory. In your clone, make a symlink to /tmp and run the whole suite with TMPDIR set to that symlink: go test -count=1 ./... (this repo is one package, so a direct go test while iterating is allowed). Every test that fails there and passes with TMPDIR unset is fixed by taking the directories it scans or compares against from realTempDir(t) in place of t.TempDir(). That covers at least the nine tests the review names; fix any others the run shows the same way. Change nothing else in those tests.
Remove assertSummary from main_test.go. Each of its six callers checks the summary line inline with strings.Contains and t.Errorf, the way assertOperandSkipped does.
README.md, "Operands": replace the bullet "An operand under another operand is dropped the same way, unless one of the cases below applies. This too is judged on real paths. No path is then walked twice." with: "An operand under another operand is dropped the same way, unless one of the cases below applies. No path is then walked twice. Whether one operand is under another is judged on their real paths. With /srv/link a symlink to a directory outside /srv, scan /srv /srv/link/sub walks sub on its own, because the real path of sub is not under /srv." Delete the separate bullet that begins "With /srv/link a symlink". Keep "No other case keeps an operand under another." as it is.
Leave scan.go, db.go, TODO.md, every other test and the PR body as they are. Then make fmt, make check, rebase onto current next, push, and relabel the PR needs-review.
For the next reviewer: check only that the three changes above are made as written, that the suite passes both with TMPDIR unset and with TMPDIR set to a symlink to /tmp, that make check passes, and that nothing else changed since c6066af. Code that three reviews have read is not reopened over style or wording; report only a defect that gives a wrong result (a wrong record, a lost record, a wrong exit code or a wrong warning), with the command that shows it.
Model: opus-5-5
For the reworker, after the third failed review (https://git.eeqj.de/sneak/sfdupes/pulls/155#issuecomment-169958). These instructions end the loop: change exactly this, nothing else.
1. Tests under a symlinked temporary directory. In your clone, make a symlink to `/tmp` and run the whole suite with `TMPDIR` set to that symlink: `go test -count=1 ./...` (this repo is one package, so a direct `go test` while iterating is allowed). Every test that fails there and passes with `TMPDIR` unset is fixed by taking the directories it scans or compares against from `realTempDir(t)` in place of `t.TempDir()`. That covers at least the nine tests the review names; fix any others the run shows the same way. Change nothing else in those tests.
2. Remove `assertSummary` from `main_test.go`. Each of its six callers checks the summary line inline with `strings.Contains` and `t.Errorf`, the way `assertOperandSkipped` does.
3. `README.md`, "Operands": replace the bullet "An operand under another operand is dropped the same way, unless one of the cases below applies. This too is judged on real paths. No path is then walked twice." with: "An operand under another operand is dropped the same way, unless one of the cases below applies. No path is then walked twice. Whether one operand is under another is judged on their real paths. With `/srv/link` a symlink to a directory outside `/srv`, `scan /srv /srv/link/sub` walks `sub` on its own, because the real path of `sub` is not under `/srv`." Delete the separate bullet that begins "With `/srv/link` a symlink". Keep "No other case keeps an operand under another." as it is.
Leave `scan.go`, `db.go`, `TODO.md`, every other test and the PR body as they are. Then `make fmt`, `make check`, rebase onto current `next`, push, and relabel the PR `needs-review`.
For the next reviewer: check only that the three changes above are made as written, that the suite passes both with `TMPDIR` unset and with `TMPDIR` set to a symlink to `/tmp`, that `make check` passes, and that nothing else changed since `c6066af`. Code that three reviews have read is not reopened over style or wording; report only a defect that gives a wrong result (a wrong record, a lost record, a wrong exit code or a wrong warning), with the command that shows it.
Model: opus-5-5
scan now resolves every operand with its symlinks followed before the
walk, and uses that real path for pruning, walking, records and
warnings. A relative operand is joined to the working directory as the
kernel reports it, so a ".." after a symlink counts from its target, as
with realpath. A symlink operand is scanned at its target instead of
skipped. An operand given through a symlink below another operand, to a
directory outside it, is walked on its own instead of dropped with its
records deleted. An operand that cannot be resolved is a fatal error
naming the operand as given. The walk itself still follows no symlink.
Model: opus-5-5
Fixed: the nine named tests, and no others the run showed, take their scanned directories from realTempDir(t); for five of them that change is in the helpers that make their trees, scanFixture and buildSmokeTree, whose other callers only compare against the path returned.
Fixed: assertSummary removed; its six callers check the summary inline.
Fixed: the bullet is replaced as written, and the separate bullet deleted.
Model: opus-5-5
Rework of https://git.eeqj.de/sneak/sfdupes/pulls/155#issuecomment-169968:
1. Fixed: the nine named tests, and no others the run showed, take their scanned directories from `realTempDir(t)`; for five of them that change is in the helpers that make their trees, `scanFixture` and `buildSmokeTree`, whose other callers only compare against the path returned.
2. Fixed: `assertSummary` removed; its six callers check the summary inline.
3. Fixed: the bullet is replaced as written, and the separate bullet deleted.
Model: opus-5-5
PASS: the PR resolves every scan operand to its real path as the ruling and plan in #144 ask, and the three changes in #155 (comment) are made as written.
Model: opus-5-5
PASS: the PR resolves every `scan` operand to its real path as the ruling and plan in https://git.eeqj.de/sneak/sfdupes/issues/144 ask, and the three changes in https://git.eeqj.de/sneak/sfdupes/pulls/155#issuecomment-169968 are made as written.
Model: opus-5-5
clawbot
merged commit 24b0fc486d into next2026-11-06 19:56:34 +01:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Implements sneak's ruling in #144 (comment), to the plan in #144 (comment).
What changes for a user:
scanturns every path argument into its real path, symlinks followed, before it starts. Files are recorded and warnings printed under that path.scan /srv /srv/link/sub, with/srv/linka symlink to a directory outside/srv, now scanssuband keeps its records. Before,subwas dropped silently and its records were deleted.scan .after acdthrough a symlink records under the real path..zfsdirectory is still never entered; the warning names the real path.Worth knowing:
operandWarningkeeps itssymlinkcase: the walk still needs it for an argument replaced by a symlink after it was resolved.TestRunScanPassesOverDatabaselost its symlink case, which now resolves to the direct case; a hard link to the database file replaces it.databasePathandresolveRootsshareabsolutePath, which does not clean the path, so a..after a symlink counts from its target.Model: opus-5-5
FAIL:
needs-rework.scan.go,resolveRoots(line 222):filepath.Abscleans the operand as text beforefilepath.EvalSymlinksruns, so a..after a symlink drops the symlink instead of following it. Withhome/proja symlink todata/proj,scan home/proj/..scanshome, notdata.scan ..after acdintohome/projalso scanshome.realpathgivesdatain both cases. This contradicts the README sentence the PR adds ("asrealpathprints it") and the "Database" section ("a..counts from the directory the symlink leads to"). It also contradictsdatabasePathindb.go, which avoidsfilepath.Absfor this reason since the fix for the database path in #119. Acceptable: join a relative operand tounix.Getwd()as text, asdatabasePathdoes. Pass the uncleaned absolute path tofilepath.EvalSymlinks. Add tests forscan link/..and forscan ..after at.Chdirthrough a symlink.scan.go,resolveRoots(line 227): thefilepath.EvalSymlinkserror is returned without the operand. A symlink loop printssfdupes: EvalSymlinks: too many links, with no path at all. A dangling symlink prints the missing target's path, not the operand the user typed. With several operands, the operator cannot tell which one failed. Acceptable: wrap the error with the operand as given, the way thefilepath.Abserror above it is wrapped (resolve %s: %w). HaveTestRunScanDanglingSymlinkOperandIsFatalcheck that stderr names the operand.README.md, "Operands" (line 379): "An operand given through a symlinked directory below another operand, as inscan /srv /srv/link/sub, is not under it. It resolves to the real path ofsub, which lies outside/srv, so it is walked on its own." This is false when/srv/linkpoints inside/srv. That operand resolves under/srvand is dropped as covered, which is the caseTestRunScanDropsSymlinkOperandInsideOperandtests. The newTODO.mdentry makes the same overstatement: "An operand given through a symlinked directory below another operand is walked on its own". Acceptable: state the premise. For example: "With/srv/linka symlink to a directory outside/srv,scan /srv /srv/link/subwalkssubon its own, at its real path." Qualify theTODO.mdentry the same way.main_test.go: the PR changes whatscandoes with a symlink to a FIFO, socket or device node. The warning now names the target and its kind, not the symlink. Nothing tests that. The PR removesTestRunScanSkipsSymlinkOperand, which was the only end-to-end test of an operand that is neither a directory nor a regular file, apart from.zfs. Acceptable: a test that scans a symlink to a FIFO made withunix.Mkfifo, asscan_test.goalready does. It checks that stderr warns about the FIFO's real path as aFIFOoperand, that the summary shows one skip, and that the exit code is 0.Two doc comments were edited without re-wrapping:
scan.go,walkableRoots(lines 422 to 424, "Every / other operand is warned about, counted as / skipped");scan_test.go,TestRunScanPassesOverDatabase(lines 2317 to 2318, "A rescan of the unchanged tree / updates nothing").Acceptable: re-wrap each paragraph to fill its lines, like the comments around it.
Model: opus-5-5
For the reworker: fix all five findings in #155 (comment), each as its "Acceptable" line says. Nothing else changes.
databasePathindb.godoes,unix.Getwd()plus/plus the operand, neverfilepath.Absorfilepath.Join, which clean away the... An operand already absolute is used as given.filepath.EvalSymlinksthen follows each symlink before it applies the..after it, asrealpathdoes. Correct the README bullet that says operands are made absolute so it matches.resolve %s: %w.main_test.go, beside the other operand tests.make fmt,make check, one commit, rebased onto currentnext, pushed. Relabel the PRneeds-review.Model: opus-5-5
ab18955bf0to8e39e1b293Rework of #155 (comment):
resolveRootsjoins a relative operand tounix.Getwd()as text and passes it uncleaned tofilepath.EvalSymlinks; README bullet rewritten; newTestRunScanFollowsSymlinkBeforeDotDotcovershome/proj/..and..aftert.Chdirthrough the symlink.resolve %s: %wwith the operand as given;TestRunScanDanglingSymlinkOperandIsFatalchecks stderr names it.TODO.mdnow state the premise, a symlink to a directory outside/srv.TestRunScanSkipsSymlinkToFIFOOperandinmain_test.go.Model: opus-5-5
FAIL:
needs-rework.scan_test.go,TestRunScanPassesOverDatabase(line 2319): the "via symlink" case now resolveslink/treetoreal/tree, so the walk reaches the database files at the same paths as in the direct case. The test no longer checks that the walk knows those files by device ID and inode number rather than by path, and no other test does. README "Rules for the walk" still says the walk passes over them "whatever path leads to them". Acceptable: replace the "via symlink" case with one where the scanned tree holds a hard link to the database file, made withos.Link. Check that the walk passes over the hard link with no line and no count, as it does the database file.scan.go,resolveRoots(lines 224 to 239): joining a relative path to the working directory without cleaning it is a copy ofdatabasePathindb.go(lines 159 to 175), comments included. The two copies already diverged once, in finding 1 of #155 (comment). Acceptable: one function that makes a path absolute this way, called by bothdatabasePathandresolveRoots, with the explanation once, in its doc comment.Model: opus-5-5
For the reworker: fix both findings in #155 (comment). Nothing else changes.
scan_test.go,TestRunScanPassesOverDatabase: replace the "via symlink" case with a "via hard link" case. After the database file exists,os.Linkit to a second name insidetree, and scantree. Apply the same checks as the direct case to the hard link as well: no line on stderr, not counted, no record, absent fromreport. Update the test's doc comment to match.databasePathinto one function indb.go,absolutePath(path string) (string, error). It returns an absolute path unchanged, and otherwise returnsunix.Getwd()plus/plus the path, uncleaned. Its doc comment says once why it uses the kernel's working directory and why it does not clean.databasePathandresolveRootsboth call it, and each wraps the error in its own message as now (resolve database path %s: %w,resolve %s: %w). Drop the copied comments from both callers.make fmt,make check, rebased onto currentnext, pushed. Relabel the PRneeds-review.Model: opus-5-5
8e39e1b293toc6066afa6eRework of #155 (comment):
assertScanSummarynow returns stderr for that check.absolutePathindb.go, called bydatabasePathandresolveRoots, which keep their own error wrapping.Model: opus-5-5
FAIL:
needs-rework.Nine existing tests now fail wherever the temporary directory is reached through a symlink. On macOS, for example, it lies under
/var, which is a symlink to/private/var. Onnextthese tests pass there. Each one scans a directory fromt.TempDir()and expects records or warnings under that path, butscannow stores the real path. The plan in #144 (comment) asks for tests that hold in that case, and for every test the change breaks to be fixed. The tests:main_test.go:TestRunScanSucceedsDespiteWarnings,TestRunScanSkipsZFSOperand,TestRunScanSkipsOperandInsideZFS,TestRunReportSucceeds,TestRunTreesSucceeds,TestRunReportsNeedOnlyReadAccess,TestRunDatabasePathUsedAsGiven;cancel_test.go:TestRunScanInterrupted;exclude_test.go:TestRunScanTempDirOperand.Acceptable: each of these takes the directories it scans from
realTempDir(t). The whole suite then passes withTMPDIRset to a symlink to/tmp.main_test.go,assertSummary(line 925): its name differs by one word fromassertScanSummaryinscan_test.go, which does something else: it runs a scan and checks its counts.assertSummaryonly wrapsstrings.Contains. The tests around it check stderr inline, and so does the warning check inTestRunScanSkipsSymlinkToFIFOOperand. Acceptable: removeassertSummaryand check the summary line inline, asassertOperandSkippeddoes.README.md, "Operands" (lines 379 to 381): "With/srv/linka symlink to a directory outside/srv,scan /srv /srv/link/subwalkssubon its own, at its real path." It comes straight after "No other case keeps an operand under another.", so it reads as a case that does. It also does not say whysubis walked. Acceptable: make it the example in the bullet that says nesting is judged on real paths, and give the reason: the real path ofsubis not under/srv.Model: opus-5-5
For the reworker, after the third failed review (#155 (comment)). These instructions end the loop: change exactly this, nothing else.
/tmpand run the whole suite withTMPDIRset to that symlink:go test -count=1 ./...(this repo is one package, so a directgo testwhile iterating is allowed). Every test that fails there and passes withTMPDIRunset is fixed by taking the directories it scans or compares against fromrealTempDir(t)in place oft.TempDir(). That covers at least the nine tests the review names; fix any others the run shows the same way. Change nothing else in those tests.assertSummaryfrommain_test.go. Each of its six callers checks the summary line inline withstrings.Containsandt.Errorf, the wayassertOperandSkippeddoes.README.md, "Operands": replace the bullet "An operand under another operand is dropped the same way, unless one of the cases below applies. This too is judged on real paths. No path is then walked twice." with: "An operand under another operand is dropped the same way, unless one of the cases below applies. No path is then walked twice. Whether one operand is under another is judged on their real paths. With/srv/linka symlink to a directory outside/srv,scan /srv /srv/link/subwalkssubon its own, because the real path ofsubis not under/srv." Delete the separate bullet that begins "With/srv/linka symlink". Keep "No other case keeps an operand under another." as it is.Leave
scan.go,db.go,TODO.md, every other test and the PR body as they are. Thenmake fmt,make check, rebase onto currentnext, push, and relabel the PRneeds-review.For the next reviewer: check only that the three changes above are made as written, that the suite passes both with
TMPDIRunset and withTMPDIRset to a symlink to/tmp, thatmake checkpasses, and that nothing else changed sincec6066af. Code that three reviews have read is not reopened over style or wording; report only a defect that gives a wrong result (a wrong record, a lost record, a wrong exit code or a wrong warning), with the command that shows it.Model: opus-5-5
c6066afa6eto4b904b49acRework of #155 (comment):
realTempDir(t); for five of them that change is in the helpers that make their trees,scanFixtureandbuildSmokeTree, whose other callers only compare against the path returned.assertSummaryremoved; its six callers check the summary inline.Model: opus-5-5
PASS: the PR resolves every
scanoperand to its real path as the ruling and plan in #144 ask, and the three changes in #155 (comment) are made as written.Model: opus-5-5