Refactor and tidy-up the merge/update branch code

services/pull/merge.go

@@ -192,8 +192,8 @@ func Merge(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.U
 	if err := pr.Issue.LoadRepo(hammerCtx); err != nil {
 		log.Error("pr.Issue.LoadRepo %-v: %v", pr, err)
 	}
-	if err := pr.Issue.Repo.GetOwner(hammerCtx); err != nil {
-		log.Error("GetOwner for %-v: %v", pr, err)
+	if err := pr.Issue.Repo.LoadOwner(hammerCtx); err != nil {
+		log.Error("LoadOwner for PR [%d]: %v", pr.ID, err)
 	}
 
 	if wasAutoMerged {
@@ -332,20 +332,11 @@ func doMergeAndPush(ctx context.Context, pr *issues_model.PullRequest, doer *use
 }
 
 func commitAndSignNoAuthor(ctx *mergeContext, message string) error {
-	if ctx.signArg == "" {
-		if err := git.NewCommand(ctx, "commit", "-m").AddDynamicArguments(message).
-			Run(ctx.RunOpts()); err != nil {
-			log.Error("git commit %-v: %v\n%s\n%s", ctx.pr, err, ctx.outbuf.String(), ctx.errbuf.String())
-			return fmt.Errorf("git commit %v: %w\n%s\n%s", ctx.pr, err, ctx.outbuf.String(), ctx.errbuf.String())
-		}
-	} else {
-		if err := git.NewCommand(ctx, "commit").AddArguments(ctx.signArg).AddArguments("-m").AddDynamicArguments(message).
-			Run(ctx.RunOpts()); err != nil {
-			log.Error("git commit %-v: %v\n%s\n%s", ctx.pr, err, ctx.outbuf.String(), ctx.errbuf.String())
-			return fmt.Errorf("git commit %v: %w\n%s\n%s", ctx.pr, err, ctx.outbuf.String(), ctx.errbuf.String())
-		}
+	if err := git.NewCommand(ctx, "commit").AddArguments(ctx.signArg...).AddOptionFormat("--message=%s", message).
+		Run(ctx.RunOpts()); err != nil {
+		log.Error("git commit %-v: %v\n%s\n%s", ctx.pr, err, ctx.outbuf.String(), ctx.errbuf.String())
+		return fmt.Errorf("git commit %v: %w\n%s\n%s", ctx.pr, err, ctx.outbuf.String(), ctx.errbuf.String())
 	}
-
 	return nil
 }
 

services/pull/merge_merge.go

@@ -1,4 +1,4 @@
-// Copyright 2022 The Gitea Authors. All rights reserved.
+// Copyright 2023 The Gitea Authors. All rights reserved.
 // SPDX-License-Identifier: MIT
 
 package pull
@@ -13,12 +13,12 @@ import (
 func doMergeStyleMerge(ctx *mergeContext, message string) error {
 	cmd := git.NewCommand(ctx, "merge", "--no-ff", "--no-commit").AddDynamicArguments(trackingBranch)
 	if err := runMergeCommand(ctx, repo_model.MergeStyleMerge, cmd); err != nil {
-		log.Error("Unable to merge tracking into base: %v", err)
+		log.Error("%-v Unable to merge tracking into base: %v", ctx.pr, err)
 		return err
 	}
 
 	if err := commitAndSignNoAuthor(ctx, message); err != nil {
-		log.Error("Unable to make final commit: %v", err)
+		log.Error("%-v Unable to make final commit: %v", ctx.pr, err)
 		return err
 	}
 	return nil

services/pull/merge_prepare.go

@@ -1,4 +1,4 @@
-// Copyright 2022 The Gitea Authors. All rights reserved.
+// Copyright 2023 The Gitea Authors. All rights reserved.
 // SPDX-License-Identifier: MIT
 
 package pull
@@ -8,6 +8,7 @@ import (
 	"bytes"
 	"context"
 	"fmt"
+	"io"
 	"os"
 	"path/filepath"
 	"strings"
@@ -27,7 +28,7 @@ type mergeContext struct {
 	doer      *user_model.User
 	sig       *git.Signature
 	committer *git.Signature
-	signArg   git.CmdArg
+	signArg   git.TrustedCmdArgs
 	env       []string
 }
 
@@ -84,12 +85,12 @@ func createTemporaryRepoForMerge(ctx context.Context, pr *issues_model.PullReque
 	// Determine if we should sign
 	sign, keyID, signer, _ := asymkey_service.SignMerge(ctx, mergeCtx.pr, mergeCtx.doer, mergeCtx.tmpBasePath, "HEAD", trackingBranch)
 	if sign {
-		mergeCtx.signArg = git.CmdArg("-S" + keyID)
+		mergeCtx.signArg = git.ToTrustedCmdArgs([]string{"-S" + keyID})
 		if pr.BaseRepo.GetTrustModel() == repo_model.CommitterTrustModel || pr.BaseRepo.GetTrustModel() == repo_model.CollaboratorCommitterTrustModel {
 			mergeCtx.committer = signer
 		}
 	} else {
-		mergeCtx.signArg = git.CmdArg("--no-gpg-sign")
+		mergeCtx.signArg = git.ToTrustedCmdArgs([]string{"--no-gpg-sign"})
 	}
 
 	commitTimeStr := time.Now().Format(time.RFC3339)
@@ -108,34 +109,39 @@ func createTemporaryRepoForMerge(ctx context.Context, pr *issues_model.PullReque
 }
 
 // prepareTemporaryRepoForMerge takes a repository that has been created using createTemporaryRepo
-// it then sets up the sparse-checkout and other thngs
+// it then sets up the sparse-checkout and other things
 func prepareTemporaryRepoForMerge(ctx *mergeContext) error {
-	// Enable sparse-checkout
-	sparseCheckoutList, err := getDiffTree(ctx, ctx.tmpBasePath, baseBranch, trackingBranch, ctx.outbuf)
-	if err != nil {
-		log.Error("getDiffTree(%s, %s, %s): %v", ctx.tmpBasePath, baseBranch, trackingBranch, err)
-		return fmt.Errorf("getDiffTree: %w", err)
-	}
-	ctx.outbuf.Reset()
-
 	infoPath := filepath.Join(ctx.tmpBasePath, ".git", "info")
 	if err := os.MkdirAll(infoPath, 0o700); err != nil {
-		log.Error("Unable to create .git/info in %s: %v", ctx.tmpBasePath, err)
+		log.Error("%-v Unable to create .git/info in %s: %v", ctx.pr, ctx.tmpBasePath, err)
 		return fmt.Errorf("Unable to create .git/info in tmpBasePath: %w", err)
 	}
 
-	sparseCheckoutListPath := filepath.Join(infoPath, "sparse-checkout")
-	if err := os.WriteFile(sparseCheckoutListPath, []byte(sparseCheckoutList), 0o600); err != nil {
-		log.Error("Unable to write .git/info/sparse-checkout file in %s: %v", ctx.tmpBasePath, err)
+	// Enable sparse-checkout
+	// Here we use the .git/info/sparse-checkout file as described in the git documentation
+	sparseCheckoutListFile, err := os.OpenFile(filepath.Join(infoPath, "sparse-checkout"), os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0o600)
+	if err != nil {
+		log.Error("%-v Unable to write .git/info/sparse-checkout file in %s: %v", ctx.pr, ctx.tmpBasePath, err)
 		return fmt.Errorf("Unable to write .git/info/sparse-checkout file in tmpBasePath: %w", err)
 	}
+	defer sparseCheckoutListFile.Close() // we will close it earlier but we need to ensure it is closed if there is an error
+
+	if err := getDiffTree(ctx, ctx.tmpBasePath, baseBranch, trackingBranch, sparseCheckoutListFile); err != nil {
+		log.Error("%-v getDiffTree(%s, %s, %s): %v", ctx.pr, ctx.tmpBasePath, baseBranch, trackingBranch, err)
+		return fmt.Errorf("getDiffTree: %w", err)
+	}
+
+	if err := sparseCheckoutListFile.Close(); err != nil {
+		log.Error("%-v Unable to close .git/info/sparse-checkout file in %s: %v", ctx.pr, ctx.tmpBasePath, err)
+		return fmt.Errorf("Unable to close .git/info/sparse-checkout file in tmpBasePath: %w", err)
+	}
 
 	gitConfigCommand := func() *git.Command {
 		return git.NewCommand(ctx, "config", "--local")
 	}
 
-	setConfig := func(key, value git.CmdArg) error {
-		if err := gitConfigCommand().AddArguments(key, value).
+	setConfig := func(key, value string) error {
+		if err := gitConfigCommand().AddArguments(git.ToTrustedCmdArgs([]string{key, value})...).
 			Run(ctx.RunOpts()); err != nil {
 			if value == "" {
 				value = "<>"
@@ -183,14 +189,12 @@ func prepareTemporaryRepoForMerge(ctx *mergeContext) error {
 }
 
 // getDiffTree returns a string containing all the files that were changed between headBranch and baseBranch
-func getDiffTree(ctx context.Context, repoPath, baseBranch, headBranch string, out *strings.Builder) (string, error) {
-	out.Reset()
-	defer out.Reset()
-
+// the filenames are escaped so as to fit the format required for .git/info/sparse-checkout
+func getDiffTree(ctx context.Context, repoPath, baseBranch, headBranch string, out io.Writer) error {
 	diffOutReader, diffOutWriter, err := os.Pipe()
 	if err != nil {
 		log.Error("Unable to create os.Pipe for %s", repoPath)
-		return "", err
+		return err
 	}
 	defer func() {
 		_ = diffOutReader.Close()
@@ -235,7 +239,7 @@ func getDiffTree(ctx context.Context, repoPath, baseBranch, headBranch string, o
 				return scanner.Err()
 			},
 		})
-	return out.String(), err
+	return err
 }
 
 // rebaseTrackingOnToBase checks out the tracking branch as staging and rebases it on to the base branch

services/pull/merge_rebase.go

@@ -1,4 +1,4 @@
-// Copyright 2022 The Gitea Authors. All rights reserved.
+// Copyright 2023 The Gitea Authors. All rights reserved.
 // SPDX-License-Identifier: MIT
 
 package pull
@@ -21,7 +21,7 @@ func doMergeStyleRebase(ctx *mergeContext, mergeStyle repo_model.MergeStyle, mes
 	if err := git.NewCommand(ctx, "checkout").AddDynamicArguments(baseBranch).
 		Run(ctx.RunOpts()); err != nil {
 		log.Error("git checkout base prior to merge post staging rebase %-v: %v\n%s\n%s", ctx.pr, err, ctx.outbuf.String(), ctx.errbuf.String())
-		return fmt.Errorf("git checkout base prior to merge post staging rebase  [%s:%s -> %s:%s]: %w\n%s\n%s", ctx.pr.HeadRepo.FullName(), ctx.pr.HeadBranch, ctx.pr.BaseRepo.FullName(), ctx.pr.BaseBranch, err, ctx.outbuf.String(), ctx.errbuf.String())
+		return fmt.Errorf("git checkout base prior to merge post staging rebase  %v: %w\n%s\n%s", ctx.pr, err, ctx.outbuf.String(), ctx.errbuf.String())
 	}
 	ctx.outbuf.Reset()
 	ctx.errbuf.Reset()

services/pull/merge_squash.go

@@ -1,4 +1,4 @@
-// Copyright 2022 The Gitea Authors. All rights reserved.
+// Copyright 2023 The Gitea Authors. All rights reserved.
 // SPDX-License-Identifier: MIT
 
 package pull
@@ -16,19 +16,21 @@ import (
 func doMergeStyleSquash(ctx *mergeContext, message string) error {
 	cmd := git.NewCommand(ctx, "merge", "--squash").AddDynamicArguments(trackingBranch)
 	if err := runMergeCommand(ctx, repo_model.MergeStyleSquash, cmd); err != nil {
-		log.Error("Unable to merge --squash tracking into base: %v", err)
+		log.Error("%-v Unable to merge --squash tracking into base: %v", ctx.pr, err)
 		return err
 	}
 
 	if err := ctx.pr.Issue.LoadPoster(ctx); err != nil {
-		log.Error("LoadPoster: %v", err)
+		log.Error("%-v Issue[%d].LoadPoster: %v", ctx.pr, ctx.pr.Issue.ID, err)
 		return fmt.Errorf("LoadPoster: %w", err)
 	}
 	sig := ctx.pr.Issue.Poster.NewGitSig()
-	if ctx.signArg == "" {
-		if err := git.NewCommand(ctx, "commit", git.CmdArg(fmt.Sprintf("--author='%s <%s>'", sig.Name, sig.Email)), "-m").AddDynamicArguments(message).
+	if len(ctx.signArg) == 0 {
+		if err := git.NewCommand(ctx, "commit").
+			AddOptionFormat("--author='%s <%s>'", sig.Name, sig.Email).
+			AddOptionFormat("--message=%s", message).
 			Run(ctx.RunOpts()); err != nil {
-			log.Error("git commit [%s:%s -> %s:%s]: %v\n%s\n%s", ctx.pr.HeadRepo.FullName(), ctx.pr.HeadBranch, ctx.pr.BaseRepo.FullName(), ctx.pr.BaseBranch, err, ctx.outbuf.String(), ctx.errbuf.String())
+			log.Error("git commit %-v: %v\n%s\n%s", ctx.pr, err, ctx.outbuf.String(), ctx.errbuf.String())
 			return fmt.Errorf("git commit [%s:%s -> %s:%s]: %w\n%s\n%s", ctx.pr.HeadRepo.FullName(), ctx.pr.HeadBranch, ctx.pr.BaseRepo.FullName(), ctx.pr.BaseBranch, err, ctx.outbuf.String(), ctx.errbuf.String())
 		}
 	} else {
@@ -37,11 +39,11 @@ func doMergeStyleSquash(ctx *mergeContext, message string) error {
 			message += fmt.Sprintf("\nCo-authored-by: %s\nCo-committed-by: %s\n", sig.String(), sig.String())
 		}
 		if err := git.NewCommand(ctx, "commit").
-			AddArguments(ctx.signArg).
-			AddArguments(git.CmdArg(fmt.Sprintf("--author='%s <%s>'", sig.Name, sig.Email))).
-			AddArguments("-m").AddDynamicArguments(message).
+			AddArguments(ctx.signArg...).
+			AddOptionFormat("--author='%s <%s>'", sig.Name, sig.Email).
+			AddOptionFormat("--message=%s", message).
 			Run(ctx.RunOpts()); err != nil {
-			log.Error("git commit [%s:%s -> %s:%s]: %v\n%s\n%s", ctx.pr.HeadRepo.FullName(), ctx.pr.HeadBranch, ctx.pr.BaseRepo.FullName(), ctx.pr.BaseBranch, err, ctx.outbuf.String(), ctx.errbuf.String())
+			log.Error("git commit %-v: %v\n%s\n%s", ctx.pr, err, ctx.outbuf.String(), ctx.errbuf.String())
 			return fmt.Errorf("git commit [%s:%s -> %s:%s]: %w\n%s\n%s", ctx.pr.HeadRepo.FullName(), ctx.pr.HeadBranch, ctx.pr.BaseRepo.FullName(), ctx.pr.BaseBranch, err, ctx.outbuf.String(), ctx.errbuf.String())
 		}
 	}

services/pull/patch.go

@@ -65,7 +65,7 @@ func TestPatch(pr *issues_model.PullRequest) error {
 	// Clone base repo.
 	prCtx, cancel, err := createTemporaryRepoForPR(ctx, pr)
 	if err != nil {
-		log.Error("CreateTemporaryPath: %v", err)
+		log.Error("createTemporaryRepoForPR %-v: %v", pr, err)
 		return err
 	}
 	defer cancel()

services/pull/pull.go

@@ -349,18 +349,14 @@ func AddTestPullRequestTask(doer *user_model.User, repoID int64, branch string,
 // checkIfPRContentChanged checks if diff to target branch has changed by push
 // A commit can be considered to leave the PR untouched if the patch/diff with its merge base is unchanged
 func checkIfPRContentChanged(ctx context.Context, pr *issues_model.PullRequest, oldCommitID, newCommitID string) (hasChanged bool, err error) {
-	tmpBasePath, err := createTemporaryRepo(ctx, pr)
+	prCtx, cancel, err := createTemporaryRepoForPR(ctx, pr)
 	if err != nil {
-		log.Error("CreateTemporaryRepo: %v", err)
+		log.Error("CreateTemporaryRepoForPR %-v: %v", pr, err)
 		return false, err
 	}
-	defer func() {
-		if err := repo_module.RemoveTemporaryPath(tmpBasePath); err != nil {
-			log.Error("checkIfPRContentChanged: RemoveTemporaryPath: %s", err)
-		}
-	}()
+	defer cancel()
 
-	tmpRepo, err := git.OpenRepository(ctx, tmpBasePath)
+	tmpRepo, err := git.OpenRepository(ctx, prCtx.tmpBasePath)
 	if err != nil {
 		return false, fmt.Errorf("OpenRepository: %w", err)
 	}
@@ -379,7 +375,7 @@ func checkIfPRContentChanged(ctx context.Context, pr *issues_model.PullRequest,
 	}
 
 	if err := cmd.Run(&git.RunOpts{
-		Dir:    tmpBasePath,
+		Dir:    prCtx.tmpBasePath,
 		Stdout: stdoutWriter,
 		PipelineFunc: func(ctx context.Context, cancel context.CancelFunc) error {
 			_ = stdoutWriter.Close()