Refactor merge/update git command calls (go-gitea#23366)

Follow go-gitea#22568 * Remove unnecessary ToTrustedCmdArgs calls * the FAQ in go-gitea#22678 * Quote: When using ToTrustedCmdArgs, the code will be very complex (see the changes for examples). Then developers and reviewers can know that something might be unreasonable. * The `signArg` couldn't be empty, it's either `-S{keyID}` or `--no-gpg-sign`. * Use `signKeyID` instead, add comment "empty for no-sign, non-empty to sign" * 5-line code could be extracted to a common `NewGitCommandCommit()` to handle the `signKeyID`, but I think it's not a must, current code is clear enough.
wxiaoguang · Mar 17, 2023 · c47ef82 · c47ef82
1 parent 68c9f1a
commit c47ef82
Show file tree

Hide file tree

Showing 4 changed files with 29 additions and 39 deletions.
diff --git a/modules/git/command.go b/modules/git/command.go
@@ -179,7 +179,7 @@ func (c *Command) AddDashesAndList(list ...string) *Command {
 }
 
 // ToTrustedCmdArgs converts a list of strings (trusted as argument) to TrustedCmdArgs
-// In most cases, it shouldn't be used. Use AddXxx function instead
+// In most cases, it shouldn't be used. Use NewCommand().AddXxx() function instead
 func ToTrustedCmdArgs(args []string) TrustedCmdArgs {
 	ret := make(TrustedCmdArgs, len(args))
 	for i, arg := range args {

diff --git a/services/pull/merge.go b/services/pull/merge.go
@@ -332,8 +332,13 @@ func doMergeAndPush(ctx context.Context, pr *issues_model.PullRequest, doer *use
 }
 
 func commitAndSignNoAuthor(ctx *mergeContext, message string) error {
-	if err := git.NewCommand(ctx, "commit").AddArguments(ctx.signArg...).AddOptionFormat("--message=%s", message).
-		Run(ctx.RunOpts()); err != nil {
+	cmdCommit := git.NewCommand(ctx, "commit").AddOptionFormat("--message=%s", message)
+	if ctx.signKeyID == "" {
+		cmdCommit.AddArguments("--no-gpg-sign")
+	} else {
+		cmdCommit.AddOptionFormat("-S%s", ctx.signKeyID)
+	}
+	if err := cmdCommit.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())
 	}

diff --git a/services/pull/merge_prepare.go b/services/pull/merge_prepare.go
@@ -28,7 +28,7 @@ type mergeContext struct {
 	doer      *user_model.User
 	sig       *git.Signature
 	committer *git.Signature
-	signArg   git.TrustedCmdArgs
+	signKeyID string // empty for no-sign, non-empty to sign
 	env       []string
 }
 
@@ -85,12 +85,10 @@ 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.ToTrustedCmdArgs([]string{"-S" + keyID})
+		mergeCtx.signKeyID = keyID
 		if pr.BaseRepo.GetTrustModel() == repo_model.CommitterTrustModel || pr.BaseRepo.GetTrustModel() == repo_model.CollaboratorCommitterTrustModel {
 			mergeCtx.committer = signer
 		}
-	} else {
-		mergeCtx.signArg = git.ToTrustedCmdArgs([]string{"--no-gpg-sign"})
 	}
 
 	commitTimeStr := time.Now().Format(time.RFC3339)
@@ -136,18 +134,11 @@ func prepareTemporaryRepoForMerge(ctx *mergeContext) error {
 		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 string) error {
-		if err := gitConfigCommand().AddArguments(git.ToTrustedCmdArgs([]string{key, value})...).
+		if err := git.NewCommand(ctx, "config", "--local").AddDynamicArguments(key, value).
 			Run(ctx.RunOpts()); err != nil {
-			if value == "" {
-				value = "<>"
-			}
-			log.Error("git config [%s -> %s ]: %v\n%s\n%s", key, value, err, ctx.outbuf.String(), ctx.errbuf.String())
-			return fmt.Errorf("git config [%s -> %s ]: %w\n%s\n%s", key, value, err, ctx.outbuf.String(), ctx.errbuf.String())
+			log.Error("git config [%s -> %q]: %v\n%s\n%s", key, value, err, ctx.outbuf.String(), ctx.errbuf.String())
+			return fmt.Errorf("git config [%s -> %q]: %w\n%s\n%s", key, value, err, ctx.outbuf.String(), ctx.errbuf.String())
 		}
 		ctx.outbuf.Reset()
 		ctx.errbuf.Reset()

diff --git a/services/pull/merge_squash.go b/services/pull/merge_squash.go
@@ -14,8 +14,8 @@ import (
 
 // doMergeStyleSquash squashes the tracking branch on the current HEAD (=base)
 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 {
+	cmdMerge := git.NewCommand(ctx, "merge", "--squash").AddDynamicArguments(trackingBranch)
+	if err := runMergeCommand(ctx, repo_model.MergeStyleSquash, cmdMerge); err != nil {
 		log.Error("%-v Unable to merge --squash tracking into base: %v", ctx.pr, err)
 		return err
 	}
@@ -25,27 +25,21 @@ func doMergeStyleSquash(ctx *mergeContext, message string) error {
 		return fmt.Errorf("LoadPoster: %w", err)
 	}
 	sig := ctx.pr.Issue.Poster.NewGitSig()
-	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 %-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())
-		}
+	if setting.Repository.PullRequest.AddCoCommitterTrailers && ctx.committer.String() != sig.String() {
+		// add trailer
+		message += fmt.Sprintf("\nCo-authored-by: %s\nCo-committed-by: %s\n", sig.String(), sig.String())
+	}
+	cmdCommit := git.NewCommand(ctx, "commit").
+		AddOptionFormat("--author='%s <%s>'", sig.Name, sig.Email).
+		AddOptionFormat("--message=%s", message)
+	if ctx.signKeyID == "" {
+		cmdCommit.AddArguments("--no-gpg-sign")
 	} else {
-		if setting.Repository.PullRequest.AddCoCommitterTrailers && ctx.committer.String() != sig.String() {
-			// add trailer
-			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...).
-			AddOptionFormat("--author='%s <%s>'", sig.Name, sig.Email).
-			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 [%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())
-		}
+		cmdCommit.AddOptionFormat("-S%s", ctx.signKeyID)
+	}
+	if err := cmdCommit.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 [%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())
 	}
 	ctx.outbuf.Reset()
 	ctx.errbuf.Reset()