maintner: misc fixes and tests

As far as I can tell, the data structure inconsistency check that
panic-failed earlier has been fixed by the last few CLs (the
"finishProcessing" ones). The new tests double check.

Also, fix up Footer to not allocate by changing the requirement of its
argument.

And fix a failing test that was crashing due to its *Gerrit not being
initialized in a non-Gerrit test.

Fixes golang/go#22753 (already fixed, but now tested)

Change-Id: If09cfad6f69d8b3c97410f3e4c3a2e2281c666a5
Reviewed-on: https://go-review.googlesource.com/78155
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>

maintner/gerrit.go

@@ -371,7 +371,7 @@ func lineValue(all, prefix string) (value, rest string) {
 // ChangeID returns the Gerrit "Change-Id: Ixxxx" line's Ixxxx
 // value from the cl.Msg, if any.
 func (cl *GerritCL) ChangeID() string {
-	id := cl.Footer("Change-Id")
+	id := cl.Footer("Change-Id:")
 	if strings.HasPrefix(id, "I") && len(id) == 41 {
 		return id
 	}
@@ -379,11 +379,15 @@ func (cl *GerritCL) ChangeID() string {
 }
 
 // Footer returns the value of a line of the form <key>: value from
-// the CL’s commit message. The key is case-sensitive.
+// the CL’s commit message. The key is case-sensitive and must end in
+// a colon.
 // An empty string is returned if there is no value for key.
 func (cl *GerritCL) Footer(key string) string {
+	if len(key) == 0 || key[len(key)-1] != ':' {
+		panic("Footer key does not end in colon")
+	}
 	// TODO: git footers are treated as multimaps. Account for this.
-	v, _ := lineValue(cl.Commit.Msg, key+":")
+	v, _ := lineValue(cl.Commit.Msg, key)
 	return v
 }
 
@@ -1174,6 +1178,9 @@ func (g *Gerrit) check() error {
 // called with its Corpus.mu locked. (called by
 // Corpus.finishProcessing; read comment there)
 func (g *Gerrit) finishProcessing() {
+	if g == nil {
+		return
+	}
 	for _, gp := range g.projects {
 		gp.finishProcessing()
 	}
@@ -1192,7 +1199,7 @@ func (gp *GerritProject) check() error {
 		}
 		for _, pc := range gc.Parents {
 			if _, ok := gp.commit[pc.Hash]; !ok {
-				return fmt.Errorf("git commit %q exits but its parent %q does not", gc.Hash, pc.Hash)
+				return fmt.Errorf("git commit %q exists but its parent %q does not", gc.Hash, pc.Hash)
 			}
 		}
 	}

maintner/git.go

@@ -99,7 +99,7 @@ func (gc *GitCommit) hasAncestor(ancestor *GitCommit, checked map[*GitCommit]boo
 			panic("nil parent")
 		}
 		if pc.Committer == placeholderCommitter {
-			panic("found placeholder")
+			log.Printf("WARNING: hasAncestor(%q, %q) found parent %q with placeholder parent", gc.Hash, ancestor.Hash, pc.Hash)
 		}
 		if pc.Hash == ancestor.Hash || pc.hasAncestor(ancestor, checked) {
 			checked[gc] = true

maintner/godata/godata_test.go

@@ -108,6 +108,12 @@ func TestGitAncestor(t *testing.T) {
 		{"8f06e217eac10bae4993ca371ade35fecd26270e", "22f1b56dab29d397d2bdbdd603d85e60fb678089", true},
 		{"22f1b56dab29d397d2bdbdd603d85e60fb678089", "8f06e217eac10bae4993ca371ade35fecd26270e", false},
 
+		// Was crashing. Issue 22753.
+		{"3a181dc7bc8fd0c61d6090a85f87c934f1874802", "f65abf6ddc8d1f3d403a9195fd74eaffa022b07f", true},
+		// The reverse of the above, to try to reproduce the
+		// panic if I got the order backwards:
+		{"f65abf6ddc8d1f3d403a9195fd74eaffa022b07f", "3a181dc7bc8fd0c61d6090a85f87c934f1874802", false},
+
 		// Same on both sides:
 		{"0bb0b61d6a85b2a1a33dcbc418089656f2754d32", "0bb0b61d6a85b2a1a33dcbc418089656f2754d32", false},
 		{"3b5637ff2bd5c03479780995e7a35c48222157c1", "3b5637ff2bd5c03479780995e7a35c48222157c1", false},

maintner/maintner.go

@@ -115,6 +115,21 @@ func (c *Corpus) Check() error {
 	if err := c.Gerrit().check(); err != nil {
 		return fmt.Errorf("gerrit: %v", err)
 	}
+
+	for hash, gc := range c.gitCommit {
+		if gc.Committer == placeholderCommitter {
+			return fmt.Errorf("corpus git commit %v has placeholder committer", hash)
+		}
+		if gc.Hash != hash {
+			return fmt.Errorf("git commit for key %q had GitCommit.Hash %q", hash, gc.Hash)
+		}
+		for _, pc := range gc.Parents {
+			if _, ok := c.gitCommit[pc.Hash]; !ok {
+				return fmt.Errorf("git commit %q exists but its parent %q does not", gc.Hash, pc.Hash)
+			}
+		}
+	}
+
 	return nil
 }