From d98c5db58fdeded983bf5c0fe781fd7b77a1235f Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Sat, 15 Oct 2022 18:49:26 +0800 Subject: [PATCH] alternative to PR "improve code quality" (#21464) This PR doesn't require new git version, and can be backported easily. Co-authored-by: 6543 <6543@obermui.de> --- modules/git/command.go | 12 ++++++++++++ modules/git/command_test.go | 17 +++++++++++++++++ modules/git/commit.go | 2 +- modules/git/repo_commit.go | 19 +++++++++++-------- modules/git/repo_stats.go | 8 ++++---- modules/gitgraph/graph.go | 19 +++++++------------ 6 files changed, 52 insertions(+), 25 deletions(-) diff --git a/modules/git/command.go b/modules/git/command.go index b24d32dbe..8ebd8898f 100644 --- a/modules/git/command.go +++ b/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 diff --git a/modules/git/command_test.go b/modules/git/command_test.go index 67d4ca388..c965ea230 100644 --- a/modules/git/command_test.go +++ b/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") } diff --git a/modules/git/commit.go b/modules/git/commit.go index 32589f534..8fac9921b 100644 --- a/modules/git/commit.go +++ b/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...) diff --git a/modules/git/repo_commit.go b/modules/git/repo_commit.go index d3731cb92..78e037511 100644 --- a/modules/git/repo_commit.go +++ b/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 - 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 { diff --git a/modules/git/repo_stats.go b/modules/git/repo_stats.go index cb44c58ce..7eccf9a19 100644 --- a/modules/git/repo_stats.go +++ b/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, diff --git a/modules/gitgraph/graph.go b/modules/gitgraph/graph.go index 271382525..0f3c02134 100644 --- a/modules/gitgraph/graph.go +++ b/modules/gitgraph/graph.go @@ -24,19 +24,17 @@ 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), @@ -44,15 +42,12 @@ func GetCommitGraph(r *git.Repository, page, maxAllowedColors int, hidePRRefs bo 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)