From 896d9a83a489c083d620254b640d4c3fc47459d2 Mon Sep 17 00:00:00 2001 From: Nils Lehnen <30603423+iderex@users.noreply.github.com> Date: Thu, 13 Aug 2026 02:23:16 +0200 Subject: [PATCH] Refuse a symbolic link under experiments/ (#61) The tree under experiments/ is written by whoever proposes an experiment, so it is the runner's input rather than its environment. A link in it was walked past in silence: an experiment directory reached only through one stated its question to nobody, because every rule about a record reads one the walk found and the walk found none. It is now refused where the walk meets it, and never opened, resolved or descended into. The walk reads through a filesystem it is given rather than through the one the process runs on, and that is the whole reason the refusal can be proved. A fixture that asks the machine for a link cannot be had on every platform the suite runs on: creating one on Windows needs SeCreateSymbolicLinkPrivilege, which an ordinary account does not hold, and os.Symlink returns error 1314 there. A tracked link arrives on such a checkout as a text file, so a case built that way asks about a link on one platform and about a file on another. Record 0007 keeps the default run unelevated and record 0012 runs the suite on windows/amd64, so neither route was available. A directory entry supplied to the walk needs no privilege and behaves the same everywhere. The case and its near neighbour are identical byte for byte and differ only in the line declaring the entry a link, so the refusal is shown to be about the link rather than about the name, the place or the bytes. What that pair leaves unproved is that an operating system reports a link as an entry of that type, which is the standard library's behaviour and not this runner's, and the harness says so where the departure is made. Subjects are unchanged: every refusal still names a path written the way the reader's machine writes one. Signed-off-by: Nils Lehnen <30603423+iderex@users.noreply.github.com> --- internal/check/check.go | 194 ++++++++++++------ internal/check/check_test.go | 11 +- internal/check/decision.go | 24 +-- internal/check/hardware.go | 32 ++- internal/check/hardware_test.go | 6 +- internal/check/harness_test.go | 165 +++++++++++++++ .../expected | 4 + .../expected-refusals | 1 + .../a-symbolic-link-under-experiments/links | 1 + .../near-neighbour | 1 + .../tree/experiments/one/EXPERIMENT.md | 3 + .../tree/experiments/up | 1 + .../expected | 4 + .../expected-refusals | 0 .../tree/experiments/one/EXPERIMENT.md | 3 + .../tree/experiments/up | 1 + 16 files changed, 353 insertions(+), 98 deletions(-) create mode 100644 testdata/cases/a-symbolic-link-under-experiments/expected create mode 100644 testdata/cases/a-symbolic-link-under-experiments/expected-refusals create mode 100644 testdata/cases/a-symbolic-link-under-experiments/links create mode 100644 testdata/cases/a-symbolic-link-under-experiments/near-neighbour create mode 100644 testdata/cases/a-symbolic-link-under-experiments/tree/experiments/one/EXPERIMENT.md create mode 100644 testdata/cases/a-symbolic-link-under-experiments/tree/experiments/up create mode 100644 testdata/cases/an-ordinary-file-where-a-link-would-be/expected create mode 100644 testdata/cases/an-ordinary-file-where-a-link-would-be/expected-refusals create mode 100644 testdata/cases/an-ordinary-file-where-a-link-would-be/tree/experiments/one/EXPERIMENT.md create mode 100644 testdata/cases/an-ordinary-file-where-a-link-would-be/tree/experiments/up diff --git a/internal/check/check.go b/internal/check/check.go index 29063df..be1cdcf 100644 --- a/internal/check/check.go +++ b/internal/check/check.go @@ -7,9 +7,11 @@ package check import ( "bytes" + "errors" "fmt" "io/fs" "os" + "path" "path/filepath" "regexp" "sort" @@ -129,6 +131,29 @@ const ( // never going to resolve here whatever the tree held. RecordNamesAPathOutsideTheRepository = "record-names-a-path-outside-the-repository" + // ExperimentsHoldsASymbolicLink refuses a symbolic link anywhere under + // experiments/, and refuses it rather than resolving it. The tree under + // that directory is written by whoever proposes an experiment, and a link + // in it is the escape a record's prose makes with a path, made by a tool + // instead: a checker that followed one would read a file nobody put in + // this repository and report the tree as being in order. Refusing is the + // cheaper rule and the one a reader can check, because a rule that follows + // a link and then judges where it landed has to be right about every + // filesystem the release targets, and one of them answers differently. + // + // It is refused rather than walked past, which is the state this replaces. + // A link where an experiment directory should be was skipped in silence, + // so an experiment reached only through one stated its question to nobody: + // every rule about a record reads one the walk found, and the walk found + // none. + // + // WHAT IT DOES NOT REACH. A link outside experiments/ is not this check's + // business, and a link inside one that points at a file inside the + // repository is refused exactly like one pointing out of it. Where a link + // goes is a question about a filesystem this cannot be right about, and + // answering it is not what the rule needs. + ExperimentsHoldsASymbolicLink = "experiments-holds-a-symbolic-link" + // RecordIsAboveTheSizeBound refuses a record larger than the bound above // without opening it. A checker that reads whatever it is pointed at // stops on the first tree somebody builds badly, and the first such tree @@ -344,39 +369,74 @@ func Walk(root string, now time.Time) (Result, error) { return res, fmt.Errorf("%s is not a directory", root) } - rootRefusals, err := refuseRootDirectories(root) + return walk(os.DirFS(root), root, now) +} + +// walk is the whole of the examination, over a filesystem it is given rather +// than over the one the process is running on. +// +// THE FILESYSTEM IS A PARAMETER SO THAT A DIRECTORY ENTRY CAN BE PUT IN FRONT +// OF THE WALK WITHOUT ASKING THE OPERATING SYSTEM FOR ONE. Refusing a symbolic +// link is a rule about what an entry is, and creating a symbolic link needs a +// privilege an ordinary Windows account does not hold: os.Symlink returns +// windows error 1314 there, measured on issue #61. A fixture that asks the +// machine for a link therefore fails on a platform record 0012 runs the suite +// on, for a reason that has nothing to do with this runner, and record 0007 +// keeps the default run unelevated. Reading through fs.FS is what lets the +// entry be supplied instead, so the rule is proved on every platform by the +// same fixture rather than on the ones whose accounts happen to be privileged. +// +// root is carried alongside and is used for one thing: building the path a +// refusal names, so that a reader is sent to the file as it sits on their +// machine rather than to a path relative to a root they would have to work out. +// Nothing is read through it. +func walk(fsys fs.FS, root string, now time.Time) (Result, error) { + res := Result{Root: root, Now: now} + + rootRefusals, err := refuseRootDirectories(fsys, root) if err != nil { return res, err } res.Refusals = append(res.Refusals, rootRefusals...) - if err := walkExperiments(root, &res); err != nil { + if err := walkExperiments(fsys, root, &res); err != nil { return res, err } - strayRefusals, err := refuseStrayRecords(root) + strayRefusals, err := refuseStrayRecords(fsys, root) if err != nil { return res, err } res.Refusals = append(res.Refusals, strayRefusals...) - decisions, decisionRefusals, err := refuseDecisions(root) + decisions, decisionRefusals, err := refuseDecisions(fsys, root) if err != nil { return res, err } - res.DecisionsPresent = decisionsPresent(root) + res.DecisionsPresent = decisionsPresent(fsys) res.Decisions = decisions res.Refusals = append(res.Refusals, decisionRefusals...) return res, nil } +// at is the path a refusal names: a location inside the walked filesystem, +// written the way the machine the reader is on writes a path. Every subject in +// this package goes through it, so the one place that decides what a reader is +// shown is here rather than at each refusal site. +func at(root, name string) string { + if name == "." { + return root + } + return filepath.Join(root, filepath.FromSlash(name)) +} + // decisionsPresent says whether the tree holds a decisions directory at all. // It is asked separately from the count because a tree with none and a tree // whose directory is empty both read zero records, and collapsing the two into // that zero is the failure this package exists to avoid. -func decisionsPresent(root string) bool { - info, err := os.Stat(filepath.Join(root, filepath.FromSlash(DecisionsDir))) +func decisionsPresent(fsys fs.FS) bool { + info, err := fs.Stat(fsys, DecisionsDir) return err == nil && info.IsDir() } @@ -384,39 +444,40 @@ func decisionsPresent(root string) bool { // every record it finds to the rules about a record. It is the walk the rest // of this package was built around; the two refusals either side of it in Walk // are about where a record is rather than about what one says. -func walkExperiments(root string, res *Result) error { +func walkExperiments(fsys fs.FS, root string, res *Result) error { var experiments []experiment - dir := filepath.Join(root, ExperimentsDir) - entries, err := os.ReadDir(dir) + entries, err := fs.ReadDir(fsys, ExperimentsDir) if err != nil { // No experiments directory is an ordinary state for this tree and // the caller is told about it rather than shown a zero that looks // like an empty one. Anything else is a tree the walk cannot read, // and reporting that as zero would be the failure this package // exists to avoid. - if os.IsNotExist(err) { + if errors.Is(err, fs.ErrNotExist) { return nil } - return fmt.Errorf("cannot read %s: %w", dir, err) + return fmt.Errorf("cannot read %s: %w", at(root, ExperimentsDir), err) } res.ExperimentsPresent = true for _, entry := range entries { - // os.ReadDir does not follow symbolic links, so a link pointing at a - // directory reports itself as a link and is not walked. The tree the - // runner reads is untrusted input, and following a link out of it is - // how a checker reads something nobody put in the repository. + // A directory listing does not follow symbolic links, so a link + // pointing at a directory reports itself as a link and is not walked + // here. What refuses it is the stray-record walk, which reaches every + // path under this one; this is the reading that leaves it alone. if !entry.IsDir() { continue } res.Directories++ - experiment := filepath.Join(dir, entry.Name()) - record := filepath.Join(experiment, RecordName) + experimentPath := path.Join(ExperimentsDir, entry.Name()) + recordPath := path.Join(experimentPath, RecordName) + experiment := at(root, experimentPath) + record := at(root, recordPath) seen := experimentAt(experiment, record, entry.Name()) - recordInfo, err := os.Stat(record) + recordInfo, err := fs.Stat(fsys, recordPath) if err != nil { - if os.IsNotExist(err) { + if errors.Is(err, fs.ErrNotExist) { res.Refusals = append(res.Refusals, Refusal{ Property: ExperimentHasNoRecord, Subject: experiment, @@ -450,13 +511,13 @@ func walkExperiments(root string, res *Result) error { experiments = append(experiments, seen) continue } - data, err := readRecord(record) + data, err := readRecord(fsys, root, recordPath) if err != nil { return err } res.Records++ res.Refusals = append(res.Refusals, refuseBytes(record, data)...) - res.Refusals = append(res.Refusals, refusePaths(root, record, data)...) + res.Refusals = append(res.Refusals, refusePaths(fsys, root, record, data)...) res.Refusals = append(res.Refusals, refuseQuestion(record, data)...) res.Refusals = append(res.Refusals, refuseState(record, data)...) res.Refusals = append(res.Refusals, refuseHeaderDates(record, data)...) @@ -466,7 +527,7 @@ func walkExperiments(root string, res *Result) error { // The only rule here that reads the directory as well as the record, // which is why it takes both and why it can fail: the others judge // bytes already in hand and this one walks. - hardwareRefusals, err := refuseHardware(experiment, record, data) + hardwareRefusals, err := refuseHardware(fsys, experimentPath, experiment, record, data) if err != nil { return err } @@ -499,8 +560,8 @@ func experimentAt(path, record, directory string) experiment { // reached a commit yet does not change that. The one exception is the // checkout's own machinery, which gitDir names and which is never part of the // tree the record describes. -func refuseRootDirectories(root string) ([]Refusal, error) { - entries, err := os.ReadDir(root) +func refuseRootDirectories(fsys fs.FS, root string) ([]Refusal, error) { + entries, err := fs.ReadDir(fsys, ".") if err != nil { return nil, fmt.Errorf("cannot read %s: %w", root, err) } @@ -512,7 +573,7 @@ func refuseRootDirectories(root string) ([]Refusal, error) { } refusals = append(refusals, Refusal{ Property: RootHoldsADirectoryTheLayoutDoesNotName, - Subject: filepath.Join(root, entry.Name()), + Subject: at(root, entry.Name()), Detail: fmt.Sprintf("record 0002 names %s at the root and this is not one of them, so adding it is a change to that record", rootDirectoriesInWords()), }) } @@ -543,23 +604,18 @@ func rootDirectoriesInWords() string { // runner that refused this repository for carrying the trees that prove it // could not be run here at all. A record that is genuinely misplaced under // testdata/ is therefore not refused, and a green run does not say otherwise. -func refuseStrayRecords(root string) ([]Refusal, error) { +func refuseStrayRecords(fsys fs.FS, root string) ([]Refusal, error) { var refusals []Refusal - // filepath.WalkDir does not follow symbolic links, so a link pointing out - // of the tree is reported as a link and never descended into. - err := filepath.WalkDir(root, func(path string, entry fs.DirEntry, err error) error { + err := fs.WalkDir(fsys, ".", func(name string, entry fs.DirEntry, err error) error { if err != nil { return err } if entry.IsDir() { - if path != root && (entry.Name() == gitDir || entry.Name() == FixturesDir) { + if name != "." && (entry.Name() == gitDir || entry.Name() == FixturesDir) { return fs.SkipDir } - deeper, err := depthOf(root, path) - if err != nil { - return err - } + deeper := depthOf(name) if deeper > WalkDepthBound { // Refused and not descended into, so the refusal is one line // naming the directory the walk stopped at rather than one @@ -568,7 +624,7 @@ func refuseStrayRecords(root string) ([]Refusal, error) { // same thing a hundred times. refusals = append(refusals, Refusal{ Property: TheTreeIsDeeperThanTheWalkReads, - Subject: path, + Subject: at(root, name), Detail: fmt.Sprintf("it is %d directories below %s and the walk reads at most %d, so nothing under it was examined", deeper, root, WalkDepthBound), }) @@ -576,23 +632,33 @@ func refuseStrayRecords(root string) ([]Refusal, error) { } return nil } + // A link is refused where the walk meets it and is never opened, + // resolved or descended into. It is the whole of the second half of + // the untrusted-input rule: the reading above judges a path a record + // wrote, and this judges a link somebody put in the tree, which is the + // same escape made by a tool rather than by a sentence. + if entry.Type()&fs.ModeSymlink != 0 && underExperiments(name) { + refusals = append(refusals, Refusal{ + Property: ExperimentsHoldsASymbolicLink, + Subject: at(root, name), + Detail: fmt.Sprintf("it is a symbolic link under %s, and a link is refused rather than followed, so what is at the other end was neither read nor examined", + ExperimentsDir), + }) + return nil + } if entry.Name() != RecordName { return nil } - relative, err := filepath.Rel(root, path) - if err != nil { - return fmt.Errorf("cannot place %s inside %s: %w", path, root, err) - } - segments := strings.Split(filepath.ToSlash(relative), "/") + segments := strings.Split(name, "/") if len(segments) == 3 && segments[0] == ExperimentsDir { return nil } refusals = append(refusals, Refusal{ Property: RecordOutsideThePlaceRecordsLive, - Subject: path, + Subject: at(root, name), Detail: fmt.Sprintf("a record lives at %s//%s and this one is at %s, so no rule about a record reaches it", - ExperimentsDir, RecordName, filepath.ToSlash(relative)), + ExperimentsDir, RecordName, name), }) return nil }) @@ -602,6 +668,16 @@ func refuseStrayRecords(root string) ([]Refusal, error) { return refusals, nil } +// underExperiments says whether a path inside the walked filesystem sits below +// the one directory an experiment may live in. It is asked of a link so that +// the refusal covers the tree whose contents this board does not write, and +// leaves alone the rest of a checkout, where a link is somebody's own +// arrangement of their own machine and no rule here has anything to say about +// it. +func underExperiments(name string) bool { + return strings.HasPrefix(name, ExperimentsDir+"/") +} + // refusePaths holds a record to the paths it names. A path that was removed // on purpose is the case this exists to catch rather than an exception to it: // the repair is to update the record to name the commit that removed the file, @@ -610,7 +686,7 @@ func refuseStrayRecords(root string) ([]Refusal, error) { // Name no path you do not intend to resolve. A record naming an example path // that was never meant to exist is refused, and that is expected rather than a // defect in the check. -func refusePaths(root, path string, data []byte) []Refusal { +func refusePaths(fsys fs.FS, root, path string, data []byte) []Refusal { var refusals []Refusal for _, named := range pathsNamedInProse(string(data)) { @@ -626,7 +702,11 @@ func refusePaths(root, path string, data []byte) []Refusal { }) continue } - if _, err := os.Stat(filepath.Join(root, filepath.FromSlash(named))); err == nil { + // The name is already a repository-relative path with forward + // slashes, which is what a filesystem reads, and the check above has + // said it stays inside the root. The one shape left to strip is the + // leading ./ the pattern allows, which fs.Stat refuses as a path. + if _, err := fs.Stat(fsys, strings.TrimPrefix(named, "./")); err == nil { continue } refusals = append(refusals, Refusal{ @@ -656,17 +736,15 @@ func insideTheRepository(root, named string) bool { return relative != ".." && !strings.HasPrefix(relative, ".."+string(filepath.Separator)) } -// depthOf says how many directories below the root a path sits. The root itself -// is zero. -func depthOf(root, path string) (int, error) { - relative, err := filepath.Rel(root, path) - if err != nil { - return 0, fmt.Errorf("cannot place %s inside %s: %w", path, root, err) - } - if relative == "." { - return 0, nil +// depthOf says how many directories below the root of the walked filesystem a +// path sits. The root itself is zero. A path inside a filesystem is already +// relative to it and already uses forward slashes, so there is nothing here to +// get wrong on a machine that writes paths another way. +func depthOf(name string) int { + if name == "." { + return 0 } - return len(strings.Split(filepath.ToSlash(relative), "/")), nil + return len(strings.Split(name, "/")) } // refuseQuestion holds a record to having written its question. Record 0008 @@ -849,10 +927,10 @@ func refuseBytes(path string, data []byte) []Refusal { // yet. It exists so that a record counted as read is one the walk actually // opened, rather than one it saw the name of, since a file that cannot be // opened would otherwise be counted as examined. -func readRecord(path string) ([]byte, error) { - data, err := os.ReadFile(path) +func readRecord(fsys fs.FS, root, name string) ([]byte, error) { + data, err := fs.ReadFile(fsys, name) if err != nil { - return nil, fmt.Errorf("cannot read %s: %w", path, err) + return nil, fmt.Errorf("cannot read %s: %w", at(root, name), err) } return data, nil } diff --git a/internal/check/check_test.go b/internal/check/check_test.go index a6087ca..c077ac0 100644 --- a/internal/check/check_test.go +++ b/internal/check/check_test.go @@ -22,12 +22,7 @@ import ( func TestCases(t *testing.T) { for name, want := range loadCases(t) { t.Run(name, func(t *testing.T) { - root := filepath.Join(casesDir, name, "tree") - if _, err := os.Stat(root); err != nil { - t.Fatalf("case %s has no tree: %v", name, err) - } - - got, err := Walk(root, fixedNow) + got, err := walkCase(t, name) if err != nil { t.Fatalf("walk failed: %v", err) } @@ -195,7 +190,7 @@ func TestFixtureBytesSurviveTheCheckout(t *testing.T) { // the source, and the reader is usually somebody who has just arrived. func TestARefusalNamesItsSubject(t *testing.T) { for name := range loadCases(t) { - result, err := Walk(filepath.Join(casesDir, name, "tree"), fixedNow) + result, err := walkCase(t, name) if err != nil { t.Fatalf("walk of %s failed: %v", name, err) } @@ -231,7 +226,7 @@ func TestWalkWritesNothing(t *testing.T) { before := fingerprint(t, casesDir) for name := range loadCases(t) { - if _, err := Walk(filepath.Join(casesDir, name, "tree"), fixedNow); err != nil { + if _, err := walkCase(t, name); err != nil { t.Fatalf("walk of %s failed: %v", name, err) } } diff --git a/internal/check/decision.go b/internal/check/decision.go index e5dd783..27274cf 100644 --- a/internal/check/decision.go +++ b/internal/check/decision.go @@ -1,9 +1,9 @@ package check import ( + "errors" "fmt" - "os" - "path/filepath" + "io/fs" "regexp" "sort" "strings" @@ -80,14 +80,13 @@ var supersession = regexp.MustCompile(`(?i)supersedes\s+(?:record\s+)?(\d{4})`) // along with what it refused. A tree with no decisions directory reads none, // which is an ordinary state for a fixture tree and is reported rather than // treated as an error. -func refuseDecisions(root string) (int, []Refusal, error) { - dir := filepath.Join(root, filepath.FromSlash(DecisionsDir)) - entries, err := os.ReadDir(dir) +func refuseDecisions(fsys fs.FS, root string) (int, []Refusal, error) { + entries, err := fs.ReadDir(fsys, DecisionsDir) if err != nil { - if os.IsNotExist(err) { + if errors.Is(err, fs.ErrNotExist) { return 0, nil, nil } - return 0, nil, fmt.Errorf("cannot read %s: %w", dir, err) + return 0, nil, fmt.Errorf("cannot read %s: %w", at(root, DecisionsDir), err) } var refusals []Refusal @@ -96,9 +95,9 @@ func refuseDecisions(root string) (int, []Refusal, error) { present := make(map[string]bool) var records []string - // os.ReadDir sorts by filename, so the record that reports a shared number - // is the later one of the pair every time this runs rather than whichever - // the filesystem happened to hand over first. + // A directory listing arrives sorted by filename, so the record that + // reports a shared number is the later one of the pair every time this + // runs rather than whichever the filesystem happened to hand over first. for _, entry := range entries { if entry.IsDir() { continue @@ -113,7 +112,8 @@ func refuseDecisions(root string) (int, []Refusal, error) { } for _, name := range records { - path := filepath.Join(dir, name) + inside := DecisionsDir + "/" + name + path := at(root, inside) number := decisionFileName.FindStringSubmatch(name)[1] if first, taken := numbers[number]; taken { @@ -126,7 +126,7 @@ func refuseDecisions(root string) (int, []Refusal, error) { numbers[number] = name } - data, err := os.ReadFile(path) + data, err := fs.ReadFile(fsys, inside) if err != nil { return read, nil, fmt.Errorf("cannot read %s: %w", path, err) } diff --git a/internal/check/hardware.go b/internal/check/hardware.go index 2f1a1b2..2316859 100644 --- a/internal/check/hardware.go +++ b/internal/check/hardware.go @@ -1,9 +1,9 @@ package check import ( + "errors" "fmt" "io/fs" - "os" "path/filepath" "strings" ) @@ -62,7 +62,7 @@ const ( // A record whose bytes do not parse as a record is not judged here, for the // reason refuseState and refuseHeaderDates both give: nothing can read a field // out of a file that has no header. -func refuseHardware(experiment, path string, data []byte) ([]Refusal, error) { +func refuseHardware(fsys fs.FS, inside, experiment, path string, data []byte) ([]Refusal, error) { record, err := ParseRecord(data) if err != nil { return nil, nil @@ -82,7 +82,7 @@ func refuseHardware(experiment, path string, data []byte) ([]Refusal, error) { }}, nil } - registered, err := harnessTestsUnder(experiment) + registered, err := harnessTestsUnder(fsys, inside, experiment) if err != nil { return nil, err } @@ -127,21 +127,21 @@ func refuseHardware(experiment, path string, data []byte) ([]Refusal, error) { // directory nobody wrote by hand. A directory below the bound is not descended // into, and TheTreeIsDeeperThanTheWalkReads is what refuses the tree that // reaches it. -func harnessTestsUnder(experiment string) ([]string, error) { +func harnessTestsUnder(fsys fs.FS, inside, experiment string) ([]string, error) { var registered []string - // filepath.WalkDir does not follow symbolic links, so a link pointing out - // of the experiment is reported as a link and never descended into. - err := filepath.WalkDir(experiment, func(path string, entry fs.DirEntry, err error) error { + // A tree walk does not follow symbolic links, so a link pointing out of + // the experiment is reported as a link and never descended into. What + // refuses such a link is the stray-record walk, which reaches every path + // under experiments/; this reading leaves it alone and registers nothing + // for it, because a name is all this reads and a link's name says nothing + // about what it points at. + err := fs.WalkDir(fsys, inside, func(name string, entry fs.DirEntry, err error) error { if err != nil { return err } if entry.IsDir() { - deeper, err := depthOf(experiment, path) - if err != nil { - return err - } - if deeper > WalkDepthBound { + if depthOf(name)-depthOf(inside) > WalkDepthBound { return fs.SkipDir } return nil @@ -149,15 +149,11 @@ func harnessTestsUnder(experiment string) ([]string, error) { if !entry.Type().IsRegular() || !strings.HasSuffix(entry.Name(), HarnessTestSuffix) { return nil } - relative, err := filepath.Rel(experiment, path) - if err != nil { - return fmt.Errorf("cannot place %s inside %s: %w", path, experiment, err) - } - registered = append(registered, filepath.ToSlash(relative)) + registered = append(registered, strings.TrimPrefix(name, inside+"/")) return nil }) if err != nil { - if os.IsNotExist(err) { + if errors.Is(err, fs.ErrNotExist) { return nil, nil } return nil, fmt.Errorf("cannot walk %s: %w", experiment, err) diff --git a/internal/check/hardware_test.go b/internal/check/hardware_test.go index 71e7f2d..64df799 100644 --- a/internal/check/hardware_test.go +++ b/internal/check/hardware_test.go @@ -59,7 +59,8 @@ func TestWhatCountsAsRegisteredWithTheHarness(t *testing.T) { for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - registered, err := harnessTestsUnder(filepath.Join(casesDir, tc.name, "tree", "experiments", "one")) + tree := filepath.Join(casesDir, tc.name, "tree") + registered, err := harnessTestsUnder(os.DirFS(tree), "experiments/one", filepath.Join(tree, "experiments", "one")) if err != nil { t.Fatalf("reading the directory failed: %v", err) } @@ -75,7 +76,8 @@ func TestWhatCountsAsRegisteredWithTheHarness(t *testing.T) { // experiments/ with no record at all is refused by ExperimentHasNoRecord, and a // second refusal about its harness files would name a repair nobody needs. func TestADirectoryThatIsNotThereRegistersNothing(t *testing.T) { - registered, err := harnessTestsUnder(filepath.Join(casesDir, "no-experiments-directory", "tree", "experiments", "nothing-here")) + tree := filepath.Join(casesDir, "no-experiments-directory", "tree") + registered, err := harnessTestsUnder(os.DirFS(tree), "experiments/nothing-here", filepath.Join(tree, "experiments", "nothing-here")) if err != nil { t.Fatalf("reading a directory that is not there failed: %v", err) } diff --git a/internal/check/harness_test.go b/internal/check/harness_test.go index 0804ce4..2e98005 100644 --- a/internal/check/harness_test.go +++ b/internal/check/harness_test.go @@ -19,10 +19,39 @@ // testdata/cases//near-neighbour the case that differs by the // smallest legal change, required // of a case that refuses +// testdata/cases//links one path per line, each an entry +// the walk is shown as a symbolic +// link, absent from most cases // // This file decides that layout. Nothing restates it, so there is nothing to // drift against it. // +// THE LAST ONE IS A DEPARTURE FROM THE PARAGRAPH ABOVE AND IT IS DELIBERATE. +// Every other case is bytes in the repository and nothing else. A link is not, +// because a checkout cannot be relied on to carry one: creating a symbolic +// link on Windows needs SeCreateSymbolicLinkPrivilege, which an ordinary +// account does not hold, and os.Symlink there returns windows error 1314. The +// measurement is on issue #61. A link stored as a tracked entry arrives on such +// a checkout as an ordinary file holding the target as text, so a case built +// that way would ask the runner about a link on one platform and about a text +// file on another while declaring one answer for both. Record 0012 runs the +// suite on windows/amd64 and #57 keeps every platform running the same suite +// with nothing skipped, so neither the tracked link nor a link built at run +// time is available. +// +// What a declared link costs and what it buys. The bytes under tree/ are still +// exactly what the walk read, and the only thing the harness supplies is the +// type of one directory entry. What that leaves unproved is that a link made +// by an operating system arrives as an entry of that type, which is the +// standard library's behaviour rather than this runner's. What it proves is +// the whole of what this runner decides: what it does when it meets one. +// +// The pair to read together is a-symbolic-link-under-experiments and its near +// neighbour an-ordinary-file-where-a-link-would-be. Their trees are identical +// byte for byte, and the only difference between them is the line declaring +// the entry a link, so the refusal is shown to be about the link and not about +// the name, the place or the bytes. +// // THE BOUND ON WHAT ANY OF THIS PROVES. Every comparison here is over which // properties were refused, and never over which line inside the runner refused // them. Two refusal sites producing one property are indistinguishable to @@ -38,6 +67,7 @@ package check import ( "fmt" + "io/fs" "os" "path/filepath" "sort" @@ -165,6 +195,141 @@ func readExpectation(t *testing.T, dir string) expectation { return exp } +// walkCase walks one case's tree and returns what the runner found. Every test +// that runs a case goes through it, so the decision about how a case is +// presented to the walk is made once. +func walkCase(t *testing.T, name string) (Result, error) { + t.Helper() + + tree := filepath.Join(casesDir, name, "tree") + if _, err := os.Stat(tree); err != nil { + t.Fatalf("case %s has no tree: %v", name, err) + } + + var fsys fs.FS = os.DirFS(tree) + if links := linksDeclaredBy(t, name); len(links) > 0 { + fsys = treeWithLinks{FS: fsys, links: links} + } + return walk(fsys, tree, fixedNow) +} + +// linksDeclaredBy reads the entries a case declares as symbolic links. A case +// declaring none is the ordinary case and reads as an empty set rather than as +// a missing file it has to apologise for. +func linksDeclaredBy(t *testing.T, name string) map[string]bool { + t.Helper() + + data, err := os.ReadFile(filepath.Join(casesDir, name, "links")) + if os.IsNotExist(err) { + return nil + } + if err != nil { + t.Fatalf("case %s: %v", name, err) + } + + links := make(map[string]bool) + for _, line := range strings.Split(string(data), "\n") { + if line = strings.TrimSpace(line); line != "" { + links[line] = true + } + } + return links +} + +// treeWithLinks is a case's tree with some of its entries reported as symbolic +// links. Everything else is read from the tree on disk, unchanged, so the case +// is still the files a reader can open. +type treeWithLinks struct { + fs.FS + links map[string]bool +} + +// ReadDir is the one thing this overrides, because a directory listing is +// where the walk learns what an entry is. +func (t treeWithLinks) ReadDir(name string) ([]fs.DirEntry, error) { + entries, err := fs.ReadDir(t.FS, name) + if err != nil { + return nil, err + } + for i, entry := range entries { + inside := entry.Name() + if name != "." { + inside = name + "/" + inside + } + if t.links[inside] { + entries[i] = declaredLink{DirEntry: entry} + } + } + return entries, nil +} + +// declaredLink is one directory entry the case declares to be a symbolic link. +// The name and the underlying file are the tree's own; the type is what this +// supplies. +type declaredLink struct { + fs.DirEntry +} + +func (d declaredLink) Type() fs.FileMode { return fs.ModeSymlink } + +func (d declaredLink) IsDir() bool { return false } + +func (d declaredLink) Info() (fs.FileInfo, error) { + info, err := d.DirEntry.Info() + if err != nil { + return nil, err + } + return linkInfo{FileInfo: info}, nil +} + +// linkInfo carries the same answers as the file on disk apart from the one bit +// that says what it is. +type linkInfo struct { + fs.FileInfo +} + +func (l linkInfo) Mode() fs.FileMode { return l.FileInfo.Mode()&^fs.ModeType | fs.ModeSymlink } + +func (l linkInfo) IsDir() bool { return false } + +// TestADeclaredLinkIsReportedAsOne holds the harness's own half of the link +// case. The refusal it feeds is proved by the case; that the harness really +// puts a link in front of the walk is proved here, because a declaration this +// dropped in silence would leave the case green against an ordinary file and +// the refusal unproved while the suite said otherwise. +func TestADeclaredLinkIsReportedAsOne(t *testing.T) { + const name = "a-symbolic-link-under-experiments" + + links := linksDeclaredBy(t, name) + if len(links) == 0 { + t.Fatalf("case %s declares no link, so the case for the link refusal is not the case it claims to be", name) + } + + tree := filepath.Join(casesDir, name, "tree") + plain, err := fs.ReadDir(os.DirFS(tree), ExperimentsDir) + if err != nil { + t.Fatalf("cannot read %s: %v", tree, err) + } + declared, err := fs.ReadDir(treeWithLinks{FS: os.DirFS(tree), links: links}, ExperimentsDir) + if err != nil { + t.Fatalf("cannot read %s: %v", tree, err) + } + + for i := range plain { + inside := ExperimentsDir + "/" + plain[i].Name() + wantLink := links[inside] + if got := declared[i].Type()&fs.ModeSymlink != 0; got != wantLink { + t.Errorf("%s is reported as a link %v, want %v", inside, got, wantLink) + } + if wantLink && plain[i].Type()&fs.ModeSymlink != 0 { + t.Errorf("%s is already a link on disk, so this case proves nothing about a declaration", inside) + } + if declared[i].Name() != plain[i].Name() { + t.Errorf("the declaration renamed %s to %s", plain[i].Name(), declared[i].Name()) + } + } +} + func mustAtoi(t *testing.T, dir, value string) int { t.Helper() n, err := strconv.Atoi(value) diff --git a/testdata/cases/a-symbolic-link-under-experiments/expected b/testdata/cases/a-symbolic-link-under-experiments/expected new file mode 100644 index 0000000..466fe70 --- /dev/null +++ b/testdata/cases/a-symbolic-link-under-experiments/expected @@ -0,0 +1,4 @@ +directories 1 +records 1 +experiments present +decisions absent diff --git a/testdata/cases/a-symbolic-link-under-experiments/expected-refusals b/testdata/cases/a-symbolic-link-under-experiments/expected-refusals new file mode 100644 index 0000000..6e370d7 --- /dev/null +++ b/testdata/cases/a-symbolic-link-under-experiments/expected-refusals @@ -0,0 +1 @@ +experiments-holds-a-symbolic-link diff --git a/testdata/cases/a-symbolic-link-under-experiments/links b/testdata/cases/a-symbolic-link-under-experiments/links new file mode 100644 index 0000000..ae476a8 --- /dev/null +++ b/testdata/cases/a-symbolic-link-under-experiments/links @@ -0,0 +1 @@ +experiments/up diff --git a/testdata/cases/a-symbolic-link-under-experiments/near-neighbour b/testdata/cases/a-symbolic-link-under-experiments/near-neighbour new file mode 100644 index 0000000..3499bd7 --- /dev/null +++ b/testdata/cases/a-symbolic-link-under-experiments/near-neighbour @@ -0,0 +1 @@ +an-ordinary-file-where-a-link-would-be diff --git a/testdata/cases/a-symbolic-link-under-experiments/tree/experiments/one/EXPERIMENT.md b/testdata/cases/a-symbolic-link-under-experiments/tree/experiments/one/EXPERIMENT.md new file mode 100644 index 0000000..71e86d7 --- /dev/null +++ b/testdata/cases/a-symbolic-link-under-experiments/tree/experiments/one/EXPERIMENT.md @@ -0,0 +1,3 @@ +# One + +The smallest tree the walk can count as an experiment with a record. diff --git a/testdata/cases/a-symbolic-link-under-experiments/tree/experiments/up b/testdata/cases/a-symbolic-link-under-experiments/tree/experiments/up new file mode 100644 index 0000000..a96aa0e --- /dev/null +++ b/testdata/cases/a-symbolic-link-under-experiments/tree/experiments/up @@ -0,0 +1 @@ +.. \ No newline at end of file diff --git a/testdata/cases/an-ordinary-file-where-a-link-would-be/expected b/testdata/cases/an-ordinary-file-where-a-link-would-be/expected new file mode 100644 index 0000000..466fe70 --- /dev/null +++ b/testdata/cases/an-ordinary-file-where-a-link-would-be/expected @@ -0,0 +1,4 @@ +directories 1 +records 1 +experiments present +decisions absent diff --git a/testdata/cases/an-ordinary-file-where-a-link-would-be/expected-refusals b/testdata/cases/an-ordinary-file-where-a-link-would-be/expected-refusals new file mode 100644 index 0000000..e69de29 diff --git a/testdata/cases/an-ordinary-file-where-a-link-would-be/tree/experiments/one/EXPERIMENT.md b/testdata/cases/an-ordinary-file-where-a-link-would-be/tree/experiments/one/EXPERIMENT.md new file mode 100644 index 0000000..71e86d7 --- /dev/null +++ b/testdata/cases/an-ordinary-file-where-a-link-would-be/tree/experiments/one/EXPERIMENT.md @@ -0,0 +1,3 @@ +# One + +The smallest tree the walk can count as an experiment with a record. diff --git a/testdata/cases/an-ordinary-file-where-a-link-would-be/tree/experiments/up b/testdata/cases/an-ordinary-file-where-a-link-would-be/tree/experiments/up new file mode 100644 index 0000000..a96aa0e --- /dev/null +++ b/testdata/cases/an-ordinary-file-where-a-link-would-be/tree/experiments/up @@ -0,0 +1 @@ +.. \ No newline at end of file