Make manual merge autodetection optional and add manual merge as merge method (#12543)

* Make auto check manual merge as a chooseable mod and add manual merge way on ui

as title, Before this pr, we use same way with GH to check manually merge.
It good, but in some special cases, misjudgments can occur. and it's hard
to fix this bug. So I add option to allow repo manager block "auto check manual merge"
function, Then it will have same style like gitlab(allow empty pr). and to compensate for
not being able to detect THE PR merge automatically, I added a manual approach.

Signed-off-by: a1012112796 <[email protected]>

* make swager

* api support

* ping ci

* fix TestPullCreate_EmptyChangesWithCommits

* Apply suggestions from code review

Co-authored-by: zeripath <[email protected]>

* Apply review suggestions and add test

* Apply suggestions from code review

Co-authored-by: zeripath <[email protected]>

* fix build

* test error message

* make fmt

* Fix indentation issues identified by @silverwind

Co-authored-by: silverwind <[email protected]>

* Fix tests and make manually merged disabled error on API the same

Signed-off-by: Andrew Thornton <[email protected]>

* a small nit

* fix wrong commit id error

* fix bug

* simple test

* fix test

Co-authored-by: zeripath <[email protected]>
Co-authored-by: silverwind <[email protected]>
Co-authored-by: techknowlogick <[email protected]>

Author: 4 people

integrations/api_helper_for_declarative_test.go

@@ -9,6 +9,7 @@ import (
 	"fmt"
 	"io/ioutil"
 	"net/http"
+	"net/url"
 	"testing"
 	"time"
 
@@ -71,6 +72,23 @@ func doAPICreateRepository(ctx APITestContext, empty bool, callback ...func(*tes
 	}
 }
 
+func doAPIEditRepository(ctx APITestContext, editRepoOption *api.EditRepoOption, callback ...func(*testing.T, api.Repository)) func(*testing.T) {
+	return func(t *testing.T) {
+		req := NewRequestWithJSON(t, "PATCH", fmt.Sprintf("/api/v1/repos/%s/%s?token=%s", url.PathEscape(ctx.Username), url.PathEscape(ctx.Reponame), ctx.Token), editRepoOption)
+		if ctx.ExpectedCode != 0 {
+			ctx.Session.MakeRequest(t, req, ctx.ExpectedCode)
+			return
+		}
+		resp := ctx.Session.MakeRequest(t, req, http.StatusOK)
+
+		var repository api.Repository
+		DecodeJSON(t, resp, &repository)
+		if len(callback) > 0 {
+			callback[0](t, repository)
+		}
+	}
+}
+
 func doAPIAddCollaborator(ctx APITestContext, username string, mode models.AccessMode) func(*testing.T) {
 	return func(t *testing.T) {
 		permission := "read"
@@ -256,6 +274,23 @@ func doAPIMergePullRequest(ctx APITestContext, owner, repo string, index int64)
 	}
 }
 
+func doAPIManuallyMergePullRequest(ctx APITestContext, owner, repo, commitID string, index int64) func(*testing.T) {
+	return func(t *testing.T) {
+		urlStr := fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/merge?token=%s",
+			owner, repo, index, ctx.Token)
+		req := NewRequestWithJSON(t, http.MethodPost, urlStr, &auth.MergePullRequestForm{
+			Do:            string(models.MergeStyleManuallyMerged),
+			MergeCommitID: commitID,
+		})
+
+		if ctx.ExpectedCode != 0 {
+			ctx.Session.MakeRequest(t, req, ctx.ExpectedCode)
+			return
+		}
+		ctx.Session.MakeRequest(t, req, 200)
+	}
+}
+
 func doAPIGetBranch(ctx APITestContext, branch string, callback ...func(*testing.T, api.Branch)) func(*testing.T) {
 	return func(t *testing.T) {
 		req := NewRequestf(t, "GET", "/api/v1/repos/%s/%s/branches/%s?token=%s", ctx.Username, ctx.Reponame, branch, ctx.Token)

integrations/git_test.go

@@ -69,6 +69,7 @@ func testGit(t *testing.T, u *url.URL) {
 		mediaTest(t, &httpContext, little, big, littleLFS, bigLFS)
 
 		t.Run("BranchProtectMerge", doBranchProtectPRMerge(&httpContext, dstPath))
+		t.Run("CreatePRAndSetManuallyMerged", doCreatePRAndSetManuallyMerged(httpContext, httpContext, dstPath, "master", "test-manually-merge"))
 		t.Run("MergeFork", func(t *testing.T) {
 			defer PrintCurrentTest(t)()
 			t.Run("CreatePRAndMerge", doMergeFork(httpContext, forkedUserCtx, "master", httpContext.Username+":master"))
@@ -468,6 +469,35 @@ func doMergeFork(ctx, baseCtx APITestContext, baseBranch, headBranch string) fun
 	}
 }
 
+func doCreatePRAndSetManuallyMerged(ctx, baseCtx APITestContext, dstPath, baseBranch, headBranch string) func(t *testing.T) {
+	return func(t *testing.T) {
+		defer PrintCurrentTest(t)()
+		var (
+			pr           api.PullRequest
+			err          error
+			lastCommitID string
+		)
+
+		trueBool := true
+		falseBool := false
+
+		t.Run("AllowSetManuallyMergedAndSwitchOffAutodetectManualMerge", doAPIEditRepository(baseCtx, &api.EditRepoOption{
+			HasPullRequests:       &trueBool,
+			AllowManualMerge:      &trueBool,
+			AutodetectManualMerge: &falseBool,
+		}))
+
+		t.Run("CreateHeadBranch", doGitCreateBranch(dstPath, headBranch))
+		t.Run("PushToHeadBranch", doGitPushTestRepository(dstPath, "origin", headBranch))
+		t.Run("CreateEmptyPullRequest", func(t *testing.T) {
+			pr, err = doAPICreatePullRequest(ctx, baseCtx.Username, baseCtx.Reponame, baseBranch, headBranch)(t)
+			assert.NoError(t, err)
+		})
+		lastCommitID = pr.Base.Sha
+		t.Run("ManuallyMergePR", doAPIManuallyMergePullRequest(ctx, baseCtx.Username, baseCtx.Reponame, lastCommitID, pr.Index))
+	}
+}
+
 func doEnsureCanSeePull(ctx APITestContext, pr api.PullRequest) func(t *testing.T) {
 	return func(t *testing.T) {
 		req := NewRequest(t, "GET", fmt.Sprintf("/%s/%s/pulls/%d", url.PathEscape(ctx.Username), url.PathEscape(ctx.Reponame), pr.Index))

integrations/pull_status_test.go

@@ -115,6 +115,6 @@ func TestPullCreate_EmptyChangesWithCommits(t *testing.T) {
 		doc := NewHTMLParser(t, resp.Body)
 
 		text := strings.TrimSpace(doc.doc.Find(".merge-section").Text())
-		assert.Contains(t, text, "This pull request can be merged automatically.")
+		assert.Contains(t, text, "This branch is equal with the target branch.")
 	})
 }

models/pull.go

@@ -35,6 +35,7 @@ const (
 	PullRequestStatusMergeable
 	PullRequestStatusManuallyMerged
 	PullRequestStatusError
+	PullRequestStatusEmpty
 )
 
 // PullRequest represents relation between pull request and repositories.
@@ -332,6 +333,11 @@ func (pr *PullRequest) CanAutoMerge() bool {
 	return pr.Status == PullRequestStatusMergeable
 }
 
+// IsEmpty returns true if this pull request is empty.
+func (pr *PullRequest) IsEmpty() bool {
+	return pr.Status == PullRequestStatusEmpty
+}
+
 // MergeStyle represents the approach to merge commits into base branch.
 type MergeStyle string
 
@@ -344,6 +350,8 @@ const (
 	MergeStyleRebaseMerge MergeStyle = "rebase-merge"
 	// MergeStyleSquash squash commits into single commit before merging
 	MergeStyleSquash MergeStyle = "squash"
+	// MergeStyleManuallyMerged pr has been merged manually, just mark it as merged directly
+	MergeStyleManuallyMerged MergeStyle = "manually-merged"
 )
 
 // SetMerged sets a pull request to merged and closes the corresponding issue

models/repo_unit.go

@@ -101,6 +101,8 @@ type PullRequestsConfig struct {
 	AllowRebase               bool
 	AllowRebaseMerge          bool
 	AllowSquash               bool
+	AllowManualMerge          bool
+	AutodetectManualMerge     bool
 }
 
 // FromDB fills up a PullRequestsConfig from serialized format.
@@ -120,7 +122,8 @@ func (cfg *PullRequestsConfig) IsMergeStyleAllowed(mergeStyle MergeStyle) bool {
 	return mergeStyle == MergeStyleMerge && cfg.AllowMerge ||
 		mergeStyle == MergeStyleRebase && cfg.AllowRebase ||
 		mergeStyle == MergeStyleRebaseMerge && cfg.AllowRebaseMerge ||
-		mergeStyle == MergeStyleSquash && cfg.AllowSquash
+		mergeStyle == MergeStyleSquash && cfg.AllowSquash ||
+		mergeStyle == MergeStyleManuallyMerged && cfg.AllowManualMerge
 }
 
 // AllowedMergeStyleCount returns the total count of allowed merge styles for the PullRequestsConfig

modules/forms/repo_form.go

@@ -156,6 +156,8 @@ type RepoSettingForm struct {
 	PullsAllowRebase                 bool
 	PullsAllowRebaseMerge            bool
 	PullsAllowSquash                 bool
+	PullsAllowManualMerge            bool
+	EnableAutodetectManualMerge      bool
 	EnableTimetracker                bool
 	AllowOnlyContributorsToTrackTime bool
 	EnableIssueDependencies          bool
@@ -556,11 +558,12 @@ func (f *InitializeLabelsForm) Validate(req *http.Request, errs binding.Errors)
 // swagger:model MergePullRequestOption
 type MergePullRequestForm struct {
 	// required: true
-	// enum: merge,rebase,rebase-merge,squash
-	Do                string `binding:"Required;In(merge,rebase,rebase-merge,squash)"`
+	// enum: merge,rebase,rebase-merge,squash,manually-merged
+	Do                string `binding:"Required;In(merge,rebase,rebase-merge,squash,manually-merged)"`
 	MergeTitleField   string
 	MergeMessageField string
-	ForceMerge        *bool `json:"force_merge,omitempty"`
+	MergeCommitID     string // only used for manually-merged
+	ForceMerge        *bool  `json:"force_merge,omitempty"`
 }
 
 // Validate validates the fields

modules/git/repo_commit.go

@@ -456,3 +456,12 @@ func (repo *Repository) GetCommitsFromIDs(commitIDs []string) (commits *list.Lis
 
 	return commits
 }
+
+// IsCommitInBranch check if the commit is on the branch
+func (repo *Repository) IsCommitInBranch(commitID, branch string) (r bool, err error) {
+	stdout, err := NewCommand("branch", "--contains", commitID, branch).RunInDir(repo.Path)
+	if err != nil {
+		return false, err
+	}
+	return len(stdout) > 0, err
+}

modules/git/repo_commit_test.go

@@ -63,3 +63,18 @@ func TestGetCommitWithBadCommitID(t *testing.T) {
 	assert.Error(t, err)
 	assert.True(t, IsErrNotExist(err))
 }
+
+func TestIsCommitInBranch(t *testing.T) {
+	bareRepo1Path := filepath.Join(testReposDir, "repo1_bare")
+	bareRepo1, err := OpenRepository(bareRepo1Path)
+	assert.NoError(t, err)
+	defer bareRepo1.Close()
+
+	result, err := bareRepo1.IsCommitInBranch("2839944139e0de9737a044f78b0e4b40d989a9e3", "branch1")
+	assert.NoError(t, err)
+	assert.Equal(t, true, result)
+
+	result, err = bareRepo1.IsCommitInBranch("2839944139e0de9737a044f78b0e4b40d989a9e3", "branch2")
+	assert.NoError(t, err)
+	assert.Equal(t, false, result)
+}

modules/structs/repo.go

@@ -167,6 +167,10 @@ type EditRepoOption struct {
 	AllowRebaseMerge *bool `json:"allow_rebase_explicit,omitempty"`
 	// either `true` to allow squash-merging pull requests, or `false` to prevent squash-merging. `has_pull_requests` must be `true`.
 	AllowSquash *bool `json:"allow_squash_merge,omitempty"`
+	// either `true` to allow mark pr as merged manually, or `false` to prevent it. `has_pull_requests` must be `true`.
+	AllowManualMerge *bool `json:"allow_manual_merge,omitempty"`
+	// either `true` to enable AutodetectManualMerge, or `false` to prevent it. `has_pull_requests` must be `true`, Note: In some special cases, misjudgments can occur.
+	AutodetectManualMerge *bool `json:"autodetect_manual_merge,omitempty"`
 	// set to `true` to archive this repository.
 	Archived *bool `json:"archived,omitempty"`
 	// set to a string like `8h30m0s` to set the mirror interval time

options/locale/locale_en-US.ini

@@ -1105,6 +1105,7 @@ issues.context.delete = Delete
 issues.no_content = There is no content yet.
 issues.close_issue = Close
 issues.pull_merged_at = `merged commit <a href="%[1]s">%[2]s</a> into <b>%[3]s</b> %[4]s`
+issues.manually_pull_merged_at = `merged commit <a href="%[1]s">%[2]s</a> into <b>%[3]s</b> %[4]s manually`
 issues.close_comment_issue = Comment and Close
 issues.reopen_issue = Reopen
 issues.reopen_comment_issue = Comment and Reopen
@@ -1273,6 +1274,7 @@ pulls.compare_compare = pull from
 pulls.filter_branch = Filter branch
 pulls.no_results = No results found.
 pulls.nothing_to_compare = These branches are equal. There is no need to create a pull request.
+pulls.nothing_to_compare_and_allow_empty_pr = These branches are equal. This PR will be empty.
 pulls.has_pull_request = `A pull request between these branches already exists: <a href="%[1]s/pulls/%[3]d">%[2]s#%[3]d</a>`
 pulls.create = Create Pull Request
 pulls.title_desc = wants to merge %[1]d commits from <code>%[2]s</code> into <code id="branch_target">%[3]s</code>
@@ -1285,13 +1287,16 @@ pulls.reopen_to_merge = Please reopen this pull request to perform a merge.
 pulls.cant_reopen_deleted_branch = This pull request cannot be reopened because the branch was deleted.
 pulls.merged = Merged
 pulls.merged_as = The pull request has been merged as <a rel="nofollow" class="ui sha" href="%[1]s"><code>%[2]s</code></a>.
+pulls.manually_merged = Manually merged
+pulls.manually_merged_as = The pull request has been manually merged as <a rel="nofollow" class="ui sha" href="%[1]s"><code>%[2]s</code></a>.
 pulls.is_closed = The pull request has been closed.
 pulls.has_merged = The pull request has been merged.
 pulls.title_wip_desc = `<a href="#">Start the title with <strong>%s</strong></a> to prevent the pull request from being merged accidentally.`
 pulls.cannot_merge_work_in_progress = This pull request is marked as a work in progress. Remove the <strong>%s</strong> prefix from the title when it's ready
 pulls.data_broken = This pull request is broken due to missing fork information.
 pulls.files_conflicted = This pull request has changes conflicting with the target branch.
 pulls.is_checking = "Merge conflict checking is in progress. Try again in few moments."
+pulls.is_empty = "This branch is equal with the target branch."
 pulls.required_status_check_failed = Some required checks were not successful.
 pulls.required_status_check_missing = Some required checks are missing.
 pulls.required_status_check_administrator = As an administrator, you may still merge this pull request.
@@ -1312,6 +1317,7 @@ pulls.reject_count_1 = "%d change request"
 pulls.reject_count_n = "%d change requests"
 pulls.waiting_count_1 = "%d waiting review"
 pulls.waiting_count_n = "%d waiting reviews"
+pulls.wrong_commit_id = "commit id must be a commit id on the target branch"
 
 pulls.no_merge_desc = This pull request cannot be merged because all repository merge options are disabled.
 pulls.no_merge_helper = Enable merge options in the repository settings or merge the pull request manually.
@@ -1322,6 +1328,8 @@ pulls.merge_pull_request = Merge Pull Request
 pulls.rebase_merge_pull_request = Rebase and Merge
 pulls.rebase_merge_commit_pull_request = Rebase and Merge (--no-ff)
 pulls.squash_merge_pull_request = Squash and Merge
+pulls.merge_manually = Manually merged
+pulls.merge_commit_id = The merge commit ID
 pulls.require_signed_wont_sign = The branch requires signed commits but this merge will not be signed
 pulls.invalid_merge_option = You cannot use this merge option for this pull request.
 pulls.merge_conflict = Merge Failed: There was a conflict whilst merging. Hint: Try a different strategy
@@ -1545,6 +1553,8 @@ settings.pulls.allow_merge_commits = Enable Commit Merging
 settings.pulls.allow_rebase_merge = Enable Rebasing to Merge Commits
 settings.pulls.allow_rebase_merge_commit = Enable Rebasing with explicit merge commits (--no-ff)
 settings.pulls.allow_squash_commits = Enable Squashing to Merge Commits
+settings.pulls.allow_manual_merge = Enable Mark PR as manually merged
+settings.pulls.enable_autodetect_manual_merge = Enable autodetect manual merge (Note: In some special cases, misjudgments can occur)
 settings.projects_desc = Enable Repository Projects
 settings.admin_settings = Administrator Settings
 settings.admin_enable_health_check = Enable Repository Health Checks (git fsck)

routers/api/v1/repo/pull.go

@@ -769,13 +769,31 @@ func MergePullRequest(ctx *context.APIContext) {
 		return
 	}
 
-	if !pr.CanAutoMerge() {
-		ctx.Error(http.StatusMethodNotAllowed, "PR not in mergeable state", "Please try again later")
+	if pr.HasMerged {
+		ctx.Error(http.StatusMethodNotAllowed, "PR already merged", "")
 		return
 	}
 
-	if pr.HasMerged {
-		ctx.Error(http.StatusMethodNotAllowed, "PR already merged", "")
+	// handle manually-merged mark
+	if models.MergeStyle(form.Do) == models.MergeStyleManuallyMerged {
+		if err = pull_service.MergedManually(pr, ctx.User, ctx.Repo.GitRepo, form.MergeCommitID); err != nil {
+			if models.IsErrInvalidMergeStyle(err) {
+				ctx.Error(http.StatusMethodNotAllowed, "Invalid merge style", fmt.Errorf("%s is not allowed an allowed merge style for this repository", models.MergeStyle(form.Do)))
+				return
+			}
+			if strings.Contains(err.Error(), "Wrong commit ID") {
+				ctx.JSON(http.StatusConflict, err)
+				return
+			}
+			ctx.Error(http.StatusInternalServerError, "Manually-Merged", err)
+			return
+		}
+		ctx.Status(http.StatusOK)
+		return
+	}
+
+	if !pr.CanAutoMerge() {
+		ctx.Error(http.StatusMethodNotAllowed, "PR not in mergeable state", "Please try again later")
 		return
 	}
 

routers/api/v1/repo/repo.go

@@ -725,6 +725,8 @@ func updateRepoUnits(ctx *context.APIContext, opts api.EditRepoOption) error {
 					AllowRebase:               true,
 					AllowRebaseMerge:          true,
 					AllowSquash:               true,
+					AllowManualMerge:          true,
+					AutodetectManualMerge:     false,
 				}
 			} else {
 				config = unit.PullRequestsConfig()
@@ -745,6 +747,12 @@ func updateRepoUnits(ctx *context.APIContext, opts api.EditRepoOption) error {
 			if opts.AllowSquash != nil {
 				config.AllowSquash = *opts.AllowSquash
 			}
+			if opts.AllowManualMerge != nil {
+				config.AllowManualMerge = *opts.AllowManualMerge
+			}
+			if opts.AutodetectManualMerge != nil {
+				config.AutodetectManualMerge = *opts.AutodetectManualMerge
+			}
 
 			units = append(units, models.RepoUnit{
 				RepoID: repo.ID,

routers/repo/compare.go

@@ -429,6 +429,14 @@ func PrepareCompareDiff(
 
 	if headCommitID == compareInfo.MergeBase {
 		ctx.Data["IsNothingToCompare"] = true
+		if unit, err := repo.GetUnit(models.UnitTypePullRequests); err == nil {
+			config := unit.PullRequestsConfig()
+			if !config.AutodetectManualMerge {
+				ctx.Data["AllowEmptyPr"] = !(baseBranch == headBranch && ctx.Repo.Repository.Name == headRepo.Name)
+			} else {
+				ctx.Data["AllowEmptyPr"] = false
+			}
+		}
 		return true
 	}
 

routers/repo/issue.go

@@ -1491,6 +1491,8 @@ func ViewIssue(ctx *context.Context) {
 				ctx.Data["MergeStyle"] = models.MergeStyleRebaseMerge
 			} else if prConfig.AllowSquash {
 				ctx.Data["MergeStyle"] = models.MergeStyleSquash
+			} else if prConfig.AllowManualMerge {
+				ctx.Data["MergeStyle"] = models.MergeStyleManuallyMerged
 			} else {
 				ctx.Data["MergeStyle"] = ""
 			}
@@ -1531,6 +1533,22 @@ func ViewIssue(ctx *context.Context) {
 			pull.HeadRepo != nil &&
 			git.IsBranchExist(pull.HeadRepo.RepoPath(), pull.HeadBranch) &&
 			(!pull.HasMerged || ctx.Data["HeadBranchCommitID"] == ctx.Data["PullHeadCommitID"])
+
+		stillCanManualMerge := func() bool {
+			if pull.HasMerged || issue.IsClosed || !ctx.IsSigned {
+				return false
+			}
+			if pull.CanAutoMerge() || pull.IsWorkInProgress() || pull.IsChecking() {
+				return false
+			}
+			if (ctx.User.IsAdmin || ctx.Repo.IsAdmin()) && prConfig.AllowManualMerge {
+				return true
+			}
+
+			return false
+		}
+
+		ctx.Data["StillCanManualMerge"] = stillCanManualMerge()
 	}
 
 	// Get Dependencies