refactor: use sourcegraph/run for command execution

[unknwon]

Summary

• Replace custom os/exec-based command execution in command.go with github.com/sourcegraph/run, delegating process lifecycle management to the library
• Preserve the entire public API surface (RunInDir, RunInDirPipeline, RunInDirWithOptions, Run) and all behavioral contracts (default timeout, cancellation → context.Canceled, deadline → ErrExecTimeout, stderr in errors, debug logging)
• Use run.Arg() to properly quote arguments for the library's shell-based parser, and Output.Stream() for raw byte-preserving output (important for binary-ish output like ls-tree -z)

All existing tests pass without modification.

[unknwon]

The _ = cancel on line 41 is a context leak. When context.WithTimeout is called, it creates internal goroutines/timers. The cancel function should be called when the context is no longer needed (after the command completes) to release these resources. By discarding it with _ = cancel, the timer will leak until it fires (after DefaultTimeout = 1 minute). While this isn't catastrophic (the timer eventually fires and cleans up), it's a resource leak and go vet / linters will flag it.

The fix would be to return the cancel function and call it in gitRun/gitPipeline, or have those higher-level functions manage the timeout instead.

Now let me compile my complete review:

{
  "summary": "Major refactor replacing os/exec Command pattern with sourcegraph/run and adding context.Context to all public APIs. The architectural direction is sound and the API is cleaner. However, there are several issues: a context/timer leak in gitCmd, dead code in mapContextError, a bug in UploadPack's --timeout flag format, and some fragile error matching. The caching pattern migration from sync.Once to Mutex+bool is a good improvement allowing retry on failure.",
  "verdict": "REQUEST_CHANGES",
  "comments": [
    {
      "path": "command.go",
      "line": 41,
      "body": "**Bug: Context/timer leak.** `_ = cancel` discards the cancel function, leaking the internal timer created by `context.WithTimeout` until it fires (up to 1 minute). Per Go docs: \"Failing to call the CancelFunc leaks the child and its children until the parent is canceled or the timer fires.\" This means every command invocation with a plain `context.Background()` leaks a timer for `DefaultTimeout`.\n\nConsider returning the cancel function so callers (`gitRun`, `gitPipeline`) can defer it, or move the timeout application into `gitRun`/`gitPipeline` directly:\n```go\nfunc gitRun(ctx context.Context, ...) ([]byte, error) {\n    if _, ok := ctx.Deadline(); !ok {\n        var cancel context.CancelFunc\n        ctx, cancel = context.WithTimeout(ctx, DefaultTimeout)\n        defer cancel()\n    }\n    cmd := gitCmd(ctx, dir, args, envs)\n    ...\n}\n```"
    },
    {
      "path": "command.go",
      "line": 199,
      "body": "**Dead code.** Lines 199-203 are unreachable. If `ctx.Err() != nil`, we already returned on lines 192-196. So when we reach line 199, `ctx.Err()` is guaranteed to be nil, and the inner check `ctx.Err() == context.DeadlineExceeded` on line 200 will always be false.\n\nThis entire block can be removed, or you could restructure: check the error string first, *then* check ctx.Err() as confirmation. But the current flow means this code never takes effect."
    },
    {
      "path": "server.go",
      "line": 108,
      "body": "**Bug: `git upload-pack --timeout` expects an integer (seconds), not a Go duration string.** `time.Duration.String()` outputs formats like `\"1m0s\"` or `\"30s\"`, but git expects a plain integer:\n```\nerror: option `timeout' expects an integer value with an optional k/m/g suffix\n```\nThis is a pre-existing bug from the old code, but since you're refactoring this, it should be fixed:\n```go\nif opt.InactivityTimeout > 0 {\n    args = append(args, \"--timeout\", strconv.FormatInt(int64(opt.InactivityTimeout.Seconds()), 10))\n}\n```"
    },
    {
      "path": "repo.go",
      "line": 396,
      "body": "**Fragile error check.** `err.Error() == \"exit status 1\"` uses exact string matching against sourcegraph/run's error format. If sourcegraph/run ever changes its format (e.g. `\"exit status 1: \"` with a trailing separator when stderr is empty), this will break silently — the method would start returning errors for 'nothing to commit' instead of nil.\n\nConsider using `strings.HasPrefix(err.Error(), \"exit status 1\")` or, better yet, check if the library exposes the exit code programmatically."
    },
    {
      "path": "command.go",
      "line": 58,
      "body": "**Minor: unnecessary allocation per command.** `append(os.Environ(), envs...)` copies the entire process environment for every git command that has custom env vars. Consider whether `run.Command` has an `Env` method that adds to the existing environment rather than replacing it, which would avoid this copy."
    },
    {
      "path": "command.go",
      "line": 188,
      "body": "**Style (nit):** Go convention is `func mapContextError(err error, ctx context.Context)` but `context.Context` should typically be the first parameter per [Go code review guidelines](https://go.dev/wiki/CodeReviewComments#contexts). Consider `func mapContextError(ctx context.Context, err error)`."
    },
    {
      "path": "blob.go",
      "line": 25,
      "body": "**Good improvement.** The added bounds check `size > 0 && size < int64(^uint(0)>>1)` correctly guards against both negative sizes (error case) and overflow when converting int64 to int for `Grow`. Much safer than the previous unconditional `Grow(int(b.Size()))`."
    },
    {
      "path": "commit_submodule.go",
      "line": 62,
      "body": "**Good improvement.** Using `strings.Cut` instead of `strings.Split` is safer — the old code would panic on lines without `=` since it unconditionally accessed `fields[1]`. The new code properly handles this with the `ok` check."
    },
    {
      "path": "go.mod",
      "line": 3,
      "body": "**Note:** Go 1.26.x doesn't exist yet (current latest is 1.24.x as of early 2026). Is this intentional for forward-looking development, or should it be set to a currently released Go version? The CI workflow also references `1.26.x`."
    },
    {
      "path": ".golangci.yml",
      "line": 10,
      "body": "**Note:** The v2 golangci-lint config format looks correct. The migration from explicit linter list to using presets for exclusions is clean. One observation: the old config had `staticcheck` checks `[\"-SA1019\"]` to suppress deprecation warnings, but the new config suppresses `QF1001` and `ST1016` instead. Was the SA1019 suppression intentionally dropped? If there are still `strings.Title` usages, the linter will now flag them."
    },
    {
      "path": "repo_tag.go",
      "line": 78,
      "body": "**Good cleanup.** Removing the `go-version` dependency for tag sorting and unconditionally using `--sort=-creatordate` (since the README now requires Git >= 2.4.9 which supports this flag) is a nice simplification."
    },
    {
      "path": "git.go",
      "line": 53,
      "body": "**Good pattern.** The mutex+bool caching that doesn't cache failures (allowing retries with fresh context) is a meaningful improvement over `sync.Once` which would permanently cache the first error. Well-documented too."
    },
    {
      "path": "command.go",
      "line": 83,
      "body": "**Question:** `.StdOut()` filters to only stdout. Does this mean stderr is silently discarded on success? If a git command writes important warnings to stderr but exits 0, they would be lost. The old code had explicit stderr handling. Consider whether stderr should be captured for logging even on success."
    }
  ]
}

[unknwon]

This PR replaces os/exec with sourcegraph/run for command execution. While the refactoring is mostly well-structured, there are three critical issues that must be fixed:

1. Context cancel leak (command.go:41): The timeout cancel function is discarded, leaking a 1-minute timer per command execution.
2. Broken error matching (repo.go:396): The error format changed from "exit status 1" to "exit status 1: ", breaking exact string matching for the "nothing to commit" case in Commit().
3. Double-quoted author flag (repo.go:390): Literal single quotes in the --author format string will be escaped by run.Arg(), causing git to receive malformed author strings with literal quote characters.

Additionally, there are 7 moderate to minor issues around error handling, stderr extraction, logging efficiency, and dependency bloat that should be addressed.

The refactoring has merit but requires these fixes before merging.

---

The following comments could not be posted as inline comments:

• go.mod:1 — Minor: The sourcegraph/run library adds 14+ transitive dependencies including OpenTelemetry, gojq, color libraries, lockfile handling, etc. For a Git wrapper that previously had only testify as a dependency, this significantly increases build time, binary size, and supply-chain risk. The main usage is straightforward command execution that os/exec handled well. Consider whether the benefits justify the dependency cost, or if a smaller, more focused library would be better.

[unknwon]

/copilot review

[unknwon]

This PR refactors the Command builder to use sourcegraph/run, simplifying the API and improving security in some cases (e.g., --end-of-options in DiffBinary). The core abstraction is well-structured, but there are several correctness issues:

1. gitRun logs empty output on error (inconsistent with gitPipeline)
2. ShowRefVerify and DeleteTag append opt.Args after --end-of-options, making them unreachable as flags
3. Error detection using strings.HasPrefix is too loose and will over-match (e.g., "exit status 1" matches "exit status 10", "exit status 128", etc.)
4. Error detection relies on undocumented string-format assumptions from sourcegraph/run
5. Significant transitive dependency increase (including gojq, OpenTelemetry) for a focused library
6. Logged commands are unquoted and will be misleading for arguments containing spaces

Recommend addressing the correctness issues (items 2, 3, 4) before merge.

---

The following comments could not be posted as inline comments:

• repo_commit.go:249 — (Fragility) Error detection uses strings.Contains(err.Error(), "bad revision"), which couples to sourcegraph/run's error formatting. Document this assumption or use a consistent extraction method.
• go.mod:1 — (Dependency impact) The sourcegraph/run v0.12.0 dependency introduces significant transitive dependencies: gojq, OpenTelemetry, go-logr, djherbis/buffer, djherbis/nio, itchyny/timefmt-go, etc. For a focused git library, this increases the dependency tree substantially. Callers will inherit all these transitive dependencies. Consider whether all of them are necessary for the actual use case (argument quoting + execution), or if a lighter-weight alternative would be preferable.

[unknwon]

End of options?

[unknwon]

Ditto

[unknwon]

Ditto

[unknwon]

Ditto

[unknwon]

Ditto