Branches not at ref commit ID should not be listed as Merged (#9614)

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
This commit is contained in:
zeripath 2020-01-07 17:06:14 +00:00 committed by GitHub
parent 9406368633
commit e5d8e2d10c
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
10 changed files with 112 additions and 39 deletions

View File

@ -0,0 +1 @@
4a357436d925b5c974181ff12a994538ddc5a269

View File

@ -122,7 +122,7 @@ func (pr *PullRequest) LoadHeadRepo() error {
if has, err := x.ID(pr.HeadRepoID).Get(&repo); err != nil { if has, err := x.ID(pr.HeadRepoID).Get(&repo); err != nil {
return err return err
} else if !has { } else if !has {
return ErrRepoNotExist{ID: pr.BaseRepoID} return ErrRepoNotExist{ID: pr.HeadRepoID}
} }
pr.HeadRepo = &repo pr.HeadRepo = &repo
} }

View File

@ -16,6 +16,7 @@ import (
"code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/log"
"code.gitea.io/gitea/modules/repofiles" "code.gitea.io/gitea/modules/repofiles"
"code.gitea.io/gitea/modules/util" "code.gitea.io/gitea/modules/util"
"gopkg.in/src-d/go-git.v4/plumbing"
) )
const ( const (
@ -33,6 +34,7 @@ type Branch struct {
CommitsAhead int CommitsAhead int
CommitsBehind int CommitsBehind int
LatestPullRequest *models.PullRequest LatestPullRequest *models.PullRequest
MergeMovedOn bool
} }
// Branches render repository branch page // Branches render repository branch page
@ -185,6 +187,12 @@ func loadBranches(ctx *context.Context) []*Branch {
return nil return nil
} }
repoIDToRepo := map[int64]*models.Repository{}
repoIDToRepo[ctx.Repo.Repository.ID] = ctx.Repo.Repository
repoIDToGitRepo := map[int64]*git.Repository{}
repoIDToGitRepo[ctx.Repo.Repository.ID] = ctx.Repo.GitRepo
branches := make([]*Branch, len(rawBranches)) branches := make([]*Branch, len(rawBranches))
for i := range rawBranches { for i := range rawBranches {
commit, err := rawBranches[i].GetCommit() commit, err := rawBranches[i].GetCommit()
@ -213,11 +221,46 @@ func loadBranches(ctx *context.Context) []*Branch {
ctx.ServerError("GetLatestPullRequestByHeadInfo", err) ctx.ServerError("GetLatestPullRequestByHeadInfo", err)
return nil return nil
} }
headCommit := commit.ID.String()
mergeMovedOn := false
if pr != nil { if pr != nil {
pr.HeadRepo = ctx.Repo.Repository
if err := pr.LoadIssue(); err != nil { if err := pr.LoadIssue(); err != nil {
ctx.ServerError("pr.LoadIssue", err) ctx.ServerError("pr.LoadIssue", err)
return nil return nil
} }
if repo, ok := repoIDToRepo[pr.BaseRepoID]; ok {
pr.BaseRepo = repo
} else if err := pr.LoadBaseRepo(); err != nil {
ctx.ServerError("pr.LoadBaseRepo", err)
return nil
} else {
repoIDToRepo[pr.BaseRepoID] = pr.BaseRepo
}
if pr.HasMerged {
baseGitRepo, ok := repoIDToGitRepo[pr.BaseRepoID]
if !ok {
baseGitRepo, err = git.OpenRepository(pr.BaseRepo.RepoPath())
if err != nil {
ctx.ServerError("OpenRepository", err)
return nil
}
defer baseGitRepo.Close()
repoIDToGitRepo[pr.BaseRepoID] = baseGitRepo
}
pullCommit, err := baseGitRepo.GetRefCommitID(pr.GetGitRefName())
if err != nil && err != plumbing.ErrReferenceNotFound {
ctx.ServerError("GetBranchCommitID", err)
return nil
}
if err == nil && headCommit != pullCommit {
// the head has moved on from the merge - we shouldn't delete
mergeMovedOn = true
}
}
} }
isIncluded := divergence.Ahead == 0 && ctx.Repo.Repository.DefaultBranch != branchName isIncluded := divergence.Ahead == 0 && ctx.Repo.Repository.DefaultBranch != branchName
@ -230,6 +273,7 @@ func loadBranches(ctx *context.Context) []*Branch {
CommitsAhead: divergence.Ahead, CommitsAhead: divergence.Ahead,
CommitsBehind: divergence.Behind, CommitsBehind: divergence.Behind,
LatestPullRequest: pr, LatestPullRequest: pr,
MergeMovedOn: mergeMovedOn,
} }
} }

View File

@ -966,7 +966,10 @@ func ViewIssue(ctx *context.Context) {
ctx.Data["IsBlockedByRejection"] = pull.ProtectedBranch.MergeBlockedByRejectedReview(pull) ctx.Data["IsBlockedByRejection"] = pull.ProtectedBranch.MergeBlockedByRejectedReview(pull)
ctx.Data["GrantedApprovals"] = cnt ctx.Data["GrantedApprovals"] = cnt
} }
ctx.Data["IsPullBranchDeletable"] = canDelete && pull.HeadRepo != nil && git.IsBranchExist(pull.HeadRepo.RepoPath(), pull.HeadBranch) ctx.Data["IsPullBranchDeletable"] = canDelete &&
pull.HeadRepo != nil &&
git.IsBranchExist(pull.HeadRepo.RepoPath(), pull.HeadBranch) &&
(!pull.HasMerged || ctx.Data["HeadBranchCommitID"] == ctx.Data["PullHeadCommitID"])
ctx.Data["PullReviewers"], err = models.GetReviewersByIssueID(issue.ID) ctx.Data["PullReviewers"], err = models.GetReviewersByIssueID(issue.ID)
if err != nil { if err != nil {

View File

@ -330,25 +330,37 @@ func PrepareViewPullInfo(ctx *context.Context, issue *models.Issue) *git.Compare
repo := ctx.Repo.Repository repo := ctx.Repo.Repository
pull := issue.PullRequest pull := issue.PullRequest
var err error if err := pull.GetHeadRepo(); err != nil {
if err = pull.GetHeadRepo(); err != nil {
ctx.ServerError("GetHeadRepo", err) ctx.ServerError("GetHeadRepo", err)
return nil return nil
} }
if err := pull.GetBaseRepo(); err != nil {
ctx.ServerError("GetBaseRepo", err)
return nil
}
setMergeTarget(ctx, pull) setMergeTarget(ctx, pull)
if err = pull.LoadProtectedBranch(); err != nil { if err := pull.LoadProtectedBranch(); err != nil {
ctx.ServerError("GetLatestCommitStatus", err) ctx.ServerError("GetLatestCommitStatus", err)
return nil return nil
} }
ctx.Data["EnableStatusCheck"] = pull.ProtectedBranch != nil && pull.ProtectedBranch.EnableStatusCheck ctx.Data["EnableStatusCheck"] = pull.ProtectedBranch != nil && pull.ProtectedBranch.EnableStatusCheck
var headGitRepo *git.Repository baseGitRepo, err := git.OpenRepository(pull.BaseRepo.RepoPath())
if err != nil {
ctx.ServerError("OpenRepository", err)
return nil
}
defer baseGitRepo.Close()
var headBranchExist bool var headBranchExist bool
var headBranchSha string
// HeadRepo may be missing // HeadRepo may be missing
if pull.HeadRepo != nil { if pull.HeadRepo != nil {
headGitRepo, err = git.OpenRepository(pull.HeadRepo.RepoPath()) var err error
headGitRepo, err := git.OpenRepository(pull.HeadRepo.RepoPath())
if err != nil { if err != nil {
ctx.ServerError("OpenRepository", err) ctx.ServerError("OpenRepository", err)
return nil return nil
@ -358,11 +370,19 @@ func PrepareViewPullInfo(ctx *context.Context, issue *models.Issue) *git.Compare
headBranchExist = headGitRepo.IsBranchExist(pull.HeadBranch) headBranchExist = headGitRepo.IsBranchExist(pull.HeadBranch)
if headBranchExist { if headBranchExist {
sha, err := headGitRepo.GetBranchCommitID(pull.HeadBranch) headBranchSha, err = headGitRepo.GetBranchCommitID(pull.HeadBranch)
if err != nil { if err != nil {
ctx.ServerError("GetBranchCommitID", err) ctx.ServerError("GetBranchCommitID", err)
return nil return nil
} }
}
}
sha, err := baseGitRepo.GetRefCommitID(pull.GetGitRefName())
if err != nil {
ctx.ServerError(fmt.Sprintf("GetRefCommitID(%s)", pull.GetGitRefName()), err)
return nil
}
commitStatuses, err := models.GetLatestCommitStatus(repo, sha, 0) commitStatuses, err := models.GetLatestCommitStatus(repo, sha, 0)
if err != nil { if err != nil {
@ -385,19 +405,18 @@ func PrepareViewPullInfo(ctx *context.Context, issue *models.Issue) *git.Compare
} }
ctx.Data["IsRequiredStatusCheckSuccess"] = pull_service.IsCommitStatusContextSuccess(commitStatuses, pull.ProtectedBranch.StatusCheckContexts) ctx.Data["IsRequiredStatusCheckSuccess"] = pull_service.IsCommitStatusContextSuccess(commitStatuses, pull.ProtectedBranch.StatusCheckContexts)
} }
}
}
if pull.HeadRepo == nil || !headBranchExist { ctx.Data["HeadBranchMovedOn"] = headBranchSha != sha
ctx.Data["HeadBranchCommitID"] = headBranchSha
ctx.Data["PullHeadCommitID"] = sha
if pull.HeadRepo == nil || !headBranchExist || headBranchSha != sha {
ctx.Data["IsPullRequestBroken"] = true ctx.Data["IsPullRequestBroken"] = true
ctx.Data["HeadTarget"] = "deleted" ctx.Data["HeadTarget"] = "deleted"
ctx.Data["NumCommits"] = 0
ctx.Data["NumFiles"] = 0
return nil
} }
compareInfo, err := headGitRepo.GetCompareInfo(models.RepoPath(repo.Owner.Name, repo.Name), compareInfo, err := baseGitRepo.GetCompareInfo(pull.BaseRepo.RepoPath(),
pull.BaseBranch, pull.HeadBranch) pull.BaseBranch, pull.GetGitRefName())
if err != nil { if err != nil {
if strings.Contains(err.Error(), "fatal: Not a valid object name") { if strings.Contains(err.Error(), "fatal: Not a valid object name") {
ctx.Data["IsPullRequestBroken"] = true ctx.Data["IsPullRequestBroken"] = true

View File

@ -84,6 +84,12 @@
<button id="new-pull-request" class="ui compact basic button">{{$.i18n.Tr "repo.pulls.compare_changes"}}</button> <button id="new-pull-request" class="ui compact basic button">{{$.i18n.Tr "repo.pulls.compare_changes"}}</button>
</a> </a>
{{end}} {{end}}
{{else if and .LatestPullRequest.HasMerged .MergeMovedOn}}
{{if and (not .IsDeleted) $.AllowsPulls (gt .CommitsAhead 0)}}
<a href="{{$.RepoLink}}/compare/{{$.DefaultBranch | EscapePound}}...{{if ne $.Repository.Owner.Name $.Owner.Name}}{{$.Owner.Name}}:{{end}}{{.Name | EscapePound}}">
<button id="new-pull-request" class="ui compact basic button">{{$.i18n.Tr "repo.pulls.compare_changes"}}</button>
</a>
{{end}}
{{else}} {{else}}
<a href="{{$.RepoLink}}/pulls/{{.LatestPullRequest.Issue.Index}}">#{{.LatestPullRequest.Issue.Index}}</a> <a href="{{$.RepoLink}}/pulls/{{.LatestPullRequest.Issue.Index}}">#{{.LatestPullRequest.Issue.Index}}</a>
{{if .LatestPullRequest.HasMerged}} {{if .LatestPullRequest.HasMerged}}

View File

@ -72,7 +72,7 @@
{{$.i18n.Tr "repo.pulls.reopen_to_merge"}} {{$.i18n.Tr "repo.pulls.reopen_to_merge"}}
{{end}} {{end}}
</div> </div>
{{if .IsPullBranchDeletable}} {{if and .IsPullBranchDeletable ( not .IsPullRequestBroken )}}
<div class="ui divider"></div> <div class="ui divider"></div>
<div> <div>
<a class="delete-button ui red button" href="" data-url="{{.DeleteBranchLink}}">{{$.i18n.Tr "repo.branch.delete" .HeadTarget}}</a> <a class="delete-button ui red button" href="" data-url="{{.DeleteBranchLink}}">{{$.i18n.Tr "repo.branch.delete" .HeadTarget}}</a>