refactor: git patch apply (#38637)

Merge duplicate code and add a unit test

There are already integration test cases in `TestEditor` ->
`WebGitCommitEmail` for these two endpoints.

modules/git/gitcmd/command.go

@@ -38,11 +38,13 @@ type Command struct {
 	configArgs []string
 
 	// Dir is the working dir for the git command, however:
-	// FIXME: this could be incorrect in many cases, for example:
+	// FIXME: GIT-DIR-ARGUMENT: this could be incorrect in many cases, for example:
 	// * /some/path/.git
 	// * /some/path/.git/gitea-data/data/repositories/user/repo.git
 	// If "user/repo.git" is invalid/broken, then running git command in it will use "/some/path/.git", and produce unexpected results
 	// The correct approach is to use `--git-dir" global argument or "GIT_DIR=..." environment variable.
+	// Actually, when working with a bare repo, the current directory should not be the git dir,
+	// otherwise some git commands might overwrite git dir internal files by a repo file.
 	gitDir string
 
 	cmd *exec.Cmd

services/repository/files/cherry_pick.go

@@ -4,7 +4,6 @@
 package files
 
 import (
-	"context"
 	"errors"
 	"fmt"
 	"strings"
@@ -12,7 +11,7 @@ import (
 	repo_model "gitea.dev/models/repo"
 	user_model "gitea.dev/models/user"
 	"gitea.dev/modules/git"
-	"gitea.dev/modules/log"
+	"gitea.dev/modules/reqctx"
 	"gitea.dev/modules/structs"
 	"gitea.dev/services/pull"
 )
@@ -34,57 +33,23 @@ func (err ErrCommitIDDoesNotMatch) Error() string {
 }
 
 // CherryPick cherry-picks or reverts a commit to the given repository
-func CherryPick(ctx context.Context, repo *repo_model.Repository, doer *user_model.User, revert bool, opts *ApplyDiffPatchOptions) (*structs.FileResponse, error) {
-	gitRepo, closer, err := git.RepositoryFromContextOrOpen(ctx, repo)
+func CherryPick(ctx reqctx.RequestContext, repo *repo_model.Repository, doer *user_model.User, revert bool, opts *ApplyDiffPatchOptions) (*structs.FileResponse, error) {
+	gitRepo, err := git.RepositoryFromRequestContextOrOpen(ctx, repo)
 	if err != nil {
 		return nil, err
 	}
-	defer closer.Close()
-
-	if err := opts.Validate(ctx, repo, gitRepo, doer); err != nil {
-		return nil, err
-	}
-	message := strings.TrimSpace(opts.Message)
-
-	t, err := NewTemporaryUploadRepository(repo)
+	t, err := gitPatchPrepare(ctx, repo, gitRepo, doer, opts)
 	if err != nil {
-		log.Error("NewTemporaryUploadRepository failed: %v", err)
+		return nil, err
 	}
 	defer t.Close()
-	if err := t.Clone(ctx, opts.OldBranch, false); err != nil {
-		return nil, err
-	}
-	if err := t.SetDefaultIndex(ctx); err != nil {
-		return nil, err
-	}
-	if err := t.RefreshIndex(ctx); err != nil {
-		return nil, err
-	}
 
-	// Get the commit of the original branch
-	commit, err := t.GetBranchCommit(ctx, opts.OldBranch)
+	err = t.RefreshIndex(ctx)
 	if err != nil {
-		return nil, err // Couldn't get a commit for the branch
+		return nil, err
 	}
 
-	// Assigned LastCommitID in opts if it hasn't been set
-	if opts.LastCommitID == "" {
-		opts.LastCommitID = commit.ID.String()
-	} else {
-		lastCommitID, err := t.gitRepo.ConvertToGitID(ctx, opts.LastCommitID)
-		if err != nil {
-			return nil, fmt.Errorf("CherryPick: Invalid last commit ID: %w", err)
-		}
-		opts.LastCommitID = lastCommitID.String()
-		if commit.ID.String() != opts.LastCommitID {
-			return nil, ErrCommitIDDoesNotMatch{
-				GivenCommitID:   opts.LastCommitID,
-				CurrentCommitID: opts.LastCommitID,
-			}
-		}
-	}
-
-	commit, err = t.GetCommit(ctx, strings.TrimSpace(opts.Content))
+	commit, err := t.GetCommit(ctx, strings.TrimSpace(opts.Content))
 	if err != nil {
 		return nil, err
 	}
@@ -100,8 +65,7 @@ func CherryPick(ctx context.Context, repo *repo_model.Repository, doer *user_mod
 	}
 
 	description := fmt.Sprintf("CherryPick %s onto %s", right, opts.OldBranch)
-	conflict, _, err := pull.AttemptThreeWayMerge(ctx,
-		t.basePath, t.gitRepo, base, opts.LastCommitID, right, description)
+	conflict, _, err := pull.AttemptThreeWayMerge(ctx, t.basePath, t.gitRepo, base, opts.LastCommitID, right, description)
 	if err != nil {
 		return nil, fmt.Errorf("failed to three-way merge %s onto %s: %w", right, opts.OldBranch, err)
 	}
@@ -110,48 +74,5 @@ func CherryPick(ctx context.Context, repo *repo_model.Repository, doer *user_mod
 		return nil, errors.New("failed to merge due to conflicts")
 	}
 
-	treeHash, err := t.WriteTree(ctx)
-	if err != nil {
-		// likely non-sensical tree due to merge conflicts...
-		return nil, err
-	}
-
-	// Now commit the tree
-	commitOpts := &CommitTreeUserOptions{
-		ParentCommitID:    "HEAD",
-		TreeHash:          treeHash,
-		CommitMessage:     message,
-		SignOff:           opts.Signoff,
-		DoerUser:          doer,
-		AuthorIdentity:    opts.Author,
-		AuthorTime:        nil,
-		CommitterIdentity: opts.Committer,
-		CommitterTime:     nil,
-	}
-	if opts.Dates != nil {
-		commitOpts.AuthorTime, commitOpts.CommitterTime = &opts.Dates.Author, &opts.Dates.Committer
-	}
-	commitHash, err := t.CommitTree(ctx, commitOpts)
-	if err != nil {
-		return nil, err
-	}
-
-	// Then push this tree to NewBranch
-	if err := t.Push(ctx, doer, commitHash, opts.NewBranch, false); err != nil {
-		return nil, err
-	}
-
-	commit, err = t.GetCommit(ctx, commitHash)
-	if err != nil {
-		return nil, err
-	}
-
-	fileCommitResponse, _ := GetFileCommitResponse(ctx, repo, gitRepo, commit) // ok if fails, then will be nil
-	verification := GetPayloadCommitVerification(ctx, commit)
-	fileResponse := &structs.FileResponse{
-		Commit:       fileCommitResponse,
-		Verification: verification,
-	}
-
-	return fileResponse, nil
+	return gitPatchCommitPush(ctx, t, repo, gitRepo, doer, opts)
 }

services/repository/files/patch.go

@@ -13,7 +13,7 @@ import (
 	user_model "gitea.dev/models/user"
 	"gitea.dev/modules/git"
 	"gitea.dev/modules/git/gitcmd"
-	"gitea.dev/modules/log"
+	"gitea.dev/modules/reqctx"
 	"gitea.dev/modules/structs"
 	"gitea.dev/modules/util"
 	asymkey_service "gitea.dev/services/asymkey"
@@ -109,31 +109,27 @@ func (opts *ApplyDiffPatchOptions) Validate(ctx context.Context, repo *repo_mode
 	return nil
 }
 
-// ApplyDiffPatch applies a patch to the given repository
-func ApplyDiffPatch(ctx context.Context, repo *repo_model.Repository, doer *user_model.User, opts *ApplyDiffPatchOptions) (*structs.FileResponse, error) {
+func gitPatchPrepare(ctx context.Context, repo *repo_model.Repository, gitRepo *git.Repository, doer *user_model.User, opts *ApplyDiffPatchOptions) (_ *TemporaryUploadRepository, retErr error) {
 	err := repo.MustNotBeArchived()
 	if err != nil {
 		return nil, err
 	}
 
-	gitRepo, closer, err := git.RepositoryFromContextOrOpen(ctx, repo)
-	if err != nil {
-		return nil, err
-	}
-	defer closer.Close()
-
 	if err := opts.Validate(ctx, repo, gitRepo, doer); err != nil {
 		return nil, err
 	}
 
-	message := strings.TrimSpace(opts.Message)
-
 	t, err := NewTemporaryUploadRepository(repo)
 	if err != nil {
-		log.Error("NewTemporaryUploadRepository failed: %v", err)
+		return nil, fmt.Errorf("NewTemporaryUploadRepository failed: %w", err)
 	}
-	defer t.Close()
-	if err := t.Clone(ctx, opts.OldBranch, true); err != nil {
+	defer func() {
+		if retErr != nil {
+			t.Close()
+		}
+	}()
+	// here must NOT use bare repo, because the following git commands might operate working tree ("--index") directly
+	if err := t.Clone(ctx, opts.OldBranch, false); err != nil {
 		return nil, err
 	}
 	if err := t.SetDefaultIndex(ctx); err != nil {
@@ -152,7 +148,7 @@ func ApplyDiffPatch(ctx context.Context, repo *repo_model.Repository, doer *user
 	} else {
 		lastCommitID, err := t.gitRepo.ConvertToGitID(ctx, opts.LastCommitID)
 		if err != nil {
-			return nil, fmt.Errorf("ApplyPatch: Invalid last commit ID: %w", err)
+			return nil, fmt.Errorf("invalid last commit ID: %w", err)
 		}
 		opts.LastCommitID = lastCommitID.String()
 		if commit.ID.String() != opts.LastCommitID {
@@ -162,6 +158,20 @@ func ApplyDiffPatch(ctx context.Context, repo *repo_model.Repository, doer *user
 			}
 		}
 	}
+	return t, nil
+}
+
+// ApplyDiffPatch applies a patch to the given repository
+func ApplyDiffPatch(ctx reqctx.RequestContext, repo *repo_model.Repository, doer *user_model.User, opts *ApplyDiffPatchOptions) (*structs.FileResponse, error) {
+	gitRepo, err := git.RepositoryFromRequestContextOrOpen(ctx, repo)
+	if err != nil {
+		return nil, err
+	}
+	t, err := gitPatchPrepare(ctx, repo, gitRepo, doer, opts)
+	if err != nil {
+		return nil, err
+	}
+	defer t.Close()
 
 	cmdApply := gitcmd.NewCommand("apply", "--index", "--recount", "--cached", "--ignore-whitespace", "--whitespace=fix", "--binary")
 	if git.DefaultFeatures().CheckVersionAtLeast("2.32") {
@@ -174,17 +184,19 @@ func ApplyDiffPatch(ctx context.Context, repo *repo_model.Repository, doer *user
 		return nil, fmt.Errorf("git apply error: %w", err)
 	}
 
-	// Now write the tree
+	return gitPatchCommitPush(ctx, t, repo, gitRepo, doer, opts)
+}
+
+func gitPatchCommitPush(ctx context.Context, t *TemporaryUploadRepository, repo *repo_model.Repository, gitRepo *git.Repository, doer *user_model.User, opts *ApplyDiffPatchOptions) (*structs.FileResponse, error) {
 	treeHash, err := t.WriteTree(ctx)
 	if err != nil {
 		return nil, err
 	}
 
-	// Now commit the tree
 	commitOpts := &CommitTreeUserOptions{
 		ParentCommitID:    "HEAD",
 		TreeHash:          treeHash,
-		CommitMessage:     message,
+		CommitMessage:     strings.TrimSpace(opts.Message),
 		SignOff:           opts.Signoff,
 		DoerUser:          doer,
 		AuthorIdentity:    opts.Author,
@@ -205,7 +217,7 @@ func ApplyDiffPatch(ctx context.Context, repo *repo_model.Repository, doer *user
 		return nil, err
 	}
 
-	commit, err = t.GetCommit(ctx, commitHash)
+	commit, err := t.GetCommit(ctx, commitHash)
 	if err != nil {
 		return nil, err
 	}

services/repository/files/patch_test.go

@@ -0,0 +1,38 @@
+// Copyright 2026 The Gitea Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package files
+
+import (
+	"os"
+	"path/filepath"
+	"testing"
+
+	repo_model "gitea.dev/models/repo"
+	"gitea.dev/models/unittest"
+	user_model "gitea.dev/models/user"
+	"gitea.dev/modules/git"
+
+	"github.com/stretchr/testify/require"
+)
+
+func TestGitPatchPrepare(t *testing.T) {
+	unittest.PrepareTestEnv(t)
+	user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
+	repo1 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1})
+
+	gitRepo, err := git.OpenRepository(repo1.CodeStorageRepo())
+	require.NoError(t, err)
+	defer gitRepo.Close()
+
+	opts := &ApplyDiffPatchOptions{}
+	tmpRepo, err := gitPatchPrepare(t.Context(), repo1, gitRepo, user2, opts)
+	require.NoError(t, err)
+	defer tmpRepo.Close()
+	// Temporary repository for patch should not be a bare repo because "--index" argument is used in some cases:
+	// * repo files might be written into current working tree when "applying the patch" then overwrite bare repo's git dir internal files.
+	// * see also "FIXME: GIT-DIR-ARGUMENT"
+	// While it is also questionable whether "--index" argument should really be used, need to figure out in the future.
+	_, err = os.Stat(filepath.Join(tmpRepo.basePath, ".git"))
+	require.NoError(t, err)
+}

services/repository/files/update.go

@@ -174,7 +174,7 @@ func ChangeRepoFiles(ctx context.Context, repo *repo_model.Repository, doer *use
 
 	t, err := NewTemporaryUploadRepository(repo)
 	if err != nil {
-		log.Error("NewTemporaryUploadRepository failed: %v", err)
+		return nil, fmt.Errorf("NewTemporaryUploadRepository failed: %w", err)
 	}
 	defer t.Close()
 	hasOldBranch := true