diff --git a/models/issues/comment.go b/models/issues/comment.go index 082ede1e199..a036b89d87c 100644 --- a/models/issues/comment.go +++ b/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 } diff --git a/modules/git/ref.go b/modules/git/ref.go index 56b2db858ad..232c4738287 100644 --- a/modules/git/ref.go +++ b/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 +} diff --git a/modules/git/repo_commit_nogogit.go b/modules/git/repo_commit_nogogit.go index 3ead3e22165..29650179f55 100644 --- a/modules/git/repo_commit_nogogit.go +++ b/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 { diff --git a/modules/git/repo_compare_test.go b/modules/git/repo_compare_test.go index 25ee4c51985..7bd6eab85d3 100644 --- a/modules/git/repo_compare_test.go +++ b/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) } diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index 64d2ad330c9..97c0c15302d 100644 --- a/routers/web/repo/pull.go +++ b/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 +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.GetGitHeadRefName()) + 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.GetGitHeadRefName(), 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 + } + } + return parentCommit +} - 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.GetGitHeadRefName()) - 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.GetGitHeadRefName(), 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 - } - } - baseCommit = parentCommit - } else { - // Keep an empty history or original commit - baseCommit = pull.MergeBase +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.GetGitHeadRefName(), false, false) diff --git a/services/issue/comments.go b/services/issue/comments.go index 63dd3171c9d..ba18099db7b 100644 --- a/services/issue/comments.go +++ b/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 diff --git a/services/issue/issue.go b/services/issue/issue.go index f03be3e18f6..9114cd27d33 100644 --- a/services/issue/issue.go +++ b/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(issue.PullRequest.GetGitHeadRefName()); err != nil { + if err := git.RemoveRef(ctx, gitRepo.Path, issue.PullRequest.GetGitHeadRefName()); 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}, diff --git a/services/pull/pull.go b/services/pull/pull.go index 2829e154410..76d5bdcb05d 100644 --- a/services/pull/pull.go +++ b/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.GetGitHeadRefName()); err1 != nil { + if err1 := git.RemoveRef(ctx, baseGitRepo.Path, pr.GetGitHeadRefName()); 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.GetGitHeadRefName(), pr.HeadCommitID).RunStdString(ctx, &git.RunOpts{Dir: pr.BaseRepo.RepoPath()}) + err = git.UpdateRef(ctx, pr.BaseRepo.RepoPath(), pr.GetGitHeadRefName(), pr.HeadCommitID) if err != nil { log.Error("Unable to update ref in base repository for PR[%d] Error: %v", pr.ID, err) } diff --git a/services/pull/review.go b/services/pull/review.go index 2586b81b535..9f03f60c376 100644 --- a/services/pull/review.go +++ b/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.GetGitHeadRefName(), 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 }) } diff --git a/services/user/delete.go b/services/user/delete.go index 39c6ef052dc..b8e51c550ff 100644 --- a/services/user/delete.go +++ b/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 } }