refactor

Author: wxiaoguang

modules/git/batch_command.go

@@ -21,7 +21,10 @@ type catFileBatchCommand struct {
 
 var _ CatFileBatch = (*catFileBatchCommand)(nil)
 
-func newBatchCommandCatFile(ctx context.Context, repoPath string) (*catFileBatchCommand, error) { //nolint:unparam // no error
+func newBatchCommandCatFile(ctx context.Context, repoPath string) (*catFileBatchCommand, error) {
+	if err := ensureValidGitRepository(ctx, repoPath); err != nil {
+		return nil, err
+	}
 	return &catFileBatchCommand{
 		ctx:      ctx,
 		repoPath: repoPath,

modules/git/batch_legacy.go

@@ -25,7 +25,6 @@ var _ CatFileBatch = (*catFileBatchLegacy)(nil)
 
 // newBatchCatFileWithCheck creates a new batch and a new batch check for the given repository, the Close must be invoked before release the batch
 func newBatchCatFileWithCheck(ctx context.Context, repoPath string) (*catFileBatchLegacy, error) {
-	// Now because of some insanity with git cat-file not immediately failing if not run in a valid git directory we need to run git rev-parse first!
 	if err := ensureValidGitRepository(ctx, repoPath); err != nil {
 		return nil, err
 	}

modules/git/batch_reader.go

@@ -43,7 +43,8 @@ func (b *catFileBatchCommunicator) Close() {
 // ensureValidGitRepository runs git rev-parse in the repository path - thus ensuring that the repository is a valid repository.
 // Run before opening git cat-file.
 // This is needed otherwise the git cat-file will hang for invalid repositories.
-// FIXME: the comment is from https://github.com/go-gitea/gitea/pull/17991 but it doesn't seem to be true, there must be some bugs in code, not git's problem
+// FIXME: the comment is from https://github.com/go-gitea/gitea/pull/17991 but it doesn't seem to be true.
+// The real problem is that Golang's Cmd.Wait hangs because it waits for the pipes to be closed, but we can't close the pipes before Wait returns
 func ensureValidGitRepository(ctx context.Context, repoPath string) error {
 	stderr := strings.Builder{}
 	err := gitcmd.NewCommand("rev-parse").

modules/git/batch_test.go

@@ -9,6 +9,7 @@ import (
 	"testing"
 
 	"code.gitea.io/gitea/modules/test"
+
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
@@ -22,6 +23,12 @@ func TestCatFileBatch(t *testing.T) {
 }
 
 func testCatFileBatch(t *testing.T) {
+	t.Run("CorruptedGitRepo", func(t *testing.T) {
+		tmpDir := t.TempDir()
+		_, err := NewBatch(t.Context(), tmpDir)
+		require.Error(t, err)
+	})
+
 	batch, err := NewBatch(t.Context(), filepath.Join(testReposDir, "repo1_bare"))
 	require.NoError(t, err)
 	defer batch.Close()

modules/git/repo_commit_nogogit.go

@@ -55,10 +55,6 @@ func (repo *Repository) GetRefCommitID(name string) (string, error) {
 
 // IsCommitExist returns true if given commit exists in current repository.
 func (repo *Repository) IsCommitExist(name string) bool {
-	if err := ensureValidGitRepository(repo.Ctx, repo.Path); err != nil {
-		log.Error("IsCommitExist: %v", err)
-		return false
-	}
 	_, _, err := gitcmd.NewCommand("cat-file", "-e").
 		AddDynamicArguments(name).
 		WithDir(repo.Path).