Skip to content

Commit dfc55f5

Browse files
lunnylunny
committed
Refactor calc mergebase
1 parent a56d1e5 commit dfc55f5

File tree

10 files changed

+149
-79
lines changed

10 files changed

+149
-79
lines changed

‎models/issues/comment.go‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -764,6 +764,10 @@ func (c *Comment) CodeCommentLink(ctx context.Context) string {
764764
return fmt.Sprintf("%s/files#%s", c.Issue.Link(), c.HashTag())
765765
}
766766

767+
func GetCodeCommentRef(prIndex, commentID int64) string {
768+
return fmt.Sprintf("refs/pull/%d/code-comment-%d", prIndex, commentID)
769+
}
770+
767771
// CreateComment creates comment with context
768772
func CreateComment(ctx context.Context, opts *CreateCommentOptions) (_ *Comment, err error) {
769773
ctx, committer, err := db.TxContext(ctx)
@@ -1151,6 +1155,15 @@ func DeleteComment(ctx context.Context, comment *Comment) error {
11511155
return err
11521156
}
11531157

1158+
// delete review if the comment is the last comment of the review
1159+
if comment.ReviewID > 0 {
1160+
if _, err := db.GetEngine(ctx).ID(comment.ReviewID).
1161+
Where("(SELECT count(id) FROM comment WHERE review_id = ?) == 0", comment.ReviewID).
1162+
Delete(new(Review)); err != nil {
1163+
return err
1164+
}
1165+
}
1166+
11541167
if err := comment.neuterCrossReferences(ctx); err != nil {
11551168
return err
11561169
}

‎modules/git/ref.go‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
package git
55

66
import (
7+
"context"
78
"regexp"
89
"strings"
910

@@ -220,3 +221,14 @@ func (ref RefName) RefWebLinkPath() string {
220221
}
221222
return string(refType) + "/" + util.PathEscapeSegments(ref.ShortName())
222223
}
224+
225+
func UpdateRef(ctx context.Context, repoPath, refName, newCommitID string) error {
226+
_, _, err := NewCommand("update-ref").AddDynamicArguments(refName, newCommitID).RunStdString(ctx, &RunOpts{Dir: repoPath})
227+
return err
228+
}
229+
230+
func RemoveRef(ctx context.Context, repoPath, refName string) error {
231+
_, _, err := NewCommand("update-ref", "--no-deref", "-d").
232+
AddDynamicArguments(refName).RunStdString(ctx, &RunOpts{Dir: repoPath})
233+
return err
234+
}

‎modules/git/repo_commit_nogogit.go‎

Lines changed: 0 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -50,18 +50,6 @@ func (repo *Repository) GetRefCommitID(name string) (string, error) {
5050
return string(shaBs), nil
5151
}
5252

53-
// SetReference sets the commit ID string of given reference (e.g. branch or tag).
54-
func (repo *Repository) SetReference(name, commitID string) error {
55-
_, _, err := NewCommand("update-ref").AddDynamicArguments(name, commitID).RunStdString(repo.Ctx, &RunOpts{Dir: repo.Path})
56-
return err
57-
}
58-
59-
// RemoveReference removes the given reference (e.g. branch or tag).
60-
func (repo *Repository) RemoveReference(name string) error {
61-
_, _, err := NewCommand("update-ref", "--no-deref", "-d").AddDynamicArguments(name).RunStdString(repo.Ctx, &RunOpts{Dir: repo.Path})
62-
return err
63-
}
64-
6553
// IsCommitExist returns true if given commit exists in current repository.
6654
func (repo *Repository) IsCommitExist(name string) bool {
6755
if err := ensureValidGitRepository(repo.Ctx, repo.Path); err != nil {

‎modules/git/repo_compare_test.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -99,7 +99,7 @@ func TestReadWritePullHead(t *testing.T) {
9999

100100
// Write a fake sha1 with only 40 zeros
101101
newCommit := "feaf4ba6bc635fec442f46ddd4512416ec43c2c2"
102-
err = repo.SetReference(PullPrefix+"1/head", newCommit)
102+
err = UpdateRef(t.Context(), repo.Path, PullPrefix+"1/head", newCommit)
103103
if err != nil {
104104
assert.NoError(t, err)
105105
return
@@ -116,7 +116,7 @@ func TestReadWritePullHead(t *testing.T) {
116116
assert.Equal(t, headContents, newCommit)
117117

118118
// Remove file after the test
119-
err = repo.RemoveReference(PullPrefix + "1/head")
119+
err = RemoveRef(t.Context(), repo.Path, PullPrefix+"1/head")
120120
assert.NoError(t, err)
121121
}
122122

‎routers/web/repo/pull.go‎

Lines changed: 43 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -190,7 +190,7 @@ func GetPullDiffStats(ctx *context.Context) {
190190
}
191191
pull := issue.PullRequest
192192

193-
mergeBaseCommitID := GetMergedBaseCommitID(ctx, issue)
193+
mergeBaseCommitID := GetMergedBaseCommitID(ctx, pull)
194194
if mergeBaseCommitID == "" {
195195
return // no merge base, do nothing, do not stop the route handler, see below
196196
}
@@ -210,48 +210,46 @@ func GetPullDiffStats(ctx *context.Context) {
210210
ctx.Data["DiffShortStat"] = diffShortStat
211211
}
212212

213-
func GetMergedBaseCommitID(ctx *context.Context, issue *issues_model.Issue) string {
214-
pull := issue.PullRequest
215-
216-
var baseCommit string
217-
// Some migrated PR won't have any Base SHA and lose history, try to get one
218-
if pull.MergeBase == "" {
219-
var commitSHA, parentCommit string
220-
// If there is a head or a patch file, and it is readable, grab info
221-
commitSHA, err := ctx.Repo.GitRepo.GetRefCommitID(pull.GetGitRefName())
222-
if err != nil {
223-
// Head File does not exist, try the patch
224-
commitSHA, err = ctx.Repo.GitRepo.ReadPatchCommit(pull.Index)
225-
if err == nil {
226-
// Recreate pull head in files for next time
227-
if err := ctx.Repo.GitRepo.SetReference(pull.GetGitRefName(), commitSHA); err != nil {
228-
log.Error("Could not write head file", err)
229-
}
230-
} else {
231-
// There is no history available
232-
log.Trace("No history file available for PR %d", pull.Index)
213+
func calcMergeBase(ctx *context.Context, pull *issues_model.PullRequest) string {
214+
var commitSHA, parentCommit string
215+
// If there is a head or a patch file, and it is readable, grab info
216+
commitSHA, err := ctx.Repo.GitRepo.GetRefCommitID(pull.GetGitRefName())
217+
if err != nil {
218+
// Head File does not exist, try the patch
219+
// FIXME: it seems this patch file is not used in the code, but it is still read
220+
commitSHA, err = ctx.Repo.GitRepo.ReadPatchCommit(pull.Index)
221+
if err == nil {
222+
// Recreate pull head in files for next time
223+
if err := git.UpdateRef(ctx, ctx.Repo.GitRepo.Path, pull.GetGitRefName(), commitSHA); err != nil {
224+
log.Error("Could not write head file", err)
233225
}
226+
} else {
227+
// There is no history available
228+
log.Trace("No history file available for PR %d", pull.Index)
234229
}
235-
if commitSHA != "" {
236-
// Get immediate parent of the first commit in the patch, grab history back
237-
parentCommit, _, err = git.NewCommand("rev-list", "-1", "--skip=1").AddDynamicArguments(commitSHA).RunStdString(ctx, &git.RunOpts{Dir: ctx.Repo.GitRepo.Path})
238-
if err == nil {
239-
parentCommit = strings.TrimSpace(parentCommit)
240-
}
241-
// Special case on Git < 2.25 that doesn't fail on immediate empty history
242-
if err != nil || parentCommit == "" {
243-
log.Info("No known parent commit for PR %d, error: %v", pull.Index, err)
244-
// bring at least partial history if it can work
245-
parentCommit = commitSHA
246-
}
230+
}
231+
if commitSHA != "" {
232+
// Get immediate parent of the first commit in the patch, grab history back
233+
parentCommit, _, err = git.NewCommand("rev-list", "-1", "--skip=1").AddDynamicArguments(commitSHA).RunStdString(ctx, &git.RunOpts{Dir: ctx.Repo.GitRepo.Path})
234+
if err == nil {
235+
parentCommit = strings.TrimSpace(parentCommit)
247236
}
248-
baseCommit = parentCommit
249-
} else {
250-
// Keep an empty history or original commit
251-
baseCommit = pull.MergeBase
237+
// Special case on Git < 2.25 that doesn't fail on immediate empty history
238+
if err != nil || parentCommit == "" {
239+
log.Info("No known parent commit for PR %d, error: %v", pull.Index, err)
240+
// bring at least partial history if it can work
241+
parentCommit = commitSHA
242+
}
243+
}
244+
return parentCommit
245+
}
246+
247+
func GetMergedBaseCommitID(ctx *context.Context, pull *issues_model.PullRequest) string {
248+
if pull.MergeBase != "" {
249+
return pull.MergeBase
252250
}
253251

254-
return baseCommit
252+
return calcMergeBase(ctx, pull)
255253
}
256254

257255
func preparePullViewPullInfo(ctx *context.Context, issue *issues_model.Issue) *git.CompareInfo {
@@ -271,7 +269,13 @@ func prepareMergedViewPullInfo(ctx *context.Context, issue *issues_model.Issue)
271269
setMergeTarget(ctx, pull)
272270
ctx.Data["HasMerged"] = true
273271

274-
baseCommit := GetMergedBaseCommitID(ctx, issue)
272+
baseCommit := GetMergedBaseCommitID(ctx, pull)
273+
if baseCommit == "" {
274+
ctx.Data["BaseTarget"] = pull.BaseBranch
275+
ctx.Data["NumCommits"] = 0
276+
ctx.Data["NumFiles"] = 0
277+
return nil // no merge base, do nothing
278+
}
275279

276280
compareInfo, err := ctx.Repo.GitRepo.GetCompareInfo(ctx.Repo.Repository.RepoPath(),
277281
baseCommit, pull.GetGitRefName(), false, false)

‎services/issue/comments.go‎

Lines changed: 44 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -13,8 +13,11 @@ import (
1313
access_model "code.gitea.io/gitea/models/perm/access"
1414
repo_model "code.gitea.io/gitea/models/repo"
1515
user_model "code.gitea.io/gitea/models/user"
16+
"code.gitea.io/gitea/modules/git"
1617
"code.gitea.io/gitea/modules/gitrepo"
1718
"code.gitea.io/gitea/modules/json"
19+
"code.gitea.io/gitea/modules/log"
20+
"code.gitea.io/gitea/modules/storage"
1821
"code.gitea.io/gitea/modules/timeutil"
1922
git_service "code.gitea.io/gitea/services/git"
2023
notify_service "code.gitea.io/gitea/services/notify"
@@ -133,9 +136,47 @@ func UpdateComment(ctx context.Context, c *issues_model.Comment, contentVersion
133136
// DeleteComment deletes the comment
134137
func DeleteComment(ctx context.Context, doer *user_model.User, comment *issues_model.Comment) error {
135138
err := db.WithTx(ctx, func(ctx context.Context) error {
136-
return issues_model.DeleteComment(ctx, comment)
137-
// TODO: delete review if the comment is the last comment of the review
138-
// TODO: delete comment attachments
139+
if err := comment.LoadAttachments(ctx); err != nil {
140+
return err
141+
}
142+
143+
if err := issues_model.DeleteComment(ctx, comment); err != nil {
144+
return err
145+
}
146+
147+
// delete comment attachments
148+
if _, err := repo_model.DeleteAttachments(ctx, comment.Attachments, true); err != nil {
149+
return fmt.Errorf("delete attachments: %w", err)
150+
}
151+
152+
if comment.ReviewID > 0 {
153+
if err := comment.LoadIssue(ctx); err != nil {
154+
return err
155+
}
156+
if err := comment.Issue.LoadRepo(ctx); err != nil {
157+
return err
158+
}
159+
if err := comment.Issue.LoadPullRequest(ctx); err != nil {
160+
return err
161+
}
162+
if err := git.RemoveRef(ctx, comment.Issue.Repo.RepoPath(), issues_model.GetCodeCommentRef(comment.Issue.PullRequest.Index, comment.ID)); err != nil {
163+
log.Error("Unable to remove ref in base repository for PR[%d] Error: %v", comment.Issue.PullRequest.ID, err)
164+
// We should not return error here, because the comment has been removed from database.
165+
// users have to delete this ref manually or we should have a synchronize between
166+
// database comment table and git refs.
167+
}
168+
}
169+
170+
for _, attachment := range comment.Attachments {
171+
if err := storage.Attachments.Delete(repo_model.AttachmentRelativePath(attachment.UUID)); err != nil {
172+
// Even delete files failed, but the attachments has been removed from database, so we
173+
// should not return error but only record the error on logs.
174+
// users have to delete this attachments manually or we should have a
175+
// synchronize between database attachment table and attachment storage
176+
log.Error("delete attachment[uuid: %s] failed: %v", attachment.UUID, err)
177+
}
178+
}
179+
return nil
139180
})
140181
if err != nil {
141182
return err

‎services/issue/issue.go‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -200,7 +200,7 @@ func DeleteIssue(ctx context.Context, doer *user_model.User, gitRepo *git.Reposi
200200

201201
// delete pull request related git data
202202
if issue.IsPull && gitRepo != nil {
203-
if err := gitRepo.RemoveReference(fmt.Sprintf("%s%d/head", git.PullPrefix, issue.PullRequest.Index)); err != nil {
203+
if err := git.RemoveRef(ctx, gitRepo.Path, issue.PullRequest.GetGitRefName()); err != nil {
204204
return err
205205
}
206206
}
@@ -301,6 +301,8 @@ func deleteIssue(ctx context.Context, issue *issues_model.Issue) ([]string, erro
301301
attachmentPaths = append(attachmentPaths, issue.Attachments[i].RelativePath())
302302
}
303303

304+
// TODO: deference all review comments
305+
304306
// delete all database data still assigned to this issue
305307
if err := db.DeleteBeans(ctx,
306308
&issues_model.ContentHistory{IssueID: issue.ID},

‎services/pull/pull.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -184,7 +184,7 @@ func NewPullRequest(ctx context.Context, opts *NewPullRequestOptions) error {
184184
return nil
185185
}); err != nil {
186186
// cleanup: this will only remove the reference, the real commit will be clean up when next GC
187-
if err1 := baseGitRepo.RemoveReference(pr.GetGitRefName()); err1 != nil {
187+
if err1 := git.RemoveRef(ctx, baseGitRepo.Path, pr.GetGitRefName()); err1 != nil {
188188
log.Error("RemoveReference: %v", err1)
189189
}
190190
return err
@@ -648,7 +648,7 @@ func UpdateRef(ctx context.Context, pr *issues_model.PullRequest) (err error) {
648648
return err
649649
}
650650

651-
_, _, err = git.NewCommand("update-ref").AddDynamicArguments(pr.GetGitRefName(), pr.HeadCommitID).RunStdString(ctx, &git.RunOpts{Dir: pr.BaseRepo.RepoPath()})
651+
err = git.UpdateRef(ctx, pr.BaseRepo.RepoPath(), pr.GetGitRefName(), pr.HeadCommitID)
652652
if err != nil {
653653
log.Error("Unable to update ref in base repository for PR[%d] Error: %v", pr.ID, err)
654654
}

‎services/pull/review.go‎

Lines changed: 29 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ import (
2121
"code.gitea.io/gitea/modules/cache"
2222
"code.gitea.io/gitea/modules/git"
2323
"code.gitea.io/gitea/modules/gitrepo"
24+
"code.gitea.io/gitea/modules/log"
2425
"code.gitea.io/gitea/modules/optional"
2526
"code.gitea.io/gitea/modules/setting"
2627
"code.gitea.io/gitea/modules/util"
@@ -141,11 +142,8 @@ func CreateCodeComment(ctx context.Context, doer *user_model.User, gitRepo *git.
141142
if err != nil {
142143
return nil, fmt.Errorf("GetRefCommitID[%s]: %w", issue.PullRequest.GetGitRefName(), err)
143144
}
144-
} else {
145-
// afterCommitID must be one of the pull request commits
146-
if !slices.Contains(prCommitIDs, afterCommitID) {
147-
return nil, fmt.Errorf("afterCommitID[%s] is not a valid pull request commit", afterCommitID)
148-
}
145+
} else if !slices.Contains(prCommitIDs, afterCommitID) { // afterCommitID must be one of the pull request commits
146+
return nil, fmt.Errorf("afterCommitID[%s] is not a valid pull request commit", afterCommitID)
149147
}
150148

151149
// CreateCodeComment() is used for:
@@ -279,22 +277,33 @@ func createCodeComment(ctx context.Context, doer *user_model.User, repo *repo_mo
279277
}
280278

281279
lineCommitID := util.Iif(line < 0, beforeCommitID, afterCommitID)
282-
// TODO: the commit ID Must be referenced in the git repository, because the branch maybe rebased or force-pushed.
280+
return db.WithTx2(ctx, func(ctx context.Context) (*issues_model.Comment, error) {
281+
comment, err := issues_model.CreateComment(ctx, &issues_model.CreateCommentOptions{
282+
Type: issues_model.CommentTypeCode,
283+
Doer: doer,
284+
Repo: repo,
285+
Issue: issue,
286+
Content: content,
287+
LineNum: line,
288+
TreePath: treePath,
289+
CommitSHA: lineCommitID,
290+
ReviewID: reviewID,
291+
Patch: patch,
292+
Invalidated: false,
293+
Attachments: attachments,
294+
})
295+
if err != nil {
296+
return nil, err
297+
}
283298

284-
// If the commit ID is not referenced, it cannot be calculated the position dynamically.
285-
return issues_model.CreateComment(ctx, &issues_model.CreateCommentOptions{
286-
Type: issues_model.CommentTypeCode,
287-
Doer: doer,
288-
Repo: repo,
289-
Issue: issue,
290-
Content: content,
291-
LineNum: line,
292-
TreePath: treePath,
293-
CommitSHA: lineCommitID,
294-
ReviewID: reviewID,
295-
Patch: patch,
296-
Invalidated: false,
297-
Attachments: attachments,
299+
// The line commit ID Must be referenced in the git repository, because the branch maybe rebased or force-pushed.
300+
// If the review commit is GC, the position will not be calculated dynamically.
301+
if err := git.UpdateRef(ctx, pr.BaseRepo.RepoPath(), issues_model.GetCodeCommentRef(pr.Index, comment.ID), lineCommitID); err != nil {
302+
log.Error("Unable to update ref in base repository for PR[%d] Error: %v", pr.ID, err)
303+
return nil, err
304+
}
305+
306+
return comment, nil
298307
})
299308
}
300309

‎services/user/delete.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -120,6 +120,7 @@ func deleteUser(ctx context.Context, u *user_model.User, purge bool) (err error)
120120
if err = issues_model.DeleteComment(ctx, comment); err != nil {
121121
return err
122122
}
123+
// TODO: delete attachments
123124
}
124125
}
125126

0 commit comments

Comments
 (0)