From d686e09a8acc2751819dc94b395ff8a9610d9113 Mon Sep 17 00:00:00 2001 From: "Ing. Jaroslav Safka" Date: Fri, 8 Jul 2022 08:34:42 +0200 Subject: [PATCH 1/7] Fix checks in PR for empty commits #19603 * Fixes issue #19603 * fill HeadCommitID in PullRequest * compare real commits ID as check for merging * basd on zeripath patch --- integrations/pull_status_test.go | 28 ++++++++++++++++++- models/issues/pull.go | 5 ++++ options/locale/locale_en-US.ini | 3 +- services/pull/check.go | 2 +- templates/repo/issue/view_content/pull.tmpl | 16 ++++++++--- .../js/components/PullRequestMergeForm.vue | 2 +- 6 files changed, 48 insertions(+), 8 deletions(-) diff --git a/integrations/pull_status_test.go b/integrations/pull_status_test.go index a5247f56ec5f3..e8b7055d548e7 100644 --- a/integrations/pull_status_test.go +++ b/integrations/pull_status_test.go @@ -105,7 +105,11 @@ func doAPICreateCommitStatus(ctx APITestContext, commitID string, status api.Com } } -func TestPullCreate_EmptyChangesWithCommits(t *testing.T) { +func TestPullCreate_EmptyChangesWithDifferentCommits(t *testing.T) { + // Merge must continue if commits SHA are different, even if content is same + // Reason: gitflow and merging master back into develop, where is high possiblity, there are no changes + // but just commit saying "Merge branch". And this meta commit can be also tagged, + // so we need to have this meta commit also in develop branch. onGiteaRun(t, func(t *testing.T, u *url.URL) { session := loginUser(t, "user1") testRepoFork(t, session, "user2", "repo1", "user1", "repo1") @@ -125,6 +129,28 @@ func TestPullCreate_EmptyChangesWithCommits(t *testing.T) { resp := session.MakeRequest(t, req, http.StatusOK) 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.") + }) +} + +func TestPullCreate_EmptyChangesWithSameCommits(t *testing.T) { + onGiteaRun(t, func(t *testing.T, u *url.URL) { + session := loginUser(t, "user1") + testRepoFork(t, session, "user2", "repo1", "user1", "repo1") + testCreateBranch(t, session, "user1", "repo1", "branch/master", "status1", http.StatusSeeOther) + url := path.Join("user1", "repo1", "compare", "master...status1") + req := NewRequestWithValues(t, "POST", url, + map[string]string{ + "_csrf": GetCSRF(t, session, url), + "title": "pull request from status1", + }, + ) + session.MakeRequest(t, req, http.StatusSeeOther) + req = NewRequest(t, "GET", "/user1/repo1/pulls/1") + resp := session.MakeRequest(t, req, http.StatusOK) + doc := NewHTMLParser(t, resp.Body) + text := strings.TrimSpace(doc.doc.Find(".merge-section").Text()) assert.Contains(t, text, "This branch is equal with the target branch.") }) diff --git a/models/issues/pull.go b/models/issues/pull.go index 52b9596889066..8265ac8dab3c1 100644 --- a/models/issues/pull.go +++ b/models/issues/pull.go @@ -423,6 +423,11 @@ func (pr *PullRequest) IsEmpty() bool { return pr.Status == PullRequestStatusEmpty } +// IsAncestor returns true if the Head Commit of this PR is an ancestor of the Base Commit +func (pr *PullRequest) IsAncestor() bool { + return pr.HeadCommitID == pr.MergeBase +} + // SetMerged sets a pull request to merged and closes the corresponding issue func (pr *PullRequest) SetMerged(ctx context.Context) (bool, error) { if pr.HasMerged { diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index eb7ae4774313b..40cbb45ae9bcb 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -1530,7 +1530,8 @@ pulls.remove_prefix = Remove %s prefix 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.is_ancestor = "This branch is already included in the target branch. There is nothing to merge." +pulls.is_empty = "The changes on this branch are already on the target branch. This will be an empty commit." 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. diff --git a/services/pull/check.go b/services/pull/check.go index 6621a281fa543..07ad5ca93ad93 100644 --- a/services/pull/check.go +++ b/services/pull/check.go @@ -89,7 +89,7 @@ func CheckPullMergable(stdCtx context.Context, doer *user_model.User, perm *acce return ErrIsWorkInProgress } - if !pr.CanAutoMerge() { + if !pr.CanAutoMerge() && !pr.IsEmpty() || pr.IsAncestor() { return ErrNotMergableState } diff --git a/templates/repo/issue/view_content/pull.tmpl b/templates/repo/issue/view_content/pull.tmpl index 7ed73e1176f52..921af8401c061 100644 --- a/templates/repo/issue/view_content/pull.tmpl +++ b/templates/repo/issue/view_content/pull.tmpl @@ -195,12 +195,12 @@ {{svg "octicon-sync"}} {{$.locale.Tr "repo.pulls.is_checking"}} - {{else if .Issue.PullRequest.IsEmpty}} + {{else if .Issue.PullRequest.IsAncestor}}
{{svg "octicon-alert" 16}} - {{$.locale.Tr "repo.pulls.is_empty"}} + {{$.locale.Tr "repo.pulls.is_ancestor"}}
- {{else if .Issue.PullRequest.CanAutoMerge}} + {{else if or .Issue.PullRequest.CanAutoMerge .Issue.PullRequest.IsEmpty}} {{if .IsBlockedByApprovals}}
{{svg "octicon-x"}} @@ -282,7 +282,6 @@
{{end}} {{end}} - {{if and (gt .Issue.PullRequest.CommitsBehind 0) (not .Issue.IsClosed) (not .Issue.PullRequest.IsChecking) (not .IsPullFilesConflicted) (not .IsPullRequestBroken) (not $canAutoMerge)}}
@@ -321,6 +320,14 @@
{{end}} + {{if .Issue.PullRequest.IsEmpty}} +
+ +
+ {{svg "octicon-alert" 16}} + {{$.locale.Tr "repo.pulls.is_empty"}} +
+ {{end}} {{if .AllowMerge}} {{/* user is allowed to merge */}} {{$prUnit := .Repository.MustGetUnit $.UnitTypePullRequests}} @@ -348,6 +355,7 @@ 'canMergeNow': {{$canMergeNow}}, 'allOverridableChecksOk': {{not $notAllOverridableChecksOk}}, + 'emptyCommit': {{.Issue.PullRequest.IsEmpty}}, 'pullHeadCommitID': {{.PullHeadCommitID}}, 'isPullBranchDeletable': {{.IsPullBranchDeletable}}, 'defaultDeleteBranchAfterMerge': {{$prUnit.PullRequestsConfig.DefaultDeleteBranchAfterMerge}}, diff --git a/web_src/js/components/PullRequestMergeForm.vue b/web_src/js/components/PullRequestMergeForm.vue index 75fbceb8007a7..29ed42620d51a 100644 --- a/web_src/js/components/PullRequestMergeForm.vue +++ b/web_src/js/components/PullRequestMergeForm.vue @@ -48,7 +48,7 @@
-
+