From 730fb0efb460c3ede74b0c563fb3cf6a4d3c0d2d Mon Sep 17 00:00:00 2001 From: Samuel Cui Date: Sat, 19 Sep 2026 16:58:32 +0800 Subject: [PATCH] docs: correct the statements the release review falsified Each of these described behaviour the code does not have: - The nil-item note on Submit and the comment on its test claimed a rejected submission ends the run, while the feed keeps accepting items; the test now pins that follow-up submissions are still accepted and complete. - The buildOption comment claimed every repeated option is last-wins, although the job and device options accumulate. - The usesCache comment claimed stored hashes are always read, while HashRead never looks one up; the function decides whether a run needs a cache at all. - TESTING.md claimed the non-linear pre-allocation on /dev/full reports ErrTargetIO. Measured on Linux, fallocate there returns ENODEV, which matches none of the classified errors (ENOSPC, EROFS, EIO), so the target carries the device's own error. - TESTING.md's "every signature cache test" filter missed TestRunHoldsTargetCacheUntilTheHashIsComplete and TestManagedSignatureIsNotOrdinaryCopiedXattr. - The Windows vet diagnostic that make check does not run is documented where the cross-platform commands live, with the reason it is accepted instead of worked around. --- TESTING.md | 16 +++++++++++++--- acp.go | 6 ++++-- copy_test.go | 18 +++++++++++++----- hash_policy.go | 5 ++++- 4 files changed, 34 insertions(+), 11 deletions(-) diff --git a/TESTING.md b/TESTING.md index bd63d3c..1edef7f 100644 --- a/TESTING.md +++ b/TESTING.md @@ -206,8 +206,11 @@ go test -race -run '^(TestRunMapsDeviceFullToTargetNoSpace|TestDeviceFullWriteFa ``` These three select no test on another platform: the file that defines them is Linux-only. Note -that `/dev/full` refuses `fallocate` with `ENODEV`, so the non-linear pre-allocation there reports -`ErrTargetIO`; the `ErrTargetNoSpace` identity for that path comes from the filled-volume test. +that `/dev/full` refuses `fallocate` with `ENODEV`, which is none of the errors a target operation +classifies (`ENOSPC`, `EROFS`, `EIO`): the non-linear pre-allocation there fails that target with +the device's own `ENODEV`, not with `ErrTargetIO` and not with `ErrTargetNoSpace`. The +`ErrTargetNoSpace` identity for that path comes from the filled-volume test, which produces a real +`ENOSPC`. Run hash policy tests, which pin the reuse, read, and refresh matrix, and the no-op policy of a file system without the managed signature attribute: @@ -221,7 +224,7 @@ cache entry never travels through a reopened path, and the publication-order tes item publishes its entry before its result: ```sh -go test -run '^(TestCachedSignatureCodec|TestSignatureCache|TestRunSignature|TestRunHashPolicy|TestRunRefresh|TestRunTransfer|TestOverwriteInvalidates|TestRunCorruptSignature|TestRunReadsTheStoredHash|TestRunWrites|TestRunPublishes)' . +go test -run '^(TestCachedSignatureCodec|TestSignatureCache|TestRunSignature|TestRunHashPolicy|TestRunRefresh|TestRunTransfer|TestOverwriteInvalidates|TestRunCorruptSignature|TestRunReadsTheStoredHash|TestRunWrites|TestRunPublishes|TestRunHoldsTargetCache|TestManagedSignature)' . ``` Run `acp-rewrite` tests, which pin one rewrite through its scratch file, the state and report @@ -263,6 +266,13 @@ Compile the Windows-specific root package tests without running them: GOOS=windows GOARCH=amd64 go test -c -o /tmp/acp-windows.test . ``` +`make check` builds for Windows but vets only the host platform. `GOOS=windows GOARCH=amd64 go +vet ./...` reports `possible misuse of unsafe.Pointer` in `mmap/mmap_windows.go`, on the +`unsafe.Slice` call that turns the address `syscall.MapViewOfFile` returned into the mapped slice. +That conversion is the supported way to read a mapped address, vet's `unsafeptr` check cannot know +that a syscall result is a valid pointer, and the only formulation it accepts is the deprecated +`reflect.SliceHeader` pattern, so the diagnostic is accepted instead of worked around. + ## Change checklist Before submitting a change: diff --git a/acp.go b/acp.go index 35063dc..7f699cf 100644 --- a/acp.go +++ b/acp.go @@ -102,7 +102,8 @@ func NewStream(ctx context.Context, onResults func([]Result) error, opts ...Opti return newStream(ctx, onResults, opt) } -// buildOption applies every option in order, so a repeated option is last-wins. +// buildOption applies every option in order, so a repeated value option is last-wins while the +// options that accumulate build on what came before them. func buildOption(opts ...Option) (*option, error) { opt := newOption() for _, o := range opts { @@ -158,7 +159,8 @@ func newStream(ctx context.Context, onResults func([]Result) error, opt *option) // results callback returned an error, or after a fatal pipeline failure. // // A single submitter is required, and Items must be submitted before Close. A nil item is a -// submission error and ends the run. +// submission error that does not end the feed: it is recorded as the run's error, so Wait reports +// it, and the items submitted after it are still accepted. func (c *StreamCopyer) Submit(items ...Item) error { if err := c.feedStop(); err != nil { // An exhausted linear target stays an item outcome: the items that reached it carry diff --git a/copy_test.go b/copy_test.go index 0351819..027b129 100644 --- a/copy_test.go +++ b/copy_test.go @@ -641,13 +641,14 @@ func TestStoppedLinearTargetRefusesSubmission(t *testing.T) { } } -// TestSubmitRejectsANilItem pins the feed's input validation: a nil item is a submission error -// that ends the run, and the items accepted before it keep their outcome. +// TestSubmitRejectsANilItem pins the feed's input validation: a nil item is a submission error that +// is recorded as the run error without ending the feed, and every accepted item keeps its outcome. func TestSubmitRejectsANilItem(t *testing.T) { root := t.TempDir() input := writeSourceFile(t, root, "source.txt", []byte("fixture")) item := newFixtureItem(input, filepath.Join(root, "target.txt")) - fixture := newStreamFixture(item) + after := newFixtureItem(input, filepath.Join(root, "target-after.txt")) + fixture := newStreamFixture(item, after) stream, err := NewStream(context.Background(), fixture.onResults) if err != nil { @@ -659,14 +660,21 @@ func TestSubmitRejectsANilItem(t *testing.T) { if err := stream.Submit(nil); err == nil { t.Fatal("Submit(nil) error = nil, want a rejected item") } + // A rejected submission is that submission's error, not a stopping reason: the run still + // accepts what the caller submits next. + if err := stream.Submit(after); err != nil { + t.Fatalf("Submit() after a rejected item error = %v, want the item accepted", err) + } if err := stream.Close(); err != nil { t.Fatalf("Close() error = %v", err) } if err := stream.Wait(); err == nil { t.Fatal("Wait() error = nil, want the rejected item") } - if _, err := item.terminal(t); err != nil { - t.Fatalf("item failed: %v", err) + for _, submitted := range []*fixtureItem{item, after} { + if _, err := submitted.terminal(t); err != nil { + t.Fatalf("item failed: %v", err) + } } } diff --git a/hash_policy.go b/hash_policy.go index f8296a9..95b1d5c 100644 --- a/hash_policy.go +++ b/hash_policy.go @@ -79,7 +79,10 @@ func (p HashPolicy) producesHash() bool { return p != HashOff } -// usesCache reports whether stored hashes are read at all. +// usesCache reports whether the policy touches the stored cache at all: it either reuses a stored +// hash, refreshes one with a computed hash, or both. It is what decides whether a run needs a +// signature cache, and it is true for HashRead, which hashes content and writes nothing: +// reusesCache reports the lookup, refreshesCache reports the write. func (p HashPolicy) usesCache() bool { return p != HashOff }