alternative to PR "improve code quality" (#21464)

This PR doesn't require new git version, and can be backported easily.

Co-authored-by: 6543 <[email protected]>

Author: wxiaoguang

modules/git/command.go

@@ -95,6 +95,18 @@ func (c *Command) AddArguments(args ...string) *Command {
 	return c
 }
 
+// AddDynamicArguments adds new dynamic argument(s) to the command.
+// If the argument is invalid (it shouldn't happen in real life), it panics to caller
+func (c *Command) AddDynamicArguments(args ...string) *Command {
+	for _, arg := range args {
+		if arg != "" && arg[0] == '-' {
+			panic("invalid argument: " + arg)
+		}
+	}
+	c.args = append(c.args, args...)
+	return c
+}
+
 // RunOpts represents parameters to run the command. If UseContextTimeout is specified, then Timeout is ignored.
 type RunOpts struct {
 	Env               []string

modules/git/command_test.go

@@ -26,4 +26,21 @@ func TestRunWithContextStd(t *testing.T) {
 		assert.Contains(t, err.Error(), "exit status 129 - unknown option:")
 		assert.Empty(t, stdout)
 	}
+
+	assert.Panics(t, func() {
+		cmd = NewCommand(context.Background())
+		cmd.AddDynamicArguments("-test")
+	})
+
+	assert.Panics(t, func() {
+		cmd = NewCommand(context.Background())
+		cmd.AddDynamicArguments("--test")
+	})
+
+	subCmd := "version"
+	cmd = NewCommand(context.Background()).AddDynamicArguments(subCmd) // for test purpose only, the sub-command should never be dynamic for production
+	stdout, stderr, err = cmd.RunStdString(&RunOpts{})
+	assert.NoError(t, err)
+	assert.Empty(t, stderr)
+	assert.Contains(t, stdout, "git version")
 }

modules/git/commit.go

@@ -166,7 +166,7 @@ func AllCommitsCount(ctx context.Context, repoPath string, hidePRRefs bool, file
 // CommitsCountFiles returns number of total commits of until given revision.
 func CommitsCountFiles(ctx context.Context, repoPath string, revision, relpath []string) (int64, error) {
 	cmd := NewCommand(ctx, "rev-list", "--count")
-	cmd.AddArguments(revision...)
+	cmd.AddDynamicArguments(revision...)
 	if len(relpath) > 0 {
 		cmd.AddArguments("--")
 		cmd.AddArguments(relpath...)

modules/git/repo_commit.go

@@ -161,7 +161,7 @@ func (repo *Repository) searchCommits(id SHA1, opts SearchCommitsOptions) ([]*Co
 				// add previous arguments except for --grep and --all
 				hashCmd.AddArguments(args...)
 				// add keyword as <commit>
-				hashCmd.AddArguments(v)
+				hashCmd.AddDynamicArguments(v)
 
 				// search with given constraints for commit matching sha hash of v
 				hashMatching, _, err := hashCmd.RunStdBytes(&RunOpts{Dir: repo.Path})
@@ -211,14 +211,17 @@ func (repo *Repository) CommitsByFileAndRange(revision, file string, page int) (
 	}()
 	go func() {
 		stderr := strings.Builder{}
-		err := NewCommand(repo.Ctx, "rev-list", revision,
+		gitCmd := NewCommand(repo.Ctx, "rev-list",
 			"--max-count="+strconv.Itoa(setting.Git.CommitsRangeSize*page),
-			"--skip="+strconv.Itoa(skip), "--", file).
-			Run(&RunOpts{
-				Dir:    repo.Path,
-				Stdout: stdoutWriter,
-				Stderr: &stderr,
-			})
+			"--skip="+strconv.Itoa(skip),
+		)
+		gitCmd.AddDynamicArguments(revision)
+		gitCmd.AddArguments("--", file)
+		err := gitCmd.Run(&RunOpts{
+			Dir:    repo.Path,
+			Stdout: stdoutWriter,
+			Stderr: &stderr,
+		})
 		if err != nil {
 			_ = stdoutWriter.CloseWithError(ConcatenateError(err, (&stderr).String()))
 		} else {

modules/git/repo_stats.go

@@ -61,15 +61,15 @@ func (repo *Repository) GetCodeActivityStats(fromTime time.Time, branch string)
 		_ = stdoutWriter.Close()
 	}()
 
-	args := []string{"log", "--numstat", "--no-merges", "--pretty=format:---%n%h%n%aN%n%aE%n", "--date=iso", fmt.Sprintf("--since='%s'", since)}
+	gitCmd := NewCommand(repo.Ctx, "log", "--numstat", "--no-merges", "--pretty=format:---%n%h%n%aN%n%aE%n", "--date=iso", fmt.Sprintf("--since='%s'", since))
 	if len(branch) == 0 {
-		args = append(args, "--branches=*")
+		gitCmd.AddArguments("--branches=*")
 	} else {
-		args = append(args, "--first-parent", branch)
+		gitCmd.AddArguments("--first-parent").AddDynamicArguments(branch)
 	}
 
 	stderr := new(strings.Builder)
-	err = NewCommand(repo.Ctx, args...).Run(&RunOpts{
+	err = gitCmd.Run(&RunOpts{
 		Env:    []string{},
 		Dir:    repo.Path,
 		Stdout: stdoutWriter,

modules/gitgraph/graph.go

@@ -24,35 +24,30 @@ func GetCommitGraph(r *git.Repository, page, maxAllowedColors int, hidePRRefs bo
 		page = 1
 	}
 
-	args := make([]string, 0, 12+len(branches)+len(files))
-
-	args = append(args, "--graph", "--date-order", "--decorate=full")
+	graphCmd := git.NewCommand(r.Ctx, "log", "--graph", "--date-order", "--decorate=full")
 
 	if hidePRRefs {
-		args = append(args, "--exclude="+git.PullPrefix+"*")
+		graphCmd.AddArguments("--exclude=" + git.PullPrefix + "*")
 	}
 
 	if len(branches) == 0 {
-		args = append(args, "--all")
+		graphCmd.AddArguments("--all")
 	}
 
-	args = append(args,
+	graphCmd.AddArguments(
 		"-C",
 		"-M",
 		fmt.Sprintf("-n %d", setting.UI.GraphMaxCommitNum*page),
 		"--date=iso",
 		fmt.Sprintf("--pretty=format:%s", format))
 
 	if len(branches) > 0 {
-		args = append(args, branches...)
+		graphCmd.AddDynamicArguments(branches...)
 	}
-	args = append(args, "--")
 	if len(files) > 0 {
-		args = append(args, files...)
+		graphCmd.AddArguments("--")
+		graphCmd.AddArguments(files...)
 	}
-
-	graphCmd := git.NewCommand(r.Ctx, "log")
-	graphCmd.AddArguments(args...)
 	graph := NewGraph()
 
 	stderr := new(strings.Builder)