From 88f2e457d891c4e91a9eddab8143d0109e08dfa9 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Sat, 11 Jun 2022 11:56:27 +0800 Subject: [PATCH] Fix data-race problems in git module (quick patch) (#19934) * Fix data-race problems in git module * use HomeDir instead of setting.RepoRootPath Co-authored-by: Lunny Xiao --- integrations/integration_test.go | 4 +- integrations/migration-test/migration_test.go | 2 +- models/migrations/migrations_test.go | 2 +- models/unittest/testdb.go | 4 +- modules/git/git.go | 80 +++++++++---------- modules/git/git_test.go | 2 +- modules/indexer/stats/indexer_test.go | 5 -- routers/init.go | 2 +- 8 files changed, 46 insertions(+), 55 deletions(-) diff --git a/integrations/integration_test.go b/integrations/integration_test.go index 0a41546554..ce21eb2ef7 100644 --- a/integrations/integration_test.go +++ b/integrations/integration_test.go @@ -275,7 +275,7 @@ func prepareTestEnv(t testing.TB, skip ...int) func() { assert.NoError(t, unittest.LoadFixtures()) assert.NoError(t, util.RemoveAll(setting.RepoRootPath)) assert.NoError(t, unittest.CopyDir(path.Join(filepath.Dir(setting.AppPath), "integrations/gitea-repositories-meta"), setting.RepoRootPath)) - assert.NoError(t, git.InitWithConfigSync(context.Background())) + assert.NoError(t, git.InitOnceWithSync(context.Background())) ownerDirs, err := os.ReadDir(setting.RepoRootPath) if err != nil { assert.NoError(t, err, "unable to read the new repo root: %v\n", err) @@ -576,7 +576,7 @@ func resetFixtures(t *testing.T) { assert.NoError(t, unittest.LoadFixtures()) assert.NoError(t, util.RemoveAll(setting.RepoRootPath)) assert.NoError(t, unittest.CopyDir(path.Join(filepath.Dir(setting.AppPath), "integrations/gitea-repositories-meta"), setting.RepoRootPath)) - assert.NoError(t, git.InitWithConfigSync(context.Background())) + assert.NoError(t, git.InitOnceWithSync(context.Background())) ownerDirs, err := os.ReadDir(setting.RepoRootPath) if err != nil { assert.NoError(t, err, "unable to read the new repo root: %v\n", err) diff --git a/integrations/migration-test/migration_test.go b/integrations/migration-test/migration_test.go index 45e31ff9ac..83c31d8018 100644 --- a/integrations/migration-test/migration_test.go +++ b/integrations/migration-test/migration_test.go @@ -62,7 +62,7 @@ func initMigrationTest(t *testing.T) func() { assert.True(t, len(setting.RepoRootPath) != 0) assert.NoError(t, util.RemoveAll(setting.RepoRootPath)) assert.NoError(t, unittest.CopyDir(path.Join(filepath.Dir(setting.AppPath), "integrations/gitea-repositories-meta"), setting.RepoRootPath)) - assert.NoError(t, git.InitWithConfigSync(context.Background())) + assert.NoError(t, git.InitOnceWithSync(context.Background())) ownerDirs, err := os.ReadDir(setting.RepoRootPath) if err != nil { assert.NoError(t, err, "unable to read the new repo root: %v\n", err) diff --git a/models/migrations/migrations_test.go b/models/migrations/migrations_test.go index 2c4d21d974..0c8e74f734 100644 --- a/models/migrations/migrations_test.go +++ b/models/migrations/migrations_test.go @@ -203,7 +203,7 @@ func prepareTestEnv(t *testing.T, skip int, syncModels ...interface{}) (*xorm.En deferFn := PrintCurrentTest(t, ourSkip) assert.NoError(t, os.RemoveAll(setting.RepoRootPath)) assert.NoError(t, unittest.CopyDir(path.Join(filepath.Dir(setting.AppPath), "integrations/gitea-repositories-meta"), setting.RepoRootPath)) - assert.NoError(t, git.InitWithConfigSync(context.Background())) + assert.NoError(t, git.InitOnceWithSync(context.Background())) ownerDirs, err := os.ReadDir(setting.RepoRootPath) if err != nil { assert.NoError(t, err, "unable to read the new repo root: %v\n", err) diff --git a/models/unittest/testdb.go b/models/unittest/testdb.go index 2a366836fe..d1a4498510 100644 --- a/models/unittest/testdb.go +++ b/models/unittest/testdb.go @@ -117,7 +117,7 @@ func MainTest(m *testing.M, testOpts *TestOptions) { if err = CopyDir(filepath.Join(testOpts.GiteaRootPath, "integrations", "gitea-repositories-meta"), setting.RepoRootPath); err != nil { fatalTestError("util.CopyDir: %v\n", err) } - if err = git.InitWithConfigSync(context.Background()); err != nil { + if err = git.InitOnceWithSync(context.Background()); err != nil { fatalTestError("git.Init: %v\n", err) } @@ -202,7 +202,7 @@ func PrepareTestEnv(t testing.TB) { assert.NoError(t, util.RemoveAll(setting.RepoRootPath)) metaPath := filepath.Join(giteaRoot, "integrations", "gitea-repositories-meta") assert.NoError(t, CopyDir(metaPath, setting.RepoRootPath)) - assert.NoError(t, git.InitWithConfigSync(context.Background())) + assert.NoError(t, git.InitOnceWithSync(context.Background())) ownerDirs, err := os.ReadDir(setting.RepoRootPath) assert.NoError(t, err) diff --git a/modules/git/git.go b/modules/git/git.go index 5817bd2c7f..0459f57dde 100644 --- a/modules/git/git.go +++ b/modules/git/git.go @@ -34,15 +34,12 @@ var ( GitExecutable = "git" // DefaultContext is the default context to run git commands in - // will be overwritten by InitWithConfigSync with HammerContext + // will be overwritten by InitXxx with HammerContext DefaultContext = context.Background() // SupportProcReceive version >= 2.29.0 SupportProcReceive bool - // initMutex is used to avoid Golang's data race error. see the comments below. - initMutex sync.Mutex - gitVersion *version.Version ) @@ -131,15 +128,6 @@ func VersionInfo() string { return fmt.Sprintf(format, args...) } -// InitSimple initializes git module with a very simple step, no config changes, no global command arguments. -// This method doesn't change anything to filesystem -func InitSimple(ctx context.Context) error { - initMutex.Lock() - defer initMutex.Unlock() - - return initSimpleInternal(ctx) -} - // HomeDir is the home dir for git to store the global config file used by Gitea internally func HomeDir() string { if setting.RepoRootPath == "" { @@ -153,11 +141,9 @@ func HomeDir() string { return setting.RepoRootPath } -func initSimpleInternal(ctx context.Context) error { - // at the moment, when running integration tests, the git.InitXxx would be called twice. - // one is called by the GlobalInitInstalled, one is called by TestMain. - // so the init functions should be protected by a mutex to avoid Golang's data race error. - +// InitSimple initializes git module with a very simple step, no config changes, no global command arguments. +// This method doesn't change anything to filesystem. At the moment, it is only used by "git serv" sub-command, no data-race +func InitSimple(ctx context.Context) error { DefaultContext = ctx if setting.Git.Timeout.Default > 0 { @@ -174,35 +160,47 @@ func initSimpleInternal(ctx context.Context) error { return nil } -// InitWithConfigSync initializes git module. This method may create directories or write files into filesystem -func InitWithConfigSync(ctx context.Context) error { - initMutex.Lock() - defer initMutex.Unlock() +var initOnce sync.Once - err := initSimpleInternal(ctx) +// InitOnceWithSync initializes git module with version check and change global variables, sync gitconfig. +// This method will update the global variables ONLY ONCE (just like git.CheckLFSVersion -- which is not ideal too), +// otherwise there will be data-race problem at the moment. +func InitOnceWithSync(ctx context.Context) (err error) { + initOnce.Do(func() { + err = InitSimple(ctx) + if err != nil { + return + } + + // Since git wire protocol has been released from git v2.18 + if setting.Git.EnableAutoGitWireProtocol && CheckGitVersionAtLeast("2.18") == nil { + globalCommandArgs = append(globalCommandArgs, "-c", "protocol.version=2") + } + + // By default partial clones are disabled, enable them from git v2.22 + if !setting.Git.DisablePartialClone && CheckGitVersionAtLeast("2.22") == nil { + globalCommandArgs = append(globalCommandArgs, "-c", "uploadpack.allowfilter=true", "-c", "uploadpack.allowAnySHA1InWant=true") + } + + // Explicitly disable credential helper, otherwise Git credentials might leak + if CheckGitVersionAtLeast("2.9") == nil { + globalCommandArgs = append(globalCommandArgs, "-c", "credential.helper=") + } + + SupportProcReceive = CheckGitVersionAtLeast("2.29") == nil + }) if err != nil { return err } + return syncGitConfig() +} - if err = os.MkdirAll(setting.RepoRootPath, os.ModePerm); err != nil { +// syncGitConfig only modifies gitconfig, won't change global variables (otherwise there will be data-race problem) +func syncGitConfig() (err error) { + if err = os.MkdirAll(HomeDir(), os.ModePerm); err != nil { return fmt.Errorf("unable to create directory %s, err: %w", setting.RepoRootPath, err) } - if CheckGitVersionAtLeast("2.9") == nil { - // Explicitly disable credential helper, otherwise Git credentials might leak - globalCommandArgs = append(globalCommandArgs, "-c", "credential.helper=") - } - - // Since git wire protocol has been released from git v2.18 - if setting.Git.EnableAutoGitWireProtocol && CheckGitVersionAtLeast("2.18") == nil { - globalCommandArgs = append(globalCommandArgs, "-c", "protocol.version=2") - } - - // By default partial clones are disabled, enable them from git v2.22 - if !setting.Git.DisablePartialClone && CheckGitVersionAtLeast("2.22") == nil { - globalCommandArgs = append(globalCommandArgs, "-c", "uploadpack.allowfilter=true", "-c", "uploadpack.allowAnySHA1InWant=true") - } - // Git requires setting user.name and user.email in order to commit changes - old comment: "if they're not set just add some defaults" // TODO: need to confirm whether users really need to change these values manually. It seems that these values are dummy only and not really used. // If these values are not really used, then they can be set (overwritten) directly without considering about existence. @@ -235,17 +233,15 @@ func InitWithConfigSync(ctx context.Context) error { } } - if CheckGitVersionAtLeast("2.29") == nil { + if SupportProcReceive { // set support for AGit flow if err := configAddNonExist("receive.procReceiveRefs", "refs/for"); err != nil { return err } - SupportProcReceive = true } else { if err := configUnsetAll("receive.procReceiveRefs", "refs/for"); err != nil { return err } - SupportProcReceive = false } if runtime.GOOS == "windows" { diff --git a/modules/git/git_test.go b/modules/git/git_test.go index 5b1cd820e8..061c876cde 100644 --- a/modules/git/git_test.go +++ b/modules/git/git_test.go @@ -28,7 +28,7 @@ func testRun(m *testing.M) error { defer util.RemoveAll(repoRootPath) setting.RepoRootPath = repoRootPath - if err = InitWithConfigSync(context.Background()); err != nil { + if err = InitOnceWithSync(context.Background()); err != nil { return fmt.Errorf("failed to call Init: %w", err) } diff --git a/modules/indexer/stats/indexer_test.go b/modules/indexer/stats/indexer_test.go index a335972c21..a4a8b63241 100644 --- a/modules/indexer/stats/indexer_test.go +++ b/modules/indexer/stats/indexer_test.go @@ -13,7 +13,6 @@ import ( "code.gitea.io/gitea/models/db" repo_model "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/models/unittest" - "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/queue" "code.gitea.io/gitea/modules/setting" @@ -30,10 +29,6 @@ func TestMain(m *testing.M) { } func TestRepoStatsIndex(t *testing.T) { - if err := git.InitWithConfigSync(context.Background()); !assert.NoError(t, err) { - return - } - assert.NoError(t, unittest.PrepareTestDatabase()) setting.Cfg = ini.Empty() diff --git a/routers/init.go b/routers/init.go index 9b6a770f27..2898c44607 100644 --- a/routers/init.go +++ b/routers/init.go @@ -102,7 +102,7 @@ func GlobalInitInstalled(ctx context.Context) { log.Fatal("Gitea is not installed") } - mustInitCtx(ctx, git.InitWithConfigSync) + mustInitCtx(ctx, git.InitOnceWithSync) log.Info("Git Version: %s (home: %s)", git.VersionInfo(), git.HomeDir()) git.CheckLFSVersion()