What was wrong: with --skip-errors, snapshot restore --verify also verified the files that had failed to restore. Each failed again, never having been written, and joined the same list as its restore failure, so the verification summary listed it twice as a checksum mismatch.
What changed:
Verification is given only the files that did not fail to restore.
Verification failures go to their own list in RestoreResult (VerifyFailedFiles), so the restore warning lists the failed restores and the verification summary the failed verifications, each path once. A restore that skipped files still exits non-zero.
README: --verify checks every restored file; a file skipped under --skip-errors is not verified.
Tests restore a snapshot with one of its two blobs deleted, with skip-errors and verify. With nothing else wrong, only the written files are verified and none fails. With another restored file written one byte too long, the restore warning lists only the deleted blob's files, the verification summary only the long file, each once, and the restore fails.
Judgement call: "each path once" applies to the two end-of-run lists; the "Failed to restore ... Skipping" line printed for each file during the restore is unchanged.
Judgement call: the new list has no count field beside it (its length is the count), unlike FilesFailed/FailedFiles.
Judgement call: the second test has its own small trailing-byte file-system wrapper; the one in restore_perms_test.go is in the package's internal tests, out of reach of the external test package holding the deleted-blob setup.
Model: opus-5-5
Fixes https://git.eeqj.de/sneak/vaultik/issues/323.
**What was wrong:** with `--skip-errors`, `snapshot restore --verify` also verified the files that had failed to restore. Each failed again, never having been written, and joined the same list as its restore failure, so the verification summary listed it twice as a checksum mismatch.
**What changed:**
- Verification is given only the files that did not fail to restore.
- Verification failures go to their own list in `RestoreResult` (`VerifyFailedFiles`), so the restore warning lists the failed restores and the verification summary the failed verifications, each path once. A restore that skipped files still exits non-zero.
- README: `--verify` checks every restored file; a file skipped under `--skip-errors` is not verified.
- Tests restore a snapshot with one of its two blobs deleted, with skip-errors and verify. With nothing else wrong, only the written files are verified and none fails. With another restored file written one byte too long, the restore warning lists only the deleted blob's files, the verification summary only the long file, each once, and the restore fails.
Judgement call: "each path once" applies to the two end-of-run lists; the "Failed to restore ... Skipping" line printed for each file during the restore is unchanged.
Judgement call: the new list has no count field beside it (its length is the count), unlike `FilesFailed`/`FailedFiles`.
Judgement call: the second test has its own small trailing-byte file-system wrapper; the one in `restore_perms_test.go` is in the package's internal tests, out of reach of the external test package holding the deleted-blob setup.
Model: opus-5-5
The separate verification list has no test.internal/vaultik/restore_skip_errors_test.go only covers a restore where no restored file fails verification. Nothing checks the case the separate VerifyFailedFiles list in internal/vaultik/restore.go exists for: one file fails to restore and another restored file fails verification. The definition of done in #323 asks for the two to be reported apart, each path once. As it stands, the verification summary could list the restore failures again, or a verification failure could be added to the restore list too, and every test would still pass. Acceptable: a test in which one file is skipped under skip-errors and another restored file fails verification (the file-system wrapper in internal/vaultik/restore_perms_test.go that appends a trailing byte can cause that). It should check that the restore warning lists only the skipped file, that the verification summary lists only the bad file, each once, and that Restore returns an error.
Model: opus-5-5
**The separate verification list has no test.** `internal/vaultik/restore_skip_errors_test.go` only covers a restore where no restored file fails verification. Nothing checks the case the separate `VerifyFailedFiles` list in `internal/vaultik/restore.go` exists for: one file fails to restore and another restored file fails verification. The definition of done in https://git.eeqj.de/sneak/vaultik/issues/323 asks for the two to be reported apart, each path once. As it stands, the verification summary could list the restore failures again, or a verification failure could be added to the restore list too, and every test would still pass. Acceptable: a test in which one file is skipped under skip-errors and another restored file fails verification (the file-system wrapper in `internal/vaultik/restore_perms_test.go` that appends a trailing byte can cause that). It should check that the restore warning lists only the skipped file, that the verification summary lists only the bad file, each once, and that `Restore` returns an error.
Model: opus-5-5
With --skip-errors, a file that failed to restore was verified anyway,
failed again because it was never written, and was added to the same
failure list as the restore failure. restore --verify then listed it
twice and called every entry a checksum mismatch.
Verification now skips the files that failed to restore and keeps its
own failures in a separate list, so failed restores and failed
verifications are reported apart, each path once. The restore still
exits non-zero. The tests cover a restore with skipped files where
nothing fails verification, and one where another restored file also
fails it.
Judgement call: the second test has its own trailing-byte file-system
wrapper; the one in restore_perms_test.go is in the internal test package.
Model: opus-5-5
Separate verification list has no test: added TestRestoreSkipErrorsVerifyReportsFailuresApart in internal/vaultik/restore_skip_errors_test.go. One blob is deleted, so its files are skipped under skip-errors, and another restored file is written one byte too long. It checks that the restore warning lists only the skipped files, the verification summary only the long file, each once, and that Restore returns an error.
Judgement call: the test has its own small trailing-byte file-system wrapper. The one in restore_perms_test.go is in the package's internal tests, out of reach of the external test package holding the deleted-blob setup.
Rebased onto current next.
Model: opus-5-5
Rework:
- Separate verification list has no test: added `TestRestoreSkipErrorsVerifyReportsFailuresApart` in `internal/vaultik/restore_skip_errors_test.go`. One blob is deleted, so its files are skipped under skip-errors, and another restored file is written one byte too long. It checks that the restore warning lists only the skipped files, the verification summary only the long file, each once, and that `Restore` returns an error.
- Judgement call: the test has its own small trailing-byte file-system wrapper. The one in `restore_perms_test.go` is in the package's internal tests, out of reach of the external test package holding the deleted-blob setup.
- Rebased onto current `next`.
Model: opus-5-5
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.
Fixes #323.
What was wrong: with
--skip-errors,snapshot restore --verifyalso verified the files that had failed to restore. Each failed again, never having been written, and joined the same list as its restore failure, so the verification summary listed it twice as a checksum mismatch.What changed:
RestoreResult(VerifyFailedFiles), so the restore warning lists the failed restores and the verification summary the failed verifications, each path once. A restore that skipped files still exits non-zero.--verifychecks every restored file; a file skipped under--skip-errorsis not verified.Judgement call: "each path once" applies to the two end-of-run lists; the "Failed to restore ... Skipping" line printed for each file during the restore is unchanged.
Judgement call: the new list has no count field beside it (its length is the count), unlike
FilesFailed/FailedFiles.Judgement call: the second test has its own small trailing-byte file-system wrapper; the one in
restore_perms_test.gois in the package's internal tests, out of reach of the external test package holding the deleted-blob setup.Model: opus-5-5
The separate verification list has no test.
internal/vaultik/restore_skip_errors_test.goonly covers a restore where no restored file fails verification. Nothing checks the case the separateVerifyFailedFileslist ininternal/vaultik/restore.goexists for: one file fails to restore and another restored file fails verification. The definition of done in #323 asks for the two to be reported apart, each path once. As it stands, the verification summary could list the restore failures again, or a verification failure could be added to the restore list too, and every test would still pass. Acceptable: a test in which one file is skipped under skip-errors and another restored file fails verification (the file-system wrapper ininternal/vaultik/restore_perms_test.gothat appends a trailing byte can cause that). It should check that the restore warning lists only the skipped file, that the verification summary lists only the bad file, each once, and thatRestorereturns an error.Model: opus-5-5
017877da44to60c83844ce60c83844cetoa92646f7a6a92646f7a6to0b6d69fbd6Rework:
TestRestoreSkipErrorsVerifyReportsFailuresApartininternal/vaultik/restore_skip_errors_test.go. One blob is deleted, so its files are skipped under skip-errors, and another restored file is written one byte too long. It checks that the restore warning lists only the skipped files, the verification summary only the long file, each once, and thatRestorereturns an error.restore_perms_test.gois in the package's internal tests, out of reach of the external test package holding the deleted-blob setup.next.Model: opus-5-5
Review passed.
Model: opus-5-5