commit dcbfa19f2482f60cf98a8ebe94c1f395e3e127d7
author Dmitri Shuralyov <dmitshur@golang.org> Fri Aug 03 16:20:39 2018 -0400
committer Dmitri Shuralyov <dmitshur@golang.org> Mon Aug 06 16:50:14 2018 +0000
tree 1cf69a6d36bb6dce902bdc3627d1441428f20045
parent 4639cd3919e4243a672a11a56543004bf1778326

maintner: propagate and use internal invariants in finishProcessingCL

This change simplifies and cleans up finishProcessingCL. It documents
a precondition for finishProcessingCL that cl.Meta must be non-nil.

finishProcessingCL is only ever called on CLs that have been noted as
dirty via noteDirtyCL, and CLs are always given a non-nil Meta before
noteDirtyCL is called on them.

foreachCommitParent sounded like it called f on all parents of commit,
not including commit itself. In reality, it called f on commit as well.
Rename it to foreachCommit and improve its documentation to make this
more visible.

As a result, it becomes more clear that cl.Metas will contain at least
one element (e.g., at least mostRecentMetaCommit). Then, cl.Metas[0]
can be used to find the CL creation time.

Also change foreachCommit to return an error if a parent commit isn't
found. Otherwise, we'd be cutting the history walk short and the caller
wouldn't know. This shouldn't happen, but if it does, it'll be logged
(by finishProcessingCL).

Define a CL to be complete if its Meta and Commit fields are non-nil,
and the Metas slice contains at least 1 element. Use this to simplify
OwnerName and OwnerID methods and remove the no longer needed
firstMetaCommit method.

Finally, use the simplified finishProcessingCL to set cl.Created to
the actual creation time of the CL (rather than "last updated" time).
It's easy now, because we can deduce that len(cl.Metas) >= 1 is
guaranteed to be true, and therefore cl.Metas[0] will not panic.
There's no need to add guards or think about what to do if cl.Metas
is empty.

Document GerritCL.Created field, since it's exported.

Background

This change was started with the goal of fixing golang/go#24744.
It was hard to fix that bug with a short diff, because there were
too few guarantees inside finishProcessingCL that could be relied on.
I wanted to improve that as part of my fix.

We already established some guarantees for the user-facing CLs (i.e.,
that their Commit, Meta fields are non-nil and len(cl.Metas) >= 1).
I started propagating and using that property in a few more places
in internal code.

It allowed simplifications such as:

	 // OwnerName returns the name of the CL’s owner or an empty string on error.
	 func (cl *GerritCL) OwnerName() string {
	-	m := cl.firstMetaCommit()
	-	if m == nil {
	-		return ""
	-	}
	-	return m.Author.Name()
	+	if !cl.complete() {
	+		return ""
	+	}
	+	return cl.Metas[0].Commit.Author.Name()
	 }
	-
	-func (cl *GerritCL) firstMetaCommit() *GitCommit {
	-	m := cl.Meta
	-	if m == nil { // TODO: Can this actually happen, besides in one of the contrived tests? Remove?
	-		return nil
	-	}
	-	c := m.Commit
	-	for c != nil && len(c.Parents) > 0 {
	-		c = c.Parents[0] // Meta commits don’t have more than one parent.
	-	}
	-	return c
	-}

	 // complete reports whether cl is complete.
	 // A CL is considered complete if its Meta and Commit fields are non-nil,
	 // and the Metas slice contains at least 1 element.
	 func (cl *GerritCL) complete() bool {
	 	return cl.Meta != nil &&
	 		len(cl.Metas) >= 1 &&
	 		cl.Commit != nil
	 }

Also, previously, it was hard to be sure that the
gc, ok := ... line wouldn't panic:

	// called with Corpus.mu Locked
	func (gp *GerritProject) finishProcessingCL(cl *GerritCL) {

		// What happens if cl.Meta is nil?
		// Should there be a if cl.Meta == nil guard here?
		// What should it do if cl.Meta is indeed nil?
		gc, ok := c.gitCommit[cl.Meta.Commit.Hash]
		if !ok {
			log.Printf("WARNING: GerritProject(%q).finishProcessingCL failed to find CL %v hash %s",
				gp.ServerSlashProject(), cl.Number, cl.Meta.Commit.Hash)
			return
		}

By documenting that cl.Meta must be non-nil as a precondition of
finishProcessingCL, it became possible to know it won't panic:

	// finishProcessingCL fixes up invariants before the cl can be returned back to the user.
	// cl.Meta must be non-nil.
	//
	// called with Corpus.mu Locked
	func (gp *GerritProject) finishProcessingCL(cl *GerritCL) {

		// cl.Meta can't be nil due to precondition.
		gc, ok := c.gitCommit[cl.Meta.Commit.Hash]
		if !ok {
			log.Printf("WARNING: GerritProject(%q).finishProcessingCL failed to find CL %v hash %s",
				gp.ServerSlashProject(), cl.Number, cl.Meta.Commit.Hash)
			return
		}

(It would be a bug to call finishProcessingCL with a cl where
cl.Meta is nil, because it'd be violating the precondition.)

I think stating preconditions and propagating more internal invariants
is a good general direction, because it allows the code to be simpler
and easier to verify as being correct. Otherwise, one needs to keep
checking if fields are non-nil everywhere before using them and think
what to do if they are nil, and that's unpleasant.

Fixes golang/go#24744.

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