diff --git a/modules/git/gitcmd/command.go b/modules/git/gitcmd/command.go index 35454603ac..f430fd4bf2 100644 --- a/modules/git/gitcmd/command.go +++ b/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 diff --git a/services/repository/files/cherry_pick.go b/services/repository/files/cherry_pick.go index b866f936c2..ceaf8f2b44 100644 --- a/services/repository/files/cherry_pick.go +++ b/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) } diff --git a/services/repository/files/patch.go b/services/repository/files/patch.go index e791e4859c..f50cef0c86 100644 --- a/services/repository/files/patch.go +++ b/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 } diff --git a/services/repository/files/patch_test.go b/services/repository/files/patch_test.go new file mode 100644 index 0000000000..b60eb791e9 --- /dev/null +++ b/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) +} diff --git a/services/repository/files/update.go b/services/repository/files/update.go index 5221203e58..159b63f103 100644 --- a/services/repository/files/update.go +++ b/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