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.

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

lafriks
Copy link
Member

@lafriks 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 Keep merged PR info in hidden branch #3222)

Screenshots:
attels

attels

attels

attels

attels

@lafriks lafriks added type/feature Completely new functionality. Can only be merged if feature freeze is not active. pr/wip This PR is not ready for review labels Dec 13, 2017
@lafriks lafriks added this to the 1.x.x milestone Dec 13, 2017
@lunny lunny mentioned this pull request Dec 13, 2017
20 tasks
@codecov-io
Copy link

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.

@tboerger tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Dec 13, 2017
Copy link
Member

@lunny lunny left a comment

Choose a reason for hiding this comment

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

And maybe splited two PRs for quick review.

sess := x.NewSession()
defer sess.Close()

//Updating existing issue units
Copy link
Member

Choose a reason for hiding this comment

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

Need sess.Begin()

@lafriks
Copy link
Member Author

lafriks commented Dec 13, 2017

@lunny how do you mean to split them?

@lafriks
Copy link
Member Author

lafriks commented Dec 13, 2017

@lunny fixed

@ethantkoenig
Copy link
Member

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

@ethantkoenig
Copy link
Member

ethantkoenig commented Dec 17, 2017

@lafriks When I do a squash, the author of the squashed commit is [email protected], even though the author of all of the original commits was [email protected].

Perhaps we should include authorship check in our tests?

@lafriks
Copy link
Member Author

lafriks commented Dec 17, 2017

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

@ethantkoenig
Copy link
Member

@lafriks [email protected] was also the poster.

@ethantkoenig
Copy link
Member

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

@lafriks
Copy link
Member Author

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
Copy link
Member

You're right, good catch

@lafriks
Copy link
Member Author

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 pr/wip This PR is not ready for review label Dec 17, 2017
@lafriks
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member Author

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 feat/add_pr_merge_options branch 4 times, most recently from aab0620 to f53d612 Compare December 18, 2017 01:49
Copy link
Member

@ethantkoenig ethantkoenig left a comment

Choose a reason for hiding this comment

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

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 {
Copy link
Member

Choose a reason for hiding this comment

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

nit: use models.MergeStyle

models/pull.go Outdated

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

Choose a reason for hiding this comment

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

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

models/pull.go Outdated

const (
// MergeStyleRegular create merge commit
MergeStyleRegular MergeStyle = "merge"
Copy link
Member

Choose a reason for hiding this comment

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

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.
Copy link
Member

Choose a reason for hiding this comment

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

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

form.Do = "merge"
}

if models.MergeStyle(form.Do) != models.MergeStyleRegular &&
Copy link
Member

Choose a reason for hiding this comment

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

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

models/pull.go Outdated
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()
Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Member

Choose a reason for hiding this comment

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

nit: whitespaces -> whitespace

models/pull.go Outdated
@@ -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)
Copy link
Member

Choose a reason for hiding this comment

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

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")
Copy link
Member

@ethantkoenig ethantkoenig Dec 18, 2017

Choose a reason for hiding this comment

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

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.)?

Copy link
Member

Choose a reason for hiding this comment

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

@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)
}
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

@lafriks
Copy link
Member Author

lafriks commented Dec 31, 2017

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

@ethantkoenig
Copy link
Member

@lafriks #3188 (comment)

@lafriks
Copy link
Member Author

lafriks commented Dec 31, 2017

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

@lunny
Copy link
Member

lunny commented Jan 1, 2018

@lafriks is there one still waiting a change?

@lafriks
Copy link
Member Author

lafriks commented Jan 1, 2018

@lunny yes @ethantkoenig wants more tests

@lunny
Copy link
Member

lunny commented Jan 5, 2018

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

@ethantkoenig
Copy link
Member

okay

@lafriks
Copy link
Member Author

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
@lafriks lafriks deleted the feat/add_pr_merge_options branch January 5, 2018 18:56
@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.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/feature Completely new functionality. Can only be merged if feature freeze is not active.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pull request merge strategy
6 participants