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.

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

Remove context from git struct #33793

Merged
merged 6 commits into from
Mar 4, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion cmd/hook.go
Original file line number Diff line number Diff line change
Expand Up @@ -316,7 +316,7 @@ func runHookPostReceive(c *cli.Context) error {
setup(ctx, c.Bool("debug"))

// First of all run update-server-info no matter what
if _, _, err := git.NewCommand(ctx, "update-server-info").RunStdString(nil); err != nil {
if _, _, err := git.NewCommand("update-server-info").RunStdString(ctx, nil); err != nil {
return fmt.Errorf("Failed to call 'git update-server-info': %w", err)
}

Expand Down
10 changes: 5 additions & 5 deletions models/migrations/v1_12/v128.go
Original file line number Diff line number Diff line change
Expand Up @@ -82,17 +82,17 @@ func FixMergeBase(x *xorm.Engine) error {

if !pr.HasMerged {
var err error
pr.MergeBase, _, err = git.NewCommand(git.DefaultContext, "merge-base").AddDashesAndList(pr.BaseBranch, gitRefName).RunStdString(&git.RunOpts{Dir: repoPath})
pr.MergeBase, _, err = git.NewCommand("merge-base").AddDashesAndList(pr.BaseBranch, gitRefName).RunStdString(git.DefaultContext, &git.RunOpts{Dir: repoPath})
if err != nil {
var err2 error
pr.MergeBase, _, err2 = git.NewCommand(git.DefaultContext, "rev-parse").AddDynamicArguments(git.BranchPrefix + pr.BaseBranch).RunStdString(&git.RunOpts{Dir: repoPath})
pr.MergeBase, _, err2 = git.NewCommand("rev-parse").AddDynamicArguments(git.BranchPrefix+pr.BaseBranch).RunStdString(git.DefaultContext, &git.RunOpts{Dir: repoPath})
if err2 != nil {
log.Error("Unable to get merge base for PR ID %d, Index %d in %s/%s. Error: %v & %v", pr.ID, pr.Index, baseRepo.OwnerName, baseRepo.Name, err, err2)
continue
}
}
} else {
parentsString, _, err := git.NewCommand(git.DefaultContext, "rev-list", "--parents", "-n", "1").AddDynamicArguments(pr.MergedCommitID).RunStdString(&git.RunOpts{Dir: repoPath})
parentsString, _, err := git.NewCommand("rev-list", "--parents", "-n", "1").AddDynamicArguments(pr.MergedCommitID).RunStdString(git.DefaultContext, &git.RunOpts{Dir: repoPath})
if err != nil {
log.Error("Unable to get parents for merged PR ID %d, Index %d in %s/%s. Error: %v", pr.ID, pr.Index, baseRepo.OwnerName, baseRepo.Name, err)
continue
Expand All @@ -104,9 +104,9 @@ func FixMergeBase(x *xorm.Engine) error {

refs := append([]string{}, parents[1:]...)
refs = append(refs, gitRefName)
cmd := git.NewCommand(git.DefaultContext, "merge-base").AddDashesAndList(refs...)
cmd := git.NewCommand("merge-base").AddDashesAndList(refs...)

pr.MergeBase, _, err = cmd.RunStdString(&git.RunOpts{Dir: repoPath})
pr.MergeBase, _, err = cmd.RunStdString(git.DefaultContext, &git.RunOpts{Dir: repoPath})
if err != nil {
log.Error("Unable to get merge base for merged PR ID %d, Index %d in %s/%s. Error: %v", pr.ID, pr.Index, baseRepo.OwnerName, baseRepo.Name, err)
continue
Expand Down
6 changes: 3 additions & 3 deletions models/migrations/v1_12/v134.go
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ func RefixMergeBase(x *xorm.Engine) error {

gitRefName := fmt.Sprintf("refs/pull/%d/head", pr.Index)

parentsString, _, err := git.NewCommand(git.DefaultContext, "rev-list", "--parents", "-n", "1").AddDynamicArguments(pr.MergedCommitID).RunStdString(&git.RunOpts{Dir: repoPath})
parentsString, _, err := git.NewCommand("rev-list", "--parents", "-n", "1").AddDynamicArguments(pr.MergedCommitID).RunStdString(git.DefaultContext, &git.RunOpts{Dir: repoPath})
if err != nil {
log.Error("Unable to get parents for merged PR ID %d, Index %d in %s/%s. Error: %v", pr.ID, pr.Index, baseRepo.OwnerName, baseRepo.Name, err)
continue
Expand All @@ -92,9 +92,9 @@ func RefixMergeBase(x *xorm.Engine) error {
// we should recalculate
refs := append([]string{}, parents[1:]...)
refs = append(refs, gitRefName)
cmd := git.NewCommand(git.DefaultContext, "merge-base").AddDashesAndList(refs...)
cmd := git.NewCommand("merge-base").AddDashesAndList(refs...)

pr.MergeBase, _, err = cmd.RunStdString(&git.RunOpts{Dir: repoPath})
pr.MergeBase, _, err = cmd.RunStdString(git.DefaultContext, &git.RunOpts{Dir: repoPath})
if err != nil {
log.Error("Unable to get merge base for merged PR ID %d, Index %d in %s/%s. Error: %v", pr.ID, pr.Index, baseRepo.OwnerName, baseRepo.Name, err)
continue
Expand Down
12 changes: 6 additions & 6 deletions modules/git/batch_reader.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,8 @@ type WriteCloserError interface {
// This is needed otherwise the git cat-file will hang for invalid repositories.
func ensureValidGitRepository(ctx context.Context, repoPath string) error {
stderr := strings.Builder{}
err := NewCommand(ctx, "rev-parse").
Run(&RunOpts{
err := NewCommand("rev-parse").
Run(ctx, &RunOpts{
Dir: repoPath,
Stderr: &stderr,
})
Expand Down Expand Up @@ -61,8 +61,8 @@ func catFileBatchCheck(ctx context.Context, repoPath string) (WriteCloserError,

go func() {
stderr := strings.Builder{}
err := NewCommand(ctx, "cat-file", "--batch-check").
Run(&RunOpts{
err := NewCommand("cat-file", "--batch-check").
Run(ctx, &RunOpts{
Dir: repoPath,
Stdin: batchStdinReader,
Stdout: batchStdoutWriter,
Expand Down Expand Up @@ -109,8 +109,8 @@ func catFileBatch(ctx context.Context, repoPath string) (WriteCloserError, *bufi

go func() {
stderr := strings.Builder{}
err := NewCommand(ctx, "cat-file", "--batch").
Run(&RunOpts{
err := NewCommand("cat-file", "--batch").
Run(ctx, &RunOpts{
Dir: repoPath,
Stdin: batchStdinReader,
Stdout: batchStdoutWriter,
Expand Down
4 changes: 2 additions & 2 deletions modules/git/blame.go
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ func CreateBlameReader(ctx context.Context, objectFormat ObjectFormat, repoPath
ignoreRevsFile = tryCreateBlameIgnoreRevsFile(commit)
}

cmd := NewCommandContextNoGlobals(ctx, "blame", "--porcelain")
cmd := NewCommandNoGlobals("blame", "--porcelain")
if ignoreRevsFile != nil {
// Possible improvement: use --ignore-revs-file /dev/stdin on unix
// There is no equivalent on Windows. May be implemented if Gitea uses an external git backend.
Expand All @@ -155,7 +155,7 @@ func CreateBlameReader(ctx context.Context, objectFormat ObjectFormat, repoPath
go func() {
stderr := bytes.Buffer{}
// TODO: it doesn't work for directories (the directories shouldn't be "blamed"), and the "err" should be returned by "Read" but not by "Close"
err := cmd.Run(&RunOpts{
err := cmd.Run(ctx, &RunOpts{
UseContextTimeout: true,
Dir: repoPath,
Stdout: stdout,
Expand Down
44 changes: 17 additions & 27 deletions modules/git/command.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,6 @@ const DefaultLocale = "C"
type Command struct {
prog string
args []string
parentContext context.Context
globalArgsLength int
brokenArgs []string
}
Expand Down Expand Up @@ -82,7 +81,7 @@ func (c *Command) LogString() string {

// NewCommand creates and returns a new Git Command based on given command and arguments.
// Each argument should be safe to be trusted. User-provided arguments should be passed to AddDynamicArguments instead.
func NewCommand(ctx context.Context, args ...internal.CmdArg) *Command {
func NewCommand(args ...internal.CmdArg) *Command {
// Make an explicit copy of globalCommandArgs, otherwise append might overwrite it
cargs := make([]string, 0, len(globalCommandArgs)+len(args))
for _, arg := range globalCommandArgs {
Expand All @@ -94,31 +93,23 @@ func NewCommand(ctx context.Context, args ...internal.CmdArg) *Command {
return &Command{
prog: GitExecutable,
args: cargs,
parentContext: ctx,
globalArgsLength: len(globalCommandArgs),
}
}

// NewCommandContextNoGlobals creates and returns a new Git Command based on given command and arguments only with the specify args and don't care global command args
// NewCommandNoGlobals creates and returns a new Git Command based on given command and arguments only with the specified args and don't use global command args
// Each argument should be safe to be trusted. User-provided arguments should be passed to AddDynamicArguments instead.
func NewCommandContextNoGlobals(ctx context.Context, args ...internal.CmdArg) *Command {
func NewCommandNoGlobals(args ...internal.CmdArg) *Command {
cargs := make([]string, 0, len(args))
for _, arg := range args {
cargs = append(cargs, string(arg))
}
return &Command{
prog: GitExecutable,
args: cargs,
parentContext: ctx,
prog: GitExecutable,
args: cargs,
}
}

// SetParentContext sets the parent context for this command
func (c *Command) SetParentContext(ctx context.Context) *Command {
c.parentContext = ctx
return c
}

// isSafeArgumentValue checks if the argument is safe to be used as a value (not an option)
func isSafeArgumentValue(s string) bool {
return s == "" || s[0] != '-'
Expand Down Expand Up @@ -277,11 +268,11 @@ func CommonCmdServEnvs() []string {
var ErrBrokenCommand = errors.New("git command is broken")

// Run runs the command with the RunOpts
func (c *Command) Run(opts *RunOpts) error {
return c.run(1, opts)
func (c *Command) Run(ctx context.Context, opts *RunOpts) error {
return c.run(ctx, 1, opts)
}

func (c *Command) run(skip int, opts *RunOpts) error {
func (c *Command) run(ctx context.Context, skip int, opts *RunOpts) error {
if len(c.brokenArgs) != 0 {
log.Error("git command is broken: %s, broken args: %s", c.LogString(), strings.Join(c.brokenArgs, " "))
return ErrBrokenCommand
Expand All @@ -305,19 +296,18 @@ func (c *Command) run(skip int, opts *RunOpts) error {
desc := fmt.Sprintf("git.Run(by:%s, repo:%s): %s", callerInfo, logArgSanitize(opts.Dir), cmdLogString)
log.Debug("git.Command: %s", desc)

_, span := gtprof.GetTracer().Start(c.parentContext, gtprof.TraceSpanGitRun)
_, span := gtprof.GetTracer().Start(ctx, gtprof.TraceSpanGitRun)
defer span.End()
span.SetAttributeString(gtprof.TraceAttrFuncCaller, callerInfo)
span.SetAttributeString(gtprof.TraceAttrGitCommand, cmdLogString)

var ctx context.Context
var cancel context.CancelFunc
var finished context.CancelFunc

if opts.UseContextTimeout {
ctx, cancel, finished = process.GetManager().AddContext(c.parentContext, desc)
ctx, cancel, finished = process.GetManager().AddContext(ctx, desc)
} else {
ctx, cancel, finished = process.GetManager().AddContextTimeout(c.parentContext, timeout, desc)
ctx, cancel, finished = process.GetManager().AddContextTimeout(ctx, timeout, desc)
}
defer finished()

Expand Down Expand Up @@ -410,8 +400,8 @@ func IsErrorExitCode(err error, code int) bool {
}

// RunStdString runs the command with options and returns stdout/stderr as string. and store stderr to returned error (err combined with stderr).
func (c *Command) RunStdString(opts *RunOpts) (stdout, stderr string, runErr RunStdError) {
stdoutBytes, stderrBytes, err := c.runStdBytes(opts)
func (c *Command) RunStdString(ctx context.Context, opts *RunOpts) (stdout, stderr string, runErr RunStdError) {
stdoutBytes, stderrBytes, err := c.runStdBytes(ctx, opts)
stdout = util.UnsafeBytesToString(stdoutBytes)
stderr = util.UnsafeBytesToString(stderrBytes)
if err != nil {
Expand All @@ -422,11 +412,11 @@ func (c *Command) RunStdString(opts *RunOpts) (stdout, stderr string, runErr Run
}

// RunStdBytes runs the command with options and returns stdout/stderr as bytes. and store stderr to returned error (err combined with stderr).
func (c *Command) RunStdBytes(opts *RunOpts) (stdout, stderr []byte, runErr RunStdError) {
return c.runStdBytes(opts)
func (c *Command) RunStdBytes(ctx context.Context, opts *RunOpts) (stdout, stderr []byte, runErr RunStdError) {
return c.runStdBytes(ctx, opts)
}

func (c *Command) runStdBytes(opts *RunOpts) (stdout, stderr []byte, runErr RunStdError) {
func (c *Command) runStdBytes(ctx context.Context, opts *RunOpts) (stdout, stderr []byte, runErr RunStdError) {
if opts == nil {
opts = &RunOpts{}
}
Expand All @@ -449,7 +439,7 @@ func (c *Command) runStdBytes(opts *RunOpts) (stdout, stderr []byte, runErr RunS
PipelineFunc: opts.PipelineFunc,
}

err := c.run(2, newOpts)
err := c.run(ctx, 2, newOpts)
stderr = stderrBuf.Bytes()
if err != nil {
return nil, stderr, &runStdError{err: err, stderr: util.UnsafeBytesToString(stderr)}
Expand Down
8 changes: 4 additions & 4 deletions modules/git/command_race_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,9 @@ func TestRunWithContextNoTimeout(t *testing.T) {
maxLoops := 10

// 'git --version' does not block so it must be finished before the timeout triggered.
cmd := NewCommand(t.Context(), "--version")
cmd := NewCommand("--version")
for i := 0; i < maxLoops; i++ {
if err := cmd.Run(&RunOpts{}); err != nil {
if err := cmd.Run(t.Context(), &RunOpts{}); err != nil {
t.Fatal(err)
}
}
Expand All @@ -27,9 +27,9 @@ func TestRunWithContextTimeout(t *testing.T) {
maxLoops := 10

// 'git hash-object --stdin' blocks on stdin so we can have the timeout triggered.
cmd := NewCommand(t.Context(), "hash-object", "--stdin")
cmd := NewCommand("hash-object", "--stdin")
for i := 0; i < maxLoops; i++ {
if err := cmd.Run(&RunOpts{Timeout: 1 * time.Millisecond}); err != nil {
if err := cmd.Run(t.Context(), &RunOpts{Timeout: 1 * time.Millisecond}); err != nil {
if err != context.DeadlineExceeded {
t.Fatalf("Testing %d/%d: %v", i, maxLoops, err)
}
Expand Down
24 changes: 12 additions & 12 deletions modules/git/command_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,32 +10,32 @@ import (
)

func TestRunWithContextStd(t *testing.T) {
cmd := NewCommand(t.Context(), "--version")
stdout, stderr, err := cmd.RunStdString(&RunOpts{})
cmd := NewCommand("--version")
stdout, stderr, err := cmd.RunStdString(t.Context(), &RunOpts{})
assert.NoError(t, err)
assert.Empty(t, stderr)
assert.Contains(t, stdout, "git version")

cmd = NewCommand(t.Context(), "--no-such-arg")
stdout, stderr, err = cmd.RunStdString(&RunOpts{})
cmd = NewCommand("--no-such-arg")
stdout, stderr, err = cmd.RunStdString(t.Context(), &RunOpts{})
if assert.Error(t, err) {
assert.Equal(t, stderr, err.Stderr())
assert.Contains(t, err.Stderr(), "unknown option:")
assert.Contains(t, err.Error(), "exit status 129 - unknown option:")
assert.Empty(t, stdout)
}

cmd = NewCommand(t.Context())
cmd = NewCommand()
cmd.AddDynamicArguments("-test")
assert.ErrorIs(t, cmd.Run(&RunOpts{}), ErrBrokenCommand)
assert.ErrorIs(t, cmd.Run(t.Context(), &RunOpts{}), ErrBrokenCommand)

cmd = NewCommand(t.Context())
cmd = NewCommand()
cmd.AddDynamicArguments("--test")
assert.ErrorIs(t, cmd.Run(&RunOpts{}), ErrBrokenCommand)
assert.ErrorIs(t, cmd.Run(t.Context(), &RunOpts{}), ErrBrokenCommand)

subCmd := "version"
cmd = NewCommand(t.Context()).AddDynamicArguments(subCmd) // for test purpose only, the sub-command should never be dynamic for production
stdout, stderr, err = cmd.RunStdString(&RunOpts{})
cmd = NewCommand().AddDynamicArguments(subCmd) // for test purpose only, the sub-command should never be dynamic for production
stdout, stderr, err = cmd.RunStdString(t.Context(), &RunOpts{})
assert.NoError(t, err)
assert.Empty(t, stderr)
assert.Contains(t, stdout, "git version")
Expand All @@ -53,9 +53,9 @@ func TestGitArgument(t *testing.T) {
}

func TestCommandString(t *testing.T) {
cmd := NewCommandContextNoGlobals(t.Context(), "a", "-m msg", "it's a test", `say "hello"`)
cmd := NewCommandNoGlobals("a", "-m msg", "it's a test", `say "hello"`)
assert.EqualValues(t, cmd.prog+` a "-m msg" "it's a test" "say \"hello\""`, cmd.LogString())

cmd = NewCommandContextNoGlobals(t.Context(), "url: https://a:b@c/", "/root/dir-a/dir-b")
cmd = NewCommandNoGlobals("url: https://a:b@c/", "/root/dir-a/dir-b")
assert.EqualValues(t, cmd.prog+` "url: https://sanitized-credential@c/" .../dir-a/dir-b`, cmd.LogString())
}
Loading