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

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>

diff --git a/modules/doctor/heads.go b/modules/doctor/heads.go
index ec14aa4eadb1b89f3d6c0f69b340b4422531e109..3ae7c6e15a9a96b7911a768fe785d4e9979adadb 100644 (file)
--- a/modules/doctor/heads.go
+++ b/modules/doctor/heads.go
@@ -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", "--", repo.DefaultBranch).RunStdString(&git.RunOpts{Dir: repo.RepoPath()})
 
-               _, _, defaultBranchErr := git.NewCommand(ctx, "rev-parse", "--", 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 {
@@ -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", "--", "HEAD", repo.DefaultBranch).Run(runOpts)
+               err := git.NewCommand(ctx, "symbolic-ref", "--", "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
@@ -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)

diff --git a/modules/git/command.go b/modules/git/command.go
index ed06dd9b08de946b1a6d0944cc0137cef7a574b0..17114ab473a84bef22f809a0f0eb527284f4f8c4 100644 (file)
--- a/modules/git/command.go
+++ b/modules/git/command.go
@@ -169,8 +169,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 {
@@ -205,7 +208,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()
 
@@ -306,9 +309,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)}