Skip to content

Commit

Permalink
Prevent panic in doctor command when running default checks (go-gitea…
Browse files Browse the repository at this point in the history
…#21791)

Backport go-gitea#21791

There was a bug introduced in go-gitea#21352 due to a change of behaviour caused
by go-gitea#19280. This causes a panic on running the default doctor checks
because the panic introduced by go-gitea#19280 assumes that the only way
opts.StdOut and opts.Stderr can be set in RunOpts is deliberately.
Unfortunately, when running a git.Command the provided RunOpts can be
set, therefore if you share a common set of RunOpts these two values can
be set by the previous commands.

This PR stops using common RunOpts for the commands in that doctor check
but secondly stops RunCommand variants from changing the provided
RunOpts.

Signed-off-by: Andrew Thornton <[email protected]>
  • Loading branch information
zeripath committed Nov 13, 2022
1 parent ac409fc commit 421f293
Show file tree
Hide file tree
Showing 2 changed files with 24 additions and 12 deletions.
10 changes: 4 additions & 6 deletions modules/doctor/heads.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,9 @@ func synchronizeRepoHeads(ctx context.Context, logger log.Logger, autofix bool)
numReposUpdated := 0
err := iterateRepositories(ctx, func(repo *repo_model.Repository) error {
numRepos++
runOpts := &git.RunOpts{Dir: repo.RepoPath()}
_, _, defaultBranchErr := git.NewCommand(ctx, "rev-parse").AddDashesAndList(repo.DefaultBranch).RunStdString(&git.RunOpts{Dir: repo.RepoPath()})

_, _, defaultBranchErr := git.NewCommand(ctx, "rev-parse").AddDashesAndList(repo.DefaultBranch).RunStdString(runOpts)

head, _, headErr := git.NewCommand(ctx, "symbolic-ref", "--short", "HEAD").RunStdString(runOpts)
head, _, headErr := git.NewCommand(ctx, "symbolic-ref", "--short", "HEAD").RunStdString(&git.RunOpts{Dir: repo.RepoPath()})

// what we expect: default branch is valid, and HEAD points to it
if headErr == nil && defaultBranchErr == nil && head == repo.DefaultBranch {
Expand All @@ -49,7 +47,7 @@ func synchronizeRepoHeads(ctx context.Context, logger log.Logger, autofix bool)
}

// otherwise, let's try fixing HEAD
err := git.NewCommand(ctx, "symbolic-ref").AddDashesAndList("HEAD", repo.DefaultBranch).Run(runOpts)
err := git.NewCommand(ctx, "symbolic-ref").AddDashesAndList("HEAD", git.BranchPrefix+repo.DefaultBranch).Run(&git.RunOpts{Dir: repo.RepoPath()})
if err != nil {
logger.Warn("Failed to fix HEAD for %s/%s: %v", repo.OwnerName, repo.Name, err)
return nil
Expand All @@ -65,7 +63,7 @@ func synchronizeRepoHeads(ctx context.Context, logger log.Logger, autofix bool)
logger.Info("Out of %d repos, HEADs for %d are now fixed and HEADS for %d are still broken", numRepos, numReposUpdated, numDefaultBranchesBroken+numHeadsBroken-numReposUpdated)
} else {
if numHeadsBroken == 0 && numDefaultBranchesBroken == 0 {
logger.Info("All %d repos have their HEADs in the correct state")
logger.Info("All %d repos have their HEADs in the correct state", numRepos)
} else {
if numHeadsBroken == 0 && numDefaultBranchesBroken != 0 {
logger.Critical("Default branches are broken for %d/%d repos", numDefaultBranchesBroken, numRepos)
Expand Down
26 changes: 20 additions & 6 deletions modules/git/command.go
Original file line number Diff line number Diff line change
Expand Up @@ -202,8 +202,11 @@ func (c *Command) Run(opts *RunOpts) error {
if opts == nil {
opts = &RunOpts{}
}
if opts.Timeout <= 0 {
opts.Timeout = defaultCommandExecutionTimeout

// We must not change the provided options
timeout := opts.Timeout
if timeout <= 0 {
timeout = defaultCommandExecutionTimeout
}

if len(opts.Dir) == 0 {
Expand Down Expand Up @@ -238,7 +241,7 @@ func (c *Command) Run(opts *RunOpts) error {
if opts.UseContextTimeout {
ctx, cancel, finished = process.GetManager().AddContext(c.parentContext, desc)
} else {
ctx, cancel, finished = process.GetManager().AddContextTimeout(c.parentContext, opts.Timeout, desc)
ctx, cancel, finished = process.GetManager().AddContextTimeout(c.parentContext, timeout, desc)
}
defer finished()

Expand Down Expand Up @@ -339,9 +342,20 @@ func (c *Command) RunStdBytes(opts *RunOpts) (stdout, stderr []byte, runErr RunS
}
stdoutBuf := &bytes.Buffer{}
stderrBuf := &bytes.Buffer{}
opts.Stdout = stdoutBuf
opts.Stderr = stderrBuf
err := c.Run(opts)

// We must not change the provided options as it could break future calls - therefore make a copy.
newOpts := &RunOpts{
Env: opts.Env,
Timeout: opts.Timeout,
UseContextTimeout: opts.UseContextTimeout,
Dir: opts.Dir,
Stdout: stdoutBuf,
Stderr: stderrBuf,
Stdin: opts.Stdin,
PipelineFunc: opts.PipelineFunc,
}

err := c.Run(newOpts)
stderr = stderrBuf.Bytes()
if err != nil {
return nil, stderr, &runStdError{err: err, stderr: bytesToString(stderr)}
Expand Down

0 comments on commit 421f293

Please sign in to comment.