Refactor git command package to improve security and maintainability (#22678)

This PR follows #21535 (and replace #22592)

## Review without space diff

https://github.com/go-gitea/gitea/pull/22678/files?diff=split&w=1

## Purpose of this PR

1. Make git module command completely safe (risky user inputs won't be
passed as argument option anymore)
2. Avoid low-level mistakes like
https://github.com/go-gitea/gitea/pull/22098#discussion_r1045234918
3. Remove deprecated and dirty `CmdArgCheck` function, hide the `CmdArg`
type
4. Simplify code when using git command

## The main idea of this PR

* Move the `git.CmdArg` to the `internal` package, then no other package
except `git` could use it. Then developers could never do
`AddArguments(git.CmdArg(userInput))` any more.
* Introduce `git.ToTrustedCmdArgs`, it's for user-provided and already
trusted arguments. It's only used in a few cases, for example: use git
arguments from config file, help unit test with some arguments.
* Introduce `AddOptionValues` and `AddOptionFormat`, they make code more
clear and simple:
    * Before: `AddArguments("-m").AddDynamicArguments(message)`
    * After: `AddOptionValues("-m", message)`
    * -
* Before: `AddArguments(git.CmdArg(fmt.Sprintf("--author='%s <%s>'",
sig.Name, sig.Email)))`
* After: `AddOptionFormat("--author='%s <%s>'", sig.Name, sig.Email)`

## FAQ

### Why these changes were not done in #21535 ?

#21535 is mainly a search&replace, it did its best to not change too
much logic.

Making the framework better needs a lot of changes, so this separate PR
is needed as the second step.


### The naming of `AddOptionXxx`

According to git's manual, the `--xxx` part is called `option`.

### How can it guarantee that `internal.CmdArg` won't be not misused?

Go's specification guarantees that. Trying to access other package's
internal package causes compilation error.

And, `golangci-lint` also denies the git/internal package. Only the
`git/command.go` can use it carefully.

### There is still a `ToTrustedCmdArgs`, will it still allow developers
to make mistakes and pass untrusted arguments?

Generally speaking, no. Because 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.

### Why there was a `CmdArgCheck` and why it's removed?

At the moment of #21535, to reduce unnecessary changes, `CmdArgCheck`
was introduced as a hacky patch. Now, almost all code could be written
as `cmd := NewCommand(); cmd.AddXxx(...)`, then there is no need for
`CmdArgCheck` anymore.


### Why many codes for `signArg == ""` is deleted?

Because in the old code, `signArg` could never be empty string, it's
either `-S[key-id]` or `--no-gpg-sign`. So the `signArg == ""` is just
dead code.

---------

Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>

2 files changed, 93 insertions, 120 deletions

diff --git a/services/pull/merge.go b/services/pull/merge.go
index 7ffbdb78b0..edd5b601da 100644
--- a/services/pull/merge.go
+++ b/services/pull/merge.go
@@ -370,16 +370,16 @@ func rawMerge(ctx context.Context, pr *issues_model.PullRequest, doer *user_mode
 	sig := doer.NewGitSig()
 	committer := sig
 
-	// Determine if we should sign
-	var signArg git.CmdArg
-	sign, keyID, signer, _ := asymkey_service.SignMerge(ctx, pr, doer, tmpBasePath, "HEAD", trackingBranch)
+	// Determine if we should sign. If no signKeyID, use --no-gpg-sign to countermand the sign config (from gitconfig)
+	var signArgs git.TrustedCmdArgs
+	sign, signKeyID, signer, _ := asymkey_service.SignMerge(ctx, pr, doer, tmpBasePath, "HEAD", trackingBranch)
 	if sign {
-		signArg = git.CmdArg("-S" + keyID)
 		if pr.BaseRepo.GetTrustModel() == repo_model.CommitterTrustModel || pr.BaseRepo.GetTrustModel() == repo_model.CollaboratorCommitterTrustModel {
 			committer = signer
 		}
+		signArgs = git.ToTrustedCmdArgs([]string{"-S" + signKeyID})
 	} else {
-		signArg = git.CmdArg("--no-gpg-sign")
+		signArgs = append(signArgs, "--no-gpg-sign")
 	}
 
 	commitTimeStr := time.Now().Format(time.RFC3339)
@@ -403,7 +403,7 @@ func rawMerge(ctx context.Context, pr *issues_model.PullRequest, doer *user_mode
 			return "", err
 		}
 
-		if err := commitAndSignNoAuthor(ctx, pr, message, signArg, tmpBasePath, env); err != nil {
+		if err := commitAndSignNoAuthor(ctx, pr, message, signArgs, tmpBasePath, env); err != nil {
 			log.Error("Unable to make final commit: %v", err)
 			return "", err
 		}
@@ -505,7 +505,7 @@ func rawMerge(ctx context.Context, pr *issues_model.PullRequest, doer *user_mode
 			return "", err
 		}
 		if mergeStyle == repo_model.MergeStyleRebaseMerge {
-			if err := commitAndSignNoAuthor(ctx, pr, message, signArg, tmpBasePath, env); err != nil {
+			if err := commitAndSignNoAuthor(ctx, pr, message, signArgs, tmpBasePath, env); err != nil {
 				log.Error("Unable to make final commit: %v", err)
 				return "", err
 			}
@@ -523,35 +523,22 @@ func rawMerge(ctx context.Context, pr *issues_model.PullRequest, doer *user_mode
 			return "", fmt.Errorf("LoadPoster: %w", err)
 		}
 		sig := pr.Issue.Poster.NewGitSig()
-		if signArg == "" {
-			if err := git.NewCommand(ctx, "commit", git.CmdArg(fmt.Sprintf("--author='%s <%s>'", sig.Name, sig.Email)), "-m").AddDynamicArguments(message).
-				Run(&git.RunOpts{
-					Env:    env,
-					Dir:    tmpBasePath,
-					Stdout: &outbuf,
-					Stderr: &errbuf,
-				}); err != nil {
-				log.Error("git commit [%s:%s -> %s:%s]: %v\n%s\n%s", pr.HeadRepo.FullName(), pr.HeadBranch, pr.BaseRepo.FullName(), pr.BaseBranch, err, outbuf.String(), errbuf.String())
-				return "", fmt.Errorf("git commit [%s:%s -> %s:%s]: %w\n%s\n%s", pr.HeadRepo.FullName(), pr.HeadBranch, pr.BaseRepo.FullName(), pr.BaseBranch, err, outbuf.String(), errbuf.String())
-			}
-		} else {
-			if setting.Repository.PullRequest.AddCoCommitterTrailers && 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(signArg).
-				AddArguments(git.CmdArg(fmt.Sprintf("--author='%s <%s>'", sig.Name, sig.Email))).
-				AddArguments("-m").AddDynamicArguments(message).
-				Run(&git.RunOpts{
-					Env:    env,
-					Dir:    tmpBasePath,
-					Stdout: &outbuf,
-					Stderr: &errbuf,
-				}); err != nil {
-				log.Error("git commit [%s:%s -> %s:%s]: %v\n%s\n%s", pr.HeadRepo.FullName(), pr.HeadBranch, pr.BaseRepo.FullName(), pr.BaseBranch, err, outbuf.String(), errbuf.String())
-				return "", fmt.Errorf("git commit [%s:%s -> %s:%s]: %w\n%s\n%s", pr.HeadRepo.FullName(), pr.HeadBranch, pr.BaseRepo.FullName(), pr.BaseBranch, err, outbuf.String(), errbuf.String())
-			}
+		if setting.Repository.PullRequest.AddCoCommitterTrailers && 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(signArgs...).
+			AddOptionFormat("--author='%s <%s>'", sig.Name, sig.Email).
+			AddOptionValues("-m", message).
+			Run(&git.RunOpts{
+				Env:    env,
+				Dir:    tmpBasePath,
+				Stdout: &outbuf,
+				Stderr: &errbuf,
+			}); err != nil {
+			log.Error("git commit [%s:%s -> %s:%s]: %v\n%s\n%s", pr.HeadRepo.FullName(), pr.HeadBranch, pr.BaseRepo.FullName(), pr.BaseBranch, err, outbuf.String(), errbuf.String())
+			return "", fmt.Errorf("git commit [%s:%s -> %s:%s]: %w\n%s\n%s", pr.HeadRepo.FullName(), pr.HeadBranch, pr.BaseRepo.FullName(), pr.BaseBranch, err, outbuf.String(), errbuf.String())
 		}
 		outbuf.Reset()
 		errbuf.Reset()
@@ -649,30 +636,17 @@ func rawMerge(ctx context.Context, pr *issues_model.PullRequest, doer *user_mode
 	return mergeCommitID, nil
 }
 
-func commitAndSignNoAuthor(ctx context.Context, pr *issues_model.PullRequest, message string, signArg git.CmdArg, tmpBasePath string, env []string) error {
+func commitAndSignNoAuthor(ctx context.Context, pr *issues_model.PullRequest, message string, signArgs git.TrustedCmdArgs, tmpBasePath string, env []string) error {
 	var outbuf, errbuf strings.Builder
-	if signArg == "" {
-		if err := git.NewCommand(ctx, "commit", "-m").AddDynamicArguments(message).
-			Run(&git.RunOpts{
-				Env:    env,
-				Dir:    tmpBasePath,
-				Stdout: &outbuf,
-				Stderr: &errbuf,
-			}); err != nil {
-			log.Error("git commit [%s:%s -> %s:%s]: %v\n%s\n%s", pr.HeadRepo.FullName(), pr.HeadBranch, pr.BaseRepo.FullName(), pr.BaseBranch, err, outbuf.String(), errbuf.String())
-			return fmt.Errorf("git commit [%s:%s -> %s:%s]: %w\n%s\n%s", pr.HeadRepo.FullName(), pr.HeadBranch, pr.BaseRepo.FullName(), pr.BaseBranch, err, outbuf.String(), errbuf.String())
-		}
-	} else {
-		if err := git.NewCommand(ctx, "commit").AddArguments(signArg).AddArguments("-m").AddDynamicArguments(message).
-			Run(&git.RunOpts{
-				Env:    env,
-				Dir:    tmpBasePath,
-				Stdout: &outbuf,
-				Stderr: &errbuf,
-			}); err != nil {
-			log.Error("git commit [%s:%s -> %s:%s]: %v\n%s\n%s", pr.HeadRepo.FullName(), pr.HeadBranch, pr.BaseRepo.FullName(), pr.BaseBranch, err, outbuf.String(), errbuf.String())
-			return fmt.Errorf("git commit [%s:%s -> %s:%s]: %w\n%s\n%s", pr.HeadRepo.FullName(), pr.HeadBranch, pr.BaseRepo.FullName(), pr.BaseBranch, err, outbuf.String(), errbuf.String())
-		}
+	if err := git.NewCommand(ctx, "commit").AddArguments(signArgs...).AddOptionValues("-m", message).
+		Run(&git.RunOpts{
+			Env:    env,
+			Dir:    tmpBasePath,
+			Stdout: &outbuf,
+			Stderr: &errbuf,
+		}); err != nil {
+		log.Error("git commit [%s:%s -> %s:%s]: %v\n%s\n%s", pr.HeadRepo.FullName(), pr.HeadBranch, pr.BaseRepo.FullName(), pr.BaseBranch, err, outbuf.String(), errbuf.String())
+		return fmt.Errorf("git commit [%s:%s -> %s:%s]: %w\n%s\n%s", pr.HeadRepo.FullName(), pr.HeadBranch, pr.BaseRepo.FullName(), pr.BaseBranch, err, outbuf.String(), errbuf.String())
 	}
 	return nil
 }
diff --git a/services/pull/patch.go b/services/pull/patch.go
index a3084196bb..c2ccc75bdc 100644
--- a/services/pull/patch.go
+++ b/services/pull/patch.go
@@ -376,16 +376,16 @@ func checkConflicts(ctx context.Context, pr *issues_model.PullRequest, gitRepo *
 	prConfig := prUnit.PullRequestsConfig()
 
 	// 6. Prepare the arguments to apply the patch against the index
-	args := []git.CmdArg{"apply", "--check", "--cached"}
+	cmdApply := git.NewCommand(gitRepo.Ctx, "apply", "--check", "--cached")
 	if prConfig.IgnoreWhitespaceConflicts {
-		args = append(args, "--ignore-whitespace")
+		cmdApply.AddArguments("--ignore-whitespace")
 	}
 	is3way := false
 	if git.CheckGitVersionAtLeast("2.32.0") == nil {
-		args = append(args, "--3way")
+		cmdApply.AddArguments("--3way")
 		is3way = true
 	}
-	args = append(args, git.CmdArgCheck(patchPath))
+	cmdApply.AddDynamicArguments(patchPath)
 
 	// 7. Prep the pipe:
 	//   - Here we could do the equivalent of:
@@ -407,71 +407,70 @@ func checkConflicts(ctx context.Context, pr *issues_model.PullRequest, gitRepo *
 
 	// 8. Run the check command
 	conflict = false
-	err = git.NewCommand(gitRepo.Ctx, args...).
-		Run(&git.RunOpts{
-			Dir:    tmpBasePath,
-			Stderr: stderrWriter,
-			PipelineFunc: func(ctx context.Context, cancel context.CancelFunc) error {
-				// Close the writer end of the pipe to begin processing
-				_ = stderrWriter.Close()
-				defer func() {
-					// Close the reader on return to terminate the git command if necessary
-					_ = stderrReader.Close()
-				}()
-
-				const prefix = "error: patch failed:"
-				const errorPrefix = "error: "
-				const threewayFailed = "Failed to perform three-way merge..."
-				const appliedPatchPrefix = "Applied patch to '"
-				const withConflicts = "' with conflicts."
-
-				conflicts := make(container.Set[string])
-
-				// Now scan the output from the command
-				scanner := bufio.NewScanner(stderrReader)
-				for scanner.Scan() {
-					line := scanner.Text()
-					log.Trace("PullRequest[%d].testPatch: stderr: %s", pr.ID, line)
-					if strings.HasPrefix(line, prefix) {
-						conflict = true
-						filepath := strings.TrimSpace(strings.Split(line[len(prefix):], ":")[0])
-						conflicts.Add(filepath)
-					} else if is3way && line == threewayFailed {
-						conflict = true
-					} else if strings.HasPrefix(line, errorPrefix) {
-						conflict = true
-						for _, suffix := range patchErrorSuffices {
-							if strings.HasSuffix(line, suffix) {
-								filepath := strings.TrimSpace(strings.TrimSuffix(line[len(errorPrefix):], suffix))
-								if filepath != "" {
-									conflicts.Add(filepath)
-								}
-								break
+	err = cmdApply.Run(&git.RunOpts{
+		Dir:    tmpBasePath,
+		Stderr: stderrWriter,
+		PipelineFunc: func(ctx context.Context, cancel context.CancelFunc) error {
+			// Close the writer end of the pipe to begin processing
+			_ = stderrWriter.Close()
+			defer func() {
+				// Close the reader on return to terminate the git command if necessary
+				_ = stderrReader.Close()
+			}()
+
+			const prefix = "error: patch failed:"
+			const errorPrefix = "error: "
+			const threewayFailed = "Failed to perform three-way merge..."
+			const appliedPatchPrefix = "Applied patch to '"
+			const withConflicts = "' with conflicts."
+
+			conflicts := make(container.Set[string])
+
+			// Now scan the output from the command
+			scanner := bufio.NewScanner(stderrReader)
+			for scanner.Scan() {
+				line := scanner.Text()
+				log.Trace("PullRequest[%d].testPatch: stderr: %s", pr.ID, line)
+				if strings.HasPrefix(line, prefix) {
+					conflict = true
+					filepath := strings.TrimSpace(strings.Split(line[len(prefix):], ":")[0])
+					conflicts.Add(filepath)
+				} else if is3way && line == threewayFailed {
+					conflict = true
+				} else if strings.HasPrefix(line, errorPrefix) {
+					conflict = true
+					for _, suffix := range patchErrorSuffices {
+						if strings.HasSuffix(line, suffix) {
+							filepath := strings.TrimSpace(strings.TrimSuffix(line[len(errorPrefix):], suffix))
+							if filepath != "" {
+								conflicts.Add(filepath)
 							}
-						}
-					} else if is3way && strings.HasPrefix(line, appliedPatchPrefix) && strings.HasSuffix(line, withConflicts) {
-						conflict = true
-						filepath := strings.TrimPrefix(strings.TrimSuffix(line, withConflicts), appliedPatchPrefix)
-						if filepath != "" {
-							conflicts.Add(filepath)
+							break
 						}
 					}
-					// only list 10 conflicted files
-					if len(conflicts) >= 10 {
-						break
+				} else if is3way && strings.HasPrefix(line, appliedPatchPrefix) && strings.HasSuffix(line, withConflicts) {
+					conflict = true
+					filepath := strings.TrimPrefix(strings.TrimSuffix(line, withConflicts), appliedPatchPrefix)
+					if filepath != "" {
+						conflicts.Add(filepath)
 					}
 				}
+				// only list 10 conflicted files
+				if len(conflicts) >= 10 {
+					break
+				}
+			}
 
-				if len(conflicts) > 0 {
-					pr.ConflictedFiles = make([]string, 0, len(conflicts))
-					for key := range conflicts {
-						pr.ConflictedFiles = append(pr.ConflictedFiles, key)
-					}
+			if len(conflicts) > 0 {
+				pr.ConflictedFiles = make([]string, 0, len(conflicts))
+				for key := range conflicts {
+					pr.ConflictedFiles = append(pr.ConflictedFiles, key)
 				}
+			}
 
-				return nil
-			},
-		})
+			return nil
+		},
+	})
 
 	// 9. Check if the found conflictedfiles is non-zero, "err" could be non-nil, so we should ignore it if we found conflicts.
 	// Note: `"err" could be non-nil` is due that if enable 3-way merge, it doesn't return any error on found conflicts.