Skip to content

Commit a2a50f2

Browse files
wxiaoguangwxiaoguang
committed
refactor git command arguments
1 parent e828564 commit a2a50f2

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

71 files changed

+423
-390
lines changed

Diff for: ‎models/migrations/v128.go

+7-6
Original file line numberDiff line numberDiff line change
@@ -83,17 +83,17 @@ func fixMergeBase(x *xorm.Engine) error {
8383

8484
if !pr.HasMerged {
8585
var err error
86-
pr.MergeBase, _, err = git.NewCommand(git.DefaultContext, "merge-base", "--", pr.BaseBranch, gitRefName).RunStdString(&git.RunOpts{Dir: repoPath})
86+
pr.MergeBase, _, err = git.NewCommand(git.DefaultContext, "merge-base").AddSlashedArguments(pr.BaseBranch, gitRefName).RunStdString(&git.RunOpts{Dir: repoPath})
8787
if err != nil {
8888
var err2 error
89-
pr.MergeBase, _, err2 = git.NewCommand(git.DefaultContext, "rev-parse", git.BranchPrefix+pr.BaseBranch).RunStdString(&git.RunOpts{Dir: repoPath})
89+
pr.MergeBase, _, err2 = git.NewCommand(git.DefaultContext, "rev-parse").AddDynamicArguments(git.BranchPrefix + pr.BaseBranch).RunStdString(&git.RunOpts{Dir: repoPath})
9090
if err2 != nil {
9191
log.Error("Unable to get merge base for PR ID %d, Index %d in %s/%s. Error: %v & %v", pr.ID, pr.Index, baseRepo.OwnerName, baseRepo.Name, err, err2)
9292
continue
9393
}
9494
}
9595
} else {
96-
parentsString, _, err := git.NewCommand(git.DefaultContext, "rev-list", "--parents", "-n", "1", pr.MergedCommitID).RunStdString(&git.RunOpts{Dir: repoPath})
96+
parentsString, _, err := git.NewCommand(git.DefaultContext, "rev-list", "--parents", "-n", "1").AddDynamicArguments(pr.MergedCommitID).RunStdString(&git.RunOpts{Dir: repoPath})
9797
if err != nil {
9898
log.Error("Unable to get parents for merged PR ID %d, Index %d in %s/%s. Error: %v", pr.ID, pr.Index, baseRepo.OwnerName, baseRepo.Name, err)
9999
continue
@@ -103,10 +103,11 @@ func fixMergeBase(x *xorm.Engine) error {
103103
continue
104104
}
105105

106-
args := append([]string{"merge-base", "--"}, parents[1:]...)
107-
args = append(args, gitRefName)
106+
refs := append([]string{}, parents[1:]...)
107+
refs = append(refs, gitRefName)
108+
cmd := git.NewCommand(git.DefaultContext, "merge-base").AddSlashedArguments(refs...)
108109

109-
pr.MergeBase, _, err = git.NewCommand(git.DefaultContext, args...).RunStdString(&git.RunOpts{Dir: repoPath})
110+
pr.MergeBase, _, err = cmd.RunStdString(&git.RunOpts{Dir: repoPath})
110111
if err != nil {
111112
log.Error("Unable to get merge base for merged PR ID %d, Index %d in %s/%s. Error: %v", pr.ID, pr.Index, baseRepo.OwnerName, baseRepo.Name, err)
112113
continue

Diff for: ‎models/migrations/v134.go

+5-4
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,7 @@ func refixMergeBase(x *xorm.Engine) error {
8080

8181
gitRefName := fmt.Sprintf("refs/pull/%d/head", pr.Index)
8282

83-
parentsString, _, err := git.NewCommand(git.DefaultContext, "rev-list", "--parents", "-n", "1", pr.MergedCommitID).RunStdString(&git.RunOpts{Dir: repoPath})
83+
parentsString, _, err := git.NewCommand(git.DefaultContext, "rev-list", "--parents", "-n", "1").AddDynamicArguments(pr.MergedCommitID).RunStdString(&git.RunOpts{Dir: repoPath})
8484
if err != nil {
8585
log.Error("Unable to get parents for merged PR ID %d, Index %d in %s/%s. Error: %v", pr.ID, pr.Index, baseRepo.OwnerName, baseRepo.Name, err)
8686
continue
@@ -91,10 +91,11 @@ func refixMergeBase(x *xorm.Engine) error {
9191
}
9292

9393
// we should recalculate
94-
args := append([]string{"merge-base", "--"}, parents[1:]...)
95-
args = append(args, gitRefName)
94+
refs := append([]string{}, parents[1:]...)
95+
refs = append(refs, gitRefName)
96+
cmd := git.NewCommand(git.DefaultContext, "merge-base").AddSlashedArguments(refs...)
9697

97-
pr.MergeBase, _, err = git.NewCommand(git.DefaultContext, args...).RunStdString(&git.RunOpts{Dir: repoPath})
98+
pr.MergeBase, _, err = cmd.RunStdString(&git.RunOpts{Dir: repoPath})
9899
if err != nil {
99100
log.Error("Unable to get merge base for merged PR ID %d, Index %d in %s/%s. Error: %v", pr.ID, pr.Index, baseRepo.OwnerName, baseRepo.Name, err)
100101
continue

Diff for: ‎modules/doctor/heads.go

+2-2
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ func synchronizeRepoHeads(ctx context.Context, logger log.Logger, autofix bool)
2121
numRepos++
2222
runOpts := &git.RunOpts{Dir: repo.RepoPath()}
2323

24-
_, _, defaultBranchErr := git.NewCommand(ctx, "rev-parse", "--", repo.DefaultBranch).RunStdString(runOpts)
24+
_, _, defaultBranchErr := git.NewCommand(ctx, "rev-parse").AddSlashedArguments(repo.DefaultBranch).RunStdString(runOpts)
2525

2626
head, _, headErr := git.NewCommand(ctx, "symbolic-ref", "--short", "HEAD").RunStdString(runOpts)
2727

@@ -49,7 +49,7 @@ func synchronizeRepoHeads(ctx context.Context, logger log.Logger, autofix bool)
4949
}
5050

5151
// otherwise, let's try fixing HEAD
52-
err := git.NewCommand(ctx, "symbolic-ref", "--", "HEAD", repo.DefaultBranch).Run(runOpts)
52+
err := git.NewCommand(ctx, "symbolic-ref").AddSlashedArguments("HEAD", repo.DefaultBranch).Run(runOpts)
5353
if err != nil {
5454
logger.Warn("Failed to fix HEAD for %s/%s: %v", repo.OwnerName, repo.Name, err)
5555
return nil

Diff for: ‎modules/doctor/mergebase.go

+7-7
Original file line numberDiff line numberDiff line change
@@ -44,17 +44,17 @@ func checkPRMergeBase(ctx context.Context, logger log.Logger, autofix bool) erro
4444

4545
if !pr.HasMerged {
4646
var err error
47-
pr.MergeBase, _, err = git.NewCommand(ctx, "merge-base", "--", pr.BaseBranch, pr.GetGitRefName()).RunStdString(&git.RunOpts{Dir: repoPath})
47+
pr.MergeBase, _, err = git.NewCommand(ctx, "merge-base").AddSlashedArguments(pr.BaseBranch, pr.GetGitRefName()).RunStdString(&git.RunOpts{Dir: repoPath})
4848
if err != nil {
4949
var err2 error
50-
pr.MergeBase, _, err2 = git.NewCommand(ctx, "rev-parse", git.BranchPrefix+pr.BaseBranch).RunStdString(&git.RunOpts{Dir: repoPath})
50+
pr.MergeBase, _, err2 = git.NewCommand(ctx, "rev-parse").AddDynamicArguments(git.BranchPrefix + pr.BaseBranch).RunStdString(&git.RunOpts{Dir: repoPath})
5151
if err2 != nil {
5252
logger.Warn("Unable to get merge base for PR ID %d, #%d onto %s in %s/%s. Error: %v & %v", pr.ID, pr.Index, pr.BaseBranch, pr.BaseRepo.OwnerName, pr.BaseRepo.Name, err, err2)
5353
return nil
5454
}
5555
}
5656
} else {
57-
parentsString, _, err := git.NewCommand(ctx, "rev-list", "--parents", "-n", "1", pr.MergedCommitID).RunStdString(&git.RunOpts{Dir: repoPath})
57+
parentsString, _, err := git.NewCommand(ctx, "rev-list", "--parents", "-n", "1").AddDynamicArguments(pr.MergedCommitID).RunStdString(&git.RunOpts{Dir: repoPath})
5858
if err != nil {
5959
logger.Warn("Unable to get parents for merged PR ID %d, #%d onto %s in %s/%s. Error: %v", pr.ID, pr.Index, pr.BaseBranch, pr.BaseRepo.OwnerName, pr.BaseRepo.Name, err)
6060
return nil
@@ -64,10 +64,10 @@ func checkPRMergeBase(ctx context.Context, logger log.Logger, autofix bool) erro
6464
return nil
6565
}
6666

67-
args := append([]string{"merge-base", "--"}, parents[1:]...)
68-
args = append(args, pr.GetGitRefName())
69-
70-
pr.MergeBase, _, err = git.NewCommand(ctx, args...).RunStdString(&git.RunOpts{Dir: repoPath})
67+
refs := append([]string{}, parents[1:]...)
68+
refs = append(refs, pr.GetGitRefName())
69+
cmd := git.NewCommand(ctx, "merge-base").AddSlashedArguments(refs...)
70+
pr.MergeBase, _, err = cmd.RunStdString(&git.RunOpts{Dir: repoPath})
7171
if err != nil {
7272
logger.Warn("Unable to get merge base for merged PR ID %d, #%d onto %s in %s/%s. Error: %v", pr.ID, pr.Index, pr.BaseBranch, pr.BaseRepo.OwnerName, pr.BaseRepo.Name, err)
7373
return nil

Diff for: ‎modules/git/command.go

+47-15
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ import (
2424

2525
var (
2626
// globalCommandArgs global command args for external package setting
27-
globalCommandArgs []string
27+
globalCommandArgs []CmdArg
2828

2929
// defaultCommandExecutionTimeout default command execution timeout duration
3030
defaultCommandExecutionTimeout = 360 * time.Second
@@ -43,6 +43,8 @@ type Command struct {
4343
brokenArgs []string
4444
}
4545

46+
type CmdArg string
47+
4648
func (c *Command) String() string {
4749
if len(c.args) == 0 {
4850
return c.name
@@ -52,30 +54,39 @@ func (c *Command) String() string {
5254

5355
// NewCommand creates and returns a new Git Command based on given command and arguments.
5456
// Each argument should be safe to be trusted. User-provided arguments should be passed to AddDynamicArguments instead.
55-
func NewCommand(ctx context.Context, args ...string) *Command {
57+
func NewCommand(ctx context.Context, args ...CmdArg) *Command {
5658
// Make an explicit copy of globalCommandArgs, otherwise append might overwrite it
57-
cargs := make([]string, len(globalCommandArgs))
58-
copy(cargs, globalCommandArgs)
59+
cargs := make([]string, 0, len(globalCommandArgs)+len(args))
60+
for _, arg := range globalCommandArgs {
61+
cargs = append(cargs, string(arg))
62+
}
63+
for _, arg := range args {
64+
cargs = append(cargs, string(arg))
65+
}
5966
return &Command{
6067
name: GitExecutable,
61-
args: append(cargs, args...),
68+
args: cargs,
6269
parentContext: ctx,
6370
globalArgsLength: len(globalCommandArgs),
6471
}
6572
}
6673

6774
// NewCommandNoGlobals creates and returns a new Git Command based on given command and arguments only with the specify args and don't care global command args
6875
// Each argument should be safe to be trusted. User-provided arguments should be passed to AddDynamicArguments instead.
69-
func NewCommandNoGlobals(args ...string) *Command {
76+
func NewCommandNoGlobals(args ...CmdArg) *Command {
7077
return NewCommandContextNoGlobals(DefaultContext, args...)
7178
}
7279

7380
// NewCommandContextNoGlobals creates and returns a new Git Command based on given command and arguments only with the specify args and don't care global command args
7481
// Each argument should be safe to be trusted. User-provided arguments should be passed to AddDynamicArguments instead.
75-
func NewCommandContextNoGlobals(ctx context.Context, args ...string) *Command {
82+
func NewCommandContextNoGlobals(ctx context.Context, args ...CmdArg) *Command {
83+
cargs := make([]string, 0, len(args))
84+
for _, arg := range args {
85+
cargs = append(cargs, string(arg))
86+
}
7687
return &Command{
7788
name: GitExecutable,
78-
args: args,
89+
args: cargs,
7990
parentContext: ctx,
8091
}
8192
}
@@ -93,10 +104,12 @@ func (c *Command) SetDescription(desc string) *Command {
93104
return c
94105
}
95106

96-
// AddArguments adds new argument(s) to the command. Each argument must be safe to be trusted.
107+
// AddArguments adds new git argument(s) to the command. Each argument must be safe to be trusted.
97108
// User-provided arguments should be passed to AddDynamicArguments instead.
98-
func (c *Command) AddArguments(args ...string) *Command {
99-
c.args = append(c.args, args...)
109+
func (c *Command) AddArguments(args ...CmdArg) *Command {
110+
for _, arg := range args {
111+
c.args = append(c.args, string(arg))
112+
}
100113
return c
101114
}
102115

@@ -115,6 +128,25 @@ func (c *Command) AddDynamicArguments(args ...string) *Command {
115128
return c
116129
}
117130

131+
// AddSlashedArguments adds the "--" and then add the arguments
132+
func (c *Command) AddSlashedArguments(list ...string) *Command {
133+
c.args = append(c.args, "--")
134+
// Some old code also checks `arg != ""`, IMO it's not necessary.
135+
// If the check is needed, the list should be prepared before the call to this function
136+
c.args = append(c.args, list...)
137+
return c
138+
}
139+
140+
// CmdArgCheck checks whether the string is safe to be used as a dynamic argument.
141+
// It panics if the check fails. Usually it should not be used, it's just for refactoring purpose
142+
// deprecated
143+
func CmdArgCheck(s string) CmdArg {
144+
if s != "" && s[0] == '-' {
145+
panic("invalid git cmd argument: " + s)
146+
}
147+
return CmdArg(s)
148+
}
149+
118150
// RunOpts represents parameters to run the command. If UseContextTimeout is specified, then Timeout is ignored.
119151
type RunOpts struct {
120152
Env []string
@@ -153,7 +185,7 @@ func CommonGitCmdEnvs() []string {
153185
}...)
154186
}
155187

156-
// CommonCmdServEnvs is like CommonGitCmdEnvs but it only returns minimal required environment variables for the "gitea serv" command
188+
// CommonCmdServEnvs is like CommonGitCmdEnvs, but it only returns minimal required environment variables for the "gitea serv" command
157189
func CommonCmdServEnvs() []string {
158190
return commonBaseEnvs()
159191
}
@@ -318,12 +350,12 @@ func (c *Command) RunStdBytes(opts *RunOpts) (stdout, stderr []byte, runErr RunS
318350
}
319351

320352
// AllowLFSFiltersArgs return globalCommandArgs with lfs filter, it should only be used for tests
321-
func AllowLFSFiltersArgs() []string {
353+
func AllowLFSFiltersArgs() []CmdArg {
322354
// Now here we should explicitly allow lfs filters to run
323-
filteredLFSGlobalArgs := make([]string, len(globalCommandArgs))
355+
filteredLFSGlobalArgs := make([]CmdArg, len(globalCommandArgs))
324356
j := 0
325357
for _, arg := range globalCommandArgs {
326-
if strings.Contains(arg, "lfs") {
358+
if strings.Contains(string(arg), "lfs") {
327359
j--
328360
} else {
329361
filteredLFSGlobalArgs[j] = arg

Diff for: ‎modules/git/commit.go

+21-29
Original file line numberDiff line numberDiff line change
@@ -92,13 +92,13 @@ func AddChanges(repoPath string, all bool, files ...string) error {
9292
}
9393

9494
// AddChangesWithArgs marks local changes to be ready for commit.
95-
func AddChangesWithArgs(repoPath string, globalArgs []string, all bool, files ...string) error {
95+
func AddChangesWithArgs(repoPath string, globalArgs []CmdArg, all bool, files ...string) error {
9696
cmd := NewCommandNoGlobals(append(globalArgs, "add")...)
9797
if all {
9898
cmd.AddArguments("--all")
9999
}
100-
cmd.AddArguments("--")
101-
_, _, err := cmd.AddArguments(files...).RunStdString(&RunOpts{Dir: repoPath})
100+
cmd.AddSlashedArguments(files...)
101+
_, _, err := cmd.RunStdString(&RunOpts{Dir: repoPath})
102102
return err
103103
}
104104

@@ -112,27 +112,27 @@ type CommitChangesOptions struct {
112112
// CommitChanges commits local changes with given committer, author and message.
113113
// If author is nil, it will be the same as committer.
114114
func CommitChanges(repoPath string, opts CommitChangesOptions) error {
115-
cargs := make([]string, len(globalCommandArgs))
115+
cargs := make([]CmdArg, len(globalCommandArgs))
116116
copy(cargs, globalCommandArgs)
117117
return CommitChangesWithArgs(repoPath, cargs, opts)
118118
}
119119

120120
// CommitChangesWithArgs commits local changes with given committer, author and message.
121121
// If author is nil, it will be the same as committer.
122-
func CommitChangesWithArgs(repoPath string, args []string, opts CommitChangesOptions) error {
122+
func CommitChangesWithArgs(repoPath string, args []CmdArg, opts CommitChangesOptions) error {
123123
cmd := NewCommandNoGlobals(args...)
124124
if opts.Committer != nil {
125-
cmd.AddArguments("-c", "user.name="+opts.Committer.Name, "-c", "user.email="+opts.Committer.Email)
125+
cmd.AddArguments("-c", CmdArg("user.name="+opts.Committer.Name), "-c", CmdArg("user.email="+opts.Committer.Email))
126126
}
127127
cmd.AddArguments("commit")
128128

129129
if opts.Author == nil {
130130
opts.Author = opts.Committer
131131
}
132132
if opts.Author != nil {
133-
cmd.AddArguments(fmt.Sprintf("--author='%s <%s>'", opts.Author.Name, opts.Author.Email))
133+
cmd.AddArguments(CmdArg(fmt.Sprintf("--author='%s <%s>'", opts.Author.Name, opts.Author.Email)))
134134
}
135-
cmd.AddArguments("-m", opts.Message)
135+
cmd.AddArguments("-m").AddDynamicArguments(opts.Message)
136136

137137
_, _, err := cmd.RunStdString(&RunOpts{Dir: repoPath})
138138
// No stderr but exit status 1 means nothing to commit.
@@ -144,15 +144,13 @@ func CommitChangesWithArgs(repoPath string, args []string, opts CommitChangesOpt
144144

145145
// AllCommitsCount returns count of all commits in repository
146146
func AllCommitsCount(ctx context.Context, repoPath string, hidePRRefs bool, files ...string) (int64, error) {
147-
args := []string{"--all", "--count"}
147+
cmd := NewCommand(ctx, "rev-list")
148148
if hidePRRefs {
149-
args = append([]string{"--exclude=" + PullPrefix + "*"}, args...)
149+
cmd.AddArguments("--exclude=" + PullPrefix + "*")
150150
}
151-
cmd := NewCommand(ctx, "rev-list")
152-
cmd.AddArguments(args...)
151+
cmd.AddArguments("--all", "--count")
153152
if len(files) > 0 {
154-
cmd.AddArguments("--")
155-
cmd.AddArguments(files...)
153+
cmd.AddSlashedArguments(files...)
156154
}
157155

158156
stdout, _, err := cmd.RunStdString(&RunOpts{Dir: repoPath})
@@ -168,8 +166,7 @@ func CommitsCountFiles(ctx context.Context, repoPath string, revision, relpath [
168166
cmd := NewCommand(ctx, "rev-list", "--count")
169167
cmd.AddDynamicArguments(revision...)
170168
if len(relpath) > 0 {
171-
cmd.AddArguments("--")
172-
cmd.AddArguments(relpath...)
169+
cmd.AddSlashedArguments(relpath...)
173170
}
174171

175172
stdout, _, err := cmd.RunStdString(&RunOpts{Dir: repoPath})
@@ -209,7 +206,7 @@ func (c *Commit) HasPreviousCommit(commitHash SHA1) (bool, error) {
209206
return false, nil
210207
}
211208

212-
_, _, err := NewCommand(c.repo.Ctx, "merge-base", "--is-ancestor", that, this).RunStdString(&RunOpts{Dir: c.repo.Path})
209+
_, _, err := NewCommand(c.repo.Ctx, "merge-base", "--is-ancestor").AddDynamicArguments(that, this).RunStdString(&RunOpts{Dir: c.repo.Path})
213210
if err == nil {
214211
return true, nil
215212
}
@@ -392,15 +389,12 @@ func (c *Commit) GetSubModule(entryname string) (*SubModule, error) {
392389

393390
// GetBranchName gets the closest branch name (as returned by 'git name-rev --name-only')
394391
func (c *Commit) GetBranchName() (string, error) {
395-
args := []string{
396-
"name-rev",
397-
}
392+
cmd := NewCommand(c.repo.Ctx, "name-rev")
398393
if CheckGitVersionAtLeast("2.13.0") == nil {
399-
args = append(args, "--exclude", "refs/tags/*")
394+
cmd.AddArguments("--exclude", "refs/tags/*")
400395
}
401-
args = append(args, "--name-only", "--no-undefined", c.ID.String())
402-
403-
data, _, err := NewCommand(c.repo.Ctx, args...).RunStdString(&RunOpts{Dir: c.repo.Path})
396+
cmd.AddArguments("--name-only", "--no-undefined").AddDynamicArguments(c.ID.String())
397+
data, _, err := cmd.RunStdString(&RunOpts{Dir: c.repo.Path})
404398
if err != nil {
405399
// handle special case where git can not describe commit
406400
if strings.Contains(err.Error(), "cannot describe") {
@@ -426,7 +420,7 @@ func (c *Commit) LoadBranchName() (err error) {
426420

427421
// GetTagName gets the current tag name for given commit
428422
func (c *Commit) GetTagName() (string, error) {
429-
data, _, err := NewCommand(c.repo.Ctx, "describe", "--exact-match", "--tags", "--always", c.ID.String()).RunStdString(&RunOpts{Dir: c.repo.Path})
423+
data, _, err := NewCommand(c.repo.Ctx, "describe", "--exact-match", "--tags", "--always").AddDynamicArguments(c.ID.String()).RunStdString(&RunOpts{Dir: c.repo.Path})
430424
if err != nil {
431425
// handle special case where there is no tag for this commit
432426
if strings.Contains(err.Error(), "no tag exactly matches") {
@@ -503,9 +497,7 @@ func GetCommitFileStatus(ctx context.Context, repoPath, commitID string) (*Commi
503497
}()
504498

505499
stderr := new(bytes.Buffer)
506-
args := []string{"log", "--name-status", "-c", "--pretty=format:", "--parents", "--no-renames", "-z", "-1", commitID}
507-
508-
err := NewCommand(ctx, args...).Run(&RunOpts{
500+
err := NewCommand(ctx, "log", "--name-status", "-c", "--pretty=format:", "--parents", "--no-renames", "-z", "-1").AddDynamicArguments(commitID).Run(&RunOpts{
509501
Dir: repoPath,
510502
Stdout: w,
511503
Stderr: stderr,
@@ -521,7 +513,7 @@ func GetCommitFileStatus(ctx context.Context, repoPath, commitID string) (*Commi
521513

522514
// GetFullCommitID returns full length (40) of commit ID by given short SHA in a repository.
523515
func GetFullCommitID(ctx context.Context, repoPath, shortID string) (string, error) {
524-
commitID, _, err := NewCommand(ctx, "rev-parse", shortID).RunStdString(&RunOpts{Dir: repoPath})
516+
commitID, _, err := NewCommand(ctx, "rev-parse").AddDynamicArguments(shortID).RunStdString(&RunOpts{Dir: repoPath})
525517
if err != nil {
526518
if strings.Contains(err.Error(), "exit status 128") {
527519
return "", ErrNotExist{shortID, ""}

0 commit comments

Comments
 (0)