diff --git a/dev/ci/gitops/BUILD.bazel b/dev/ci/gitops/BUILD.bazel index 0cd0852c62e..0242df8922a 100644 --- a/dev/ci/gitops/BUILD.bazel +++ b/dev/ci/gitops/BUILD.bazel @@ -7,6 +7,7 @@ go_library( tags = [TAG_INFRA_DEVINFRA], visibility = ["//visibility:public"], deps = [ + "//internal/execute", "//internal/oobmigration", "//lib/errors", ], diff --git a/dev/ci/gitops/git_ops.go b/dev/ci/gitops/git_ops.go index d0686f8120b..053d10305c1 100644 --- a/dev/ci/gitops/git_ops.go +++ b/dev/ci/gitops/git_ops.go @@ -1,10 +1,12 @@ package gitops import ( + "context" "fmt" "os/exec" "strings" + "github.com/sourcegraph/sourcegraph/internal/execute" "github.com/sourcegraph/sourcegraph/internal/oobmigration" "github.com/sourcegraph/sourcegraph/lib/errors" ) @@ -32,7 +34,7 @@ func determineDiffArgs(baseBranch, commit string) (string, error) { } func GetHEADChangedFiles() ([]string, error) { - output, err := exec.Command("git", "diff", "--name-only", "@^").CombinedOutput() + output, err := execute.Git(context.Background(), "diff", "--name-only", "@^") if err != nil { return nil, err } @@ -46,7 +48,7 @@ func GetBranchChangedFiles(baseBranch, commit string) ([]string, error) { return nil, err } - output, err := exec.Command("git", "diff", "--name-only", diffArgs).CombinedOutput() + output, err := execute.Git(context.Background(), "diff", "--name-only", diffArgs) if err != nil { return nil, err } @@ -55,7 +57,7 @@ func GetBranchChangedFiles(baseBranch, commit string) ([]string, error) { } func GetLatestTag() (string, error) { - output, err := exec.Command("git", "tag", "--list", "v*").CombinedOutput() + output, err := execute.Git(context.Background(), "tag", "--list", "v*") if err != nil { return "", err } @@ -84,7 +86,7 @@ func HasIncludedCommit(commits ...string) (bool, error) { found := false var errs error for _, mustIncludeCommit := range commits { - output, err := exec.Command("git", "merge-base", "--is-ancestor", mustIncludeCommit, "HEAD").CombinedOutput() + output, err := execute.Git(context.Background(), "merge-base", "--is-ancestor", mustIncludeCommit, "HEAD") if err == nil { found = true break diff --git a/dev/ci/internal/ci/BUILD.bazel b/dev/ci/internal/ci/BUILD.bazel index ac8522363c1..61434b61f8d 100644 --- a/dev/ci/internal/ci/BUILD.bazel +++ b/dev/ci/internal/ci/BUILD.bazel @@ -33,6 +33,7 @@ go_library( "//dev/ci/internal/ci/operations", "//dev/ci/runtype", "//dev/sg/root", + "//internal/execute", "//internal/lazyregexp", "//lib/errors", "@com_github_masterminds_semver//:semver", diff --git a/dev/ci/internal/ci/config.go b/dev/ci/internal/ci/config.go index 7e98d3b0a31..7a7acaded2c 100644 --- a/dev/ci/internal/ci/config.go +++ b/dev/ci/internal/ci/config.go @@ -1,9 +1,9 @@ package ci import ( + "context" "fmt" "os" - "os/exec" "strconv" "strings" "time" @@ -12,6 +12,7 @@ import ( "github.com/sourcegraph/sourcegraph/dev/ci/images" "github.com/sourcegraph/sourcegraph/dev/ci/internal/ci/changed" "github.com/sourcegraph/sourcegraph/dev/ci/runtype" + "github.com/sourcegraph/sourcegraph/internal/execute" "github.com/sourcegraph/sourcegraph/lib/errors" ) @@ -97,7 +98,7 @@ func NewConfig(now time.Time) Config { changedFiles, err = gitops.GetBranchChangedFiles(baseBranch, commit) } } else { - out, giterr := exec.Command("git", "rev-parse", "HEAD").Output() + out, giterr := execute.Git(context.Background(), "rev-parse", "HEAD") if giterr != nil { panic(giterr) } diff --git a/dev/sg/internal/backport/BUILD.bazel b/dev/sg/internal/backport/BUILD.bazel index 502d0001bd4..e356340d0fc 100644 --- a/dev/sg/internal/backport/BUILD.bazel +++ b/dev/sg/internal/backport/BUILD.bazel @@ -7,8 +7,8 @@ go_library( tags = [TAG_INFRA_RELEASE], visibility = ["//dev/sg:__subpackages__"], deps = [ - "//dev/sg/internal/execute", "//dev/sg/internal/std", + "//internal/execute", "//lib/errors", "//lib/output", "@com_github_urfave_cli_v2//:cli", diff --git a/dev/sg/internal/backport/backport.go b/dev/sg/internal/backport/backport.go index 4aaae62c711..72eb6bdf46e 100644 --- a/dev/sg/internal/backport/backport.go +++ b/dev/sg/internal/backport/backport.go @@ -7,8 +7,8 @@ import ( "github.com/urfave/cli/v2" - "github.com/sourcegraph/sourcegraph/dev/sg/internal/execute" "github.com/sourcegraph/sourcegraph/dev/sg/internal/std" + "github.com/sourcegraph/sourcegraph/internal/execute" "github.com/sourcegraph/sourcegraph/lib/errors" "github.com/sourcegraph/sourcegraph/lib/output" ) diff --git a/dev/sg/internal/execute/BUILD.bazel b/dev/sg/internal/execute/BUILD.bazel index cd6bc0505ec..e69de29bb2d 100644 --- a/dev/sg/internal/execute/BUILD.bazel +++ b/dev/sg/internal/execute/BUILD.bazel @@ -1,8 +0,0 @@ -load("@io_bazel_rules_go//go:def.bzl", "go_library") - -go_library( - name = "execute", - srcs = ["exec.go"], - importpath = "github.com/sourcegraph/sourcegraph/dev/sg/internal/execute", - visibility = ["//dev/sg:__subpackages__"], -) diff --git a/dev/sg/internal/execute/exec.go b/dev/sg/internal/execute/exec.go deleted file mode 100644 index 43d53d25ed2..00000000000 --- a/dev/sg/internal/execute/exec.go +++ /dev/null @@ -1,46 +0,0 @@ -package execute - -import ( - "bytes" - "context" - "os" - "os/exec" -) - -func Git(ctx context.Context, args ...string) ([]byte, error) { - cmd := GitCmd(ctx, args...) - - var stdout bytes.Buffer - var stderr bytes.Buffer - - cmd.Stdout = &stdout - cmd.Stderr = &stderr - cmd.Stdin = os.Stdin - - if err := cmd.Run(); err != nil { - return nil, err - } - return stdout.Bytes(), nil -} - -func GitCmd(ctx context.Context, args ...string) *exec.Cmd { - return exec.CommandContext(ctx, "git", args...) -} - -func GHCmd(ctx context.Context, args ...string) *exec.Cmd { - return exec.CommandContext(ctx, "gh", args...) -} - -func GH(ctx context.Context, args ...string) ([]byte, error) { - cmd := GHCmd(ctx, args...) - - var stdout bytes.Buffer - cmd.Stdout = &stdout - cmd.Stderr = os.Stderr - cmd.Stdin = os.Stdin - - if err := cmd.Run(); err != nil { - return nil, err - } - return stdout.Bytes(), nil -} diff --git a/dev/sg/internal/release/BUILD.bazel b/dev/sg/internal/release/BUILD.bazel index 67fed764edf..d5f363f0330 100644 --- a/dev/sg/internal/release/BUILD.bazel +++ b/dev/sg/internal/release/BUILD.bazel @@ -17,9 +17,9 @@ go_library( deps = [ "//dev/sg/internal/bk", "//dev/sg/internal/category", - "//dev/sg/internal/execute", "//dev/sg/internal/repo", "//dev/sg/internal/std", + "//internal/execute", "//internal/jsonc", "//internal/releaseregistry", "//lib/errors", diff --git a/dev/sg/internal/release/cut.go b/dev/sg/internal/release/cut.go index 83d8d59c119..e494ff91030 100644 --- a/dev/sg/internal/release/cut.go +++ b/dev/sg/internal/release/cut.go @@ -15,8 +15,8 @@ import ( "github.com/sourcegraph/run" - "github.com/sourcegraph/sourcegraph/dev/sg/internal/execute" "github.com/sourcegraph/sourcegraph/dev/sg/internal/repo" + "github.com/sourcegraph/sourcegraph/internal/execute" ) func cutReleaseBranch(cctx *cli.Context) error { diff --git a/internal/execute/BUILD.bazel b/internal/execute/BUILD.bazel new file mode 100644 index 00000000000..6492c6230b7 --- /dev/null +++ b/internal/execute/BUILD.bazel @@ -0,0 +1,17 @@ +load("@io_bazel_rules_go//go:def.bzl", "go_library") +load("//dev:go_defs.bzl", "go_test") + +go_library( + name = "execute", + srcs = ["git.go"], + importpath = "github.com/sourcegraph/sourcegraph/internal/execute", + visibility = ["//:__subpackages__"], + deps = ["//lib/errors"], +) + +go_test( + name = "execute_test", + srcs = ["git_test.go"], + embed = [":execute"], + deps = ["@com_github_stretchr_testify//require"], +) diff --git a/internal/execute/git.go b/internal/execute/git.go new file mode 100644 index 00000000000..83ce27db088 --- /dev/null +++ b/internal/execute/git.go @@ -0,0 +1,83 @@ +package execute + +import ( + "bytes" + "context" + "os" + "os/exec" + "strings" + + "github.com/sourcegraph/sourcegraph/lib/errors" +) + +type cmdErr struct { + err error + exitCode int +} + +func (g *cmdErr) Error() string { + return g.err.Error() +} + +func (g *cmdErr) ExitCode() int { + return g.exitCode +} + +func (g *cmdErr) Unwrap() error { + return g.err +} + +// HandleGitCommandExec There's a weird behavior that occurs where an error isn't accessible in the err variable +// from a *Cmd executing a git command after calling CombinedOutput(). +// This occurs due to how Git handles errors and how the exec package in Go interprets the command's output. +// Git often writes error messages to stderr, but it might still exit with a status code of 0 (indicating success). +// In this case, CombinedOutput() won't return an error, but the error message will be in the out variable. +func handleGitCommandExec(cmd *exec.Cmd) ([]byte, error) { + var stdout, stderr bytes.Buffer + cmd.Stdout = &stdout + cmd.Stderr = &stderr + + if err := cmd.Run(); err != nil { + maybeErrMessage := strings.Trim(stderr.String(), "\n") + if strings.HasPrefix(maybeErrMessage, "fatal:") || strings.HasPrefix(maybeErrMessage, "error:") { + exitCode := 1 + var exitErr *exec.ExitError + if errors.As(err, &exitErr) { + exitCode = exitErr.ExitCode() + } + return nil, &cmdErr{ + err: errors.New(maybeErrMessage), + exitCode: exitCode, + } + } + return nil, err + } + + return stdout.Bytes(), nil +} + +func Git(ctx context.Context, args ...string) ([]byte, error) { + return handleGitCommandExec(GitCmd(ctx, args...)) +} + +func GitCmd(ctx context.Context, args ...string) *exec.Cmd { + return exec.CommandContext(ctx, "git", args...) +} + +func GHCmd(ctx context.Context, args ...string) *exec.Cmd { + return exec.CommandContext(ctx, "gh", args...) +} + +func GH(ctx context.Context, args ...string) ([]byte, error) { + cmd := GHCmd(ctx, args...) + + var stdout bytes.Buffer + cmd.Stdout = &stdout + cmd.Stderr = os.Stderr + cmd.Stdin = os.Stdin + + if err := cmd.Run(); err != nil { + return nil, err + } + return stdout.Bytes(), nil +} diff --git a/internal/execute/git_test.go b/internal/execute/git_test.go new file mode 100644 index 00000000000..eb3da3a4f97 --- /dev/null +++ b/internal/execute/git_test.go @@ -0,0 +1,76 @@ +package execute + +import ( + "os" + "os/exec" + "testing" + + "github.com/stretchr/testify/require" +) + +func TestHandleGitCommandExec(t *testing.T) { + // Store the current working directory + originalDir, err := os.Getwd() + if err != nil { + t.Fatalf("Failed to get current directory: %v", err) + } + + // Create a temporary directory for the test + tempDir, err := os.MkdirTemp("", "git-ops-test-temp-dir") + if err != nil { + t.Fatalf("Failed to create temp directory: %v", err) + } + defer os.RemoveAll(tempDir) // Clean up after the test + + // Change to the temporary directory + if err := os.Chdir(tempDir); err != nil { + t.Fatalf("Failed to change to temp directory: %v", err) + } + + // Defer changing back to the original directory + defer func() { + if err := os.Chdir(originalDir); err != nil { + t.Fatalf("Failed to change back to original directory: %v", err) + } + }() + + tests := []struct { + name string + cmdSetup func() *exec.Cmd + expectedOutput string + expectedError string + }{ + { + name: "Successful command", + cmdSetup: func() *exec.Cmd { + return exec.Command("echo", "file1.txt\nfile2.txt") + }, + expectedOutput: "file1.txt\nfile2.txt\n", + expectedError: "", + }, + { + name: "Git fatal error", + cmdSetup: func() *exec.Cmd { + return exec.Command("git", "rev-parse", "--is-inside-work-tree") + }, + expectedOutput: "", + expectedError: "fatal: not a git repository", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + cmd := tt.cmdSetup() + output, err := handleGitCommandExec(cmd) + + if tt.expectedError != "" { + require.Error(t, err) + require.ErrorContains(t, err, tt.expectedError) + } else { + require.NoError(t, err) + } + + require.Equal(t, tt.expectedOutput, string(output)) + }) + } +}