-
-
Notifications
You must be signed in to change notification settings - Fork 5.8k
Branches not at ref commit ID should not be listed as Merged #9614
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 9 commits
93364b9
4af6395
84decfe
3db7335
6f8d95e
ba13b0f
a9d65b8
4c40b8e
d3a5b4b
6ebbe30
705420a
d6d2f87
cf502ac
d76f586
13175c0
c440e52
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
4a357436d925b5c974181ff12a994538ddc5a269 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,6 +16,7 @@ import ( | |
"code.gitea.io/gitea/modules/log" | ||
"code.gitea.io/gitea/modules/repofiles" | ||
"code.gitea.io/gitea/modules/util" | ||
"gopkg.in/src-d/go-git.v4/plumbing" | ||
) | ||
|
||
const ( | ||
|
@@ -33,6 +34,7 @@ type Branch struct { | |
CommitsAhead int | ||
CommitsBehind int | ||
LatestPullRequest *models.PullRequest | ||
MergeMovedOn bool | ||
} | ||
|
||
// Branches render repository branch page | ||
|
@@ -185,6 +187,12 @@ func loadBranches(ctx *context.Context) []*Branch { | |
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)) | ||
for i := range rawBranches { | ||
commit, err := rawBranches[i].GetCommit() | ||
|
@@ -213,11 +221,68 @@ func loadBranches(ctx *context.Context) []*Branch { | |
ctx.ServerError("GetLatestPullRequestByHeadInfo", err) | ||
return nil | ||
} | ||
mergeMovedOn := false | ||
if pr != nil { | ||
if err := pr.LoadIssue(); err != nil { | ||
ctx.ServerError("pr.LoadIssue", err) | ||
return nil | ||
} | ||
if repo, ok := repoIDToRepo[pr.BaseRepoID]; ok { | ||
pr.HeadRepo = repo | ||
} else if err := pr.LoadBaseRepo(); err != nil { | ||
ctx.ServerError("pr.LoadBaseRepo", err) | ||
return nil | ||
} else { | ||
repoIDToRepo[pr.BaseRepoID] = pr.BaseRepo | ||
} | ||
|
||
if repo, ok := repoIDToRepo[pr.HeadRepoID]; ok { | ||
pr.HeadRepo = repo | ||
} else if err := pr.LoadHeadRepo(); err != nil { | ||
ctx.ServerError("pr.LoadHeadRepo", err) | ||
return nil | ||
} else { | ||
repoIDToRepo[pr.HeadRepoID] = pr.HeadRepo | ||
} | ||
|
||
if pr.HasMerged && pr.HeadRepo != nil { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ~~Well... not the way I'd written the code... ~~
I'm an idiot. Of course it can't be... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Line 219 only gets the PRs by HeadInfo - that is those that are in the head repo. Therefore the head repo is always going to be the ctx.Repo.Repository. |
||
headRepo, ok := repoIDToGitRepo[pr.HeadRepoID] | ||
if !ok { | ||
headRepo, err = git.OpenRepository(pr.HeadRepo.RepoPath()) | ||
if err != nil { | ||
ctx.ServerError("OpenRepository", err) | ||
return nil | ||
} | ||
defer headRepo.Close() | ||
repoIDToGitRepo[pr.HeadRepoID] = headRepo | ||
} | ||
baseRepo, ok := repoIDToGitRepo[pr.BaseRepoID] | ||
if !ok { | ||
baseRepo, err = git.OpenRepository(pr.BaseRepo.RepoPath()) | ||
if err != nil { | ||
ctx.ServerError("OpenRepository", err) | ||
return nil | ||
} | ||
defer baseRepo.Close() | ||
repoIDToGitRepo[pr.BaseRepoID] = baseRepo | ||
} | ||
pullCommit, err := baseRepo.GetRefCommitID(pr.GetGitRefName()) | ||
if err != nil && err != plumbing.ErrReferenceNotFound { | ||
ctx.ServerError("GetBranchCommitID", err) | ||
return nil | ||
} | ||
if err == nil { | ||
headCommit, err := headRepo.GetBranchCommitID(pr.HeadBranch) | ||
if err != nil && err != plumbing.ErrReferenceNotFound { | ||
ctx.ServerError("GetBranchCommitID", err) | ||
return nil | ||
} else 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 | ||
|
@@ -230,6 +295,7 @@ func loadBranches(ctx *context.Context) []*Branch { | |
CommitsAhead: divergence.Ahead, | ||
CommitsBehind: divergence.Behind, | ||
LatestPullRequest: pr, | ||
MergeMovedOn: mergeMovedOn, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe this is an edge case and is covered elsewhere but, perhaps if we've got There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this referring to when I thought it was possible to not get the head branch commit id? We actually always know that's there because otherwise there would have been a ServerError at line 200. So that can't happen and I've optimised that check away. OR is this referring to if we can't find the PR pulls head in the base repo? I'm not sure what to do in that case. If I try to enforce that the pulls head is actually there I suspect that will break a lot of integration tests - as our fixtures do not necessarily match the ground truth. I also expect that there is a race between the db and the presence of the pull head. (We should check what happens here - the index is needed to create the pull head - so that means that the db has to have a pull request committed before we can push to the pull head.) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I meant that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand. Perhaps some of what you're discussing is already fixed within the logic of the template? If you don't have a PR already then merge moved on is of no consequence. The head branch we already know exists because it's the branch we've provided. If the base branch does not exist - that's a server error - although this won't show that. You shouldn't be able to delete a branch with PRs against it - (although I haven't checked that.) If the git pull head isn't there well... that's technically a server error but I think there could be a race between between a pull request being made and the ref being updated so I think we should allow it. |
||
} | ||
} | ||
|
||
|
Uh oh!
There was an error while loading. Please reload this page.