Skip to content
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

Jump to bottom

Change pull description text (#2075) #2646

Merged
merged 4 commits into from
Oct 4, 2017
Merged

Change pull description text (#2075) #2646

merged 4 commits into from
Oct 4, 2017

Conversation

kaz
Copy link
Contributor

@kaz kaz commented Oct 4, 2017

Change description text on pull request page

from ... from fork_owner/branch into base_owner/branch style
to ... from fork_owner/repo:branch into branch style

it was suggested on #2075

Closes #2075

@codecov-io
Copy link

codecov-io commented Oct 4, 2017 •
edited
Loading

Codecov Report

Merging #2646 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2646   +/-   ##
=======================================
  Coverage   27.12%   27.12%           
=======================================
  Files          86       86           
  Lines       17064    17064           
=======================================
  Hits         4629     4629           
  Misses      11757    11757           
  Partials      678      678

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fcca037...9e6caa1. Read the comment docs.

@tboerger tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Oct 4, 2017
lafriks
lafriks requested changes Oct 4, 2017
View reviewed changes
routers/repo/pull.go

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

@lafriks
Copy link
Member

lafriks commented Oct 4, 2017

Also if ctx.Repo.Owner.Name == pull.HeadUserName than message should look something like ... from branch into branch (as you can create PR form same repository one branch to other)

@kaz
Copy link
Contributor Author

kaz commented Oct 4, 2017

Thank you for your review.

I made some change:

  • if pull.HeadRepo == nil then fall back to message ... from fork_owner:branch into branch format
  • if ctx.Repo.Owner.Name == pull.HeadUserName then show such like ... from branch into branch

ethantkoenig
ethantkoenig reviewed Oct 4, 2017
View reviewed changes
routers/repo/pull.go
} 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

@ethantkoenig
Copy link
Member

LGTM

@tboerger tboerger added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Oct 4, 2017
@lafriks
Copy link
Member

lafriks commented Oct 4, 2017

LGTM

@tboerger tboerger added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Oct 4, 2017
@lafriks lafriks added topic/ui Change the appearance of the Gitea UI type/enhancement An improvement of existing functionality labels Oct 4, 2017
@lafriks lafriks added this to the 1.3.0 milestone Oct 4, 2017
@lafriks lafriks merged commit e1266a1 into go-gitea:master Oct 4, 2017
@go-gitea go-gitea locked and limited conversation to collaborators Nov 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@lafriks lafriks lafriks approved these changes

@ethantkoenig ethantkoenig ethantkoenig approved these changes

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. topic/ui Change the appearance of the Gitea UI type/enhancement An improvement of existing functionality
Projects
None yet
Milestone
1.3.0
Development

Successfully merging this pull request may close these issues.

[UI] Pull request page doesn't contain repo name
5 participants
@kaz @codecov-io @lafriks @ethantkoenig @tboerger