Make error message wording consistent (closes #165)

Error messages in mfer/ and internal/cli/ are lowercase except names and
acronyms, carry no "failed to" or command-name prefix, and each wrap names
only the operation and thing the wrapped error does not already name, so a
stacked message names what failed once. Wraps around errors that already
name their operation and path (os and afero path errors, url.Error, the
builder's path errors, the gpg helpers' own errors) are dropped. gpg's
stderr is appended to a gpg failure, and to the error for a signing key gpg
did not report, only when gpg wrote some. errHTTPStatus reads "unexpected
HTTP status"; both inner-not-set sentinels read "inner message not set".
No sentinel, errors.Is result or exit status changes.

Model: opus-5-5

README.md

@@ -9,8 +9,9 @@ downloading, streaming, and mirroring. It was first published in 2022. The
 manifest files' data is serialized with Google's
 [protobuf serialization format](https://developers.google.com/protocol-buffers).
 The structure of these files can be found
-[in the format specification](https://git.eeqj.de/sneak/mfer/src/branch/main/mfer/mf.proto)
+[in the format specification](docs/FORMAT.md), which refers to the protobuf
-which is included in the [project repository](https://git.eeqj.de/sneak/mfer).
+schema `mfer/mf.proto` for exact field numbers and types. Both are included in
+the [project repository](https://git.eeqj.de/sneak/mfer).
 
 The current version is pre-1.0 and while the repo was published in 2022, there
 has not yet been any versioned release. [SemVer](https://semver.org) will be

internal/cli/errmsg_test.go

@@ -405,6 +405,48 @@ func TestFetchHashMismatchMessage(t *testing.T) {
 	assert.EqualError(t, err, "download "+testFileTxt+": hash mismatch")
 }
 
+// TestFreshenBackslashPathMessage runs freshen on a tree that has gained a
+// file whose name holds a backslash, which a manifest path may not contain.
+func TestFreshenBackslashPathMessage(t *testing.T) {
+	t.Parallel()
+
+	fs := afero.NewOsFs()
+	root, manifestPath := setupFreshenDir(t, fs,
+		map[string]string{testFileTxt: "content"})
+	writeTestFile(t, fs, filepath.Join(root, `a\b.txt`), "new")
+
+	mfa := &CLIApp{Fs: fs}
+	cmd := mfa.freshenCommand()
+	cmd.Action = mfa.freshenManifestOperation
+
+	// freshenManifestOperation logs to the process-global logger.
+	err := runLocked(func() error {
+		return cmd.Run(context.Background(),
+			[]string{cmdFreshen, testFlagBase, root, manifestPath})
+	})
+	assert.EqualError(t, err,
+		`path "a\\b.txt" contains backslash; use forward slashes only`)
+}
+
+// TestFreshenReadErrorMessage has freshen hash a directory as though it
+// were a file, so reading it fails.
+func TestFreshenReadErrorMessage(t *testing.T) {
+	t.Parallel()
+
+	root := t.TempDir()
+	require.NoError(t, os.Mkdir(filepath.Join(root, "sub"), 0o750))
+
+	hasher := &freshenHasher{
+		fs:      afero.NewOsFs(),
+		absBase: root,
+		builder: mfer.NewBuilder(),
+	}
+
+	err := hasher.processEntry(&freshenEntry{path: "sub", needsHash: true})
+	assert.EqualError(t, err,
+		"read "+filepath.Join(root, "sub")+": is a directory")
+}
+
 func TestURLRequiredMessage(t *testing.T) {
 	t.Parallel()
 

internal/cli/freshen.go

@@ -295,19 +295,14 @@ func (h *freshenHasher) processEntry(e *freshenEntry) error {
 	_ = f.Close()
 
 	if err != nil {
-		return fmt.Errorf("hash %s: %w", e.path, err)
+		return err
 	}
 
 	h.hashedBytes += bytesRead
 	h.hashedFiles++
 
-	// Add to builder with computed hash
+	// Add to builder with computed hash; a refused path is named in the error
-	err = addFileToBuilder(h.builder, e.path, e.size, e.mtime, e.mode, hash)
+	return addFileToBuilder(h.builder, e.path, e.size, e.mtime, e.mode, hash)
-	if err != nil {
-		return fmt.Errorf("add %s: %w", e.path, err)
-	}
-
-	return nil
 }
 
 // writeFreshenedManifest writes the manifest atomically (write to a
@@ -600,7 +595,7 @@ func hashFile(r io.Reader, progress func(int64)) ([]byte, int64, error) {
 			break
 		}
 
-		// Returned unwrapped: the caller adds the path.
+		// Returned unwrapped: a read error already names the file.
 		if err != nil {
 			return nil, total, err
 		}

mfer/gpg.go

@@ -88,12 +88,12 @@ func gpgArgs(opts []string, positional ...string) []string {
 }
 
 // runGPG runs the gpg binary in batch mode with the given arguments and
-// optional stdin, returning captured stdout. If gpg fails, the error ends
+// optional stdin, returning captured stdout and stderr. If gpg fails, the
-// with what gpg wrote to stderr. gpg is killed when ctx ends or gpgTimeout
+// error ends with what gpg wrote to stderr. gpg is killed when ctx ends or
-// passes, whichever comes first.
+// gpgTimeout passes, whichever comes first.
 func runGPG(
 	ctx context.Context, stdin io.Reader, args ...string,
-) (*bytes.Buffer, error) {
+) (*bytes.Buffer, *bytes.Buffer, error) {
 	// exec.CommandContext kills only gpg itself. A gpg-agent that gpg
 	// starts runs detached and holds none of gpg's output, but another
 	// process gpg leaves behind (a wrapper script that runs the real gpg
@@ -130,12 +130,22 @@ func runGPG(
 		}
 	}
 
-	messages := strings.TrimSpace(stderr.String())
+	if err != nil {
-	if err != nil && messages != "" {
+		err = withStderr(err, &stderr)
-		err = fmt.Errorf("%w: %s", err, messages)
 	}
 
-	return &stdout, err
+	return &stdout, &stderr, err
+}
+
+// withStderr returns err followed by what gpg wrote to stderr, or err alone
+// when gpg wrote nothing.
+func withStderr(err error, stderr *bytes.Buffer) error {
+	messages := strings.TrimSpace(stderr.String())
+	if messages == "" {
+		return err
+	}
+
+	return fmt.Errorf("%w: %s", err, messages)
 }
 
 // parseFingerprint extracts the first fingerprint from gpg --with-colons
@@ -189,7 +199,7 @@ func gpgSign(
 
 	// The signature goes to sigFile, so --status-fd 1 can send gpg's status
 	// lines to stdout; its messages go to stderr.
-	stdout, err := runGPG(ctx, bytes.NewReader(data),
+	stdout, stderr, err := runGPG(ctx, bytes.NewReader(data),
 		"--detach-sign",
 		gpgOptArmor,
 		"--output", sigFile,
@@ -204,7 +214,7 @@ func gpgSign(
 	// made the signature.
 	created, ok := parseStatusLine(stdout.String(), "SIG_CREATED")
 	if !ok {
-		return nil, "", errSigningKeyNotReported
+		return nil, "", withStderr(errSigningKeyNotReported, stderr)
 	}
 
 	sig, err := os.ReadFile(sigFile) //nolint:gosec // G304: inside tmpDir, made above
@@ -218,7 +228,7 @@ func gpgSign(
 // gpgExportPublicKey exports the public key for the specified key ID.
 // Returns the armored public key.
 func gpgExportPublicKey(ctx context.Context, keyID GPGKeyID) ([]byte, error) {
-	stdout, err := runGPG(ctx, nil,
+	stdout, _, err := runGPG(ctx, nil,
 		gpgArgs([]string{"--export", gpgOptArmor}, string(keyID))...,
 	)
 	if err != nil {
@@ -234,7 +244,7 @@ func gpgExportPublicKey(ctx context.Context, keyID GPGKeyID) ([]byte, error) {
 
 // gpgGetKeyFingerprint gets the full fingerprint for a key ID.
 func gpgGetKeyFingerprint(ctx context.Context, keyID GPGKeyID) ([]byte, error) {
-	stdout, err := runGPG(ctx, nil,
+	stdout, _, err := runGPG(ctx, nil,
 		gpgArgs([]string{"--with-colons", "--fingerprint"}, string(keyID))...,
 	)
 	if err != nil {
@@ -254,7 +264,7 @@ func gpgGetKeyFingerprint(ctx context.Context, keyID GPGKeyID) ([]byte, error) {
 func gpgImportOneKey(ctx context.Context, gpgHome, pubKeyFile string) error {
 	// --status-fd 1 sends gpg's status lines to stdout, which importing
 	// otherwise leaves empty; its messages go to stderr.
-	importStdout, err := runGPG(ctx, nil,
+	importStdout, _, err := runGPG(ctx, nil,
 		gpgArgs([]string{gpgOptHomedir, gpgHome, gpgOptStatusFD, "1", "--import"},
 			pubKeyFile)...,
 	)
@@ -328,7 +338,7 @@ func gpgVerify(ctx context.Context, data, signature, pubKey []byte) (string, err
 
 	// --status-fd 1 sends gpg's status lines to stdout, which verifying a
 	// detached signature otherwise leaves empty; its messages go to stderr.
-	verifyStdout, err := runGPG(ctx, nil,
+	verifyStdout, _, err := runGPG(ctx, nil,
 		gpgArgs([]string{gpgOptHomedir, tmpDir, gpgOptStatusFD, "1", gpgOptVerify},
 			sigFile, dataFile)...,
 	)

mfer/gpg_test.go

@@ -451,7 +451,7 @@ func TestManifestRefusesSecondEmbeddedKeyWithoutUserID(t *testing.T) {
 	t.Setenv("GNUPGHOME", otherHome)
 
 	// Keeping only the user IDs that match "nobody" exports none.
-	otherPubKey, err := runGPG(context.Background(), nil,
+	otherPubKey, _, err := runGPG(context.Background(), nil,
 		gpgArgs([]string{
 			"--export", gpgOptArmor, "--export-filter", "keep-uid=uid = nobody",
 		}, string(otherKey))...)
@@ -616,6 +616,20 @@ func TestGPGTimeoutKillsGPG(t *testing.T) {
 	assert.EqualError(t, err, "gpg sign: timed out: context deadline exceeded")
 }
 
+// TestGPGSignKeyNotReportedKeepsStderr puts a fake gpg first on PATH that
+// exits cleanly without reporting the key that signed, and checks that what
+// it wrote to stderr is in the message.
+func TestGPGSignKeyNotReportedKeepsStderr(t *testing.T) {
+	t.Setenv("PATH", fakeGPGPath(t,
+		"#!/bin/sh\necho 'gpg: note from the fake gpg' >&2\n"))
+
+	_, _, err := gpgSign(context.Background(), []byte("data"), GPGKeyID("any"))
+	require.ErrorIs(t, err, errSigningKeyNotReported)
+	assert.EqualError(t, err,
+		"gpg did not report the key that made the signature: "+
+			"gpg: note from the fake gpg")
+}
+
 // TestGPGCancelWhenChildHoldsOutput uses a fake gpg that runs sleep as a
 // child instead of exec-ing it, the way a wrapper script around the real
 // gpg might. Killing the fake gpg leaves sleep holding its stdout and