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 <xiaolunwen@gmail.com>

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)

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)

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)

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)

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 {
+// InitSimple initializes git module with a very simple step, no config changes, no global command arguments.
-	// at the moment, when running integration tests, the git.InitXxx would be called twice.
+// This method doesn't change anything to filesystem. At the moment, it is only used by "git serv" sub-command, no data-race
-	// one is called by the GlobalInitInstalled, one is called by TestMain.
+func InitSimple(ctx context.Context) error {
-	// so the init functions should be protected by a mutex to avoid Golang's data race error.
-
 	DefaultContext = ctx
 
 	if setting.Git.Timeout.Default > 0 {
@@ -174,23 +160,16 @@ func initSimpleInternal(ctx context.Context) error {
 	return nil
 }
 
-// InitWithConfigSync initializes git module. This method may create directories or write files into filesystem
+var initOnce sync.Once
-func InitWithConfigSync(ctx context.Context) error {
-	initMutex.Lock()
-	defer initMutex.Unlock()
 
-	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 err
+			return
-	}
-
-	if err = os.MkdirAll(setting.RepoRootPath, 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
@@ -203,6 +182,25 @@ func InitWithConfigSync(ctx context.Context) error {
 			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()
+}
+
+// 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)
+	}
+
 	// 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" {

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)
 	}
 

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()
 

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()