From 35fdefc1ff253522f101ffb1337437b59676c302 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sun, 23 Jan 2022 13:57:52 +0800 Subject: [PATCH] Always use git command but not os.Command (#18363) --- modules/git/diff.go | 32 +++++++++++-------------- routers/web/repo/http.go | 23 ++++++++---------- services/gitdiff/gitdiff.go | 47 +++++++++++++++++-------------------- 3 files changed, 46 insertions(+), 56 deletions(-) diff --git a/modules/git/diff.go b/modules/git/diff.go index 02ed2c60c..38aefabf1 100644 --- a/modules/git/diff.go +++ b/modules/git/diff.go @@ -11,13 +11,11 @@ import ( "fmt" "io" "os" - "os/exec" "regexp" "strconv" "strings" "code.gitea.io/gitea/modules/log" - "code.gitea.io/gitea/modules/process" ) // RawDiffType type of a raw diff. @@ -55,43 +53,41 @@ func GetRepoRawDiffForFile(repo *Repository, startCommit, endCommit string, diff if len(file) > 0 { fileArgs = append(fileArgs, "--", file) } - // FIXME: graceful: These commands should have a timeout - ctx, _, finished := process.GetManager().AddContext(repo.Ctx, fmt.Sprintf("GetRawDiffForFile: [repo_path: %s]", repo.Path)) - defer finished() - var cmd *exec.Cmd + var args []string switch diffType { case RawDiffNormal: if len(startCommit) != 0 { - cmd = exec.CommandContext(ctx, GitExecutable, append([]string{"diff", "-M", startCommit, endCommit}, fileArgs...)...) + args = append([]string{"diff", "-M", startCommit, endCommit}, fileArgs...) } else if commit.ParentCount() == 0 { - cmd = exec.CommandContext(ctx, GitExecutable, append([]string{"show", endCommit}, fileArgs...)...) + args = append([]string{"show", endCommit}, fileArgs...) } else { c, _ := commit.Parent(0) - cmd = exec.CommandContext(ctx, GitExecutable, append([]string{"diff", "-M", c.ID.String(), endCommit}, fileArgs...)...) + args = append([]string{"diff", "-M", c.ID.String(), endCommit}, fileArgs...) } case RawDiffPatch: if len(startCommit) != 0 { query := fmt.Sprintf("%s...%s", endCommit, startCommit) - cmd = exec.CommandContext(ctx, GitExecutable, append([]string{"format-patch", "--no-signature", "--stdout", "--root", query}, fileArgs...)...) + args = append([]string{"format-patch", "--no-signature", "--stdout", "--root", query}, fileArgs...) } else if commit.ParentCount() == 0 { - cmd = exec.CommandContext(ctx, GitExecutable, append([]string{"format-patch", "--no-signature", "--stdout", "--root", endCommit}, fileArgs...)...) + args = append([]string{"format-patch", "--no-signature", "--stdout", "--root", endCommit}, fileArgs...) } else { c, _ := commit.Parent(0) query := fmt.Sprintf("%s...%s", endCommit, c.ID.String()) - cmd = exec.CommandContext(ctx, GitExecutable, append([]string{"format-patch", "--no-signature", "--stdout", query}, fileArgs...)...) + args = append([]string{"format-patch", "--no-signature", "--stdout", query}, fileArgs...) } default: return fmt.Errorf("invalid diffType: %s", diffType) } stderr := new(bytes.Buffer) - - cmd.Dir = repo.Path - cmd.Stdout = writer - cmd.Stderr = stderr - - if err = cmd.Run(); err != nil { + cmd := NewCommandContextNoGlobals(repo.Ctx, args...) + if err = cmd.RunWithContext(&RunContext{ + Timeout: -1, + Dir: repo.Path, + Stdout: writer, + Stderr: stderr, + }); err != nil { return fmt.Errorf("Run: %v - %s", err, stderr) } return nil diff --git a/routers/web/repo/http.go b/routers/web/repo/http.go index 3805ceea7..1b5004017 100644 --- a/routers/web/repo/http.go +++ b/routers/web/repo/http.go @@ -12,7 +12,6 @@ import ( "fmt" "net/http" "os" - "os/exec" "path" "regexp" "strconv" @@ -30,7 +29,6 @@ import ( "code.gitea.io/gitea/modules/context" "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/log" - "code.gitea.io/gitea/modules/process" "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/structs" "code.gitea.io/gitea/modules/util" @@ -486,18 +484,17 @@ func serviceRPC(ctx gocontext.Context, h serviceHandler, service string) { h.environ = append(h.environ, "GIT_PROTOCOL="+protocol) } - ctx, _, finished := process.GetManager().AddContext(h.r.Context(), fmt.Sprintf("%s %s %s [repo_path: %s]", git.GitExecutable, service, "--stateless-rpc", h.dir)) - defer finished() - var stderr bytes.Buffer - cmd := exec.CommandContext(ctx, git.GitExecutable, service, "--stateless-rpc", h.dir) - cmd.Dir = h.dir - cmd.Env = append(os.Environ(), h.environ...) - cmd.Stdout = h.w - cmd.Stdin = reqBody - cmd.Stderr = &stderr - - if err := cmd.Run(); err != nil { + cmd := git.NewCommandContextNoGlobals(h.r.Context(), service, "--stateless-rpc", h.dir) + cmd.SetDescription(fmt.Sprintf("%s %s %s [repo_path: %s]", git.GitExecutable, service, "--stateless-rpc", h.dir)) + if err := cmd.RunWithContext(&git.RunContext{ + Timeout: -1, + Dir: h.dir, + Env: append(os.Environ(), h.environ...), + Stdout: h.w, + Stdin: reqBody, + Stderr: &stderr, + }); err != nil { log.Error("Fail to serve RPC(%s) in %s: %v - %s", service, h.dir, err, stderr.String()) return } diff --git a/services/gitdiff/gitdiff.go b/services/gitdiff/gitdiff.go index 25d5e139d..80e706b5e 100644 --- a/services/gitdiff/gitdiff.go +++ b/services/gitdiff/gitdiff.go @@ -15,7 +15,6 @@ import ( "io" "net/url" "os" - "os/exec" "regexp" "sort" "strings" @@ -30,7 +29,6 @@ import ( "code.gitea.io/gitea/modules/highlight" "code.gitea.io/gitea/modules/lfs" "code.gitea.io/gitea/modules/log" - "code.gitea.io/gitea/modules/process" "code.gitea.io/gitea/modules/setting" "github.com/sergi/go-diff/diffmatchpatch" @@ -1322,10 +1320,6 @@ func GetDiff(gitRepo *git.Repository, opts *DiffOptions, files ...string) (*Diff return nil, err } - timeout := time.Duration(setting.Git.Timeout.Default) * time.Second - ctx, _, finished := process.GetManager().AddContextTimeout(gitRepo.Ctx, timeout, fmt.Sprintf("GetDiffRange [repo_path: %s]", repoPath)) - defer finished() - argsLength := 6 if len(opts.WhitespaceBehavior) > 0 { argsLength++ @@ -1375,21 +1369,28 @@ func GetDiff(gitRepo *git.Repository, opts *DiffOptions, files ...string) (*Diff diffArgs = append(diffArgs, files...) } - cmd := exec.CommandContext(ctx, git.GitExecutable, diffArgs...) - - cmd.Dir = repoPath - cmd.Stderr = os.Stderr + reader, writer := io.Pipe() + defer func() { + _ = reader.Close() + _ = writer.Close() + }() - stdout, err := cmd.StdoutPipe() - if err != nil { - return nil, fmt.Errorf("error creating StdoutPipe: %w", err) - } + go func(ctx context.Context, diffArgs []string, repoPath string, writer *io.PipeWriter) { + cmd := git.NewCommandContextNoGlobals(ctx, diffArgs...) + cmd.SetDescription(fmt.Sprintf("GetDiffRange [repo_path: %s]", repoPath)) + if err := cmd.RunWithContext(&git.RunContext{ + Timeout: time.Duration(setting.Git.Timeout.Default) * time.Second, + Dir: repoPath, + Stderr: os.Stderr, + Stdout: writer, + }); err != nil { + log.Error("error during RunWithContext: %w", err) + } - if err = cmd.Start(); err != nil { - return nil, fmt.Errorf("error during Start: %w", err) - } + _ = writer.Close() + }(gitRepo.Ctx, diffArgs, repoPath, writer) - diff, err := ParsePatch(opts.MaxLines, opts.MaxLineCharacters, opts.MaxFiles, stdout, parsePatchSkipToFile) + diff, err := ParsePatch(opts.MaxLines, opts.MaxLineCharacters, opts.MaxFiles, reader, parsePatchSkipToFile) if err != nil { return nil, fmt.Errorf("unable to ParsePatch: %w", err) } @@ -1408,7 +1409,7 @@ func GetDiff(gitRepo *git.Repository, opts *DiffOptions, files ...string) (*Diff IndexFile: indexFilename, WorkTree: worktree, } - ctx, cancel := context.WithCancel(ctx) + ctx, cancel := context.WithCancel(gitRepo.Ctx) if err := checker.Init(ctx); err != nil { log.Error("Unable to open checker for %s. Error: %v", opts.AfterCommitID, err) } else { @@ -1472,10 +1473,6 @@ func GetDiff(gitRepo *git.Repository, opts *DiffOptions, files ...string) (*Diff } } - if err = cmd.Wait(); err != nil { - return nil, fmt.Errorf("error during cmd.Wait: %w", err) - } - separator := "..." if opts.DirectComparison { separator = ".." @@ -1485,12 +1482,12 @@ func GetDiff(gitRepo *git.Repository, opts *DiffOptions, files ...string) (*Diff if len(opts.BeforeCommitID) == 0 || opts.BeforeCommitID == git.EmptySHA { shortstatArgs = []string{git.EmptyTreeSHA, opts.AfterCommitID} } - diff.NumFiles, diff.TotalAddition, diff.TotalDeletion, err = git.GetDiffShortStat(ctx, repoPath, shortstatArgs...) + diff.NumFiles, diff.TotalAddition, diff.TotalDeletion, err = git.GetDiffShortStat(gitRepo.Ctx, repoPath, shortstatArgs...) if err != nil && strings.Contains(err.Error(), "no merge base") { // git >= 2.28 now returns an error if base and head have become unrelated. // previously it would return the results of git diff --shortstat base head so let's try that... shortstatArgs = []string{opts.BeforeCommitID, opts.AfterCommitID} - diff.NumFiles, diff.TotalAddition, diff.TotalDeletion, err = git.GetDiffShortStat(ctx, repoPath, shortstatArgs...) + diff.NumFiles, diff.TotalAddition, diff.TotalDeletion, err = git.GetDiffShortStat(gitRepo.Ctx, repoPath, shortstatArgs...) } if err != nil { return nil, err