Refactor git command arguments and make all arguments to be safe to be used (#21535)

Follow #21464

Make all git command arguments strictly safe. Most changes are one-to-one replacing, keep all existing logic.

1 files changed, 45 insertions, 31 deletions

diff --git a/modules/git/repo_commit.go b/modules/git/repo_commit.go
index ec72593b80..90259fd746 100644
--- a/modules/git/repo_commit.go
+++ b/modules/git/repo_commit.go
@@ -61,7 +61,7 @@ func (repo *Repository) getCommitByPathWithID(id SHA1, relpath string) (*Commit,
 		relpath = `\` + relpath
 	}
 
-	stdout, _, runErr := NewCommand(repo.Ctx, "log", "-1", prettyLogFormat, id.String(), "--", relpath).RunStdString(&RunOpts{Dir: repo.Path})
+	stdout, _, runErr := NewCommand(repo.Ctx, "log", "-1", prettyLogFormat).AddDynamicArguments(id.String()).AddDashesAndList(relpath).RunStdString(&RunOpts{Dir: repo.Path})
 	if runErr != nil {
 		return nil, runErr
 	}
@@ -76,7 +76,7 @@ func (repo *Repository) getCommitByPathWithID(id SHA1, relpath string) (*Commit,
 
 // GetCommitByPath returns the last commit of relative path.
 func (repo *Repository) GetCommitByPath(relpath string) (*Commit, error) {
-	stdout, _, runErr := NewCommand(repo.Ctx, "log", "-1", prettyLogFormat, "--", relpath).RunStdBytes(&RunOpts{Dir: repo.Path})
+	stdout, _, runErr := NewCommand(repo.Ctx, "log", "-1", prettyLogFormat).AddDashesAndList(relpath).RunStdBytes(&RunOpts{Dir: repo.Path})
 	if runErr != nil {
 		return nil, runErr
 	}
@@ -89,8 +89,10 @@ func (repo *Repository) GetCommitByPath(relpath string) (*Commit, error) {
 }
 
 func (repo *Repository) commitsByRange(id SHA1, page, pageSize int) ([]*Commit, error) {
-	stdout, _, err := NewCommand(repo.Ctx, "log", id.String(), "--skip="+strconv.Itoa((page-1)*pageSize),
-		"--max-count="+strconv.Itoa(pageSize), prettyLogFormat).RunStdBytes(&RunOpts{Dir: repo.Path})
+	stdout, _, err := NewCommand(repo.Ctx, "log").
+		AddArguments(CmdArg("--skip="+strconv.Itoa((page-1)*pageSize)), CmdArg("--max-count="+strconv.Itoa(pageSize)), prettyLogFormat).
+		AddDynamicArguments(id.String()).
+		RunStdBytes(&RunOpts{Dir: repo.Path})
 	if err != nil {
 		return nil, err
 	}
@@ -99,30 +101,30 @@ func (repo *Repository) commitsByRange(id SHA1, page, pageSize int) ([]*Commit,
 
 func (repo *Repository) searchCommits(id SHA1, opts SearchCommitsOptions) ([]*Commit, error) {
 	// create new git log command with limit of 100 commis
-	cmd := NewCommand(repo.Ctx, "log", id.String(), "-100", prettyLogFormat)
+	cmd := NewCommand(repo.Ctx, "log", "-100", prettyLogFormat).AddDynamicArguments(id.String())
 	// ignore case
-	args := []string{"-i"}
+	args := []CmdArg{"-i"}
 
 	// add authors if present in search query
 	if len(opts.Authors) > 0 {
 		for _, v := range opts.Authors {
-			args = append(args, "--author="+v)
+			args = append(args, CmdArg("--author="+v))
 		}
 	}
 
 	// add committers if present in search query
 	if len(opts.Committers) > 0 {
 		for _, v := range opts.Committers {
-			args = append(args, "--committer="+v)
+			args = append(args, CmdArg("--committer="+v))
 		}
 	}
 
 	// add time constraints if present in search query
 	if len(opts.After) > 0 {
-		args = append(args, "--after="+opts.After)
+		args = append(args, CmdArg("--after="+opts.After))
 	}
 	if len(opts.Before) > 0 {
-		args = append(args, "--before="+opts.Before)
+		args = append(args, CmdArg("--before="+opts.Before))
 	}
 
 	// pretend that all refs along with HEAD were listed on command line as <commis>
@@ -136,7 +138,7 @@ func (repo *Repository) searchCommits(id SHA1, opts SearchCommitsOptions) ([]*Co
 	// note this is done only for command created above
 	if len(opts.Keywords) > 0 {
 		for _, v := range opts.Keywords {
-			cmd.AddArguments("--grep=" + v)
+			cmd.AddArguments(CmdArg("--grep=" + v))
 		}
 	}
 
@@ -178,7 +180,7 @@ func (repo *Repository) searchCommits(id SHA1, opts SearchCommitsOptions) ([]*Co
 }
 
 func (repo *Repository) getFilesChanged(id1, id2 string) ([]string, error) {
-	stdout, _, err := NewCommand(repo.Ctx, "diff", "--name-only", id1, id2).RunStdBytes(&RunOpts{Dir: repo.Path})
+	stdout, _, err := NewCommand(repo.Ctx, "diff", "--name-only").AddDynamicArguments(id1, id2).RunStdBytes(&RunOpts{Dir: repo.Path})
 	if err != nil {
 		return nil, err
 	}
@@ -188,7 +190,7 @@ func (repo *Repository) getFilesChanged(id1, id2 string) ([]string, error) {
 // FileChangedBetweenCommits Returns true if the file changed between commit IDs id1 and id2
 // You must ensure that id1 and id2 are valid commit ids.
 func (repo *Repository) FileChangedBetweenCommits(filename, id1, id2 string) (bool, error) {
-	stdout, _, err := NewCommand(repo.Ctx, "diff", "--name-only", "-z", id1, id2, "--", filename).RunStdBytes(&RunOpts{Dir: repo.Path})
+	stdout, _, err := NewCommand(repo.Ctx, "diff", "--name-only", "-z").AddDynamicArguments(id1, id2).AddDashesAndList(filename).RunStdBytes(&RunOpts{Dir: repo.Path})
 	if err != nil {
 		return false, err
 	}
@@ -211,12 +213,11 @@ func (repo *Repository) CommitsByFileAndRange(revision, file string, page int) (
 	}()
 	go func() {
 		stderr := strings.Builder{}
-		gitCmd := NewCommand(repo.Ctx, "rev-list",
-			"--max-count="+strconv.Itoa(setting.Git.CommitsRangeSize*page),
-			"--skip="+strconv.Itoa(skip),
-		)
+		gitCmd := NewCommand(repo.Ctx, "rev-list").
+			AddArguments(CmdArg("--max-count=" + strconv.Itoa(setting.Git.CommitsRangeSize*page))).
+			AddArguments(CmdArg("--skip=" + strconv.Itoa(skip)))
 		gitCmd.AddDynamicArguments(revision)
-		gitCmd.AddArguments("--", file)
+		gitCmd.AddDashesAndList(file)
 		err := gitCmd.Run(&RunOpts{
 			Dir:    repo.Path,
 			Stdout: stdoutWriter,
@@ -257,11 +258,11 @@ func (repo *Repository) CommitsByFileAndRange(revision, file string, page int) (
 
 // FilesCountBetween return the number of files changed between two commits
 func (repo *Repository) FilesCountBetween(startCommitID, endCommitID string) (int, error) {
-	stdout, _, err := NewCommand(repo.Ctx, "diff", "--name-only", startCommitID+"..."+endCommitID).RunStdString(&RunOpts{Dir: repo.Path})
+	stdout, _, err := NewCommand(repo.Ctx, "diff", "--name-only").AddDynamicArguments(startCommitID + "..." + endCommitID).RunStdString(&RunOpts{Dir: repo.Path})
 	if err != nil && strings.Contains(err.Error(), "no merge base") {
 		// git >= 2.28 now returns an error if startCommitID and endCommitID have become unrelated.
 		// previously it would return the results of git diff --name-only startCommitID endCommitID so let's try that...
-		stdout, _, err = NewCommand(repo.Ctx, "diff", "--name-only", startCommitID, endCommitID).RunStdString(&RunOpts{Dir: repo.Path})
+		stdout, _, err = NewCommand(repo.Ctx, "diff", "--name-only").AddDynamicArguments(startCommitID, endCommitID).RunStdString(&RunOpts{Dir: repo.Path})
 	}
 	if err != nil {
 		return 0, err
@@ -275,13 +276,13 @@ func (repo *Repository) CommitsBetween(last, before *Commit) ([]*Commit, error)
 	var stdout []byte
 	var err error
 	if before == nil {
-		stdout, _, err = NewCommand(repo.Ctx, "rev-list", last.ID.String()).RunStdBytes(&RunOpts{Dir: repo.Path})
+		stdout, _, err = NewCommand(repo.Ctx, "rev-list").AddDynamicArguments(last.ID.String()).RunStdBytes(&RunOpts{Dir: repo.Path})
 	} else {
-		stdout, _, err = NewCommand(repo.Ctx, "rev-list", before.ID.String()+".."+last.ID.String()).RunStdBytes(&RunOpts{Dir: repo.Path})
+		stdout, _, err = NewCommand(repo.Ctx, "rev-list").AddDynamicArguments(before.ID.String() + ".." + last.ID.String()).RunStdBytes(&RunOpts{Dir: repo.Path})
 		if err != nil && strings.Contains(err.Error(), "no merge base") {
 			// future versions of git >= 2.28 are likely to return an error if before and last have become unrelated.
 			// previously it would return the results of git rev-list before last so let's try that...
-			stdout, _, err = NewCommand(repo.Ctx, "rev-list", before.ID.String(), last.ID.String()).RunStdBytes(&RunOpts{Dir: repo.Path})
+			stdout, _, err = NewCommand(repo.Ctx, "rev-list").AddDynamicArguments(before.ID.String(), last.ID.String()).RunStdBytes(&RunOpts{Dir: repo.Path})
 		}
 	}
 	if err != nil {
@@ -295,13 +296,22 @@ func (repo *Repository) CommitsBetweenLimit(last, before *Commit, limit, skip in
 	var stdout []byte
 	var err error
 	if before == nil {
-		stdout, _, err = NewCommand(repo.Ctx, "rev-list", "--max-count", strconv.Itoa(limit), "--skip", strconv.Itoa(skip), last.ID.String()).RunStdBytes(&RunOpts{Dir: repo.Path})
+		stdout, _, err = NewCommand(repo.Ctx, "rev-list",
+			"--max-count", CmdArg(strconv.Itoa(limit)),
+			"--skip", CmdArg(strconv.Itoa(skip))).
+			AddDynamicArguments(last.ID.String()).RunStdBytes(&RunOpts{Dir: repo.Path})
 	} else {
-		stdout, _, err = NewCommand(repo.Ctx, "rev-list", "--max-count", strconv.Itoa(limit), "--skip", strconv.Itoa(skip), before.ID.String()+".."+last.ID.String()).RunStdBytes(&RunOpts{Dir: repo.Path})
+		stdout, _, err = NewCommand(repo.Ctx, "rev-list",
+			"--max-count", CmdArg(strconv.Itoa(limit)),
+			"--skip", CmdArg(strconv.Itoa(skip))).
+			AddDynamicArguments(before.ID.String() + ".." + last.ID.String()).RunStdBytes(&RunOpts{Dir: repo.Path})
 		if err != nil && strings.Contains(err.Error(), "no merge base") {
 			// future versions of git >= 2.28 are likely to return an error if before and last have become unrelated.
 			// previously it would return the results of git rev-list --max-count n before last so let's try that...
-			stdout, _, err = NewCommand(repo.Ctx, "rev-list", "--max-count", strconv.Itoa(limit), "--skip", strconv.Itoa(skip), before.ID.String(), last.ID.String()).RunStdBytes(&RunOpts{Dir: repo.Path})
+			stdout, _, err = NewCommand(repo.Ctx, "rev-list",
+				"--max-count", CmdArg(strconv.Itoa(limit)),
+				"--skip", CmdArg(strconv.Itoa(skip))).
+				AddDynamicArguments(before.ID.String(), last.ID.String()).RunStdBytes(&RunOpts{Dir: repo.Path})
 		}
 	}
 	if err != nil {
@@ -342,9 +352,9 @@ func (repo *Repository) CommitsCountBetween(start, end string) (int64, error) {
 func (repo *Repository) commitsBefore(id SHA1, limit int) ([]*Commit, error) {
 	cmd := NewCommand(repo.Ctx, "log")
 	if limit > 0 {
-		cmd.AddArguments("-"+strconv.Itoa(limit), prettyLogFormat, id.String())
+		cmd.AddArguments(CmdArg("-"+strconv.Itoa(limit)), prettyLogFormat).AddDynamicArguments(id.String())
 	} else {
-		cmd.AddArguments(prettyLogFormat, id.String())
+		cmd.AddArguments(prettyLogFormat).AddDynamicArguments(id.String())
 	}
 
 	stdout, _, runErr := cmd.RunStdBytes(&RunOpts{Dir: repo.Path})
@@ -384,7 +394,11 @@ func (repo *Repository) getCommitsBeforeLimit(id SHA1, num int) ([]*Commit, erro
 
 func (repo *Repository) getBranches(commit *Commit, limit int) ([]string, error) {
 	if CheckGitVersionAtLeast("2.7.0") == nil {
-		stdout, _, err := NewCommand(repo.Ctx, "for-each-ref", "--count="+strconv.Itoa(limit), "--format=%(refname:strip=2)", "--contains", commit.ID.String(), BranchPrefix).RunStdString(&RunOpts{Dir: repo.Path})
+		stdout, _, err := NewCommand(repo.Ctx, "for-each-ref",
+			CmdArg("--count="+strconv.Itoa(limit)),
+			"--format=%(refname:strip=2)", "--contains").
+			AddDynamicArguments(commit.ID.String(), BranchPrefix).
+			RunStdString(&RunOpts{Dir: repo.Path})
 		if err != nil {
 			return nil, err
 		}
@@ -393,7 +407,7 @@ func (repo *Repository) getBranches(commit *Commit, limit int) ([]string, error)
 		return branches, nil
 	}
 
-	stdout, _, err := NewCommand(repo.Ctx, "branch", "--contains", commit.ID.String()).RunStdString(&RunOpts{Dir: repo.Path})
+	stdout, _, err := NewCommand(repo.Ctx, "branch", "--contains").AddDynamicArguments(commit.ID.String()).RunStdString(&RunOpts{Dir: repo.Path})
 	if err != nil {
 		return nil, err
 	}
@@ -432,7 +446,7 @@ func (repo *Repository) GetCommitsFromIDs(commitIDs []string) []*Commit {
 
 // IsCommitInBranch check if the commit is on the branch
 func (repo *Repository) IsCommitInBranch(commitID, branch string) (r bool, err error) {
-	stdout, _, err := NewCommand(repo.Ctx, "branch", "--contains", commitID, branch).RunStdString(&RunOpts{Dir: repo.Path})
+	stdout, _, err := NewCommand(repo.Ctx, "branch", "--contains").AddDynamicArguments(commitID, branch).RunStdString(&RunOpts{Dir: repo.Path})
 	if err != nil {
 		return false, err
 	}