Skip to content

Commit

Permalink
Return to loading commits from the pull request (#76)
Browse files Browse the repository at this point in the history 
This is the only way to get only the commits included in the PR without
performing our own comparisons against the commits on the target branch.
This property is important to correctly handle merge commits.

In order to load pushedDate values from fork commits, conditionally make
a second query listing commit history and match these commits with the
ones in the pull request.

To avoid additional hazards of commit loading, simplify the detection of
update merges by relying on the principle that GitHub will automatically
exclude any commits already on the target branch from the PR listing.
This removes the need to load target branch commits.

Finally, change the pushed date backfill logic to walk the parent commit
chain instead of using a specific order that differs between APIs.
  • Loading branch information
@bluekeyes
bluekeyes authored Apr 30, 2019
1 parent 758ff8c commit 93cf299
Show file tree
Hide file tree
Showing 8 changed files with 276 additions and 302 deletions.
4 changes: 2 additions & 2 deletions README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -323,8 +323,8 @@ For a commit on a branch to count as an "update merge" for the purpose of the

1. The commit must have exactly two parents
2. The commit must have the `committedViaWeb` property set to `true`
3. One parent must exist in the last 100 commits on the target branch of the
pull request
3. The first parent must exist in the pull request while the second parent
must not exist in the pull request (meaning it is on the target branch)

These will all be true after updating a branch using the UI, but historic
merges on long-running branches or merges created with the API may not be
Expand Down
67 changes: 33 additions & 34 deletions policy/approval/approve.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -126,23 +126,24 @@ func (r *Rule) IsApproved(ctx context.Context, prctx pull.Context) (bool, string
return false, "", err
}

if len(commits) > 0 {
lastCommit := commits[len(commits)-1]
last := findLastPushed(commits)
if last == nil {
return false, "", errors.New("no commit contained a push date")
}

var allowedCandidates []*common.Candidate
for _, candidate := range candidates {
if candidate.CreatedAt.After(lastCommit.CreatedAt) {
allowedCandidates = append(allowedCandidates, candidate)
}
var allowedCandidates []*common.Candidate
for _, candidate := range candidates {
if candidate.CreatedAt.After(*last.PushedAt) {
allowedCandidates = append(allowedCandidates, candidate)
}
}

log.Debug().Msgf("discarded %d candidates invalidated by push of %s at %s",
len(candidates)-len(allowedCandidates),
lastCommit.SHA,
lastCommit.CreatedAt.Format(time.RFC3339))
log.Debug().Msgf("discarded %d candidates invalidated by push of %s at %s",
len(candidates)-len(allowedCandidates),
last.SHA,
last.PushedAt.Format(time.RFC3339))

candidates = allowedCandidates
}
candidates = allowedCandidates
}

log.Debug().Msgf("found %d candidates for approval", len(candidates))
Expand Down Expand Up @@ -213,59 +214,57 @@ func (r *Rule) IsApproved(ctx context.Context, prctx pull.Context) (bool, string
return false, msg, nil
}

// filteredCommits returns relevant commits ordered from oldest to newest.
func (r *Rule) filteredCommits(prctx pull.Context) ([]*pull.Commit, error) {
commits, err := prctx.Commits()
if err != nil {
return nil, errors.Wrap(err, "failed to list commits")
}

sort.Stable(pull.CommitsByCreationTime(commits))

needsFiltering := r.Options.IgnoreUpdateMerges
if !needsFiltering {
return commits, nil
}

var filtered []*pull.Commit
for _, c := range commits {
isUpdate, err := isUpdateMerge(prctx, c)
if err != nil {
return nil, errors.Wrap(err, "failed to detemine update merge status")
}

switch {
case isUpdate:
case isUpdateMerge(commits, c):
default:
filtered = append(filtered, c)
}
}
return filtered, nil
}

func isUpdateMerge(prctx pull.Context, c *pull.Commit) (bool, error) {
func isUpdateMerge(commits []*pull.Commit, c *pull.Commit) bool {
// must be a simple merge commit (exactly 2 parents)
if len(c.Parents) != 2 {
return false, nil
return false
}

// must be created via the UI or the API (no local merges)
if !c.CommittedViaWeb {
return false, nil
return false
}

// one parent must exist in recent history on the target branch
targets, err := prctx.TargetCommits()
if err != nil {
return false, err
shas := make(map[string]bool)
for _, c := range commits {
shas[c.SHA] = true
}
for _, target := range targets {
if c.Parents[0] == target.SHA || c.Parents[1] == target.SHA {
return true, nil

// first parent must exist: it is a commit on the head branch
// second parent must not exist: it is already in the base branch
return shas[c.Parents[0]] && !shas[c.Parents[1]]
}

func findLastPushed(commits []*pull.Commit) *pull.Commit {
var last *pull.Commit
for _, c := range commits {
if c.PushedAt != nil && (last == nil || c.PushedAt.After(*last.PushedAt)) {
last = c
}
}

return false, nil
return last
}

func numberOfApprovals(count int) string {
Expand Down
32 changes: 11 additions & 21 deletions policy/approval/approve_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -85,38 +85,24 @@ func TestIsApproved(t *testing.T) {
},
CommitsValue: []*pull.Commit{
{
CreatedAt: now.Add(5 * time.Second),
PushedAt: newTime(now.Add(5 * time.Second)),
SHA: "c6ade256ecfc755d8bc877ef22cc9e01745d46bb",
Author: "mhaypenny",
Committer: "mhaypenny",
},
{
CreatedAt: now.Add(15 * time.Second),
PushedAt: newTime(now.Add(15 * time.Second)),
SHA: "674832587eaaf416371b30f5bc5a47e377f534ec",
Author: "contributor-author",
Committer: "mhaypenny",
},
{
CreatedAt: now.Add(45 * time.Second),
PushedAt: newTime(now.Add(45 * time.Second)),
SHA: "97d5ea26da319a987d80f6db0b7ef759f2f2e441",
Author: "mhaypenny",
Committer: "contributor-committer",
},
},
TargetCommitsValue: []*pull.Commit{
{
CreatedAt: now.Add(5 * time.Second),
SHA: "dc594ff5aca4133070a76f9006568b656a251770",
Author: "developer",
Committer: "developer",
},
{
CreatedAt: now.Add(0 * time.Second),
SHA: "2e1b0bb6ab144bf7a1b7a1df9d3bdcb0fe85a206",
Author: "developer",
Committer: "developer",
},
},
OrgMemberships: map[string][]string{
"mhaypenny": {"everyone"},
"contributor-author": {"everyone"},
Expand Down Expand Up @@ -273,7 +259,7 @@ func TestIsApproved(t *testing.T) {
prctx := basePullContext()
prctx.CommitsValue = []*pull.Commit{
{
CreatedAt: now.Add(25 * time.Second),
PushedAt: newTime(now.Add(25 * time.Second)),
SHA: "c6ade256ecfc755d8bc877ef22cc9e01745d46bb",
Author: "mhaypenny",
Committer: "mhaypenny",
Expand All @@ -298,7 +284,7 @@ func TestIsApproved(t *testing.T) {
prctx := basePullContext()
prctx.CommitsValue = []*pull.Commit{
{
CreatedAt: now.Add(85 * time.Second),
PushedAt: newTime(now.Add(85 * time.Second)),
SHA: "c6ade256ecfc755d8bc877ef22cc9e01745d46bb",
Author: "mhaypenny",
Committer: "mhaypenny",
Expand All @@ -322,7 +308,7 @@ func TestIsApproved(t *testing.T) {
t.Run("ignoreUpdateMergeAfterReview", func(t *testing.T) {
prctx := basePullContext()
prctx.CommitsValue = append(prctx.CommitsValue[:1], &pull.Commit{
CreatedAt: now.Add(25 * time.Second),
PushedAt: newTime(now.Add(25 * time.Second)),
SHA: "647c5078288f0ea9de27b5c280f25edaf2089045",
CommittedViaWeb: true,
Parents: []string{
Expand Down Expand Up @@ -352,7 +338,7 @@ func TestIsApproved(t *testing.T) {
t.Run("ignoreUpdateMergeContributor", func(t *testing.T) {
prctx := basePullContext()
prctx.CommitsValue = append(prctx.CommitsValue[:1], &pull.Commit{
CreatedAt: now.Add(25 * time.Second),
PushedAt: newTime(now.Add(25 * time.Second)),
SHA: "647c5078288f0ea9de27b5c280f25edaf2089045",
CommittedViaWeb: true,
Parents: []string{
Expand Down Expand Up @@ -381,3 +367,7 @@ func TestIsApproved(t *testing.T) {
assertApproved(t, prctx, r, "Approved by merge-committer")
})
}

func newTime(t time.Time) *time.Time {
return &t
}
17 changes: 4 additions & 13 deletions pull/context.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -76,10 +76,6 @@ type Context interface {
// Reviews lists all reviews on a Pull Request. The review order is
// implementation dependent.
Reviews() ([]*Review, error)

// TargetCommits returns recent commits on the target branch of the pull
// request. The exact number of commits is an implementation detail.
TargetCommits() ([]*Commit, error)
}

type FileStatus int
Expand All @@ -98,7 +94,6 @@ type File struct {
}

type Commit struct {
CreatedAt time.Time
SHA string
Parents []string
CommittedViaWeb bool
Expand All @@ -110,6 +105,10 @@ type Commit struct {
// Commiter is the login name of the committer. It is empty if the
// committer is not a real user.
Committer string

// PushedAt is the timestamp when the commit was pushed. It is nil if that
// information is not available for this commit.
PushedAt *time.Time
}

// Users returns the login names of the users associated with this commit.
Expand All @@ -124,14 +123,6 @@ func (c *Commit) Users() []string {
return users
}

type CommitsByCreationTime []*Commit

func (cs CommitsByCreationTime) Len() int { return len(cs) }
func (cs CommitsByCreationTime) Swap(i, j int) { cs[i], cs[j] = cs[j], cs[i] }
func (cs CommitsByCreationTime) Less(i, j int) bool {
return cs[i].CreatedAt.Before(cs[j].CreatedAt)
}

type Comment struct {
CreatedAt time.Time
Author string
Expand Down
Loading

0 comments on commit 93cf299

Please sign in to comment.