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

Fix checks in PR for empty commits #19603 #20290

Merged
merged 9 commits into from
Jul 13, 2022
28 changes: 27 additions & 1 deletion integrations/pull_status_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand All @@ -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.")
})
Expand Down
6 changes: 6 additions & 0 deletions models/issues/pull.go
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,7 @@ const (
PullRequestStatusManuallyMerged
PullRequestStatusError
PullRequestStatusEmpty
PullRequestStatusAncestor
)

// PullRequestFlow the flow of pull request
Expand Down Expand Up @@ -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 {
Expand Down
3 changes: 2 additions & 1 deletion options/locale/locale_en-US.ini
Original file line number Diff line number Diff line change
Expand Up @@ -1532,7 +1532,8 @@ pulls.remove_prefix = Remove <strong>%s</strong> 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.
Expand Down
2 changes: 1 addition & 1 deletion services/pull/check.go
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
zeripath marked this conversation as resolved.
Show resolved Hide resolved
return ErrNotMergableState
}

Expand Down
8 changes: 8 additions & 0 deletions services/pull/patch.go
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,14 @@ func TestPatch(pr *issues_model.PullRequest) error {
}
}
pr.MergeBase = strings.TrimSpace(pr.MergeBase)
if pr.HeadCommitID, err = gitRepo.GetBranchCommitID("tracking"); err != nil {
jedi7 marked this conversation as resolved.
Show resolved Hide resolved
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 {
Expand Down
16 changes: 12 additions & 4 deletions templates/repo/issue/view_content/pull.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -195,12 +195,12 @@
<i class="icon icon-octicon">{{svg "octicon-sync"}}</i>
{{$.locale.Tr "repo.pulls.is_checking"}}
</div>
{{else if .Issue.PullRequest.IsEmpty}}
{{else if .Issue.PullRequest.IsAncestor}}
<div class="item">
<i class="icon icon-octicon">{{svg "octicon-alert" 16}}</i>
{{$.locale.Tr "repo.pulls.is_empty"}}
{{$.locale.Tr "repo.pulls.is_ancestor"}}
</div>
{{else if .Issue.PullRequest.CanAutoMerge}}
{{else if or .Issue.PullRequest.CanAutoMerge .Issue.PullRequest.IsEmpty}}
{{if .IsBlockedByApprovals}}
<div class="item">
<i class="icon icon-octicon">{{svg "octicon-x"}}</i>
Expand Down Expand Up @@ -282,7 +282,6 @@
</div>
{{end}}
{{end}}

{{if and (gt .Issue.PullRequest.CommitsBehind 0) (not .Issue.IsClosed) (not .Issue.PullRequest.IsChecking) (not .IsPullFilesConflicted) (not .IsPullRequestBroken) (not $canAutoMerge)}}
<div class="ui divider"></div>
<div class="item item-section">
Expand Down Expand Up @@ -321,6 +320,14 @@
</div>
</div>
{{end}}
{{if .Issue.PullRequest.IsEmpty}}
<div class="ui divider"></div>

<div class="item">
<i class="icon icon-octicon">{{svg "octicon-alert" 16}}</i>
{{$.locale.Tr "repo.pulls.is_empty"}}
</div>
{{end}}

{{if .AllowMerge}} {{/* user is allowed to merge */}}
{{$prUnit := .Repository.MustGetUnit $.UnitTypePullRequests}}
Expand Down Expand Up @@ -348,6 +355,7 @@

'canMergeNow': {{$canMergeNow}},
'allOverridableChecksOk': {{not $notAllOverridableChecksOk}},
'emptyCommit': {{.Issue.PullRequest.IsEmpty}},
'pullHeadCommitID': {{.PullHeadCommitID}},
'isPullBranchDeletable': {{.IsPullBranchDeletable}},
'defaultDeleteBranchAfterMerge': {{$prUnit.PullRequestsConfig.DefaultDeleteBranchAfterMerge}},
Expand Down
2 changes: 1 addition & 1 deletion web_src/js/components/PullRequestMergeForm.vue
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@

<div v-if="!showActionForm" class="df">
<!-- the merge button -->
<div class="ui buttons merge-button" :class="mergeButtonStyleClass" @click="toggleActionForm(true)" >
<div class="ui buttons merge-button" :class="[mergeForm.emptyCommit?'grey':mergeForm.allOverridableChecksOk?'green':'red']" @click="toggleActionForm(true)" >
jedi7 marked this conversation as resolved.
Show resolved Hide resolved
<button class="ui button">
<svg-icon name="octicon-git-merge"/>
<span class="button-text">
Expand Down