Skip to content

Commit 319c921

Browse files
zeripathlafriks
zeripath
authored and
lafriks
committed
Branches not at ref commit ID should not be listed as Merged (#9614) (#9639)
Once a branch has been merged if the commit ID no longer equals that of the pulls ref commit id don't offer to delete the branch on the pull screen and don't list it as merged on branches. Fix #9201 When looking at the pull page we should also get the commits from the refs/pulls/x/head Fix #9158
1 parent a15a644 commit 319c921

File tree

10 files changed

+106
-33
lines changed

10 files changed

+106
-33
lines changed

‎integrations/gitea-repositories-meta/user2/repo1.git/objects/00/750edc07d6415dcc07ae0351e9397b0222b7ba

17 Bytes
Binary file not shown.

‎integrations/gitea-repositories-meta/user2/repo1.git/objects/4a/357436d925b5c974181ff12a994538ddc5a269

840 Bytes
Binary file not shown.

‎integrations/gitea-repositories-meta/user2/repo1.git/objects/dc/7a8ba127fee870dd683310ce660dfe59333a1b

78 Bytes
Binary file not shown.

‎integrations/gitea-repositories-meta/user2/repo1.git/refs/pull/3/head

+1
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
4a357436d925b5c974181ff12a994538ddc5a269

‎models/pull.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -136,7 +136,7 @@ func (pr *PullRequest) LoadHeadRepo() error {
136136
if has, err := x.ID(pr.HeadRepoID).Get(&repo); err != nil {
137137
return err
138138
} else if !has {
139-
return ErrRepoNotExist{ID: pr.BaseRepoID}
139+
return ErrRepoNotExist{ID: pr.HeadRepoID}
140140
}
141141
pr.HeadRepo = &repo
142142
}

‎routers/repo/branch.go

+44
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import (
1616
"code.gitea.io/gitea/modules/log"
1717
"code.gitea.io/gitea/modules/repofiles"
1818
"code.gitea.io/gitea/modules/util"
19+
"gopkg.in/src-d/go-git.v4/plumbing"
1920
)
2021

2122
const (
@@ -32,6 +33,7 @@ type Branch struct {
3233
CommitsAhead int
3334
CommitsBehind int
3435
LatestPullRequest *models.PullRequest
36+
MergeMovedOn bool
3537
}
3638

3739
// Branches render repository branch page
@@ -168,6 +170,12 @@ func loadBranches(ctx *context.Context) []*Branch {
168170
return nil
169171
}
170172

173+
repoIDToRepo := map[int64]*models.Repository{}
174+
repoIDToRepo[ctx.Repo.Repository.ID] = ctx.Repo.Repository
175+
176+
repoIDToGitRepo := map[int64]*git.Repository{}
177+
repoIDToGitRepo[ctx.Repo.Repository.ID] = ctx.Repo.GitRepo
178+
171179
branches := make([]*Branch, len(rawBranches))
172180
for i := range rawBranches {
173181
commit, err := rawBranches[i].GetCommit()
@@ -196,11 +204,46 @@ func loadBranches(ctx *context.Context) []*Branch {
196204
ctx.ServerError("GetLatestPullRequestByHeadInfo", err)
197205
return nil
198206
}
207+
headCommit := commit.ID.String()
208+
209+
mergeMovedOn := false
199210
if pr != nil {
211+
pr.HeadRepo = ctx.Repo.Repository
200212
if err := pr.LoadIssue(); err != nil {
201213
ctx.ServerError("pr.LoadIssue", err)
202214
return nil
203215
}
216+
if repo, ok := repoIDToRepo[pr.BaseRepoID]; ok {
217+
pr.BaseRepo = repo
218+
} else if err := pr.LoadBaseRepo(); err != nil {
219+
ctx.ServerError("pr.LoadBaseRepo", err)
220+
return nil
221+
} else {
222+
repoIDToRepo[pr.BaseRepoID] = pr.BaseRepo
223+
}
224+
225+
if pr.HasMerged {
226+
baseGitRepo, ok := repoIDToGitRepo[pr.BaseRepoID]
227+
if !ok {
228+
baseGitRepo, err = git.OpenRepository(pr.BaseRepo.RepoPath())
229+
if err != nil {
230+
ctx.ServerError("OpenRepository", err)
231+
return nil
232+
}
233+
defer baseGitRepo.Close()
234+
repoIDToGitRepo[pr.BaseRepoID] = baseGitRepo
235+
}
236+
pullCommit, err := baseGitRepo.GetRefCommitID(pr.GetGitRefName())
237+
if err != nil && err != plumbing.ErrReferenceNotFound {
238+
ctx.ServerError("GetBranchCommitID", err)
239+
return nil
240+
}
241+
if err == nil && headCommit != pullCommit {
242+
// the head has moved on from the merge - we shouldn't delete
243+
mergeMovedOn = true
244+
}
245+
}
246+
204247
}
205248

206249
branches[i] = &Branch{
@@ -210,6 +253,7 @@ func loadBranches(ctx *context.Context) []*Branch {
210253
CommitsAhead: divergence.Ahead,
211254
CommitsBehind: divergence.Behind,
212255
LatestPullRequest: pr,
256+
MergeMovedOn: mergeMovedOn,
213257
}
214258
}
215259

‎routers/repo/issue.go

+4-1
Original file line numberDiff line numberDiff line change
@@ -931,7 +931,10 @@ func ViewIssue(ctx *context.Context) {
931931
ctx.Data["IsBlockedByApprovals"] = pull.ProtectedBranch.RequiredApprovals > 0 && cnt < pull.ProtectedBranch.RequiredApprovals
932932
ctx.Data["GrantedApprovals"] = cnt
933933
}
934-
ctx.Data["IsPullBranchDeletable"] = canDelete && pull.HeadRepo != nil && git.IsBranchExist(pull.HeadRepo.RepoPath(), pull.HeadBranch)
934+
ctx.Data["IsPullBranchDeletable"] = canDelete &&
935+
pull.HeadRepo != nil &&
936+
git.IsBranchExist(pull.HeadRepo.RepoPath(), pull.HeadBranch) &&
937+
(!pull.HasMerged || ctx.Data["HeadBranchCommitID"] == ctx.Data["PullHeadCommitID"])
935938

936939
ctx.Data["PullReviewersWithType"], err = models.GetReviewersByPullID(issue.ID)
937940
if err != nil {

‎routers/repo/pull.go

+49-30
Original file line numberDiff line numberDiff line change
@@ -315,25 +315,37 @@ func PrepareViewPullInfo(ctx *context.Context, issue *models.Issue) *git.Compare
315315
repo := ctx.Repo.Repository
316316
pull := issue.PullRequest
317317

318-
var err error
319-
if err = pull.GetHeadRepo(); err != nil {
318+
if err := pull.GetHeadRepo(); err != nil {
320319
ctx.ServerError("GetHeadRepo", err)
321320
return nil
322321
}
323322

323+
if err := pull.GetBaseRepo(); err != nil {
324+
ctx.ServerError("GetBaseRepo", err)
325+
return nil
326+
}
327+
324328
setMergeTarget(ctx, pull)
325329

326-
if err = pull.LoadProtectedBranch(); err != nil {
330+
if err := pull.LoadProtectedBranch(); err != nil {
327331
ctx.ServerError("GetLatestCommitStatus", err)
328332
return nil
329333
}
330334
ctx.Data["EnableStatusCheck"] = pull.ProtectedBranch != nil && pull.ProtectedBranch.EnableStatusCheck
331335

332-
var headGitRepo *git.Repository
336+
baseGitRepo, err := git.OpenRepository(pull.BaseRepo.RepoPath())
337+
if err != nil {
338+
ctx.ServerError("OpenRepository", err)
339+
return nil
340+
}
341+
defer baseGitRepo.Close()
333342
var headBranchExist bool
343+
var headBranchSha string
334344
// HeadRepo may be missing
335345
if pull.HeadRepo != nil {
336-
headGitRepo, err = git.OpenRepository(pull.HeadRepo.RepoPath())
346+
var err error
347+
348+
headGitRepo, err := git.OpenRepository(pull.HeadRepo.RepoPath())
337349
if err != nil {
338350
ctx.ServerError("OpenRepository", err)
339351
return nil
@@ -343,46 +355,53 @@ func PrepareViewPullInfo(ctx *context.Context, issue *models.Issue) *git.Compare
343355
headBranchExist = headGitRepo.IsBranchExist(pull.HeadBranch)
344356

345357
if headBranchExist {
346-
sha, err := headGitRepo.GetBranchCommitID(pull.HeadBranch)
358+
headBranchSha, err = headGitRepo.GetBranchCommitID(pull.HeadBranch)
347359
if err != nil {
348360
ctx.ServerError("GetBranchCommitID", err)
349361
return nil
350362
}
363+
}
364+
}
351365

352-
commitStatuses, err := models.GetLatestCommitStatus(repo, sha, 0)
353-
if err != nil {
354-
ctx.ServerError("GetLatestCommitStatus", err)
355-
return nil
356-
}
357-
if len(commitStatuses) > 0 {
358-
ctx.Data["LatestCommitStatuses"] = commitStatuses
359-
ctx.Data["LatestCommitStatus"] = models.CalcCommitStatus(commitStatuses)
360-
}
366+
sha, err := baseGitRepo.GetRefCommitID(pull.GetGitRefName())
367+
if err != nil {
368+
ctx.ServerError(fmt.Sprintf("GetRefCommitID(%s)", pull.GetGitRefName()), err)
369+
return nil
370+
}
361371

362-
if pull.ProtectedBranch != nil && pull.ProtectedBranch.EnableStatusCheck {
363-
ctx.Data["is_context_required"] = func(context string) bool {
364-
for _, c := range pull.ProtectedBranch.StatusCheckContexts {
365-
if c == context {
366-
return true
367-
}
368-
}
369-
return false
372+
commitStatuses, err := models.GetLatestCommitStatus(repo, sha, 0)
373+
if err != nil {
374+
ctx.ServerError("GetLatestCommitStatus", err)
375+
return nil
376+
}
377+
if len(commitStatuses) > 0 {
378+
ctx.Data["LatestCommitStatuses"] = commitStatuses
379+
ctx.Data["LatestCommitStatus"] = models.CalcCommitStatus(commitStatuses)
380+
}
381+
382+
if pull.ProtectedBranch != nil && pull.ProtectedBranch.EnableStatusCheck {
383+
ctx.Data["is_context_required"] = func(context string) bool {
384+
for _, c := range pull.ProtectedBranch.StatusCheckContexts {
385+
if c == context {
386+
return true
370387
}
371-
ctx.Data["IsRequiredStatusCheckSuccess"] = pull_service.IsCommitStatusContextSuccess(commitStatuses, pull.ProtectedBranch.StatusCheckContexts)
372388
}
389+
return false
373390
}
391+
ctx.Data["IsRequiredStatusCheckSuccess"] = pull_service.IsCommitStatusContextSuccess(commitStatuses, pull.ProtectedBranch.StatusCheckContexts)
374392
}
375393

376-
if pull.HeadRepo == nil || !headBranchExist {
394+
ctx.Data["HeadBranchMovedOn"] = headBranchSha != sha
395+
ctx.Data["HeadBranchCommitID"] = headBranchSha
396+
ctx.Data["PullHeadCommitID"] = sha
397+
398+
if pull.HeadRepo == nil || !headBranchExist || headBranchSha != sha {
377399
ctx.Data["IsPullRequestBroken"] = true
378400
ctx.Data["HeadTarget"] = "deleted"
379-
ctx.Data["NumCommits"] = 0
380-
ctx.Data["NumFiles"] = 0
381-
return nil
382401
}
383402

384-
compareInfo, err := headGitRepo.GetCompareInfo(models.RepoPath(repo.Owner.Name, repo.Name),
385-
pull.BaseBranch, pull.HeadBranch)
403+
compareInfo, err := baseGitRepo.GetCompareInfo(pull.BaseRepo.RepoPath(),
404+
pull.BaseBranch, pull.GetGitRefName())
386405
if err != nil {
387406
if strings.Contains(err.Error(), "fatal: Not a valid object name") {
388407
ctx.Data["IsPullRequestBroken"] = true

‎templates/repo/branch/list.tmpl

+6
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,12 @@
8080
<button id="new-pull-request" class="ui compact basic button">{{$.i18n.Tr "repo.pulls.compare_changes"}}</button>
8181
</a>
8282
{{end}}
83+
{{else if and .LatestPullRequest.HasMerged .MergeMovedOn}}
84+
{{if and (not .IsDeleted) $.AllowsPulls (gt .CommitsAhead 0)}}
85+
<a href="{{$.RepoLink}}/compare/{{$.DefaultBranch | EscapePound}}...{{if ne $.Repository.Owner.Name $.Owner.Name}}{{$.Owner.Name}}:{{end}}{{.Name | EscapePound}}">
86+
<button id="new-pull-request" class="ui compact basic button">{{$.i18n.Tr "repo.pulls.compare_changes"}}</button>
87+
</a>
88+
{{end}}
8389
{{else}}
8490
<a href="{{$.RepoLink}}/pulls/{{.LatestPullRequest.Issue.Index}}">#{{.LatestPullRequest.Issue.Index}}</a>
8591
{{if .LatestPullRequest.HasMerged}}

‎templates/repo/issue/view_content/pull.tmpl

+1-1
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,7 @@
7171
{{$.i18n.Tr "repo.pulls.reopen_to_merge"}}
7272
{{end}}
7373
</div>
74-
{{if .IsPullBranchDeletable}}
74+
{{if and .IsPullBranchDeletable ( not .IsPullRequestBroken )}}
7575
<div class="ui divider"></div>
7676
<div>
7777
<a class="delete-button ui red button" href="" data-url="{{.DeleteBranchLink}}">{{$.i18n.Tr "repo.branch.delete" .HeadTarget}}</a>

0 commit comments

Comments
 (0)