From ffc08c1914fbe6a5a5ebe9c8571b790ac6024d71 Mon Sep 17 00:00:00 2001 From: zeripath Date: Thu, 23 Dec 2021 13:44:00 +0000 Subject: [PATCH] Do not read or write git reference files directly (#18079) Git will and can pack references into packfiles and therefore if you write/read the files directly you will get false results. Instead you should use update-ref and show-ref. To that end I have created three new functions in git/repo_commit.go that will do this correctly. Related #17191 Signed-off-by: Andrew Thornton --- modules/git/repo_commit_gogit.go | 10 ++++++++ modules/git/repo_commit_nogogit.go | 12 ++++++++++ modules/git/repo_compare.go | 33 --------------------------- modules/git/repo_compare_test.go | 15 ++++++------ routers/web/repo/pull.go | 4 ++-- services/migrations/gitea_uploader.go | 3 +-- 6 files changed, 32 insertions(+), 45 deletions(-) diff --git a/modules/git/repo_commit_gogit.go b/modules/git/repo_commit_gogit.go index f00b340d15..39be183f30 100644 --- a/modules/git/repo_commit_gogit.go +++ b/modules/git/repo_commit_gogit.go @@ -31,6 +31,16 @@ func (repo *Repository) GetRefCommitID(name string) (string, error) { return ref.Hash().String(), nil } +// SetReference sets the commit ID string of given reference (e.g. branch or tag). +func (repo *Repository) SetReference(name, commitID string) error { + return repo.gogitRepo.Storer.SetReference(plumbing.NewReferenceFromStrings(name, commitID)) +} + +// RemoveReference removes the given reference (e.g. branch or tag). +func (repo *Repository) RemoveReference(name string) error { + return repo.gogitRepo.Storer.RemoveReference(plumbing.ReferenceName(name)) +} + // ConvertToSHA1 returns a Hash object from a potential ID string func (repo *Repository) ConvertToSHA1(commitID string) (SHA1, error) { if len(commitID) == 40 { diff --git a/modules/git/repo_commit_nogogit.go b/modules/git/repo_commit_nogogit.go index c8cd7ec882..4c1670742c 100644 --- a/modules/git/repo_commit_nogogit.go +++ b/modules/git/repo_commit_nogogit.go @@ -49,6 +49,18 @@ func (repo *Repository) GetRefCommitID(name string) (string, error) { return string(shaBs), nil } +// SetReference sets the commit ID string of given reference (e.g. branch or tag). +func (repo *Repository) SetReference(name, commitID string) error { + _, err := NewCommandContext(repo.Ctx, "update-ref", name, commitID).RunInDir(repo.Path) + return err +} + +// RemoveReference removes the given reference (e.g. branch or tag). +func (repo *Repository) RemoveReference(name string) error { + _, err := NewCommandContext(repo.Ctx, "update-ref", "--no-deref", "-d", name).RunInDir(repo.Path) + return err +} + // IsCommitExist returns true if given commit exists in current repository. func (repo *Repository) IsCommitExist(name string) bool { _, err := NewCommandContext(repo.Ctx, "cat-file", "-e", name).RunInDir(repo.Path) diff --git a/modules/git/repo_compare.go b/modules/git/repo_compare.go index 3690b74532..fb0ba91265 100644 --- a/modules/git/repo_compare.go +++ b/modules/git/repo_compare.go @@ -11,7 +11,6 @@ import ( "errors" "fmt" "io" - "io/ioutil" "os" "path/filepath" "regexp" @@ -275,25 +274,6 @@ func (repo *Repository) GetDiffFromMergeBase(base, head string, w io.Writer) err return err } -// ReadPullHead will fetch a pull ref if possible or return an error -func (repo *Repository) ReadPullHead(prID int64) (commitSHA string, err error) { - headPath := fmt.Sprintf("refs/pull/%d/head", prID) - fullHeadPath := filepath.Join(repo.Path, headPath) - loadHead, err := os.Open(fullHeadPath) - if err != nil { - return "", err - } - defer loadHead.Close() - // Read only the first line of the patch - usually it contains the first commit made in patch - scanner := bufio.NewScanner(loadHead) - scanner.Scan() - commitHead := scanner.Text() - if len(commitHead) != 40 { - return "", errors.New("head file doesn't contain valid commit ID") - } - return commitHead, nil -} - // ReadPatchCommit will check if a diff patch exists and return stats func (repo *Repository) ReadPatchCommit(prID int64) (commitSHA string, err error) { // Migrated repositories download patches to "pulls" location @@ -315,16 +295,3 @@ func (repo *Repository) ReadPatchCommit(prID int64) (commitSHA string, err error } return commitSHA, nil } - -// WritePullHead will populate a PR head retrieved from patch file -func (repo *Repository) WritePullHead(prID int64, commitSHA string) error { - headPath := fmt.Sprintf("refs/pull/%d", prID) - fullHeadPath := filepath.Join(repo.Path, headPath) - // Create missing directory just in case - if err := os.MkdirAll(fullHeadPath, os.ModePerm); err != nil { - return err - } - commitBytes := []byte(commitSHA) - pullPath := filepath.Join(fullHeadPath, "head") - return ioutil.WriteFile(pullPath, commitBytes, os.ModePerm) -} diff --git a/modules/git/repo_compare_test.go b/modules/git/repo_compare_test.go index 4790170d10..301e085aae 100644 --- a/modules/git/repo_compare_test.go +++ b/modules/git/repo_compare_test.go @@ -8,7 +8,6 @@ import ( "bytes" "io" "path/filepath" - "strings" "testing" "code.gitea.io/gitea/modules/util" @@ -63,18 +62,18 @@ func TestReadWritePullHead(t *testing.T) { assert.NoError(t, err) defer repo.Close() // Try to open non-existing Pull - _, err = repo.ReadPullHead(0) + _, err = repo.GetRefCommitID(PullPrefix + "0/head") assert.Error(t, err) // Write a fake sha1 with only 40 zeros - newCommit := strings.Repeat("0", 40) - err = repo.WritePullHead(1, newCommit) + newCommit := "feaf4ba6bc635fec442f46ddd4512416ec43c2c2" + err = repo.SetReference(PullPrefix+"1/head", newCommit) assert.NoError(t, err) - headFile := filepath.Join(repo.Path, "refs/pull/1/head") // Remove file after the test - defer util.Remove(headFile) - assert.FileExists(t, headFile) + defer func() { + _ = repo.RemoveReference(PullPrefix + "1/head") + }() // Read the file created - headContents, err := repo.ReadPullHead(1) + headContents, err := repo.GetRefCommitID(PullPrefix + "1/head") assert.NoError(t, err) assert.Len(t, string(headContents), 40) assert.True(t, string(headContents) == newCommit) diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index ba775d761b..1a6a5e55e6 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -325,13 +325,13 @@ func PrepareMergedViewPullInfo(ctx *context.Context, issue *models.Issue) *git.C if pull.MergeBase == "" { var commitSHA, parentCommit string // If there is a head or a patch file, and it is readable, grab info - commitSHA, err := ctx.Repo.GitRepo.ReadPullHead(pull.Index) + commitSHA, err := ctx.Repo.GitRepo.GetRefCommitID(pull.GetGitRefName()) if err != nil { // Head File does not exist, try the patch commitSHA, err = ctx.Repo.GitRepo.ReadPatchCommit(pull.Index) if err == nil { // Recreate pull head in files for next time - if err := ctx.Repo.GitRepo.WritePullHead(pull.Index, commitSHA); err != nil { + if err := ctx.Repo.GitRepo.SetReference(pull.GetGitRefName(), commitSHA); err != nil { log.Error("Could not write head file", err) } } else { diff --git a/services/migrations/gitea_uploader.go b/services/migrations/gitea_uploader.go index 33ca3127fb..79225d75a0 100644 --- a/services/migrations/gitea_uploader.go +++ b/services/migrations/gitea_uploader.go @@ -698,8 +698,7 @@ func (g *GiteaLocalUploader) newPullRequest(pr *base.PullRequest) (*models.PullR if pr.Head.SHA != "" { // Git update-ref remove bad references with a relative path log.Warn("Deprecated local head, removing : %v", pr.Head.SHA) - relPath := pr.GetGitRefName() - _, err = git.NewCommand("update-ref", "--no-deref", "-d", relPath).RunInDir(g.repo.RepoPath()) + err = g.gitRepo.RemoveReference(pr.GetGitRefName()) } else { // The SHA is empty, remove the head file log.Warn("Empty reference, removing : %v", pullHead)