From 7b7d382b8b414e7da67dfec7c7e1ef9e0e269d68 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 27 Nov 2019 08:35:52 +0800 Subject: [PATCH] Fix datarace on git.GlobalCommandArgs on tests (#9162) * fix datarace on git.GlobalCommandArgs on tests * fix tests * fix tests * fix tests --- integrations/git_helper_for_declarative_test.go | 17 ++++------------- integrations/git_test.go | 12 +++++------- modules/git/command.go | 8 ++++++++ modules/git/commit.go | 17 +++++++++++++++-- modules/git/repo.go | 9 ++++++++- 5 files changed, 40 insertions(+), 23 deletions(-) diff --git a/integrations/git_helper_for_declarative_test.go b/integrations/git_helper_for_declarative_test.go index 024bf87a3..1eb1f1dc8 100644 --- a/integrations/git_helper_for_declarative_test.go +++ b/integrations/git_helper_for_declarative_test.go @@ -63,7 +63,6 @@ func createSSHUrl(gitPath string, u *url.URL) *url.URL { func allowLFSFilters() []string { // Now here we should explicitly allow lfs filters to run - globalArgs := git.GlobalCommandArgs filteredLFSGlobalArgs := make([]string, len(git.GlobalCommandArgs)) j := 0 for _, arg := range git.GlobalCommandArgs { @@ -74,9 +73,7 @@ func allowLFSFilters() []string { j++ } } - filteredLFSGlobalArgs = filteredLFSGlobalArgs[:j] - git.GlobalCommandArgs = filteredLFSGlobalArgs - return globalArgs + return filteredLFSGlobalArgs[:j] } func onGiteaRun(t *testing.T, callback func(*testing.T, *url.URL), prepare ...bool) { @@ -107,9 +104,7 @@ func onGiteaRun(t *testing.T, callback func(*testing.T, *url.URL), prepare ...bo func doGitClone(dstLocalPath string, u *url.URL) func(*testing.T) { return func(t *testing.T) { - oldGlobals := allowLFSFilters() - assert.NoError(t, git.Clone(u.String(), dstLocalPath, git.CloneRepoOptions{})) - git.GlobalCommandArgs = oldGlobals + assert.NoError(t, git.CloneWithArgs(u.String(), dstLocalPath, allowLFSFilters(), git.CloneRepoOptions{})) assert.True(t, com.IsExist(filepath.Join(dstLocalPath, "README.md"))) } } @@ -170,9 +165,7 @@ func doGitCreateBranch(dstPath, branch string) func(*testing.T) { func doGitCheckoutBranch(dstPath string, args ...string) func(*testing.T) { return func(t *testing.T) { - oldGlobals := allowLFSFilters() - _, err := git.NewCommand(append([]string{"checkout"}, args...)...).RunInDir(dstPath) - git.GlobalCommandArgs = oldGlobals + _, err := git.NewCommandNoGlobals(append(append(allowLFSFilters(), "checkout"), args...)...).RunInDir(dstPath) assert.NoError(t, err) } } @@ -186,9 +179,7 @@ func doGitMerge(dstPath string, args ...string) func(*testing.T) { func doGitPull(dstPath string, args ...string) func(*testing.T) { return func(t *testing.T) { - oldGlobals := allowLFSFilters() - _, err := git.NewCommand(append([]string{"pull"}, args...)...).RunInDir(dstPath) - git.GlobalCommandArgs = oldGlobals + _, err := git.NewCommandNoGlobals(append(append(allowLFSFilters(), "pull"), args...)...).RunInDir(dstPath) assert.NoError(t, err) } } diff --git a/integrations/git_test.go b/integrations/git_test.go index f6137a947..3ca4cc54c 100644 --- a/integrations/git_test.go +++ b/integrations/git_test.go @@ -148,8 +148,8 @@ func lfsCommitAndPushTest(t *testing.T, dstPath string) (littleLFS, bigLFS strin assert.NoError(t, err) err = git.AddChanges(dstPath, false, ".gitattributes") assert.NoError(t, err) - oldGlobals := allowLFSFilters() - err = git.CommitChanges(dstPath, git.CommitChangesOptions{ + + err = git.CommitChangesWithArgs(dstPath, allowLFSFilters(), git.CommitChangesOptions{ Committer: &git.Signature{ Email: "user2@example.com", Name: "User Two", @@ -163,7 +163,6 @@ func lfsCommitAndPushTest(t *testing.T, dstPath string) (littleLFS, bigLFS strin Message: fmt.Sprintf("Testing commit @ %v", time.Now()), }) assert.NoError(t, err) - git.GlobalCommandArgs = oldGlobals littleLFS, bigLFS = commitAndPushTest(t, dstPath, prefix) @@ -307,12 +306,12 @@ func generateCommitWithNewData(size int, repoPath, email, fullName, prefix strin //Commit // Now here we should explicitly allow lfs filters to run - oldGlobals := allowLFSFilters() - err = git.AddChanges(repoPath, false, filepath.Base(tmpFile.Name())) + globalArgs := allowLFSFilters() + err = git.AddChangesWithArgs(repoPath, globalArgs, false, filepath.Base(tmpFile.Name())) if err != nil { return "", err } - err = git.CommitChanges(repoPath, git.CommitChangesOptions{ + err = git.CommitChangesWithArgs(repoPath, globalArgs, git.CommitChangesOptions{ Committer: &git.Signature{ Email: email, Name: fullName, @@ -325,7 +324,6 @@ func generateCommitWithNewData(size int, repoPath, email, fullName, prefix strin }, Message: fmt.Sprintf("Testing commit @ %v", time.Now()), }) - git.GlobalCommandArgs = oldGlobals return filepath.Base(tmpFile.Name()), err } diff --git a/modules/git/command.go b/modules/git/command.go index 7772abd2d..65878edb7 100644 --- a/modules/git/command.go +++ b/modules/git/command.go @@ -52,6 +52,14 @@ func NewCommand(args ...string) *Command { } } +// 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 +func NewCommandNoGlobals(args ...string) *Command { + return &Command{ + name: GitExecutable, + args: args, + } +} + // AddArguments adds new argument(s) to the command. func (c *Command) AddArguments(args ...string) *Command { c.args = append(c.args, args...) diff --git a/modules/git/commit.go b/modules/git/commit.go index ce55dd55f..0388d5e9b 100644 --- a/modules/git/commit.go +++ b/modules/git/commit.go @@ -207,7 +207,12 @@ func (c *Commit) GetCommitByPath(relpath string) (*Commit, error) { // AddChanges marks local changes to be ready for commit. func AddChanges(repoPath string, all bool, files ...string) error { - cmd := NewCommand("add") + return AddChangesWithArgs(repoPath, GlobalCommandArgs, all, files...) +} + +// AddChangesWithArgs marks local changes to be ready for commit. +func AddChangesWithArgs(repoPath string, gloablArgs []string, all bool, files ...string) error { + cmd := NewCommandNoGlobals(append(gloablArgs, "add")...) if all { cmd.AddArguments("--all") } @@ -226,7 +231,15 @@ 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 { - cmd := NewCommand() + cargs := make([]string, 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 { + cmd := NewCommandNoGlobals(args...) if opts.Committer != nil { cmd.AddArguments("-c", "user.name="+opts.Committer.Name, "-c", "user.email="+opts.Committer.Email) } diff --git a/modules/git/repo.go b/modules/git/repo.go index ffca2524b..e277f896b 100644 --- a/modules/git/repo.go +++ b/modules/git/repo.go @@ -165,12 +165,19 @@ type CloneRepoOptions struct { // Clone clones original repository to target path. func Clone(from, to string, opts CloneRepoOptions) (err error) { + cargs := make([]string, len(GlobalCommandArgs)) + copy(cargs, GlobalCommandArgs) + return CloneWithArgs(from, to, cargs, opts) +} + +// CloneWithArgs original repository to target path. +func CloneWithArgs(from, to string, args []string, opts CloneRepoOptions) (err error) { toDir := path.Dir(to) if err = os.MkdirAll(toDir, os.ModePerm); err != nil { return err } - cmd := NewCommand("clone") + cmd := NewCommandNoGlobals(args...).AddArguments("clone") if opts.Mirror { cmd.AddArguments("--mirror") }