Refactor calc mergebase

Author: lunny

models/issues/comment.go

@@ -764,6 +764,10 @@ func (c *Comment) CodeCommentLink(ctx context.Context) string {
 	return fmt.Sprintf("%s/files#%s", c.Issue.Link(), c.HashTag())
 }
 
+func GetCodeCommentRef(prIndex, commentID int64) string {
+	return fmt.Sprintf("refs/pull/%d/code-comment-%d", prIndex, commentID)
+}
+
 // CreateComment creates comment with context
 func CreateComment(ctx context.Context, opts *CreateCommentOptions) (_ *Comment, err error) {
 	ctx, committer, err := db.TxContext(ctx)
@@ -1151,6 +1155,15 @@ func DeleteComment(ctx context.Context, comment *Comment) error {
 		return err
 	}
 
+	// delete review if the comment is the last comment of the review
+	if comment.ReviewID > 0 {
+		if _, err := db.GetEngine(ctx).ID(comment.ReviewID).
+			Where("(SELECT count(id) FROM comment WHERE review_id = ?) == 0", comment.ReviewID).
+			Delete(new(Review)); err != nil {
+			return err
+		}
+	}
+
 	if err := comment.neuterCrossReferences(ctx); err != nil {
 		return err
 	}

modules/git/ref.go

@@ -4,6 +4,7 @@
 package git
 
 import (
+	"context"
 	"regexp"
 	"strings"
 
@@ -220,3 +221,14 @@ func (ref RefName) RefWebLinkPath() string {
 	}
 	return string(refType) + "/" + util.PathEscapeSegments(ref.ShortName())
 }
+
+func UpdateRef(ctx context.Context, repoPath, refName, newCommitID string) error {
+	_, _, err := NewCommand("update-ref").AddDynamicArguments(refName, newCommitID).RunStdString(ctx, &RunOpts{Dir: repoPath})
+	return err
+}
+
+func RemoveRef(ctx context.Context, repoPath, refName string) error {
+	_, _, err := NewCommand("update-ref", "--no-deref", "-d").
+		AddDynamicArguments(refName).RunStdString(ctx, &RunOpts{Dir: repoPath})
+	return err
+}

modules/git/repo_commit_nogogit.go

@@ -50,18 +50,6 @@ 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 := NewCommand("update-ref").AddDynamicArguments(name, commitID).RunStdString(repo.Ctx, &RunOpts{Dir: repo.Path})
-	return err
-}
-
-// RemoveReference removes the given reference (e.g. branch or tag).
-func (repo *Repository) RemoveReference(name string) error {
-	_, _, err := NewCommand("update-ref", "--no-deref", "-d").AddDynamicArguments(name).RunStdString(repo.Ctx, &RunOpts{Dir: repo.Path})
-	return err
-}
-
 // 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 {

modules/git/repo_compare_test.go

@@ -99,7 +99,7 @@ func TestReadWritePullHead(t *testing.T) {
 
 	// Write a fake sha1 with only 40 zeros
 	newCommit := "feaf4ba6bc635fec442f46ddd4512416ec43c2c2"
-	err = repo.SetReference(PullPrefix+"1/head", newCommit)
+	err = UpdateRef(t.Context(), repo.Path, PullPrefix+"1/head", newCommit)
 	if err != nil {
 		assert.NoError(t, err)
 		return
@@ -116,7 +116,7 @@ func TestReadWritePullHead(t *testing.T) {
 	assert.Equal(t, headContents, newCommit)
 
 	// Remove file after the test
-	err = repo.RemoveReference(PullPrefix + "1/head")
+	err = RemoveRef(t.Context(), repo.Path, PullPrefix+"1/head")
 	assert.NoError(t, err)
 }
 

routers/web/repo/pull.go

@@ -190,7 +190,7 @@ func GetPullDiffStats(ctx *context.Context) {
 	}
 	pull := issue.PullRequest
 
-	mergeBaseCommitID := GetMergedBaseCommitID(ctx, issue)
+	mergeBaseCommitID := GetMergedBaseCommitID(ctx, pull)
 	if mergeBaseCommitID == "" {
 		return // no merge base, do nothing, do not stop the route handler, see below
 	}
@@ -210,48 +210,46 @@ func GetPullDiffStats(ctx *context.Context) {
 	ctx.Data["DiffShortStat"] = diffShortStat
 }
 
-func GetMergedBaseCommitID(ctx *context.Context, issue *issues_model.Issue) string {
-	pull := issue.PullRequest
-
-	var baseCommit string
-	// Some migrated PR won't have any Base SHA and lose history, try to get one
-	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.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.SetReference(pull.GetGitRefName(), commitSHA); err != nil {
-					log.Error("Could not write head file", err)
-				}
-			} else {
-				// There is no history available
-				log.Trace("No history file available for PR %d", pull.Index)
+func calcMergeBase(ctx *context.Context, pull *issues_model.PullRequest) string {
+	var commitSHA, parentCommit string
+	// If there is a head or a patch file, and it is readable, grab info
+	commitSHA, err := ctx.Repo.GitRepo.GetRefCommitID(pull.GetGitRefName())
+	if err != nil {
+		// Head File does not exist, try the patch
+		// FIXME: it seems this patch file is not used in the code, but it is still read
+		commitSHA, err = ctx.Repo.GitRepo.ReadPatchCommit(pull.Index)
+		if err == nil {
+			// Recreate pull head in files for next time
+			if err := git.UpdateRef(ctx, ctx.Repo.GitRepo.Path, pull.GetGitRefName(), commitSHA); err != nil {
+				log.Error("Could not write head file", err)
 			}
+		} else {
+			// There is no history available
+			log.Trace("No history file available for PR %d", pull.Index)
 		}
-		if commitSHA != "" {
-			// Get immediate parent of the first commit in the patch, grab history back
-			parentCommit, _, err = git.NewCommand("rev-list", "-1", "--skip=1").AddDynamicArguments(commitSHA).RunStdString(ctx, &git.RunOpts{Dir: ctx.Repo.GitRepo.Path})
-			if err == nil {
-				parentCommit = strings.TrimSpace(parentCommit)
-			}
-			// Special case on Git < 2.25 that doesn't fail on immediate empty history
-			if err != nil || parentCommit == "" {
-				log.Info("No known parent commit for PR %d, error: %v", pull.Index, err)
-				// bring at least partial history if it can work
-				parentCommit = commitSHA
-			}
+	}
+	if commitSHA != "" {
+		// Get immediate parent of the first commit in the patch, grab history back
+		parentCommit, _, err = git.NewCommand("rev-list", "-1", "--skip=1").AddDynamicArguments(commitSHA).RunStdString(ctx, &git.RunOpts{Dir: ctx.Repo.GitRepo.Path})
+		if err == nil {
+			parentCommit = strings.TrimSpace(parentCommit)
 		}
-		baseCommit = parentCommit
-	} else {
-		// Keep an empty history or original commit
-		baseCommit = pull.MergeBase
+		// Special case on Git < 2.25 that doesn't fail on immediate empty history
+		if err != nil || parentCommit == "" {
+			log.Info("No known parent commit for PR %d, error: %v", pull.Index, err)
+			// bring at least partial history if it can work
+			parentCommit = commitSHA
+		}
+	}
+	return parentCommit
+}
+
+func GetMergedBaseCommitID(ctx *context.Context, pull *issues_model.PullRequest) string {
+	if pull.MergeBase != "" {
+		return pull.MergeBase
 	}
 
-	return baseCommit
+	return calcMergeBase(ctx, pull)
 }
 
 func preparePullViewPullInfo(ctx *context.Context, issue *issues_model.Issue) *git.CompareInfo {
@@ -271,7 +269,13 @@ func prepareMergedViewPullInfo(ctx *context.Context, issue *issues_model.Issue)
 	setMergeTarget(ctx, pull)
 	ctx.Data["HasMerged"] = true
 
-	baseCommit := GetMergedBaseCommitID(ctx, issue)
+	baseCommit := GetMergedBaseCommitID(ctx, pull)
+	if baseCommit == "" {
+		ctx.Data["BaseTarget"] = pull.BaseBranch
+		ctx.Data["NumCommits"] = 0
+		ctx.Data["NumFiles"] = 0
+		return nil // no merge base, do nothing
+	}
 
 	compareInfo, err := ctx.Repo.GitRepo.GetCompareInfo(ctx.Repo.Repository.RepoPath(),
 		baseCommit, pull.GetGitRefName(), false, false)

services/issue/comments.go

@@ -13,8 +13,11 @@ import (
 	access_model "code.gitea.io/gitea/models/perm/access"
 	repo_model "code.gitea.io/gitea/models/repo"
 	user_model "code.gitea.io/gitea/models/user"
+	"code.gitea.io/gitea/modules/git"
 	"code.gitea.io/gitea/modules/gitrepo"
 	"code.gitea.io/gitea/modules/json"
+	"code.gitea.io/gitea/modules/log"
+	"code.gitea.io/gitea/modules/storage"
 	"code.gitea.io/gitea/modules/timeutil"
 	git_service "code.gitea.io/gitea/services/git"
 	notify_service "code.gitea.io/gitea/services/notify"
@@ -133,9 +136,47 @@ func UpdateComment(ctx context.Context, c *issues_model.Comment, contentVersion
 // DeleteComment deletes the comment
 func DeleteComment(ctx context.Context, doer *user_model.User, comment *issues_model.Comment) error {
 	err := db.WithTx(ctx, func(ctx context.Context) error {
-		return issues_model.DeleteComment(ctx, comment)
-		// TODO: delete review if the comment is the last comment of the review
-		// TODO: delete comment attachments
+		if err := comment.LoadAttachments(ctx); err != nil {
+			return err
+		}
+
+		if err := issues_model.DeleteComment(ctx, comment); err != nil {
+			return err
+		}
+
+		// delete comment attachments
+		if _, err := repo_model.DeleteAttachments(ctx, comment.Attachments, true); err != nil {
+			return fmt.Errorf("delete attachments: %w", err)
+		}
+
+		if comment.ReviewID > 0 {
+			if err := comment.LoadIssue(ctx); err != nil {
+				return err
+			}
+			if err := comment.Issue.LoadRepo(ctx); err != nil {
+				return err
+			}
+			if err := comment.Issue.LoadPullRequest(ctx); err != nil {
+				return err
+			}
+			if err := git.RemoveRef(ctx, comment.Issue.Repo.RepoPath(), issues_model.GetCodeCommentRef(comment.Issue.PullRequest.Index, comment.ID)); err != nil {
+				log.Error("Unable to remove ref in base repository for PR[%d] Error: %v", comment.Issue.PullRequest.ID, err)
+				// We should not return error here, because the comment has been removed from database.
+				// users have to delete this ref manually or we should have a synchronize between
+				// database comment table and git refs.
+			}
+		}
+
+		for _, attachment := range comment.Attachments {
+			if err := storage.Attachments.Delete(repo_model.AttachmentRelativePath(attachment.UUID)); err != nil {
+				// Even delete files failed, but the attachments has been removed from database, so we
+				// should not return error but only record the error on logs.
+				// users have to delete this attachments manually or we should have a
+				// synchronize between database attachment table and attachment storage
+				log.Error("delete attachment[uuid: %s] failed: %v", attachment.UUID, err)
+			}
+		}
+		return nil
 	})
 	if err != nil {
 		return err

services/issue/issue.go

@@ -200,7 +200,7 @@ func DeleteIssue(ctx context.Context, doer *user_model.User, gitRepo *git.Reposi
 
 	// delete pull request related git data
 	if issue.IsPull && gitRepo != nil {
-		if err := gitRepo.RemoveReference(fmt.Sprintf("%s%d/head", git.PullPrefix, issue.PullRequest.Index)); err != nil {
+		if err := git.RemoveRef(ctx, gitRepo.Path, issue.PullRequest.GetGitRefName()); err != nil {
 			return err
 		}
 	}
@@ -301,6 +301,8 @@ func deleteIssue(ctx context.Context, issue *issues_model.Issue) ([]string, erro
 		attachmentPaths = append(attachmentPaths, issue.Attachments[i].RelativePath())
 	}
 
+	// TODO: deference all review comments
+
 	// delete all database data still assigned to this issue
 	if err := db.DeleteBeans(ctx,
 		&issues_model.ContentHistory{IssueID: issue.ID},

services/pull/pull.go

@@ -184,7 +184,7 @@ func NewPullRequest(ctx context.Context, opts *NewPullRequestOptions) error {
 		return nil
 	}); err != nil {
 		// cleanup: this will only remove the reference, the real commit will be clean up when next GC
-		if err1 := baseGitRepo.RemoveReference(pr.GetGitRefName()); err1 != nil {
+		if err1 := git.RemoveRef(ctx, baseGitRepo.Path, pr.GetGitRefName()); err1 != nil {
 			log.Error("RemoveReference: %v", err1)
 		}
 		return err
@@ -648,7 +648,7 @@ func UpdateRef(ctx context.Context, pr *issues_model.PullRequest) (err error) {
 		return err
 	}
 
-	_, _, err = git.NewCommand("update-ref").AddDynamicArguments(pr.GetGitRefName(), pr.HeadCommitID).RunStdString(ctx, &git.RunOpts{Dir: pr.BaseRepo.RepoPath()})
+	err = git.UpdateRef(ctx, pr.BaseRepo.RepoPath(), pr.GetGitRefName(), pr.HeadCommitID)
 	if err != nil {
 		log.Error("Unable to update ref in base repository for PR[%d] Error: %v", pr.ID, err)
 	}

services/pull/review.go

@@ -21,6 +21,7 @@ import (
 	"code.gitea.io/gitea/modules/cache"
 	"code.gitea.io/gitea/modules/git"
 	"code.gitea.io/gitea/modules/gitrepo"
+	"code.gitea.io/gitea/modules/log"
 	"code.gitea.io/gitea/modules/optional"
 	"code.gitea.io/gitea/modules/setting"
 	"code.gitea.io/gitea/modules/util"
@@ -141,11 +142,8 @@ func CreateCodeComment(ctx context.Context, doer *user_model.User, gitRepo *git.
 		if err != nil {
 			return nil, fmt.Errorf("GetRefCommitID[%s]: %w", issue.PullRequest.GetGitRefName(), err)
 		}
-	} else {
-		// afterCommitID must be one of the pull request commits
-		if !slices.Contains(prCommitIDs, afterCommitID) {
-			return nil, fmt.Errorf("afterCommitID[%s] is not a valid pull request commit", afterCommitID)
-		}
+	} else if !slices.Contains(prCommitIDs, afterCommitID) { // afterCommitID must be one of the pull request commits
+		return nil, fmt.Errorf("afterCommitID[%s] is not a valid pull request commit", afterCommitID)
 	}
 
 	// CreateCodeComment() is used for:
@@ -279,22 +277,33 @@ func createCodeComment(ctx context.Context, doer *user_model.User, repo *repo_mo
 	}
 
 	lineCommitID := util.Iif(line < 0, beforeCommitID, afterCommitID)
-	// TODO: the commit ID Must be referenced in the git repository, because the branch maybe rebased or force-pushed.
+	return db.WithTx2(ctx, func(ctx context.Context) (*issues_model.Comment, error) {
+		comment, err := issues_model.CreateComment(ctx, &issues_model.CreateCommentOptions{
+			Type:        issues_model.CommentTypeCode,
+			Doer:        doer,
+			Repo:        repo,
+			Issue:       issue,
+			Content:     content,
+			LineNum:     line,
+			TreePath:    treePath,
+			CommitSHA:   lineCommitID,
+			ReviewID:    reviewID,
+			Patch:       patch,
+			Invalidated: false,
+			Attachments: attachments,
+		})
+		if err != nil {
+			return nil, err
+		}
 
-	// If the commit ID is not referenced, it cannot be calculated the position dynamically.
-	return issues_model.CreateComment(ctx, &issues_model.CreateCommentOptions{
-		Type:        issues_model.CommentTypeCode,
-		Doer:        doer,
-		Repo:        repo,
-		Issue:       issue,
-		Content:     content,
-		LineNum:     line,
-		TreePath:    treePath,
-		CommitSHA:   lineCommitID,
-		ReviewID:    reviewID,
-		Patch:       patch,
-		Invalidated: false,
-		Attachments: attachments,
+		// The line commit ID Must be referenced in the git repository, because the branch maybe rebased or force-pushed.
+		// If the review commit is GC, the position will not be calculated dynamically.
+		if err := git.UpdateRef(ctx, pr.BaseRepo.RepoPath(), issues_model.GetCodeCommentRef(pr.Index, comment.ID), lineCommitID); err != nil {
+			log.Error("Unable to update ref in base repository for PR[%d] Error: %v", pr.ID, err)
+			return nil, err
+		}
+
+		return comment, nil
 	})
 }
 

services/user/delete.go

@@ -120,6 +120,7 @@ func deleteUser(ctx context.Context, u *user_model.User, purge bool) (err error)
 				if err = issues_model.DeleteComment(ctx, comment); err != nil {
 					return err
 				}
+				// TODO: delete attachments
 			}
 		}