femsci/forgejo
femsci/forgejo
1
0
Fork 0
forked from NYANDEV/forgejo
Code Pull requests Activity

Prevent panic in doctor command when running default checks (#21791) (#21807)

Browse source 
Backport #21791

There was a bug introduced in #21352 due to a change of behaviour caused
by #19280. This causes a panic on running the default doctor checks
because the panic introduced by #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 <art27@cantab.net>
This commit is contained in:
zeripath 2022-11-14 02:58:32 +00:00 committed by GitHub
parent ac409fcfba
commit 0d25292fbc
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
2 changed files with 24 additions and 12 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

10
modules/doctor/heads.go
View file

@ -19,11 +19,9 @@ func synchronizeRepoHeads(ctx context.Context, logger log.Logger, autofix bool)
numReposUpdated := 0 numReposUpdated := 0
err := iterateRepositories(ctx, func(repo *repo_model.Repository) error { err := iterateRepositories(ctx, func(repo *repo_model.Repository) error {
numRepos++ 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(&git.RunOpts{Dir: repo.RepoPath()})
head, _, headErr := git.NewCommand(ctx, "symbolic-ref", "--short", "HEAD").RunStdString(runOpts)
// what we expect: default branch is valid, and HEAD points to it // what we expect: default branch is valid, and HEAD points to it
if headErr == nil && defaultBranchErr == nil && head == repo.DefaultBranch { if headErr == nil && defaultBranchErr == nil && head == repo.DefaultBranch {
@ -49,7 +47,7 @@ func synchronizeRepoHeads(ctx context.Context, logger log.Logger, autofix bool)
} }
// otherwise, let's try fixing HEAD // 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 { if err != nil {
logger.Warn("Failed to fix HEAD for %s/%s: %v", repo.OwnerName, repo.Name, err) logger.Warn("Failed to fix HEAD for %s/%s: %v", repo.OwnerName, repo.Name, err)
return nil return nil
@ -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) 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 { } else {
if numHeadsBroken == 0 && numDefaultBranchesBroken == 0 { 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 { } else {
if numHeadsBroken == 0 && numDefaultBranchesBroken != 0 { if numHeadsBroken == 0 && numDefaultBranchesBroken != 0 {
logger.Critical("Default branches are broken for %d/%d repos", numDefaultBranchesBroken, numRepos) logger.Critical("Default branches are broken for %d/%d repos", numDefaultBranchesBroken, numRepos)

26
modules/git/command.go
View file

@ -202,8 +202,11 @@ func (c *Command) Run(opts *RunOpts) error {
if opts == nil { if opts == nil {
opts = &RunOpts{} 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 { if len(opts.Dir) == 0 {
@ -238,7 +241,7 @@ func (c *Command) Run(opts *RunOpts) error {
if opts.UseContextTimeout { if opts.UseContextTimeout {
ctx, cancel, finished = process.GetManager().AddContext(c.parentContext, desc) ctx, cancel, finished = process.GetManager().AddContext(c.parentContext, desc)
} else { } else {
ctx, cancel, finished = process.GetManager().AddContextTimeout(c.parentContext, opts.Timeout, desc) ctx, cancel, finished = process.GetManager().AddContextTimeout(c.parentContext, timeout, desc)
} }
defer finished() defer finished()
@ -339,9 +342,20 @@ func (c *Command) RunStdBytes(opts *RunOpts) (stdout, stderr []byte, runErr RunS
} }
stdoutBuf := &bytes.Buffer{} stdoutBuf := &bytes.Buffer{}
stderrBuf := &bytes.Buffer{} stderrBuf := &bytes.Buffer{}
opts.Stdout = stdoutBuf
opts.Stderr = stderrBuf // We must not change the provided options as it could break future calls - therefore make a copy.
err := c.Run(opts) 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() stderr = stderrBuf.Bytes()
if err != nil { if err != nil {
return nil, stderr, &runStdError{err: err, stderr: bytesToString(stderr)} return nil, stderr, &runStdError{err: err, stderr: bytesToString(stderr)}