repo/pull: allow rebase before merging (#4805)

conf/locale/locale_en-US.ini

@@ -632,8 +632,9 @@ pulls.is_checking = The conflict checking is still in progress, please refresh p
 pulls.can_auto_merge_desc = This pull request can be merged automatically.
 pulls.cannot_auto_merge_desc = This pull request can't be merged automatically because there are conflicts.
 pulls.cannot_auto_merge_helper = Please merge manually in order to resolve the conflicts.
+pulls.create_merge_commit = Create a merge commit
+pulls.rebase_before_merging = Rebase before merging
 pulls.merge_pull_request = Merge Pull Request
-pulls.rebase_merge_pull_request = Rebase and Merge Pull Request
 pulls.open_unmerged_pull_exists = `You can't perform reopen operation because there is already an open pull request (#%d) from same repository with same merge information and is waiting for merging.`
 pulls.delete_branch = Delete Branch
 pulls.delete_branch_has_new_commits = Branch cannot be deleted because it has new commits after mergence.
@@ -738,7 +739,7 @@ settings.tracker_issue_style.numeric = Numeric
 settings.tracker_issue_style.alphanumeric = Alphanumeric
 settings.tracker_url_format_desc = You can use placeholder <code>{user} {repo} {index}</code> for user name, repository name and issue index.
 settings.pulls_desc = Enable pull requests to accept public contributions
-settings.use_rebase_desc = Use rebase to merge pull requests
+settings.pulls.allow_rebase_merge = Allow use rebase to merge commits
 settings.danger_zone = Danger Zone
 settings.cannot_fork_to_same_owner = You cannot fork a repository to its original owner.
 settings.new_owner_has_same_repo = The new owner already has a repository with same name. Please choose another name.

models/pull.go

@@ -183,9 +183,17 @@ func (pr *PullRequest) CanAutoMerge() bool {
 	return pr.Status == PULL_REQUEST_STATUS_MERGEABLE
 }
 
+// MergeStyle represents the approach to merge commits into base branch.
+type MergeStyle string
+
+const (
+	MERGE_STYLE_REGULAR MergeStyle = "create_merge_commit"
+	MERGE_STYLE_REBASE  MergeStyle = "rebase_before_merging"
+)
+
 // Merge merges pull request to base repository.
 // FIXME: add repoWorkingPull make sure two merges does not happen at same time.
-func (pr *PullRequest) Merge(doer *User, baseGitRepo *git.Repository) (err error) {
+func (pr *PullRequest) Merge(doer *User, baseGitRepo *git.Repository, mergeStyle MergeStyle) (err error) {
 	defer func() {
 		go HookQueue.Add(pr.BaseRepo.ID)
 		go AddTestPullRequestTask(doer, pr.BaseRepo.ID, pr.BaseBranch, false)
@@ -239,15 +247,13 @@ func (pr *PullRequest) Merge(doer *User, baseGitRepo *git.Repository) (err error
 		return fmt.Errorf("git fetch [%s -> %s]: %s", headRepoPath, tmpBasePath, stderr)
 	}
 
-	if (pr.BaseRepo.PullUseRebase) {
-		// Rebase.
-		if _, stderr, err = process.ExecDir(-1, tmpBasePath,
-			fmt.Sprintf("PullRequest.Rebase (git rebase): %s", tmpBasePath),
-			"git", "rebase", "-q", pr.BaseBranch, "head_repo/"+pr.HeadBranch); err != nil {
-			return fmt.Errorf("git rebase [%s -> %s]: %s", headRepoPath, tmpBasePath, stderr)
-		}
-	} else {
-		// Merge commits.
+	// Check if merge style is allowed, reset to default style if not
+	if mergeStyle == MERGE_STYLE_REBASE && !pr.BaseRepo.PullsAllowRebase {
+		mergeStyle = MERGE_STYLE_REGULAR
+	}
+
+	switch mergeStyle {
+	case MERGE_STYLE_REGULAR: // Create merge commit
 		if _, stderr, err = process.ExecDir(-1, tmpBasePath,
 			fmt.Sprintf("PullRequest.Merge (git merge --no-ff --no-commit): %s", tmpBasePath),
 			"git", "merge", "--no-ff", "--no-commit", "head_repo/"+pr.HeadBranch); err != nil {
@@ -261,6 +267,16 @@ func (pr *PullRequest) Merge(doer *User, baseGitRepo *git.Repository) (err error
 			"-m", fmt.Sprintf("Merge branch '%s' of %s/%s into %s", pr.HeadBranch, pr.HeadUserName, pr.HeadRepo.Name, pr.BaseBranch)); err != nil {
 			return fmt.Errorf("git commit [%s]: %v - %s", tmpBasePath, err, stderr)
 		}
+
+	case MERGE_STYLE_REBASE: // Rebase before merging
+		if _, stderr, err = process.ExecDir(-1, tmpBasePath,
+			fmt.Sprintf("PullRequest.Merge (git rebase): %s", tmpBasePath),
+			"git", "rebase", "-q", pr.BaseBranch, "head_repo/"+pr.HeadBranch); err != nil {
+			return fmt.Errorf("git rebase [%s -> %s]: %s", headRepoPath, tmpBasePath, stderr)
+		}
+
+	default:
+		return fmt.Errorf("unknown merge style: %s", mergeStyle)
 	}
 
 	// Push back to upstream.

models/repo.go

@@ -184,7 +184,7 @@ type Repository struct {
 	ExternalTrackerStyle  string
 	ExternalMetas         map[string]string `xorm:"-"`
 	EnablePulls           bool              `xorm:"NOT NULL DEFAULT true"`
-	PullUseRebase         bool              `xorm:"NOT NULL DEFAULT false"`
+	PullsAllowRebase      bool              `xorm:"NOT NULL DEFAULT false"`
 
 	IsFork   bool `xorm:"NOT NULL DEFAULT false"`
 	ForkID   int64

pkg/form/repo.go

@@ -102,7 +102,7 @@ type RepoSetting struct {
 	TrackerURLFormat      string
 	TrackerIssueStyle     string
 	EnablePulls           bool
-	PullUseRebase         bool
+	PullsAllowRebase      bool
 }
 
 func (f *RepoSetting) Validate(ctx *macaron.Context, errs binding.Errors) binding.Errors {

routes/repo/pull.go

@@ -389,7 +389,7 @@ func MergePullRequest(c *context.Context) {
 		return
 	}
 	if issue.IsClosed {
-		c.Handle(404, "MergePullRequest", nil)
+		c.NotFound()
 		return
 	}
 
@@ -400,13 +400,13 @@ func MergePullRequest(c *context.Context) {
 	}
 
 	if !pr.CanAutoMerge() || pr.HasMerged {
-		c.Handle(404, "MergePullRequest", nil)
+		c.NotFound()
 		return
 	}
 
 	pr.Issue = issue
 	pr.Issue.Repo = c.Repo.Repository
-	if err = pr.Merge(c.User, c.Repo.GitRepo); err != nil {
+	if err = pr.Merge(c.User, c.Repo.GitRepo, models.MergeStyle(c.Query("merge_style"))); err != nil {
 		c.ServerError("Merge", err)
 		return
 	}

routes/repo/setting.go

@@ -147,7 +147,7 @@ func SettingsPost(c *context.Context, f form.RepoSetting) {
 		repo.ExternalTrackerFormat = f.TrackerURLFormat
 		repo.ExternalTrackerStyle = f.TrackerIssueStyle
 		repo.EnablePulls = f.EnablePulls
-		repo.PullUseRebase = f.PullUseRebase
+		repo.PullsAllowRebase = f.PullsAllowRebase
 
 		if err := models.UpdateRepository(repo, false); err != nil {
 			c.ServerError("UpdateRepository", err)

templates/repo/issue/view_content.tmpl

@@ -190,20 +190,29 @@
 									<span class="octicon octicon-check"></span>
 									{{$.i18n.Tr "repo.pulls.can_auto_merge_desc"}}
 								</div>
+
 								{{if .IsRepositoryWriter}}
 									<div class="ui divider"></div>
-									<div>
-										<form class="ui form" action="{{.Link}}/merge" method="post">
-											{{.CSRFTokenHTML}}
-											<button class="ui green button">
-												{{if .Issue.Repo.PullUseRebase }}
-													<span class="octicon octicon-git-pull-request"></span> {{$.i18n.Tr "repo.pulls.rebase_merge_pull_request"}}
-												{{else}}
-													<span class="octicon octicon-git-merge"></span> {{$.i18n.Tr "repo.pulls.merge_pull_request"}}
-												{{end}}
-											</button>
-										</form>
-									</div>
+									<form class="ui form" action="{{.Link}}/merge" method="post">
+										{{.CSRFTokenHTML}}
+										<div class="field">
+											<div class="ui radio checkbox">
+											  <input type="radio" name="merge_style" value="create_merge_commit" checked="checked">
+											  <label>{{$.i18n.Tr "repo.pulls.create_merge_commit"}}</label>
+											</div>
+										</div>
+										{{if .Issue.Repo.PullsAllowRebase}}
+											<div class="field">
+												<div class="ui radio checkbox">
+												  <input type="radio" name="merge_style" value="rebase_before_merging">
+												  <label>{{$.i18n.Tr "repo.pulls.rebase_before_merging"}}</label>
+												</div>
+											</div>
+										{{end}}
+										<button class="ui green button">
+											<span class="octicon octicon-git-merge"></span> {{$.i18n.Tr "repo.pulls.merge_pull_request"}}
+										</button>
+									</form>
 								{{end}}
 							{{else}}
 								<div class="item text red">

templates/repo/settings/options.tmpl

@@ -199,8 +199,8 @@
 							</div>
 							<div class="ui segment field {{if not .Repository.EnablePulls}}disabled{{end}}" id="pull_box">
 								<div class="ui checkbox">
-									<input name="pull_use_rebase" type="checkbox" {{if .Repository.PullUseRebase}}checked{{end}}>
-									<label>{{.i18n.Tr "repo.settings.use_rebase_desc"}}</label>
+									<input name="pulls_allow_rebase" type="checkbox" {{if .Repository.PullsAllowRebase}}checked{{end}}>
+									<label>{{.i18n.Tr "repo.settings.pulls.allow_rebase_merge"}}</label>
 								</div>
 							</div>
 						{{end}}