Refactor legacy git init (#20376)

* merge `CheckLFSVersion` into `InitFull` (renamed from `InitWithSyncOnce`)
* remove the `Once` during git init, no data-race now
* for doctor sub-commands, `InitFull` should only be called in initialization stage

Co-authored-by: zeripath <art27@cantab.net>
Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>

14 files changed, 58 insertions, 144 deletions

diff --git a/cmd/doctor.go b/cmd/doctor.go
index 3f16c6e2a6..1a15dd2941 100644
--- a/cmd/doctor.go
+++ b/cmd/doctor.go
@@ -14,6 +14,7 @@ import (
 	"code.gitea.io/gitea/models/db"
 	"code.gitea.io/gitea/models/migrations"
 	"code.gitea.io/gitea/modules/doctor"
+	"code.gitea.io/gitea/modules/git"
 	"code.gitea.io/gitea/modules/log"
 	"code.gitea.io/gitea/modules/setting"
 
@@ -124,13 +125,18 @@ func runRecreateTable(ctx *cli.Context) error {
 }
 
 func runDoctor(ctx *cli.Context) error {
+	stdCtx, cancel := installSignals()
+	defer cancel()
+
+	// some doctor sub-commands need to use git command
+	if err := git.InitFull(stdCtx); err != nil {
+		return err
+	}
+
 	// Silence the default loggers
 	log.DelNamedLogger("console")
 	log.DelNamedLogger(log.DEFAULT)
 
-	stdCtx, cancel := installSignals()
-	defer cancel()
-
 	// Now setup our own
 	logFile := ctx.String("log-file")
 	if !ctx.IsSet("log-file") {
diff --git a/contrib/pr/checkout.go b/contrib/pr/checkout.go
index f6d29f3c5b..65762a91e3 100644
--- a/contrib/pr/checkout.go
+++ b/contrib/pr/checkout.go
@@ -80,7 +80,6 @@ func runPR() {
 	setting.RunUser = curUser.Username
 
 	log.Printf("[PR] Loading fixtures data ...\n")
-	gitea_git.CheckLFSVersion()
 	//models.LoadConfigs()
 	/*
 		setting.Database.Type = "sqlite3"
diff --git a/integrations/git_test.go b/integrations/git_test.go
index d6bd673822..f9e1eafb65 100644
--- a/integrations/git_test.go
+++ b/integrations/git_test.go
@@ -156,11 +156,6 @@ func standardCommitAndPushTest(t *testing.T, dstPath string) (little, big string
 func lfsCommitAndPushTest(t *testing.T, dstPath string) (littleLFS, bigLFS string) {
 	t.Run("LFS", func(t *testing.T) {
 		defer PrintCurrentTest(t)()
-		git.CheckLFSVersion()
-		if !setting.LFS.StartServer {
-			t.Skip()
-			return
-		}
 		prefix := "lfs-data-file-"
 		err := git.NewCommand(git.DefaultContext, "lfs").AddArguments("install").Run(&git.RunOpts{Dir: dstPath})
 		assert.NoError(t, err)
@@ -226,7 +221,6 @@ func rawTest(t *testing.T, ctx *APITestContext, little, big, littleLFS, bigLFS s
 		resp := session.MakeRequestNilResponseRecorder(t, req, http.StatusOK)
 		assert.Equal(t, littleSize, resp.Length)
 
-		git.CheckLFSVersion()
 		if setting.LFS.StartServer {
 			req = NewRequest(t, "GET", path.Join("/", username, reponame, "/raw/branch/master/", littleLFS))
 			resp := session.MakeRequest(t, req, http.StatusOK)
@@ -268,12 +262,9 @@ func mediaTest(t *testing.T, ctx *APITestContext, little, big, littleLFS, bigLFS
 		resp := session.MakeRequestNilResponseRecorder(t, req, http.StatusOK)
 		assert.Equal(t, littleSize, resp.Length)
 
-		git.CheckLFSVersion()
-		if setting.LFS.StartServer {
-			req = NewRequest(t, "GET", path.Join("/", username, reponame, "/media/branch/master/", littleLFS))
-			resp = session.MakeRequestNilResponseRecorder(t, req, http.StatusOK)
-			assert.Equal(t, littleSize, resp.Length)
-		}
+		req = NewRequest(t, "GET", path.Join("/", username, reponame, "/media/branch/master/", littleLFS))
+		resp = session.MakeRequestNilResponseRecorder(t, req, http.StatusOK)
+		assert.Equal(t, littleSize, resp.Length)
 
 		if !testing.Short() {
 			req = NewRequest(t, "GET", path.Join("/", username, reponame, "/media/branch/master/", big))
diff --git a/integrations/integration_test.go b/integrations/integration_test.go
index 230f780175..3c379f5c84 100644
--- a/integrations/integration_test.go
+++ b/integrations/integration_test.go
@@ -175,10 +175,9 @@ func initIntegrationTest() {
 	setting.Repository.DefaultBranch = "master" // many test code still assume that default branch is called "master"
 	_ = util.RemoveAll(repo_module.LocalCopyPath())
 
-	if err := git.InitOnceWithSync(context.Background()); err != nil {
+	if err := git.InitFull(context.Background()); err != nil {
 		log.Fatal("git.InitOnceWithSync: %v", err)
 	}
-	git.CheckLFSVersion()
 
 	setting.InitDBConfig()
 	if err := storage.Init(); err != nil {
@@ -285,7 +284,6 @@ 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.InitOnceWithSync(context.Background())) // the gitconfig has been removed above, so sync the gitconfig again
 	ownerDirs, err := os.ReadDir(setting.RepoRootPath)
 	if err != nil {
 		assert.NoError(t, err, "unable to read the new repo root: %v\n", err)
@@ -586,7 +584,6 @@ 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.InitOnceWithSync(context.Background())) // the gitconfig has been removed above, so sync the gitconfig again
 	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/lfs_getobject_test.go b/integrations/lfs_getobject_test.go
index 4b6bb140d3..14a8ac253e 100644
--- a/integrations/lfs_getobject_test.go
+++ b/integrations/lfs_getobject_test.go
@@ -14,7 +14,6 @@ import (
 
 	git_model "code.gitea.io/gitea/models/git"
 	repo_model "code.gitea.io/gitea/models/repo"
-	"code.gitea.io/gitea/modules/git"
 	"code.gitea.io/gitea/modules/json"
 	"code.gitea.io/gitea/modules/lfs"
 	"code.gitea.io/gitea/modules/setting"
@@ -83,11 +82,6 @@ func checkResponseTestContentEncoding(t *testing.T, content *[]byte, resp *httpt
 
 func TestGetLFSSmall(t *testing.T) {
 	defer prepareTestEnv(t)()
-	git.CheckLFSVersion()
-	if !setting.LFS.StartServer {
-		t.Skip()
-		return
-	}
 	content := []byte("A very small file\n")
 
 	resp := storeAndGetLfs(t, &content, nil, http.StatusOK)
@@ -96,11 +90,6 @@ func TestGetLFSSmall(t *testing.T) {
 
 func TestGetLFSLarge(t *testing.T) {
 	defer prepareTestEnv(t)()
-	git.CheckLFSVersion()
-	if !setting.LFS.StartServer {
-		t.Skip()
-		return
-	}
 	content := make([]byte, web.GzipMinSize*10)
 	for i := range content {
 		content[i] = byte(i % 256)
@@ -112,11 +101,6 @@ func TestGetLFSLarge(t *testing.T) {
 
 func TestGetLFSGzip(t *testing.T) {
 	defer prepareTestEnv(t)()
-	git.CheckLFSVersion()
-	if !setting.LFS.StartServer {
-		t.Skip()
-		return
-	}
 	b := make([]byte, web.GzipMinSize*10)
 	for i := range b {
 		b[i] = byte(i % 256)
@@ -133,11 +117,6 @@ func TestGetLFSGzip(t *testing.T) {
 
 func TestGetLFSZip(t *testing.T) {
 	defer prepareTestEnv(t)()
-	git.CheckLFSVersion()
-	if !setting.LFS.StartServer {
-		t.Skip()
-		return
-	}
 	b := make([]byte, web.GzipMinSize*10)
 	for i := range b {
 		b[i] = byte(i % 256)
@@ -156,11 +135,6 @@ func TestGetLFSZip(t *testing.T) {
 
 func TestGetLFSRangeNo(t *testing.T) {
 	defer prepareTestEnv(t)()
-	git.CheckLFSVersion()
-	if !setting.LFS.StartServer {
-		t.Skip()
-		return
-	}
 	content := []byte("123456789\n")
 
 	resp := storeAndGetLfs(t, &content, nil, http.StatusOK)
@@ -169,11 +143,6 @@ func TestGetLFSRangeNo(t *testing.T) {
 
 func TestGetLFSRange(t *testing.T) {
 	defer prepareTestEnv(t)()
-	git.CheckLFSVersion()
-	if !setting.LFS.StartServer {
-		t.Skip()
-		return
-	}
 	content := []byte("123456789\n")
 
 	tests := []struct {
diff --git a/integrations/migration-test/migration_test.go b/integrations/migration-test/migration_test.go
index 20a5c903a9..80093d66f1 100644
--- a/integrations/migration-test/migration_test.go
+++ b/integrations/migration-test/migration_test.go
@@ -82,8 +82,7 @@ func initMigrationTest(t *testing.T) func() {
 		}
 	}
 
-	assert.NoError(t, git.InitOnceWithSync(context.Background()))
-	git.CheckLFSVersion()
+	assert.NoError(t, git.InitFull(context.Background()))
 	setting.InitDBConfig()
 	setting.NewLogServices(true)
 	return deferFn
diff --git a/models/migrations/migrations_test.go b/models/migrations/migrations_test.go
index 46782f24a1..53e4f35395 100644
--- a/models/migrations/migrations_test.go
+++ b/models/migrations/migrations_test.go
@@ -66,11 +66,10 @@ func TestMain(m *testing.M) {
 
 	setting.SetCustomPathAndConf("", "", "")
 	setting.LoadForTest()
-	if err = git.InitOnceWithSync(context.Background()); err != nil {
-		fmt.Printf("Unable to InitOnceWithSync: %v\n", err)
+	if err = git.InitFull(context.Background()); err != nil {
+		fmt.Printf("Unable to InitFull: %v\n", err)
 		os.Exit(1)
 	}
-	git.CheckLFSVersion()
 	setting.InitDBConfig()
 	setting.NewLogServices(true)
 
@@ -207,7 +206,6 @@ 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.InitOnceWithSync(context.Background())) // the gitconfig has been removed above, so sync the gitconfig again
 	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 7f9af55374..58656f781f 100644
--- a/models/unittest/testdb.go
+++ b/models/unittest/testdb.go
@@ -120,11 +120,9 @@ func MainTest(m *testing.M, testOpts *TestOptions) {
 		fatalTestError("util.CopyDir: %v\n", err)
 	}
 
-	if err = git.InitOnceWithSync(context.Background()); err != nil {
+	if err = git.InitFull(context.Background()); err != nil {
 		fatalTestError("git.Init: %v\n", err)
 	}
-	git.CheckLFSVersion()
-
 	ownerDirs, err := os.ReadDir(setting.RepoRootPath)
 	if err != nil {
 		fatalTestError("unable to read the new repo root: %v\n", err)
@@ -206,8 +204,6 @@ 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.InitOnceWithSync(context.Background())) // the gitconfig has been removed above, so sync the gitconfig again
-
 	ownerDirs, err := os.ReadDir(setting.RepoRootPath)
 	assert.NoError(t, err)
 	for _, ownerDir := range ownerDirs {
diff --git a/modules/doctor/mergebase.go b/modules/doctor/mergebase.go
index 2da91cdcc3..46369290a1 100644
--- a/modules/doctor/mergebase.go
+++ b/modules/doctor/mergebase.go
@@ -30,9 +30,6 @@ func iteratePRs(ctx context.Context, repo *repo_model.Repository, each func(*rep
 }
 
 func checkPRMergeBase(ctx context.Context, logger log.Logger, autofix bool) error {
-	if err := git.InitOnceWithSync(ctx); err != nil {
-		return err
-	}
 	numRepos := 0
 	numPRs := 0
 	numPRsUpdated := 0
diff --git a/modules/doctor/misc.go b/modules/doctor/misc.go
index 24175fcaf4..2d2bcb910d 100644
--- a/modules/doctor/misc.go
+++ b/modules/doctor/misc.go
@@ -190,10 +190,6 @@ func checkDaemonExport(ctx context.Context, logger log.Logger, autofix bool) err
 }
 
 func checkCommitGraph(ctx context.Context, logger log.Logger, autofix bool) error {
-	if err := git.InitOnceWithSync(ctx); err != nil {
-		return err
-	}
-
 	numRepos := 0
 	numNeedUpdate := 0
 	numWritten := 0
diff --git a/modules/git/git.go b/modules/git/git.go
index b8317396c0..99849f1f09 100644
--- a/modules/git/git.go
+++ b/modules/git/git.go
@@ -15,7 +15,6 @@ import (
 	"regexp"
 	"runtime"
 	"strings"
-	"sync"
 	"time"
 
 	"code.gitea.io/gitea/modules/log"
@@ -24,8 +23,8 @@ import (
 	"github.com/hashicorp/go-version"
 )
 
-// GitVersionRequired is the minimum Git version required
-const GitVersionRequired = "2.0.0"
+// RequiredVersion is the minimum Git version required
+const RequiredVersion = "2.0.0"
 
 var (
 	// GitExecutable is the command name of git
@@ -43,7 +42,7 @@ var (
 
 // loadGitVersion returns current Git version from shell. Internal usage only.
 func loadGitVersion() (*version.Version, error) {
-	// doesn't need RWMutex because its exec by Init()
+	// doesn't need RWMutex because it's executed by Init()
 	if gitVersion != nil {
 		return gitVersion, nil
 	}
@@ -90,7 +89,7 @@ func SetExecutablePath(path string) error {
 		return fmt.Errorf("unable to load git version: %w", err)
 	}
 
-	versionRequired, err := version.NewVersion(GitVersionRequired)
+	versionRequired, err := version.NewVersion(RequiredVersion)
 	if err != nil {
 		return err
 	}
@@ -104,7 +103,7 @@ func SetExecutablePath(path string) error {
 				moreHint = "get git: https://git-scm.com/download/linux and https://ius.io"
 			}
 		}
-		return fmt.Errorf("installed git version %q is not supported, Gitea requires git version >= %q, %s", gitVersion.Original(), GitVersionRequired, moreHint)
+		return fmt.Errorf("installed git version %q is not supported, Gitea requires git version >= %q, %s", gitVersion.Original(), RequiredVersion, moreHint)
 	}
 
 	return nil
@@ -131,7 +130,7 @@ func checkInit() error {
 		return errors.New("unable to init Git's HomeDir, incorrect initialization of the setting and git modules")
 	}
 	if DefaultContext != nil {
-		log.Warn("git module has been initialized already, duplicate init should be fixed")
+		log.Warn("git module has been initialized already, duplicate init may work but it's better to fix it")
 	}
 	return nil
 }
@@ -140,7 +139,7 @@ func checkInit() error {
 func HomeDir() string {
 	if setting.Git.HomePath == "" {
 		// strict check, make sure the git module is initialized correctly.
-		// attention: when the git module is called in gitea sub-command (serv/hook), the log module is not able to show messages to users.
+		// attention: when the git module is called in gitea sub-command (serv/hook), the log module might not obviously show messages to users/developers.
 		// for example: if there is gitea git hook code calling git.NewCommand before git.InitXxx, the integration test won't show the real failure reasons.
 		log.Fatal("Unable to init Git's HomeDir, incorrect initialization of the setting and git modules")
 		return ""
@@ -149,14 +148,14 @@ func HomeDir() string {
 }
 
 // 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
-// However, in integration test, the sub-command function may be called in the current process, so the InitSimple would be called multiple times, too
+// This method doesn't change anything to filesystem. At the moment, it is only used by some Gitea sub-commands.
 func InitSimple(ctx context.Context) error {
 	if err := checkInit(); err != nil {
 		return err
 	}
 
 	DefaultContext = ctx
+	globalCommandArgs = nil
 
 	if setting.Git.Timeout.Default > 0 {
 		defaultCommandExecutionTimeout = time.Duration(setting.Git.Timeout.Default) * time.Second
@@ -165,46 +164,46 @@ func InitSimple(ctx context.Context) error {
 	return SetExecutablePath(setting.Git.Path)
 }
 
-var initOnce sync.Once
-
-// 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) {
+// InitFull initializes git module with version check and change global variables, sync gitconfig.
+// It should only be called once at the beginning of the program initialization (TestMain/GlobalInitInstalled) as this code makes unsynchronized changes to variables.
+func InitFull(ctx context.Context) (err error) {
 	if err = checkInit(); err != nil {
 		return err
 	}
 
-	initOnce.Do(func() {
-		if err = InitSimple(ctx); err != nil {
-			return
-		}
+	if err = InitSimple(ctx); err != nil {
+		return
+	}
 
-		// when git works with gnupg (commit signing), there should be a stable home for gnupg commands
-		if _, ok := os.LookupEnv("GNUPGHOME"); !ok {
-			_ = os.Setenv("GNUPGHOME", filepath.Join(HomeDir(), ".gnupg"))
-		}
+	// when git works with gnupg (commit signing), there should be a stable home for gnupg commands
+	if _, ok := os.LookupEnv("GNUPGHOME"); !ok {
+		_ = os.Setenv("GNUPGHOME", filepath.Join(HomeDir(), ".gnupg"))
+	}
 
-		// 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")
-		}
+	// 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")
-		}
+	// 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=")
-		}
+	// 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
+	SupportProcReceive = CheckGitVersionAtLeast("2.29") == nil
+
+	if setting.LFS.StartServer {
+		if CheckGitVersionAtLeast("2.1.2") != nil {
+			return errors.New("LFS server support requires Git >= 2.1.2")
+		}
+		globalCommandArgs = append(globalCommandArgs, "-c", "filter.lfs.required=", "-c", "filter.lfs.smudge=", "-c", "filter.lfs.clean=")
 	}
+
 	return syncGitConfig()
 }
 
diff --git a/modules/git/git_test.go b/modules/git/git_test.go
index c5a63de064..0915737878 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(gitHomePath)
 	setting.Git.HomePath = gitHomePath
 
-	if err = InitOnceWithSync(context.Background()); err != nil {
+	if err = InitFull(context.Background()); err != nil {
 		return fmt.Errorf("failed to call Init: %w", err)
 	}
 
diff --git a/modules/git/lfs.go b/modules/git/lfs.go
deleted file mode 100644
index c5d8354b6d..0000000000
--- a/modules/git/lfs.go
+++ /dev/null
@@ -1,31 +0,0 @@
-// Copyright 2021 The Gitea Authors. All rights reserved.
-// Use of this source code is governed by a MIT-style
-// license that can be found in the LICENSE file.
-
-package git
-
-import (
-	"sync"
-
-	logger "code.gitea.io/gitea/modules/log"
-	"code.gitea.io/gitea/modules/setting"
-)
-
-var once sync.Once
-
-// CheckLFSVersion will check lfs version, if not satisfied, then disable it.
-func CheckLFSVersion() {
-	if setting.LFS.StartServer {
-		// Disable LFS client hooks if installed for the current OS user
-		// Needs at least git v2.1.2
-		if CheckGitVersionAtLeast("2.1.2") != nil {
-			setting.LFS.StartServer = false
-			logger.Error("LFS server support needs at least Git v2.1.2")
-		} else {
-			once.Do(func() {
-				globalCommandArgs = append(globalCommandArgs, "-c", "filter.lfs.required=",
-					"-c", "filter.lfs.smudge=", "-c", "filter.lfs.clean=")
-			})
-		}
-	}
-}
diff --git a/routers/init.go b/routers/init.go
index e640ca4845..612fc5a83d 100644
--- a/routers/init.go
+++ b/routers/init.go
@@ -100,10 +100,8 @@ func GlobalInitInstalled(ctx context.Context) {
 		log.Fatal("Gitea is not installed")
 	}
 
-	mustInitCtx(ctx, git.InitOnceWithSync)
+	mustInitCtx(ctx, git.InitFull)
 	log.Info("Git Version: %s (home: %s)", git.VersionInfo(), git.HomeDir())
-
-	git.CheckLFSVersion()
 	log.Info("AppPath: %s", setting.AppPath)
 	log.Info("AppWorkPath: %s", setting.AppWorkPath)
 	log.Info("Custom path: %s", setting.CustomPath)