From 82337cf7ad516aff3a02a5ebb3c67d7bd52b6428 Mon Sep 17 00:00:00 2001 From: Snider Date: Thu, 30 Apr 2026 17:52:20 +0100 Subject: [PATCH] refactor(go): use core.E consistently instead of core.NewError / core.Errorf (Mantis #1233) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Migrate go-git error construction to core.E(op, msg, cause) — the canonical core/go primitive. Replaces ad-hoc core.NewError / core.Errorf call sites in: - go/git.go — resultError uses core.E with the op label - go/service.go — gitServiceErrorWithArgs uses the first arg as op discriminator (more specific than the previous flat "git.service" label) - go/tests/cli/git/main.go — non-test CLI driver migrated Build, vet, test all clean (sandbox-safe cache). Out of scope (left as follow-up): - core.Cmd git execution (push/pull) stays in place — those helpers don't have *core.Core access and need attached stdio for interactive SSH prompts. Migration to c.Process() would lose interactive behaviour and require public API changes. - 2 result-discards in production code (audit.sh dim) — separate ticket. Closes tasks.lthn.sh/view.php?id=1233 Co-authored-by: Codex --- go/git.go | 4 +-- go/service.go | 8 +++--- go/tests/cli/git/main.go | 53 +++++++++++++++++++++------------------- 3 files changed, 34 insertions(+), 31 deletions(-) diff --git a/go/git.go b/go/git.go index 393eb35..102426a 100644 --- a/go/git.go +++ b/go/git.go @@ -31,9 +31,9 @@ func resultError(r core.Result) *GitError { } if r.Value != nil { msg := core.Sprint(r.Value) - return &GitError{Err: core.NewError(msg), Stderr: msg} + return &GitError{Err: core.E("git.resultError", msg, nil), Stderr: msg} } - return &GitError{Err: core.NewError("operation failed"), Stderr: "operation failed"} + return &GitError{Err: core.E("git.resultError", "operation failed", nil), Stderr: "operation failed"} } func gitCmd(dir string, args ...string) *core.Cmd { diff --git a/go/service.go b/go/service.go index 282d9d3..1a1ae8b 100644 --- a/go/service.go +++ b/go/service.go @@ -376,11 +376,11 @@ func gitServiceError(op, msg string, err error) *GitError { } func gitServiceErrorWithArgs(args []string, msg string, err error) *GitError { - if err == nil { - err = core.NewError(msg) - } else { - err = core.E("git.service", msg, err) + op := "git.service" + if len(args) > 0 && args[0] != "" { + op = args[0] } + err = core.E(op, msg, err) return &GitError{ Args: args, Err: err, diff --git a/go/tests/cli/git/main.go b/go/tests/cli/git/main.go index 699032e..c693dd2 100644 --- a/go/tests/cli/git/main.go +++ b/go/tests/cli/git/main.go @@ -90,41 +90,42 @@ func verifyStatus(ctx core.Context) core.Result { func verifyStatusResults(statuses []gitlib.RepoStatus, clean, dirty string) core.Result { if len(statuses) != 2 { - return core.Fail(core.Errorf("expected 2 statuses, got %d", len(statuses))) + return core.Fail(core.E("git.cli.verifyStatusResults", core.Sprintf("expected 2 statuses, got %d", len(statuses)), nil)) } cleanStatus := statuses[0] if cleanStatus.Name != "clean-repo" { - return core.Fail(core.Errorf("clean name = %q", cleanStatus.Name)) + return core.Fail(core.E("git.cli.verifyStatusResults", core.Sprintf("clean name = %q", cleanStatus.Name), nil)) } if cleanStatus.Path != clean { - return core.Fail(core.Errorf("clean file = %q", cleanStatus.Path)) + return core.Fail(core.E("git.cli.verifyStatusResults", core.Sprintf("clean file = %q", cleanStatus.Path), nil)) } if cleanStatus.Error != nil { return core.Fail(cleanStatus.Error) } if cleanStatus.Branch == "" { - return core.Fail(core.NewError("clean branch should not be empty")) + return core.Fail(core.E("git.cli.verifyStatusResults", "clean branch should not be empty", nil)) } if cleanStatus.IsDirty() { - return core.Fail(core.NewError("clean repo reported dirty")) + return core.Fail(core.E("git.cli.verifyStatusResults", "clean repo reported dirty", nil)) } dirtyStatus := statuses[1] if dirtyStatus.Name != "dirty-repo" { - return core.Fail(core.Errorf("dirty name = %q", dirtyStatus.Name)) + return core.Fail(core.E("git.cli.verifyStatusResults", core.Sprintf("dirty name = %q", dirtyStatus.Name), nil)) } if dirtyStatus.Path != dirty { - return core.Fail(core.Errorf("dirty file = %q", dirtyStatus.Path)) + return core.Fail(core.E("git.cli.verifyStatusResults", core.Sprintf("dirty file = %q", dirtyStatus.Path), nil)) } if dirtyStatus.Error != nil { return core.Fail(dirtyStatus.Error) } if !dirtyStatus.IsDirty() { - return core.Fail(core.NewError("dirty repo reported clean")) + return core.Fail(core.E("git.cli.verifyStatusResults", "dirty repo reported clean", nil)) } if dirtyStatus.Modified != 1 || dirtyStatus.Staged != 1 || dirtyStatus.Untracked != 1 { - return core.Fail(core.Errorf("dirty counts = modified:%d staged:%d untracked:%d", dirtyStatus.Modified, dirtyStatus.Staged, dirtyStatus.Untracked)) + msg := core.Sprintf("dirty counts = modified:%d staged:%d untracked:%d", dirtyStatus.Modified, dirtyStatus.Staged, dirtyStatus.Untracked) + return core.Fail(core.E("git.cli.verifyStatusResults", msg, nil)) } return core.Ok(nil) @@ -232,7 +233,7 @@ func verifyPushAndPullMultiple(ctx core.Context, workspace pushPullWorkspace) co } pushResults := pushMultiple.Value.([]gitlib.PushResult) if len(pushResults) != 1 || !pushResults[0].Success || pushResults[0].Name != "push" { - return core.Fail(core.Errorf("unexpected push multiple results: %+v", pushResults)) + return core.Fail(core.E("git.cli.verifyPushAndPullMultiple", core.Sprintf("unexpected push multiple results: %+v", pushResults), nil)) } pullMultiple := gitlib.PullMultiple(ctx, []string{workspace.pullClone}, map[string]string{workspace.pullClone: "pull"}) @@ -241,7 +242,7 @@ func verifyPushAndPullMultiple(ctx core.Context, workspace pushPullWorkspace) co } pullResults := pullMultiple.Value.([]gitlib.PullResult) if len(pullResults) != 1 || !pullResults[0].Success || pullResults[0].Name != "pull" { - return core.Fail(core.Errorf("unexpected pull multiple results: %+v", pullResults)) + return core.Fail(core.E("git.cli.verifyPushAndPullMultiple", core.Sprintf("unexpected pull multiple results: %+v", pullResults), nil)) } return core.Ok(nil) @@ -249,23 +250,24 @@ func verifyPushAndPullMultiple(ctx core.Context, workspace pushPullWorkspace) co func expectSingleStatus(statuses []gitlib.RepoStatus, name string, ahead, behind int) core.Result { if len(statuses) != 1 { - return core.Fail(core.Errorf("expected 1 status, got %d", len(statuses))) + return core.Fail(core.E("git.cli.expectSingleStatus", core.Sprintf("expected 1 status, got %d", len(statuses)), nil)) } status := statuses[0] if status.Error != nil { return core.Fail(status.Error) } if status.Name != name { - return core.Fail(core.Errorf("status name = %q", status.Name)) + return core.Fail(core.E("git.cli.expectSingleStatus", core.Sprintf("status name = %q", status.Name), nil)) } if status.Ahead != ahead || status.Behind != behind { - return core.Fail(core.Errorf("%s ahead/behind = %d/%d, want %d/%d", name, status.Ahead, status.Behind, ahead, behind)) + msg := core.Sprintf("%s ahead/behind = %d/%d, want %d/%d", name, status.Ahead, status.Behind, ahead, behind) + return core.Fail(core.E("git.cli.expectSingleStatus", msg, nil)) } if ahead > 0 && !status.HasUnpushed() { - return core.Fail(core.Errorf("%s should report unpushed commits", name)) + return core.Fail(core.E("git.cli.expectSingleStatus", core.Sprintf("%s should report unpushed commits", name), nil)) } if behind > 0 && !status.HasUnpulled() { - return core.Fail(core.Errorf("%s should report unpulled commits", name)) + return core.Fail(core.E("git.cli.expectSingleStatus", core.Sprintf("%s should report unpulled commits", name), nil)) } return core.Ok(nil) } @@ -273,23 +275,23 @@ func expectSingleStatus(statuses []gitlib.RepoStatus, name string, ahead, behind func verifyErrors(ctx core.Context) core.Result { statuses := gitlib.Status(ctx, gitlib.StatusOptions{Paths: []string{relativeRepoPath}}) if len(statuses) != 1 || statuses[0].Error == nil { - return core.Fail(core.NewError("relative status should fail")) + return core.Fail(core.E("git.cli.verifyErrors", "relative status should fail", nil)) } if !core.Contains(statuses[0].Error.Error(), "path must be absolute") { - return core.Fail(core.Errorf("relative status error = %v", statuses[0].Error)) + return core.Fail(core.E("git.cli.verifyErrors", core.Sprintf("relative status error = %v", statuses[0].Error), nil)) } if r := gitlib.Push(ctx, relativeRepoPath); r.OK { - return core.Fail(core.NewError("relative push should fail")) + return core.Fail(core.E("git.cli.verifyErrors", "relative push should fail", nil)) } if r := gitlib.Pull(ctx, relativeRepoPath); r.OK { - return core.Fail(core.NewError("relative pull should fail")) + return core.Fail(core.E("git.cli.verifyErrors", "relative pull should fail", nil)) } - if !gitlib.IsNonFastForward(core.NewError("Updates were rejected: fetch first")) { - return core.Fail(core.NewError("non-fast-forward detection should match fetch first errors")) + if !gitlib.IsNonFastForward(core.E("git.cli.verifyErrors", "Updates were rejected: fetch first", nil)) { + return core.Fail(core.E("git.cli.verifyErrors", "non-fast-forward detection should match fetch first errors", nil)) } - if gitlib.IsNonFastForward(core.NewError("connection refused")) { - return core.Fail(core.NewError("non-fast-forward detection should ignore unrelated errors")) + if gitlib.IsNonFastForward(core.E("git.cli.verifyErrors", "connection refused", nil)) { + return core.Fail(core.E("git.cli.verifyErrors", "non-fast-forward detection should ignore unrelated errors", nil)) } return core.Ok(nil) @@ -345,7 +347,8 @@ func runGit(dir string, args ...string) core.Result { cmd := &core.Cmd{Path: "/usr/bin/env", Args: cmdArgs, Dir: dir} out, runErr := cmd.CombinedOutput() if runErr != nil { - return core.Fail(core.Errorf("git %s: %s: %w", core.Join(" ", args...), core.Trim(string(out)), runErr)) + msg := core.Sprintf("git %s: %s", core.Join(" ", args...), core.Trim(string(out))) + return core.Fail(core.E("git.cli.runGit", msg, runErr)) } return core.Ok(string(out)) }