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

Add Pull Request merge options - Ignore white-space for conflict checking, Rebase, Squash merge #3188

Merged
merged 17 commits into from Jan 5, 2018

Conversation

6 participants
@lafriks
Member

lafriks commented Dec 13, 2017

Fixes #712

Add Pull Request merge options:

  • Ignore whitespaces when checking conflicts
  • Allow merge
  • Allow rebase
  • Allow squash

Tasks:

  • Add config to Pull Request unit
  • Add migration to set defaults
  • Add UI and functionality in Settings section
  • Add ignore whitespace functionality
  • UI changes for merge PR button
  • Add rebase functionality
  • Add squash functionality
  • Add tests
    Fix wrong display of merged PR commits and changes - should be implemented as separate PR (issue #3222)

Screenshots:
attels

attels

attels

attels

attels

@lafriks lafriks added this to the 1.x.x milestone Dec 13, 2017

@lunny lunny referenced this pull request Dec 13, 2017

Open

Gitea hosted Gitea #1029

9 of 9 tasks complete
@codecov-io

This comment has been minimized.

codecov-io commented Dec 13, 2017

Codecov Report

Merging #3188 into master will increase coverage by 0.24%.
The diff coverage is 34.89%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3188      +/-   ##
==========================================
+ Coverage   34.62%   34.86%   +0.24%     
==========================================
  Files         277      278       +1     
  Lines       40298    40506     +208     
==========================================
+ Hits        13952    14122     +170     
- Misses      24298    24299       +1     
- Partials     2048     2085      +37
Impacted Files Coverage Δ
models/migrations/migrations.go 2.89% <ø> (ø) ⬆️
models/error.go 32.43% <0%> (-0.6%) ⬇️
models/repo.go 43.19% <0%> (-0.01%) ⬇️
routers/api/v1/repo/pull.go 6.33% <0%> (-0.23%) ⬇️
routers/repo/setting.go 4.67% <0%> (-0.06%) ⬇️
models/migrations/v54.go 0% <0%> (ø)
routers/routes/routes.go 87.08% <100%> (ø) ⬆️
routers/api/v1/api.go 75.24% <100%> (ø) ⬆️
modules/auth/repo_form.go 33.76% <100%> (+1.76%) ⬆️
routers/repo/issue.go 32.74% <42.1%> (+0.15%) ⬆️
... and 11 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 a192f30...8abe208. Read the comment docs.

@lunny

And maybe splited two PRs for quick review.

sess := x.NewSession()
defer sess.Close()
//Updating existing issue units

This comment has been minimized.

@lunny

lunny Dec 13, 2017

Member

Need sess.Begin()

@lafriks

This comment has been minimized.

Member

lafriks commented Dec 13, 2017

@lunny how do you mean to split them?

@lafriks

This comment has been minimized.

Member

lafriks commented Dec 13, 2017

@lunny fixed

@lunny

lunny approved these changes Dec 14, 2017

@ethantkoenig

This comment has been minimized.

Member

ethantkoenig commented Dec 14, 2017

Fair and early notice: please add tests (at some point, I understand it's a WIP right now)

@ethantkoenig

This comment has been minimized.

Member

ethantkoenig commented Dec 17, 2017

@lafriks When I do a squash, the author of the squashed commit is ethantkoenig@noreply.example.org, even though the author of all of the original commits was ethantkoenig@gmail.com.

Perhaps we should include authorship check in our tests?

@lafriks

This comment has been minimized.

Member

lafriks commented Dec 17, 2017

@ethantkoenig when squashing author is set as PR poster (as theoretically there could be multiple commit authors)

@ethantkoenig

This comment has been minimized.

Member

ethantkoenig commented Dec 17, 2017

@lafriks ethantkoenig@gmail.com was also the poster.

@ethantkoenig

This comment has been minimized.

Member

ethantkoenig commented Dec 17, 2017

Tried again, same thing happened. Let me know if you aren't able to reproduce

@lafriks

This comment has been minimized.

Member

lafriks commented Dec 17, 2017

@ethantkoenig it is same as currently with merge commit. It uses NewGitSig function to get data (name/user). This function uses getEmail function to get user email:

// getEmail returns an noreply email, if the user has set to keep his
// email address private, otherwise the primary email address.

So you have probably set for user to keep email address private.

@ethantkoenig

This comment has been minimized.

Member

ethantkoenig commented Dec 17, 2017

You're right, good catch

@lafriks

This comment has been minimized.

Member

lafriks commented Dec 17, 2017

I'm out of ideas how to preserve commit info in PR when merged with squash or rebase :( As there is no merge commit to reference. Especially for squash merge as original commits are lost completely when PR head branch is deleted.
Any ideas are more than welcome...

@lafriks lafriks removed the status/wip label Dec 17, 2017

@lafriks

This comment has been minimized.

Member

lafriks commented Dec 17, 2017

Looks like github does that by keeping commit info in refs/pull/x/head. But that should probably be made as different PR as this one is already too big.

@lafriks

This comment has been minimized.

Member

lafriks commented Dec 17, 2017

I think it is ready for review now, please test it and comment :)

@lafriks lafriks modified the milestones: 1.x.x, 1.4.0 Dec 17, 2017

@lafriks

This comment has been minimized.

Member

lafriks commented Dec 17, 2017

Automatically adding all commit messages to extended commit message for squash should be implemented as other PR to not make this PR even bigger

@lafriks lafriks force-pushed the lafriks:feat/add_pr_merge_options branch 4 times, most recently from aab0620 to f53d612 Dec 18, 2017

@ethantkoenig

Generally looks good, mostly nits

@@ -16,16 +16,17 @@ import (
"github.com/stretchr/testify/assert"
)
func testPullMerge(t *testing.T, session *TestSession, user, repo, pullnum string) *httptest.ResponseRecorder {
func testPullMerge(t *testing.T, session *TestSession, user, repo, pullnum, mergeStyle string) *httptest.ResponseRecorder {

This comment has been minimized.

@ethantkoenig

ethantkoenig Dec 18, 2017

Member

nit: use models.MergeStyle

// GetDefaultSquashMessage returns default message used when squash and merging pull request
func (pr *PullRequest) GetDefaultSquashMessage() string {
if pr.Issue == nil {

This comment has been minimized.

@ethantkoenig

ethantkoenig Dec 18, 2017

Member

nit: This check is unnecessary, pr.LoadIssue() already does it

const (
// MergeStyleRegular create merge commit
MergeStyleRegular MergeStyle = "merge"

This comment has been minimized.

@ethantkoenig

ethantkoenig Dec 18, 2017

Member

Maybe MergeStyleMerge would be better?

@@ -755,7 +755,12 @@ pulls.is_checking = "The conflict checking is still in progress; please refresh
pulls.can_auto_merge_desc = This pull request can be merged automatically.
pulls.cannot_auto_merge_desc = This pull request cannot be merged automatically because there are conflicts.
pulls.cannot_auto_merge_helper = Please merge manually in order to resolve the conflicts.
pulls.no_merge_desc = This pull request can not be merged as no merge options enabled.

This comment has been minimized.

@ethantkoenig

ethantkoenig Dec 18, 2017

Member

This pull request can not be merged as no merge options are enabled.

form.Do = "merge"
}
if models.MergeStyle(form.Do) != models.MergeStyleRegular &&

This comment has been minimized.

@ethantkoenig

ethantkoenig Dec 18, 2017

Member

nit: This is check unnecessary, since pr.Merge already checks the provided mergeStyle.

if err = pr.GetHeadRepo(); err != nil {
return fmt.Errorf("GetHeadRepo: %v", err)
} else if err = pr.GetBaseRepo(); err != nil {
return fmt.Errorf("GetBaseRepo: %v", err)
}
pullsConfig := pr.BaseRepo.MustGetUnit(UnitTypePullRequests).PullRequestsConfig()

This comment has been minimized.

@ethantkoenig

ethantkoenig Dec 18, 2017

Member

Please use .GetUnit(..), so we don't unnecessarily ignore an error.

pulls.merge_pull_request = Merge Pull Request
pulls.rebase_merge_pull_request = Rebase and Merge
pulls.squash_merge_pull_request = Squash and Merge
pulls.invalid_merge_option = You can not use this merge option for this Pull Request

This comment has been minimized.

@ethantkoenig

ethantkoenig Dec 18, 2017

Member

nit: Pull Request -> pull request

@@ -894,6 +899,10 @@ settings.tracker_url_format_desc = You can use placeholder <code>{user} {repo} {
settings.enable_timetracker = Enable time tracker
settings.allow_only_contributors_to_track_time = Allow only contributors to track time
settings.pulls_desc = Enable pull requests to accept public contributions
settings.pulls.ignore_whitespace = Ignore changes in whitespaces when checking conflicts

This comment has been minimized.

@ethantkoenig

ethantkoenig Dec 18, 2017

Member

nit: whitespaces -> whitespace

@@ -109,6 +109,22 @@ func (pr *PullRequest) loadIssue(e Engine) (err error) {
return err
}
// GetDefaultMergeMessage returns default message used when merging pull request
func (pr *PullRequest) GetDefaultMergeMessage() string {
return fmt.Sprintf("Merge branch '%s' of %s/%s into %s", pr.HeadBranch, pr.HeadUserName, pr.HeadRepo.Name, pr.BaseBranch)

This comment has been minimized.

@ethantkoenig

ethantkoenig Dec 18, 2017

Member

pr.HeadRepo may not always loaded, I think that is why CI is failing.

@@ -58,7 +59,34 @@ func TestPullMerge(t *testing.T) {
elem := strings.Split(test.RedirectURL(resp), "/")
assert.EqualValues(t, "pulls", elem[3])
testPullMerge(t, session, elem[1], elem[2], elem[4])
testPullMerge(t, session, elem[1], elem[2], elem[4], "merge")

This comment has been minimized.

@ethantkoenig

ethantkoenig Dec 18, 2017

Member

If it's not too hard, could we check that the merge itself was successful, (that a commit was created, that correct merge style was used, etc.)?

This comment has been minimized.

@ethantkoenig

ethantkoenig Dec 31, 2017

Member

@lafriks Is this doable?

fmt.Sprintf("PullRequest.Merge (git merge --no-ff --no-commit): %s", tmpBasePath),
"git", "merge", "--no-ff", "--no-commit", "head_repo/"+pr.HeadBranch); err != nil {
return fmt.Errorf("git merge --no-ff --no-commit [%s]: %v - %s", tmpBasePath, err, stderr)
}

This comment has been minimized.

@strk

strk Dec 18, 2017

Member

I know this is not the right place, but shouldn't this functionality be moved to the "git" module ?

This comment has been minimized.

@lafriks

lafriks Dec 18, 2017

Member

Probably that would be better place but as it was there already that should be done in other PR

@lafriks lafriks force-pushed the lafriks:feat/add_pr_merge_options branch from 03bfe37 to 60b2092 Dec 31, 2017

@lafriks

This comment has been minimized.

Member

lafriks commented Dec 31, 2017

@ethantkoenig can I merge? as now approvals also count as LG-TM :)

@ethantkoenig

This comment has been minimized.

Member

ethantkoenig commented Dec 31, 2017

@lafriks

This comment has been minimized.

Member

lafriks commented Dec 31, 2017

@ethantkoenig oh, ok will add that next year :))

@lunny

This comment has been minimized.

Member

lunny commented Jan 1, 2018

@lafriks is there one still waiting a change?

@lafriks

This comment has been minimized.

Member

lafriks commented Jan 1, 2018

@lunny yes @ethantkoenig wants more tests

@lunny

This comment has been minimized.

Member

lunny commented Jan 5, 2018

@lafriks @ethantkoenig I think we can merge this and @lafriks could take another PR to add tests.

@ethantkoenig

This comment has been minimized.

Member

ethantkoenig commented Jan 5, 2018

okay

@lafriks

This comment has been minimized.

Member

lafriks commented Jan 5, 2018

@lunny @ethantkoenig ok, I will improve tests in other PR

@lafriks lafriks merged commit 8ac1501 into go-gitea:master Jan 5, 2018

3 checks passed

Codacy/PR Quality Review Good work! A positive pull request.
Details
approvals/lgtm this commit looks good
continuous-integration/drone/pr the build was successful
Details

@lafriks lafriks deleted the lafriks:feat/add_pr_merge_options branch Jan 5, 2018

AlbertoGP added a commit to AlbertoGP/gitea that referenced this pull request Jan 5, 2018

Add Pull Request merge options - Ignore white-space for conflict chec…
…king, Rebase, Squash merge (go-gitea#3188)

* Pull request options migration and UI in settings

* Add ignore whitespace functionality

* Fix settings if pull requests are disabled

* Fix migration transaction

* Merge with Rebase functionality

* UI changes and related functionality for pull request merging button

* Implement squash functionality

* Fix rebase merging

* Fix pull request merge tests

* Add squash and rebase tests

* Fix API method to reuse default message functions

* Some refactoring and small fixes

* Remove more hardcoded values from tests

* Remove unneeded check from API method

* Fix variable name and comment typo

* Fix reset commit count after PR merge
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment