Skip to content

Change pull description text (#2075) #2646

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Oct 4, 2017
Merged

Change pull description text (#2075) #2646

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d4dbe38
Change pull description text
kaz Oct 3, 2017
b6c7933
avoid panic / show branch name only (in same repository pullreq)
kaz Oct 4, 2017
10d075f
move duplicated code into helper function
kaz Oct 4, 2017
9e6caa1
Merge branch 'master' into 2075
lafriks Oct 4, 2017
File filter

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 26 additions & 11 deletions routers/repo/pull.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,11 +182,23 @@ func checkPullInfo(ctx *context.Context) *models.Issue {
// PrepareMergedViewPullInfo show meta information for a merged pull request view page
func PrepareMergedViewPullInfo(ctx *context.Context, issue *models.Issue) {
pull := issue.PullRequest
ctx.Data["HasMerged"] = true
ctx.Data["HeadTarget"] = issue.PullRequest.HeadUserName + "/" + pull.HeadBranch
ctx.Data["BaseTarget"] = ctx.Repo.Owner.Name + "/" + pull.BaseBranch

var err error
if err = pull.GetHeadRepo(); err != nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If PR is made and than forked repository is deleted there will be error ErrRepoNotExist and it will not be possible to open PR even to close it. It should at least fall back to message ... from fork_owner:branch into branch format in such case.
Currently in such case this codes gets PANIC: runtime error: invalid memory address or nil pointer dereference
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably I could be wrong that it returns error.., More looks like it just sets pull.HeadRepo as nil
ctx.Handle(500, "GetHeadRepo", err)
return
}

if ctx.Repo.Owner.Name == pull.HeadUserName {
ctx.Data["HeadTarget"] = pull.HeadBranch
} else if pull.HeadRepo == nil {
ctx.Data["HeadTarget"] = pull.HeadUserName + ":" + pull.HeadBranch
} else {
ctx.Data["HeadTarget"] = pull.HeadUserName + "/" + pull.HeadRepo.Name + ":" + pull.HeadBranch
}
ctx.Data["BaseTarget"] = pull.BaseBranch
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since lines 192-199 seem to be duplicated in PrepareViewPullInfo(..), maybe we should move them to a helper function?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would best option
ctx.Data["HasMerged"] = true

ctx.Data["NumCommits"], err = ctx.Repo.GitRepo.CommitsCountBetween(pull.MergeBase, pull.MergedCommitID)
if err != nil {
ctx.Handle(500, "Repo.GitRepo.CommitsCountBetween", err)
Expand All @@ -204,19 +216,22 @@ func PrepareViewPullInfo(ctx *context.Context, issue *models.Issue) *git.PullReq
repo := ctx.Repo.Repository
pull := issue.PullRequest

ctx.Data["HeadTarget"] = pull.HeadUserName + "/" + pull.HeadBranch
ctx.Data["BaseTarget"] = ctx.Repo.Owner.Name + "/" + pull.BaseBranch

var (
headGitRepo *git.Repository
err error
)

var err error
if err = pull.GetHeadRepo(); err != nil {
ctx.Handle(500, "GetHeadRepo", err)
return nil
}

if ctx.Repo.Owner.Name == pull.HeadUserName {
ctx.Data["HeadTarget"] = pull.HeadBranch
} else if pull.HeadRepo == nil {
ctx.Data["HeadTarget"] = pull.HeadUserName + ":" + pull.HeadBranch
} else {
ctx.Data["HeadTarget"] = pull.HeadUserName + "/" + pull.HeadRepo.Name + ":" + pull.HeadBranch
}
ctx.Data["BaseTarget"] = pull.BaseBranch

var headGitRepo *git.Repository
if pull.HeadRepo != nil {
headGitRepo, err = git.OpenRepository(pull.HeadRepo.RepoPath())
if err != nil {
Expand Down