From b8270240bf53cdf4ed3267a395347272cf48b176 Mon Sep 17 00:00:00 2001 From: Mihir Joshi Date: Tue, 16 Jan 2024 20:36:51 +0530 Subject: [PATCH] Fix reverting a merge commit failing (#28794) Fixes #22236 --- Error occurring currently while trying to revert commit using read-tree -m approach: > 2022/12/26 16:04:43 ...rvices/pull/patch.go:240:AttemptThreeWayMerge() [E] [63a9c61a] Unable to run read-tree -m! Error: exit status 128 - fatal: this operation must be run in a work tree > - fatal: this operation must be run in a work tree We need to clone a non-bare repository for `git read-tree -m` to work. https://github.com/go-gitea/gitea/commit/bb371aee6ecf5e570cdf7b5f7f0d6f47a607a325 adds support to create a non-bare cloned temporary upload repository. After cloning a non-bare temporary upload repository, we [set default index](https://github.com/go-gitea/gitea/blob/main/services/repository/files/cherry_pick.go#L37) (`git read-tree HEAD`). This operation ends up resetting the git index file (see investigation details below), due to which, we need to call `git update-index --refresh` afterward. Here's the diff of the index file before and after we execute SetDefaultIndex: https://www.diffchecker.com/hyOP3eJy/ Notice the **ctime**, **mtime** are set to 0 after SetDefaultIndex. You can reproduce the same behavior using these steps: ```bash $ git clone https://try.gitea.io/me-heer/test.git -s -b main $ cd test $ git read-tree HEAD $ git read-tree -m 1f085d7ed8 1f085d7ed8 9933caed00 error: Entry '1' not uptodate. Cannot merge. ``` After which, we can fix like this: ``` $ git update-index --refresh $ git read-tree -m 1f085d7ed8 1f085d7ed8 9933caed00 ``` --- models/fixtures/repo_unit.yml | 7 ++++ models/fixtures/repository.yml | 13 +++++++ models/fixtures/user.yml | 2 +- services/packages/cargo/index.go | 2 +- services/repository/files/cherry_pick.go | 5 ++- services/repository/files/diff.go | 2 +- services/repository/files/patch.go | 2 +- services/repository/files/temp_repo.go | 17 +++++++-- services/repository/files/update.go | 2 +- services/repository/files/upload.go | 2 +- .../user2/test_commit_revert.git/HEAD | 1 + .../user2/test_commit_revert.git/config | 8 +++++ .../user2/test_commit_revert.git/description | 1 + .../user2/test_commit_revert.git/info/exclude | 6 ++++ ...200c8e6707636a6cc3e0d8101fba08b19dcb91.idx | Bin 0 -> 1268 bytes ...00c8e6707636a6cc3e0d8101fba08b19dcb91.pack | Bin 0 -> 609 bytes .../user2/test_commit_revert.git/packed-refs | 3 ++ .../repo_mergecommit_revert_test.go | 34 ++++++++++++++++++ 18 files changed, 98 insertions(+), 9 deletions(-) create mode 100644 tests/gitea-repositories-meta/user2/test_commit_revert.git/HEAD create mode 100644 tests/gitea-repositories-meta/user2/test_commit_revert.git/config create mode 100644 tests/gitea-repositories-meta/user2/test_commit_revert.git/description create mode 100644 tests/gitea-repositories-meta/user2/test_commit_revert.git/info/exclude create mode 100644 tests/gitea-repositories-meta/user2/test_commit_revert.git/objects/pack/pack-91200c8e6707636a6cc3e0d8101fba08b19dcb91.idx create mode 100644 tests/gitea-repositories-meta/user2/test_commit_revert.git/objects/pack/pack-91200c8e6707636a6cc3e0d8101fba08b19dcb91.pack create mode 100644 tests/gitea-repositories-meta/user2/test_commit_revert.git/packed-refs create mode 100644 tests/integration/repo_mergecommit_revert_test.go diff --git a/models/fixtures/repo_unit.yml b/models/fixtures/repo_unit.yml index 0104419550572..e6c59f527a4df 100644 --- a/models/fixtures/repo_unit.yml +++ b/models/fixtures/repo_unit.yml @@ -669,3 +669,10 @@ type: 10 config: "{}" created_unix: 946684810 + +- + id: 101 + repo_id: 59 + type: 1 + config: "{}" + created_unix: 946684810 diff --git a/models/fixtures/repository.yml b/models/fixtures/repository.yml index 373c1caa6257a..f4e837673541d 100644 --- a/models/fixtures/repository.yml +++ b/models/fixtures/repository.yml @@ -1693,3 +1693,16 @@ size: 0 is_fsck_enabled: true close_issues_via_commit_in_any_branch: false + +- + id: 59 + owner_id: 2 + owner_name: user2 + lower_name: test_commit_revert + name: test_commit_revert + default_branch: main + is_empty: false + is_archived: false + is_private: true + status: 0 + num_issues: 0 diff --git a/models/fixtures/user.yml b/models/fixtures/user.yml index fd513798167d4..79fbb981f647a 100644 --- a/models/fixtures/user.yml +++ b/models/fixtures/user.yml @@ -66,7 +66,7 @@ num_followers: 2 num_following: 1 num_stars: 2 - num_repos: 14 + num_repos: 15 num_teams: 0 num_members: 0 visibility: 0 diff --git a/services/packages/cargo/index.go b/services/packages/cargo/index.go index 9514e35bedd9a..e8a8313625dcb 100644 --- a/services/packages/cargo/index.go +++ b/services/packages/cargo/index.go @@ -267,7 +267,7 @@ func alterRepositoryContent(ctx context.Context, doer *user_model.User, repo *re defer t.Close() var lastCommitID string - if err := t.Clone(repo.DefaultBranch); err != nil { + if err := t.Clone(repo.DefaultBranch, true); err != nil { if !git.IsErrBranchNotExist(err) || !repo.IsEmpty { return err } diff --git a/services/repository/files/cherry_pick.go b/services/repository/files/cherry_pick.go index e88ea16119799..613b46d8f6629 100644 --- a/services/repository/files/cherry_pick.go +++ b/services/repository/files/cherry_pick.go @@ -31,12 +31,15 @@ func CherryPick(ctx context.Context, repo *repo_model.Repository, doer *user_mod log.Error("%v", err) } defer t.Close() - if err := t.Clone(opts.OldBranch); err != nil { + if err := t.Clone(opts.OldBranch, false); err != nil { return nil, err } if err := t.SetDefaultIndex(); err != nil { return nil, err } + if err := t.RefreshIndex(); err != nil { + return nil, err + } // Get the commit of the original branch commit, err := t.GetBranchCommit(opts.OldBranch) diff --git a/services/repository/files/diff.go b/services/repository/files/diff.go index 373249b114541..bf8b938e2178f 100644 --- a/services/repository/files/diff.go +++ b/services/repository/files/diff.go @@ -21,7 +21,7 @@ func GetDiffPreview(ctx context.Context, repo *repo_model.Repository, branch, tr return nil, err } defer t.Close() - if err := t.Clone(branch); err != nil { + if err := t.Clone(branch, true); err != nil { return nil, err } if err := t.SetDefaultIndex(); err != nil { diff --git a/services/repository/files/patch.go b/services/repository/files/patch.go index 96899e6e0e9f1..14f8caaa8cf73 100644 --- a/services/repository/files/patch.go +++ b/services/repository/files/patch.go @@ -113,7 +113,7 @@ func ApplyDiffPatch(ctx context.Context, repo *repo_model.Repository, doer *user log.Error("%v", err) } defer t.Close() - if err := t.Clone(opts.OldBranch); err != nil { + if err := t.Clone(opts.OldBranch, true); err != nil { return nil, err } if err := t.SetDefaultIndex(); err != nil { diff --git a/services/repository/files/temp_repo.go b/services/repository/files/temp_repo.go index 6a0b7b675c81e..9fcd335c55a6a 100644 --- a/services/repository/files/temp_repo.go +++ b/services/repository/files/temp_repo.go @@ -51,8 +51,13 @@ func (t *TemporaryUploadRepository) Close() { } // Clone the base repository to our path and set branch as the HEAD -func (t *TemporaryUploadRepository) Clone(branch string) error { - if _, _, err := git.NewCommand(t.ctx, "clone", "-s", "--bare", "-b").AddDynamicArguments(branch, t.repo.RepoPath(), t.basePath).RunStdString(nil); err != nil { +func (t *TemporaryUploadRepository) Clone(branch string, bare bool) error { + cmd := git.NewCommand(t.ctx, "clone", "-s", "-b").AddDynamicArguments(branch, t.repo.RepoPath(), t.basePath) + if bare { + cmd.AddArguments("--bare") + } + + if _, _, err := cmd.RunStdString(nil); err != nil { stderr := err.Error() if matched, _ := regexp.MatchString(".*Remote branch .* not found in upstream origin.*", stderr); matched { return git.ErrBranchNotExist{ @@ -97,6 +102,14 @@ func (t *TemporaryUploadRepository) SetDefaultIndex() error { return nil } +// RefreshIndex looks at the current index and checks to see if merges or updates are needed by checking stat() information. +func (t *TemporaryUploadRepository) RefreshIndex() error { + if _, _, err := git.NewCommand(t.ctx, "update-index", "--refresh").RunStdString(&git.RunOpts{Dir: t.basePath}); err != nil { + return fmt.Errorf("RefreshIndex: %w", err) + } + return nil +} + // LsFiles checks if the given filename arguments are in the index func (t *TemporaryUploadRepository) LsFiles(filenames ...string) ([]string, error) { stdOut := new(bytes.Buffer) diff --git a/services/repository/files/update.go b/services/repository/files/update.go index dd8d9ee42563d..1892043304e29 100644 --- a/services/repository/files/update.go +++ b/services/repository/files/update.go @@ -146,7 +146,7 @@ func ChangeRepoFiles(ctx context.Context, repo *repo_model.Repository, doer *use } defer t.Close() hasOldBranch := true - if err := t.Clone(opts.OldBranch); err != nil { + if err := t.Clone(opts.OldBranch, true); err != nil { for _, file := range opts.Files { if file.Operation == "delete" { return nil, err diff --git a/services/repository/files/upload.go b/services/repository/files/upload.go index 61e38b55a3705..cbfaf49d13176 100644 --- a/services/repository/files/upload.go +++ b/services/repository/files/upload.go @@ -87,7 +87,7 @@ func UploadRepoFiles(ctx context.Context, repo *repo_model.Repository, doer *use defer t.Close() hasOldBranch := true - if err = t.Clone(opts.OldBranch); err != nil { + if err = t.Clone(opts.OldBranch, true); err != nil { if !git.IsErrBranchNotExist(err) || !repo.IsEmpty { return err } diff --git a/tests/gitea-repositories-meta/user2/test_commit_revert.git/HEAD b/tests/gitea-repositories-meta/user2/test_commit_revert.git/HEAD new file mode 100644 index 0000000000000..b870d82622c1a --- /dev/null +++ b/tests/gitea-repositories-meta/user2/test_commit_revert.git/HEAD @@ -0,0 +1 @@ +ref: refs/heads/main diff --git a/tests/gitea-repositories-meta/user2/test_commit_revert.git/config b/tests/gitea-repositories-meta/user2/test_commit_revert.git/config new file mode 100644 index 0000000000000..57bbcba5beed0 --- /dev/null +++ b/tests/gitea-repositories-meta/user2/test_commit_revert.git/config @@ -0,0 +1,8 @@ +[core] + repositoryformatversion = 0 + filemode = true + bare = true + ignorecase = true + precomposeunicode = true +[remote "origin"] + url = https://try.gitea.io/me-heer/test_commit_revert.git diff --git a/tests/gitea-repositories-meta/user2/test_commit_revert.git/description b/tests/gitea-repositories-meta/user2/test_commit_revert.git/description new file mode 100644 index 0000000000000..498b267a8c781 --- /dev/null +++ b/tests/gitea-repositories-meta/user2/test_commit_revert.git/description @@ -0,0 +1 @@ +Unnamed repository; edit this file 'description' to name the repository. diff --git a/tests/gitea-repositories-meta/user2/test_commit_revert.git/info/exclude b/tests/gitea-repositories-meta/user2/test_commit_revert.git/info/exclude new file mode 100644 index 0000000000000..a5196d1be8fb5 --- /dev/null +++ b/tests/gitea-repositories-meta/user2/test_commit_revert.git/info/exclude @@ -0,0 +1,6 @@ +# git ls-files --others --exclude-from=.git/info/exclude +# Lines that start with '#' are comments. +# For a project mostly in C, the following would be a good set of +# exclude patterns (uncomment them if you want to use them): +# *.[oa] +# *~ diff --git a/tests/gitea-repositories-meta/user2/test_commit_revert.git/objects/pack/pack-91200c8e6707636a6cc3e0d8101fba08b19dcb91.idx b/tests/gitea-repositories-meta/user2/test_commit_revert.git/objects/pack/pack-91200c8e6707636a6cc3e0d8101fba08b19dcb91.idx new file mode 100644 index 0000000000000000000000000000000000000000..77bcbe7fb4caeaf783d375068e6ffad1a61ed6f9 GIT binary patch literal 1268 zcmexg;-AdGz`z8=v<8eo3&_PF_YD+g2HH(a%mOrj6c0=oumZz=pfMZJ4dh~WpkAtA ziT!>0sdIKX`Q+r7_O6Ur@4djIQr(9k=xzwdk9FE7To^R}r(d6a^sw8itW75x*G+gB zJLmjK_lrp}ojOnB{w=l+mP?F$x@Cip-r*TtF8d5W?KM9hE4Ed{`{q?8<@3(p#qS*Z z6?X6S>El699d>OwXZKwA@}K@YrklRMmHNzh>-92&^UoPf=J!P_8Ex$p5RAF4$7r)< z|M#umx9-w6^fNoBCjH=l=F9XikL7_y$T?t9-~gm0f!H62{{h2*2Ph^mQGur~ojo}# m=kS9Y0`j{!HqJdgaY~07L(uVgcZ<)l8XQVHe#P^wn?3-RA9Cve literal 0 HcmV?d00001 diff --git a/tests/gitea-repositories-meta/user2/test_commit_revert.git/objects/pack/pack-91200c8e6707636a6cc3e0d8101fba08b19dcb91.pack b/tests/gitea-repositories-meta/user2/test_commit_revert.git/objects/pack/pack-91200c8e6707636a6cc3e0d8101fba08b19dcb91.pack new file mode 100644 index 0000000000000000000000000000000000000000..7271cdaeb87741f0ed005bdae83367b7e3dd76ff GIT binary patch literal 609 zcmWG=boORoU|<4b_8F2DbLRH>`W-e9Y58t>baI49%d8{yD`ORpu-;JE=(f!`Pg%9F zAf9`#t*c0&blku9iJsbyWO}4;yFP z%7iHn>OIYzR$HvRT%NA^8pWv~9j*;B@8BN$p*G1 z_wCH-hqq<^duMh%m^c1IzWwt1r&vRF{EV|RS@eskb64Y;*G!GP6?0}E{4=r8>CO>5 zV>RZ(oyTRC@bZXm;caN>*50C^x=|%mQP)`gYQXv>i(0rD^doqK=5Ygq@QfqhAq5fF ziJvnG&>DY92Q5*&f8<>p`?>?xR*O$FPdH&E=56J^q+*+ic!g3mS89{@ zv53icL`*t=JX*?svmmBHQQ!W~yxN55@}tS&{iixseOrBH-khy&zZLku_@3Fj^;FM) z{zvmR>y);J{Ym@CsKRK~v6QJ|jfw5tr$LTYkrdPCAY9!vsuCoctS>5sd>}s!5 z&*$#%zHj+Wn}^}W1io*UAmv784~*?~gJPg+!^9h?kOkiYS{K0T~q5@A}I(u?f&fy0)1mt&dY@B;~A^`875