Remove legacy git code (ver < 2.0), fine tune markup tests (#19930)

* clean git support for ver < 2.0

* fine tune tests for markup (which requires git module)

* remove unnecessary comments

* try to fix tests

* try test again

* use const for GitVersionRequired instead of var

* try to fix integration test

* Refactor CheckAttributeReader to make a *git.Repository version

* update document for commit signing with Gitea's internal gitconfig

* update document for commit signing with Gitea's internal gitconfig

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

modules/git/commit.go

@@ -206,26 +206,17 @@ func (c *Commit) HasPreviousCommit(commitHash SHA1) (bool, error) {
 		return false, nil
 	}
 
-	if err := CheckGitVersionAtLeast("1.8"); err == nil {
-		_, _, err := NewCommand(c.repo.Ctx, "merge-base", "--is-ancestor", that, this).RunStdString(&RunOpts{Dir: c.repo.Path})
-		if err == nil {
-			return true, nil
-		}
-		var exitError *exec.ExitError
-		if errors.As(err, &exitError) {
-			if exitError.ProcessState.ExitCode() == 1 && len(exitError.Stderr) == 0 {
-				return false, nil
-			}
-		}
-		return false, err
+	_, _, err := NewCommand(c.repo.Ctx, "merge-base", "--is-ancestor", that, this).RunStdString(&RunOpts{Dir: c.repo.Path})
+	if err == nil {
+		return true, nil
 	}
-
-	result, _, err := NewCommand(c.repo.Ctx, "rev-list", "--ancestry-path", "-n1", that+".."+this, "--").RunStdString(&RunOpts{Dir: c.repo.Path})
-	if err != nil {
-		return false, err
+	var exitError *exec.ExitError
+	if errors.As(err, &exitError) {
+		if exitError.ProcessState.ExitCode() == 1 && len(exitError.Stderr) == 0 {
+			return false, nil
+		}
 	}
-
-	return len(strings.TrimSpace(result)) > 0, nil
+	return false, err
 }
 
 // CommitsBeforeLimit returns num commits before current revision

modules/git/git.go

@@ -7,10 +7,10 @@ package git
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"os"
 	"os/exec"
-	"path/filepath"
 	"runtime"
 	"strings"
 	"sync"
@@ -22,20 +22,16 @@ import (
 	"github.com/hashicorp/go-version"
 )
 
-var (
-	// GitVersionRequired is the minimum Git version required
-	// At the moment, all code for git 1.x are not changed, if some users want to test with old git client
-	// or bypass the check, they still have a chance to edit this variable manually.
-	// If everything works fine, the code for git 1.x could be removed in a separate PR before 1.17 frozen.
-	GitVersionRequired = "2.0.0"
+// GitVersionRequired is the minimum Git version required
+const GitVersionRequired = "2.0.0"
 
+var (
 	// GitExecutable is the command name of git
 	// Could be updated to an absolute path while initialization
 	GitExecutable = "git"
 
-	// DefaultContext is the default context to run git commands in
-	// will be overwritten by InitXxx with HammerContext
-	DefaultContext = context.Background()
+	// DefaultContext is the default context to run git commands in, must be initialized by git.InitXxx
+	DefaultContext context.Context
 
 	// SupportProcReceive version >= 2.29.0
 	SupportProcReceive bool
@@ -128,36 +124,43 @@ func VersionInfo() string {
 	return fmt.Sprintf(format, args...)
 }
 
+func checkInit() error {
+	if setting.RepoRootPath == "" {
+		return errors.New("can not init Git's HomeDir (RepoRootPath is empty), the setting and git modules are not initialized correctly")
+	}
+	if DefaultContext != nil {
+		log.Warn("git module has been initialized already, duplicate init should be fixed")
+	}
+	return nil
+}
+
 // HomeDir is the home dir for git to store the global config file used by Gitea internally
 func HomeDir() string {
 	if setting.RepoRootPath == "" {
-		// TODO: now, some unit test code call the git module directly without initialization, which is incorrect.
-		// at the moment, we just use a temp HomeDir to prevent from conflicting with user's git config
-		// in the future, the git module should be initialized first before use.
-		tmpHomeDir := filepath.Join(os.TempDir(), "gitea-temp-home")
-		log.Error("Git's HomeDir is empty (RepoRootPath is empty), the git module is not initialized correctly, using a temp HomeDir (%s) temporarily", tmpHomeDir)
-		return tmpHomeDir
+		// 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.
+		// 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("can not get Git's HomeDir (RepoRootPath is empty), the setting and git modules are not initialized correctly")
+		return ""
 	}
 	return setting.RepoRootPath
 }
 
 // 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
 func InitSimple(ctx context.Context) error {
+	if err := checkInit(); err != nil {
+		return err
+	}
+
 	DefaultContext = ctx
 
 	if setting.Git.Timeout.Default > 0 {
 		defaultCommandExecutionTimeout = time.Duration(setting.Git.Timeout.Default) * time.Second
 	}
 
-	if err := SetExecutablePath(setting.Git.Path); err != nil {
-		return err
-	}
-
-	// force cleanup args
-	globalCommandArgs = []string{}
-
-	return nil
+	return SetExecutablePath(setting.Git.Path)
 }
 
 var initOnce sync.Once
@@ -166,6 +169,10 @@ var initOnce sync.Once
 // 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) {
+	if err = checkInit(); err != nil {
+		return err
+	}
+
 	initOnce.Do(func() {
 		err = InitSimple(ctx)
 		if err != nil {

modules/git/repo_attribute.go

@@ -30,10 +30,10 @@ type CheckAttributeOpts struct {
 func (repo *Repository) CheckAttribute(opts CheckAttributeOpts) (map[string]map[string]string, error) {
 	env := []string{}
 
-	if len(opts.IndexFile) > 0 && CheckGitVersionAtLeast("1.7.8") == nil {
+	if len(opts.IndexFile) > 0 {
 		env = append(env, "GIT_INDEX_FILE="+opts.IndexFile)
 	}
-	if len(opts.WorkTree) > 0 && CheckGitVersionAtLeast("1.7.8") == nil {
+	if len(opts.WorkTree) > 0 {
 		env = append(env, "GIT_WORK_TREE="+opts.WorkTree)
 	}
 
@@ -56,8 +56,7 @@ func (repo *Repository) CheckAttribute(opts CheckAttributeOpts) (map[string]map[
 		}
 	}
 
-	// git check-attr --cached first appears in git 1.7.8
-	if opts.CachedOnly && CheckGitVersionAtLeast("1.7.8") == nil {
+	if opts.CachedOnly {
 		cmdArgs = append(cmdArgs, "--cached")
 	}
 
@@ -125,12 +124,12 @@ type CheckAttributeReader struct {
 func (c *CheckAttributeReader) Init(ctx context.Context) error {
 	cmdArgs := []string{"check-attr", "--stdin", "-z"}
 
-	if len(c.IndexFile) > 0 && CheckGitVersionAtLeast("1.7.8") == nil {
+	if len(c.IndexFile) > 0 {
 		cmdArgs = append(cmdArgs, "--cached")
 		c.env = append(c.env, "GIT_INDEX_FILE="+c.IndexFile)
 	}
 
-	if len(c.WorkTree) > 0 && CheckGitVersionAtLeast("1.7.8") == nil {
+	if len(c.WorkTree) > 0 {
 		c.env = append(c.env, "GIT_WORK_TREE="+c.WorkTree)
 	}
 
@@ -160,17 +159,10 @@ func (c *CheckAttributeReader) Init(ctx context.Context) error {
 		return err
 	}
 
-	if CheckGitVersionAtLeast("1.8.5") == nil {
-		lw := new(nulSeparatedAttributeWriter)
-		lw.attributes = make(chan attributeTriple, 5)
-		lw.closed = make(chan struct{})
-		c.stdOut = lw
-	} else {
-		lw := new(lineSeparatedAttributeWriter)
-		lw.attributes = make(chan attributeTriple, 5)
-		lw.closed = make(chan struct{})
-		c.stdOut = lw
-	}
+	lw := new(nulSeparatedAttributeWriter)
+	lw.attributes = make(chan attributeTriple, 5)
+	lw.closed = make(chan struct{})
+	c.stdOut = lw
 	return nil
 }
 
@@ -400,3 +392,37 @@ func (wr *lineSeparatedAttributeWriter) Close() error {
 	close(wr.closed)
 	return nil
 }
+
+// Create a check attribute reader for the current repository and provided commit ID
+func (repo *Repository) CheckAttributeReader(commitID string) (*CheckAttributeReader, context.CancelFunc) {
+	indexFilename, worktree, deleteTemporaryFile, err := repo.ReadTreeToTemporaryIndex(commitID)
+	if err != nil {
+		return nil, func() {}
+	}
+
+	checker := &CheckAttributeReader{
+		Attributes: []string{"linguist-vendored", "linguist-generated", "linguist-language", "gitlab-language"},
+		Repo:       repo,
+		IndexFile:  indexFilename,
+		WorkTree:   worktree,
+	}
+	ctx, cancel := context.WithCancel(repo.Ctx)
+	if err := checker.Init(ctx); err != nil {
+		log.Error("Unable to open checker for %s. Error: %v", commitID, err)
+	} else {
+		go func() {
+			err := checker.Run()
+			if err != nil && err != ctx.Err() {
+				log.Error("Unable to open checker for %s. Error: %v", commitID, err)
+			}
+			cancel()
+		}()
+	}
+	deferable := func() {
+		_ = checker.Close()
+		cancel()
+		deleteTemporaryFile()
+	}
+
+	return checker, deferable
+}

modules/git/repo_compare.go

@@ -255,13 +255,7 @@ func (repo *Repository) GetDiff(base, head string, w io.Writer) error {
 
 // GetDiffBinary generates and returns patch data between given revisions, including binary diffs.
 func (repo *Repository) GetDiffBinary(base, head string, w io.Writer) error {
-	if CheckGitVersionAtLeast("1.7.7") == nil {
-		return NewCommand(repo.Ctx, "diff", "-p", "--binary", "--histogram", base, head).Run(&RunOpts{
-			Dir:    repo.Path,
-			Stdout: w,
-		})
-	}
-	return NewCommand(repo.Ctx, "diff", "-p", "--binary", "--patience", base, head).Run(&RunOpts{
+	return NewCommand(repo.Ctx, "diff", "-p", "--binary", "--histogram", base, head).Run(&RunOpts{
 		Dir:    repo.Path,
 		Stdout: w,
 	})

modules/git/repo_language_stats_gogit.go

@@ -8,12 +8,10 @@ package git
 
 import (
 	"bytes"
-	"context"
 	"io"
 	"strings"
 
 	"code.gitea.io/gitea/modules/analyze"
-	"code.gitea.io/gitea/modules/log"
 
 	"github.com/go-enry/go-enry/v2"
 	"github.com/go-git/go-git/v5"
@@ -43,33 +41,8 @@ func (repo *Repository) GetLanguageStats(commitID string) (map[string]int64, err
 		return nil, err
 	}
 
-	var checker *CheckAttributeReader
-
-	if CheckGitVersionAtLeast("1.7.8") == nil {
-		indexFilename, workTree, deleteTemporaryFile, err := repo.ReadTreeToTemporaryIndex(commitID)
-		if err == nil {
-			defer deleteTemporaryFile()
-			checker = &CheckAttributeReader{
-				Attributes: []string{"linguist-vendored", "linguist-generated", "linguist-language", "gitlab-language"},
-				Repo:       repo,
-				IndexFile:  indexFilename,
-				WorkTree:   workTree,
-			}
-			ctx, cancel := context.WithCancel(DefaultContext)
-			if err := checker.Init(ctx); err != nil {
-				log.Error("Unable to open checker for %s. Error: %v", commitID, err)
-			} else {
-				go func() {
-					err = checker.Run()
-					if err != nil {
-						log.Error("Unable to open checker for %s. Error: %v", commitID, err)
-						cancel()
-					}
-				}()
-			}
-			defer cancel()
-		}
-	}
+	checker, deferable := repo.CheckAttributeReader(commitID)
+	defer deferable()
 
 	sizes := make(map[string]int64)
 	err = tree.Files().ForEach(func(f *object.File) error {

modules/git/repo_language_stats_nogogit.go

@@ -9,7 +9,6 @@ package git
 import (
 	"bufio"
 	"bytes"
-	"context"
 	"io"
 	"math"
 	"strings"
@@ -63,36 +62,8 @@ func (repo *Repository) GetLanguageStats(commitID string) (map[string]int64, err
 		return nil, err
 	}
 
-	var checker *CheckAttributeReader
-
-	if CheckGitVersionAtLeast("1.7.8") == nil {
-		indexFilename, worktree, deleteTemporaryFile, err := repo.ReadTreeToTemporaryIndex(commitID)
-		if err == nil {
-			defer deleteTemporaryFile()
-			checker = &CheckAttributeReader{
-				Attributes: []string{"linguist-vendored", "linguist-generated", "linguist-language", "gitlab-language"},
-				Repo:       repo,
-				IndexFile:  indexFilename,
-				WorkTree:   worktree,
-			}
-			ctx, cancel := context.WithCancel(repo.Ctx)
-			if err := checker.Init(ctx); err != nil {
-				log.Error("Unable to open checker for %s. Error: %v", commitID, err)
-			} else {
-				go func() {
-					err = checker.Run()
-					if err != nil {
-						log.Error("Unable to open checker for %s. Error: %v", commitID, err)
-						cancel()
-					}
-				}()
-			}
-			defer func() {
-				_ = checker.Close()
-				cancel()
-			}()
-		}
-	}
+	checker, deferable := repo.CheckAttributeReader(commitID)
+	defer deferable()
 
 	contentBuf := bytes.Buffer{}
 	var content []byte

modules/git/repo_tree.go

@@ -45,11 +45,11 @@ func (repo *Repository) CommitTree(author, committer *Signature, tree *Tree, opt
 	_, _ = messageBytes.WriteString(opts.Message)
 	_, _ = messageBytes.WriteString("\n")
 
-	if CheckGitVersionAtLeast("1.7.9") == nil && (opts.KeyID != "" || opts.AlwaysSign) {
+	if opts.KeyID != "" || opts.AlwaysSign {
 		cmd.AddArguments(fmt.Sprintf("-S%s", opts.KeyID))
 	}
 
-	if CheckGitVersionAtLeast("2.0.0") == nil && opts.NoGPGSign {
+	if opts.NoGPGSign {
 		cmd.AddArguments("--no-gpg-sign")
 	}