From 92a43d577d1fbe8e901c303f98a93e0c035de066 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ing=2E=20Jaroslav=20=C5=A0afka?= Date: Wed, 13 Jul 2022 19:36:23 +0200 Subject: [PATCH] Fix checks in PR for empty commits (#20290) (#20352) Backport #20290 * Fix #19603 * fill HeadCommitID in PullRequest * compare real commits ID as check for merging Signed-off-by: Andrew Thornton Co-authored-by: Andrew Thornton --- integrations/pull_status_test.go | 30 +++++++++++++++++-- models/issues/pull.go | 6 ++++ options/locale/locale_en-US.ini | 3 +- services/pull/check.go | 2 +- services/pull/patch.go | 8 +++++ templates/repo/issue/view_content/pull.tmpl | 16 +++++++--- .../js/components/PullRequestMergeForm.vue | 2 +- 7 files changed, 58 insertions(+), 9 deletions(-) diff --git a/integrations/pull_status_test.go b/integrations/pull_status_test.go index a5247f56e..33a27cd81 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") @@ -126,6 +130,28 @@ 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 branch is equal with the target branch.") + 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 already included in the target branch. There is nothing to merge.") }) } diff --git a/models/issues/pull.go b/models/issues/pull.go index f2ca19b03..0524acb21 100644 --- a/models/issues/pull.go +++ b/models/issues/pull.go @@ -122,6 +122,7 @@ const ( PullRequestStatusManuallyMerged PullRequestStatusError PullRequestStatusEmpty + PullRequestStatusAncestor ) // PullRequestFlow the flow of pull request @@ -423,6 +424,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.Status == PullRequestStatusAncestor +} + // 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 347022fbd..3c03dc9c4 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -1531,7 +1531,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 6621a281f..288f4dc0b 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() { return ErrNotMergableState } diff --git a/services/pull/patch.go b/services/pull/patch.go index c7a69501c..bb09acc89 100644 --- a/services/pull/patch.go +++ b/services/pull/patch.go @@ -87,6 +87,14 @@ func TestPatch(pr *issues_model.PullRequest) error { } } pr.MergeBase = strings.TrimSpace(pr.MergeBase) + if pr.HeadCommitID, err = gitRepo.GetRefCommitID(git.BranchPrefix + "tracking"); err != nil { + return fmt.Errorf("GetBranchCommitID: can't find commit ID for head: %w", err) + } + + if pr.HeadCommitID == pr.MergeBase { + pr.Status = issues_model.PullRequestStatusAncestor + return nil + } // 2. Check for conflicts if conflicts, err := checkConflicts(ctx, pr, gitRepo, tmpBasePath); err != nil || conflicts || pr.Status == issues_model.PullRequestStatusEmpty { diff --git a/templates/repo/issue/view_content/pull.tmpl b/templates/repo/issue/view_content/pull.tmpl index 5a23bfa33..23aa97217 100644 --- a/templates/repo/issue/view_content/pull.tmpl +++ b/templates/repo/issue/view_content/pull.tmpl @@ -195,12 +195,12 @@ {{svg "octicon-sync"}} {{$.i18n.Tr "repo.pulls.is_checking"}} - {{else if .Issue.PullRequest.IsEmpty}} + {{else if .Issue.PullRequest.IsAncestor}}
{{svg "octicon-alert" 16}} - {{$.i18n.Tr "repo.pulls.is_empty"}} + {{$.i18n.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}} + {{$.i18n.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 75fbceb80..08b1f9cb8 100644 --- a/web_src/js/components/PullRequestMergeForm.vue +++ b/web_src/js/components/PullRequestMergeForm.vue @@ -48,7 +48,7 @@
-
+