Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 3 commits into from
Oct 23, 2022
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a2a50f2
refactor git command arguments
wxiaoguang Oct 21, 2022
87253d2
AddSlashedArguments -> AddDashesAndList
wxiaoguang Oct 23, 2022
0805423
Merge branch 'main' into refactor-git-args
wxiaoguang Oct 23, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 7 additions & 6 deletions models/migrations/v128.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,17 +83,17 @@ func fixMergeBase(x *xorm.Engine) error {

if !pr.HasMerged {
var err error
pr.MergeBase, _, err = git.NewCommand(git.DefaultContext, "merge-base", "--", pr.BaseBranch, gitRefName).RunStdString(&git.RunOpts{Dir: repoPath})
pr.MergeBase, _, err = git.NewCommand(git.DefaultContext, "merge-base").AddSlashedArguments(pr.BaseBranch, gitRefName).RunStdString(&git.RunOpts{Dir: repoPath})
if err != nil {
var err2 error
pr.MergeBase, _, err2 = git.NewCommand(git.DefaultContext, "rev-parse", git.BranchPrefix+pr.BaseBranch).RunStdString(&git.RunOpts{Dir: repoPath})
pr.MergeBase, _, err2 = git.NewCommand(git.DefaultContext, "rev-parse").AddDynamicArguments(git.BranchPrefix + pr.BaseBranch).RunStdString(&git.RunOpts{Dir: repoPath})
wxiaoguang marked this conversation as resolved.
Show resolved Hide resolved
if err2 != nil {
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)
continue
}
}
} else {
parentsString, _, err := git.NewCommand(git.DefaultContext, "rev-list", "--parents", "-n", "1", pr.MergedCommitID).RunStdString(&git.RunOpts{Dir: repoPath})
parentsString, _, err := git.NewCommand(git.DefaultContext, "rev-list", "--parents", "-n", "1").AddDynamicArguments(pr.MergedCommitID).RunStdString(&git.RunOpts{Dir: repoPath})
if err != nil {
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)
continue
Expand All @@ -103,10 +103,11 @@ func fixMergeBase(x *xorm.Engine) error {
continue
}

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

pr.MergeBase, _, err = git.NewCommand(git.DefaultContext, args...).RunStdString(&git.RunOpts{Dir: repoPath})
pr.MergeBase, _, err = cmd.RunStdString(&git.RunOpts{Dir: repoPath})
if err != nil {
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)
continue
Expand Down
9 changes: 5 additions & 4 deletions models/migrations/v134.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ func refixMergeBase(x *xorm.Engine) error {

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

parentsString, _, err := git.NewCommand(git.DefaultContext, "rev-list", "--parents", "-n", "1", pr.MergedCommitID).RunStdString(&git.RunOpts{Dir: repoPath})
parentsString, _, err := git.NewCommand(git.DefaultContext, "rev-list", "--parents", "-n", "1").AddDynamicArguments(pr.MergedCommitID).RunStdString(&git.RunOpts{Dir: repoPath})
if err != nil {
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)
continue
Expand All @@ -91,10 +91,11 @@ func refixMergeBase(x *xorm.Engine) error {
}

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

pr.MergeBase, _, err = git.NewCommand(git.DefaultContext, args...).RunStdString(&git.RunOpts{Dir: repoPath})
pr.MergeBase, _, err = cmd.RunStdString(&git.RunOpts{Dir: repoPath})
if err != nil {
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)
continue
Expand Down
4 changes: 2 additions & 2 deletions modules/doctor/heads.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ func synchronizeRepoHeads(ctx context.Context, logger log.Logger, autofix bool)
numRepos++
runOpts := &git.RunOpts{Dir: repo.RepoPath()}

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

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

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

// otherwise, let's try fixing HEAD
err := git.NewCommand(ctx, "symbolic-ref", "--", "HEAD", repo.DefaultBranch).Run(runOpts)
err := git.NewCommand(ctx, "symbolic-ref").AddSlashedArguments("HEAD", repo.DefaultBranch).Run(runOpts)
if err != nil {
logger.Warn("Failed to fix HEAD for %s/%s: %v", repo.OwnerName, repo.Name, err)
return nil
Expand Down
14 changes: 7 additions & 7 deletions modules/doctor/mergebase.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,17 +44,17 @@ func checkPRMergeBase(ctx context.Context, logger log.Logger, autofix bool) erro

if !pr.HasMerged {
var err error
pr.MergeBase, _, err = git.NewCommand(ctx, "merge-base", "--", pr.BaseBranch, pr.GetGitRefName()).RunStdString(&git.RunOpts{Dir: repoPath})
pr.MergeBase, _, err = git.NewCommand(ctx, "merge-base").AddSlashedArguments(pr.BaseBranch, pr.GetGitRefName()).RunStdString(&git.RunOpts{Dir: repoPath})
if err != nil {
var err2 error
pr.MergeBase, _, err2 = git.NewCommand(ctx, "rev-parse", git.BranchPrefix+pr.BaseBranch).RunStdString(&git.RunOpts{Dir: repoPath})
pr.MergeBase, _, err2 = git.NewCommand(ctx, "rev-parse").AddDynamicArguments(git.BranchPrefix + pr.BaseBranch).RunStdString(&git.RunOpts{Dir: repoPath})
if err2 != nil {
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)
return nil
}
}
} else {
parentsString, _, err := git.NewCommand(ctx, "rev-list", "--parents", "-n", "1", pr.MergedCommitID).RunStdString(&git.RunOpts{Dir: repoPath})
parentsString, _, err := git.NewCommand(ctx, "rev-list", "--parents", "-n", "1").AddDynamicArguments(pr.MergedCommitID).RunStdString(&git.RunOpts{Dir: repoPath})
if err != nil {
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)
return nil
Expand All @@ -64,10 +64,10 @@ func checkPRMergeBase(ctx context.Context, logger log.Logger, autofix bool) erro
return nil
}

args := append([]string{"merge-base", "--"}, parents[1:]...)
args = append(args, pr.GetGitRefName())

pr.MergeBase, _, err = git.NewCommand(ctx, args...).RunStdString(&git.RunOpts{Dir: repoPath})
refs := append([]string{}, parents[1:]...)
refs = append(refs, pr.GetGitRefName())
cmd := git.NewCommand(ctx, "merge-base").AddSlashedArguments(refs...)
pr.MergeBase, _, err = cmd.RunStdString(&git.RunOpts{Dir: repoPath})
if err != nil {
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)
return nil
Expand Down
62 changes: 47 additions & 15 deletions modules/git/command.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ import (

var (
// globalCommandArgs global command args for external package setting
globalCommandArgs []string
globalCommandArgs []CmdArg

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

type CmdArg string

func (c *Command) String() string {
if len(c.args) == 0 {
return c.name
Expand All @@ -52,30 +54,39 @@ func (c *Command) String() string {

// NewCommand creates and returns a new Git Command based on given command and arguments.
// Each argument should be safe to be trusted. User-provided arguments should be passed to AddDynamicArguments instead.
func NewCommand(ctx context.Context, args ...string) *Command {
func NewCommand(ctx context.Context, args ...CmdArg) *Command {
// Make an explicit copy of globalCommandArgs, otherwise append might overwrite it
cargs := make([]string, len(globalCommandArgs))
copy(cargs, globalCommandArgs)
cargs := make([]string, 0, len(globalCommandArgs)+len(args))
for _, arg := range globalCommandArgs {
cargs = append(cargs, string(arg))
}
for _, arg := range args {
cargs = append(cargs, string(arg))
}
return &Command{
name: GitExecutable,
args: append(cargs, args...),
args: cargs,
parentContext: ctx,
globalArgsLength: len(globalCommandArgs),
}
}

// 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
// Each argument should be safe to be trusted. User-provided arguments should be passed to AddDynamicArguments instead.
func NewCommandNoGlobals(args ...string) *Command {
func NewCommandNoGlobals(args ...CmdArg) *Command {
return NewCommandContextNoGlobals(DefaultContext, args...)
}

// 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
// Each argument should be safe to be trusted. User-provided arguments should be passed to AddDynamicArguments instead.
func NewCommandContextNoGlobals(ctx context.Context, args ...string) *Command {
func NewCommandContextNoGlobals(ctx context.Context, args ...CmdArg) *Command {
cargs := make([]string, 0, len(args))
for _, arg := range args {
cargs = append(cargs, string(arg))
}
return &Command{
name: GitExecutable,
args: args,
args: cargs,
parentContext: ctx,
}
}
Expand All @@ -93,10 +104,12 @@ func (c *Command) SetDescription(desc string) *Command {
return c
}

// AddArguments adds new argument(s) to the command. Each argument must be safe to be trusted.
// AddArguments adds new git argument(s) to the command. Each argument must be safe to be trusted.
// User-provided arguments should be passed to AddDynamicArguments instead.
func (c *Command) AddArguments(args ...string) *Command {
c.args = append(c.args, args...)
func (c *Command) AddArguments(args ...CmdArg) *Command {
for _, arg := range args {
c.args = append(c.args, string(arg))
}
return c
}

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

// AddSlashedArguments adds the "--" and then add the arguments
func (c *Command) AddSlashedArguments(list ...string) *Command {
c.args = append(c.args, "--")
// Some old code also checks `arg != ""`, IMO it's not necessary.
// If the check is needed, the list should be prepared before the call to this function
c.args = append(c.args, list...)
return c
}

// CmdArgCheck checks whether the string is safe to be used as a dynamic argument.
// It panics if the check fails. Usually it should not be used, it's just for refactoring purpose
// deprecated
func CmdArgCheck(s string) CmdArg {
if s != "" && s[0] == '-' {
panic("invalid git cmd argument: " + s)
}
return CmdArg(s)
}

// RunOpts represents parameters to run the command. If UseContextTimeout is specified, then Timeout is ignored.
type RunOpts struct {
Env []string
Expand Down Expand Up @@ -153,7 +185,7 @@ func CommonGitCmdEnvs() []string {
}...)
}

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

// AllowLFSFiltersArgs return globalCommandArgs with lfs filter, it should only be used for tests
func AllowLFSFiltersArgs() []string {
func AllowLFSFiltersArgs() []CmdArg {
// Now here we should explicitly allow lfs filters to run
filteredLFSGlobalArgs := make([]string, len(globalCommandArgs))
filteredLFSGlobalArgs := make([]CmdArg, len(globalCommandArgs))
j := 0
for _, arg := range globalCommandArgs {
if strings.Contains(arg, "lfs") {
if strings.Contains(string(arg), "lfs") {
j--
} else {
filteredLFSGlobalArgs[j] = arg
Expand Down
50 changes: 21 additions & 29 deletions modules/git/commit.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,13 +92,13 @@ func AddChanges(repoPath string, all bool, files ...string) error {
}

// AddChangesWithArgs marks local changes to be ready for commit.
func AddChangesWithArgs(repoPath string, globalArgs []string, all bool, files ...string) error {
func AddChangesWithArgs(repoPath string, globalArgs []CmdArg, all bool, files ...string) error {
cmd := NewCommandNoGlobals(append(globalArgs, "add")...)
if all {
cmd.AddArguments("--all")
}
cmd.AddArguments("--")
_, _, err := cmd.AddArguments(files...).RunStdString(&RunOpts{Dir: repoPath})
cmd.AddSlashedArguments(files...)
_, _, err := cmd.RunStdString(&RunOpts{Dir: repoPath})
return err
}

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

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

if opts.Author == nil {
opts.Author = opts.Committer
}
if opts.Author != nil {
cmd.AddArguments(fmt.Sprintf("--author='%s <%s>'", opts.Author.Name, opts.Author.Email))
cmd.AddArguments(CmdArg(fmt.Sprintf("--author='%s <%s>'", opts.Author.Name, opts.Author.Email)))
}
cmd.AddArguments("-m", opts.Message)
cmd.AddArguments("-m").AddDynamicArguments(opts.Message)

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

// AllCommitsCount returns count of all commits in repository
func AllCommitsCount(ctx context.Context, repoPath string, hidePRRefs bool, files ...string) (int64, error) {
args := []string{"--all", "--count"}
cmd := NewCommand(ctx, "rev-list")
if hidePRRefs {
args = append([]string{"--exclude=" + PullPrefix + "*"}, args...)
cmd.AddArguments("--exclude=" + PullPrefix + "*")
}
cmd := NewCommand(ctx, "rev-list")
cmd.AddArguments(args...)
cmd.AddArguments("--all", "--count")
if len(files) > 0 {
cmd.AddArguments("--")
cmd.AddArguments(files...)
cmd.AddSlashedArguments(files...)
}

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

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

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

// GetBranchName gets the closest branch name (as returned by 'git name-rev --name-only')
func (c *Commit) GetBranchName() (string, error) {
args := []string{
"name-rev",
}
cmd := NewCommand(c.repo.Ctx, "name-rev")
if CheckGitVersionAtLeast("2.13.0") == nil {
args = append(args, "--exclude", "refs/tags/*")
cmd.AddArguments("--exclude", "refs/tags/*")
}
args = append(args, "--name-only", "--no-undefined", c.ID.String())

data, _, err := NewCommand(c.repo.Ctx, args...).RunStdString(&RunOpts{Dir: c.repo.Path})
cmd.AddArguments("--name-only", "--no-undefined").AddDynamicArguments(c.ID.String())
data, _, err := cmd.RunStdString(&RunOpts{Dir: c.repo.Path})
if err != nil {
// handle special case where git can not describe commit
if strings.Contains(err.Error(), "cannot describe") {
Expand All @@ -426,7 +420,7 @@ func (c *Commit) LoadBranchName() (err error) {

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

stderr := new(bytes.Buffer)
args := []string{"log", "--name-status", "-c", "--pretty=format:", "--parents", "--no-renames", "-z", "-1", commitID}

err := NewCommand(ctx, args...).Run(&RunOpts{
err := NewCommand(ctx, "log", "--name-status", "-c", "--pretty=format:", "--parents", "--no-renames", "-z", "-1").AddDynamicArguments(commitID).Run(&RunOpts{
Dir: repoPath,
Stdout: w,
Stderr: stderr,
Expand All @@ -521,7 +513,7 @@ func GetCommitFileStatus(ctx context.Context, repoPath, commitID string) (*Commi

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