dashboard/app: use Reported-by tags to fix bugs

Accept and use Reported-by tags in commits to mark bugs as fixed.

dashboard/app/api.go

@@ -10,6 +10,7 @@ import (
 	"fmt"
 	"io/ioutil"
 	"net/http"
+	"reflect"
 	"sort"
 	"strings"
 	"time"
@@ -291,9 +292,14 @@ func uploadBuild(c context.Context, ns string, req *dashapi.Build, typ BuildType
 
 func addCommitsToBugs(c context.Context, ns, manager string,
 	titles []string, fixCommits []dashapi.FixCommit) error {
-	commitMap := make(map[string]bool)
+	presentCommits := make(map[string]bool)
+	bugFixedBy := make(map[string][]string)
 	for _, com := range titles {
-		commitMap[com] = true
+		presentCommits[com] = true
+	}
+	for _, com := range fixCommits {
+		presentCommits[com.Title] = true
+		bugFixedBy[com.BugID] = append(bugFixedBy[com.BugID], com.Title)
 	}
 	managers, err := managerList(c, ns)
 	if err != nil {
@@ -309,7 +315,12 @@ func addCommitsToBugs(c context.Context, ns, manager string,
 	}
 	now := timeNow(c)
 	for i, bug := range bugs {
-		if !fixedWith(bug, manager, commitMap) {
+		var fixCommits []string
+		for i := range bug.Reporting {
+			fixCommits = append(fixCommits, bugFixedBy[bug.Reporting[i].ID]...)
+		}
+		sort.Strings(fixCommits)
+		if !bugNeedsCommitUpdate(c, bug, manager, fixCommits, presentCommits) {
 			continue
 		}
 		tx := func(c context.Context) error {
@@ -317,9 +328,13 @@ func addCommitsToBugs(c context.Context, ns, manager string,
 			if err := datastore.Get(c, keys[i], bug); err != nil {
 				return fmt.Errorf("failed to get bug %v: %v", keys[i].StringID(), err)
 			}
-			if !fixedWith(bug, manager, commitMap) {
+			if !bugNeedsCommitUpdate(nil, bug, manager, fixCommits, presentCommits) {
 				return nil
 			}
+			if len(fixCommits) != 0 && !reflect.DeepEqual(bug.Commits, fixCommits) {
+				bug.Commits = fixCommits
+				bug.PatchedOn = nil
+			}
 			bug.PatchedOn = append(bug.PatchedOn, manager)
 			if bug.Status == BugStatusOpen {
 				fixed := true
@@ -367,16 +382,23 @@ func managerList(c context.Context, ns string) ([]string, error) {
 	return managers, nil
 }
 
-func fixedWith(bug *Bug, manager string, commits map[string]bool) bool {
-	if stringInList(bug.PatchedOn, manager) {
+func bugNeedsCommitUpdate(c context.Context, bug *Bug, manager string, fixCommits []string,
+	presentCommits map[string]bool) bool {
+	if len(fixCommits) != 0 && !reflect.DeepEqual(bug.Commits, fixCommits) {
+		if c != nil {
+			log.Infof(c, "bug %q is fixed with %q", bug.Title, fixCommits)
+		}
+		return true
+	}
+	if len(bug.Commits) == 0 || stringInList(bug.PatchedOn, manager) {
 		return false
 	}
 	for _, com := range bug.Commits {
-		if !commits[com] {
+		if !presentCommits[com] {
 			return false
 		}
 	}
-	return len(bug.Commits) > 0
+	return true
 }
 
 func stringInList(list []string, str string) bool {

dashboard/app/fix_test.go

@@ -439,3 +439,59 @@ func TestReFixedTwoManagers(t *testing.T) {
 	reports = reportAllBugs(c, 1)
 	c.expectEQ(reports[0].Title, "title1 (2)")
 }
+
+// TestFixedWithCommitTags tests fixing of bugs with Reported-by commit tags.
+func TestFixedWithCommitTags(t *testing.T) {
+	c := NewCtx(t)
+	defer c.Close()
+
+	build1 := testBuild(1)
+	c.expectOK(c.API(client1, key1, "upload_build", build1, nil))
+
+	build2 := testBuild(2)
+	c.expectOK(c.API(client1, key1, "upload_build", build2, nil))
+
+	crash1 := testCrash(build1, 1)
+	c.expectOK(c.API(client1, key1, "report_crash", crash1, nil))
+
+	rep := reportAllBugs(c, 1)[0]
+
+	// Upload build with 2 fixing commits for this bug.
+	build1.FixCommits = []dashapi.FixCommit{{"fix commit 1", rep.ID}, {"fix commit 2", rep.ID}}
+	c.expectOK(c.API(client1, key1, "upload_build", build1, nil))
+
+	// Now the commits must be associated with the bug and the second
+	// manager must get them as pending.
+	builderPollReq := &dashapi.BuilderPollReq{build2.Manager}
+	builderPollResp := new(dashapi.BuilderPollResp)
+	c.expectOK(c.API(client1, key1, "builder_poll", builderPollReq, builderPollResp))
+	c.expectEQ(len(builderPollResp.PendingCommits), 2)
+	c.expectEQ(builderPollResp.PendingCommits[0], "fix commit 1")
+	c.expectEQ(builderPollResp.PendingCommits[1], "fix commit 2")
+
+	// The first manager must not get them.
+	builderPollReq = &dashapi.BuilderPollReq{build1.Manager}
+	builderPollResp = new(dashapi.BuilderPollResp)
+	c.expectOK(c.API(client1, key1, "builder_poll", builderPollReq, builderPollResp))
+	c.expectEQ(len(builderPollResp.PendingCommits), 0)
+
+	// The bug is still not fixed.
+	c.expectOK(c.API(client1, key1, "report_crash", crash1, nil))
+	reportAllBugs(c, 0)
+
+	// Now the second manager reports the same commits.
+	// This must close the bug.
+	build2.FixCommits = build1.FixCommits
+	c.expectOK(c.API(client1, key1, "upload_build", build2, nil))
+
+	// Commits must not be passed to managers.
+	builderPollReq = &dashapi.BuilderPollReq{build2.Manager}
+	builderPollResp = new(dashapi.BuilderPollResp)
+	c.expectOK(c.API(client1, key1, "builder_poll", builderPollReq, builderPollResp))
+	c.expectEQ(len(builderPollResp.PendingCommits), 0)
+
+	// Ensure that a new crash creates a new bug.
+	c.expectOK(c.API(client1, key1, "report_crash", crash1, nil))
+	rep = reportAllBugs(c, 1)[0]
+	c.expectEQ(rep.Title, "title1 (2)")
+}

dashboard/app/reporting.go

@@ -6,6 +6,7 @@ package dash
 import (
 	"encoding/json"
 	"fmt"
+	"reflect"
 	"sort"
 	"strings"
 	"time"
@@ -477,27 +478,9 @@ func incomingCommandTx(c context.Context, now time.Time, cmd *dashapi.BugUpdate,
 		return false, internalError, fmt.Errorf("unknown bug status %v", cmd.Status)
 	}
 	if len(cmd.FixCommits) != 0 && (bug.Status == BugStatusOpen || bug.Status == BugStatusDup) {
-		m := make(map[string]bool)
-		for _, com := range cmd.FixCommits {
-			m[com] = true
-		}
-		same := false
-		if len(bug.Commits) == len(m) {
-			same = true
-			for _, com := range bug.Commits {
-				if !m[com] {
-					same = false
-					break
-				}
-			}
-		}
-		if !same {
-			commits := make([]string, 0, len(m))
-			for com := range m {
-				commits = append(commits, com)
-			}
-			sort.Strings(commits)
-			bug.Commits = commits
+		sort.Strings(cmd.FixCommits)
+		if !reflect.DeepEqual(bug.Commits, cmd.FixCommits) {
+			bug.Commits = cmd.FixCommits
 			bug.PatchedOn = nil
 		}
 	}