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

Repo name added to automatically generated commit message when mergin… #9997

Merged
merged 8 commits into from Jan 28, 2020
Merged

Repo name added to automatically generated commit message when mergin… #9997

merged 8 commits into from Jan 28, 2020

Conversation

shashvat-kedia
Copy link
Contributor

Fix for issue #9898.

zeripath
zeripath approved these changes Jan 26, 2020
View reviewed changes
Copy link
Contributor

@zeripath zeripath left a comment

Choose a reason for hiding this comment

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

Small nit but otherwise approve.

models/pull.go Outdated Show resolved Hide resolved
@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Jan 26, 2020
@zeripath
Copy link
Contributor

zeripath commented Jan 26, 2020 •
edited

The current behaviour is actually completely specified as you can't have more than one fork of a repo and can't pr from something that isn't a fork. See the top of this page on GH right now...

However I can see that it is confusing.

If we're going to go for fully specified repo names then we should have a different separator between branch and repo name. Either colon or space.

Whatever we choose it should then propagate through to the compare page.

At present I think that uses a colon so colon is recommendeded.

@codecov-io
Copy link

codecov-io commented Jan 26, 2020 •
edited

Codecov Report

Merging #9997 into master will decrease coverage by <.01%.
The diff coverage is 54.54%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9997      +/-   ##
==========================================
- Coverage   42.27%   42.26%   -0.01%     
==========================================
  Files         610      611       +1     
  Lines       80375    80394      +19     
==========================================
  Hits        33979    33979              
- Misses      42218    42237      +19     
  Partials     4178     4178
Impacted Files Coverage Δ
services/pull/review.go 0% <0%> (ø) ⬆️
routers/repo/commit.go 29.66% <0%> (ø) ⬆️
modules/migrations/gitea.go 6.14% <0%> (-0.02%) ⬇️
routers/repo/pull.go 28.7% <0%> (-0.37%) ⬇️
services/gitdiff/gitdiff.go 67.62% <100%> (+1.36%) ⬆️
models/issue_milestone.go 67.9% <100%> (+0.23%) ⬆️
modules/git/diff.go 62.03% <62.03%> (ø)
services/pull/temp_repo.go 31.62% <0%> (-2.57%) ⬇️
services/pull/check.go 54.54% <0%> (-2.1%) ⬇️
... and 1 more

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 4234db2...771871a. Read the comment docs.

@zeripath zeripath added the topic/ui Change the appearance of the Gitea UI label Jan 26, 2020
@zeripath zeripath modified the milestones: 1.12.0, 1.11.0 Jan 26, 2020
zeripath
zeripath reviewed Jan 26, 2020
View reviewed changes
models/pull.go Outdated Show resolved Hide resolved
@zeripath
Copy link
Contributor

Actually we should use FullName() for this. (That might make backporting a bit of a problem as we've made a small improvement in master/1.12-dev whereby the repo.OwnerName is stored on the repo table.

@GiteaBot GiteaBot 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 Jan 26, 2020
lunny
lunny previously requested changes Jan 27, 2020
View reviewed changes
models/pull.go
@@ -185,7 +185,7 @@ func (pr *PullRequest) GetDefaultMergeMessage() string {
log.Error("Cannot load issue %d for PR id %d: Error: %v", pr.IssueID, pr.ID, err)
Copy link
Member

@lunny lunny Jan 27, 2020 •
edited

Choose a reason for hiding this comment

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

I would like to confirm if the head repository is the same as the base repository. Then return different message. When that is the same repository, we could ignore %s/%s:

Copy link
Contributor

Choose a reason for hiding this comment

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

@lunny take a look I've just pushed some changes up.

@zeripath zeripath dismissed lunny’s stale review January 28, 2020 08:29

Review is stale

@zeripath zeripath merged commit a08175a into go-gitea:master Jan 28, 2020
@zeripath
Copy link
Contributor

Please send backport

zeripath added a commit to zeripath/gitea that referenced this pull request Feb 15, 2020
@shashvat-kedia @lafriks @zeripath
Repo name added to automatically generated commit message when mergin… (
fe78087 
go-gitea#9997)

* Repo name added to automatically generated commit message when merging pull request

* As per @lunny

Co-authored-by: Lauris BH <lauris@nix.lv>
Co-authored-by: zeripath <art27@cantab.net>
@zeripath zeripath mentioned this pull request Feb 15, 2020
Merged
zeripath added a commit that referenced this pull request Feb 15, 2020
@zeripath @shashvat-kedia @lafriks
Repo name added to automatically generated commit message when mergin… (
ef89260 
#9997) (#10285)

* Repo name added to automatically generated commit message when merging pull request

* As per @lunny

Co-authored-by: Shashvat Kedia <sk261@snu.edu.in>
Co-authored-by: Lauris BH <lauris@nix.lv>
@zeripath zeripath added the backport/done All backports for this PR have been created label Feb 16, 2020
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@zeripath zeripath zeripath approved these changes

@lafriks lafriks lafriks approved these changes

@adelowo adelowo adelowo approved these changes

@6543 6543 6543 approved these changes

@lunny lunny lunny left review comments

Assignees
No one assigned
Labels
backport/done All backports for this PR have been created 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/bug
Projects
None yet
Milestone
1.12.0
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@shashvat-kedia @zeripath @codecov-io @lunny @lafriks @adelowo @6543 @GiteaBot