Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Always use git command but not os.Command #18363

Merged
merged 4 commits into from
Jan 23, 2022
Merged

Always use git command but not os.Command #18363

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
318e989
Always use git command but not os.Command
lunny Jan 22, 2022
4068c60
Fix description
lunny Jan 22, 2022
3045eb6
Merge branch 'main' into lunny/git_command
6543 Jan 22, 2022
000b9d2
Merge branch 'main' into lunny/git_command
wxiaoguang Jan 23, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 14 additions & 18 deletions modules/git/diff.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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
Expand Down
23 changes: 10 additions & 13 deletions routers/web/repo/http.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ import (
"fmt"
"net/http"
"os"
"os/exec"
"path"
"regexp"
"strconv"
Expand All @@ -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"
Expand Down Expand Up @@ -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
}
Expand Down
47 changes: 22 additions & 25 deletions services/gitdiff/gitdiff.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ import (
"io"
"net/url"
"os"
"os/exec"
"regexp"
"sort"
"strings"
Expand All @@ -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"
Expand Down Expand Up @@ -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++
Expand Down Expand Up @@ -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...)
Copy link
Contributor

@zeripath zeripath Jan 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be NewCommandContext too. no it doesn't sorry Actually I'm really not sure about this.

I have a feeling that this should be NewCommandContext and we need to create our own lfs.filter.

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)
}
Expand All @@ -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 {
Expand Down Expand Up @@ -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 = ".."
Expand All @@ -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
Expand Down