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

x/build/maintner: GerritCL.Created is not accurate (it equals to "CL Updated" time instead) #24744

Closed
dmitshur opened this issue Apr 7, 2018 · 6 comments
Labels
Builders x/build issues (builders, bots, dashboards) Documentation FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@dmitshur
Copy link
Contributor

dmitshur commented Apr 7, 2018

What version of Go are you using (go version)?

$ go version
go version go1.10.1 darwin/amd64

Does this issue reproduce with the latest release?

Yes.

What did you do?

I queried the maintner.GerritCL.Created value for CL 92456.

Sample Program
package main

import (
	"context"
	"fmt"
	"log"
	"time"

	"golang.org/x/build/maintner/godata"
)

func main() {
	c, err := godata.Get(context.Background())
	if err != nil {
		log.Fatalln(err)
	}
	cl := c.Gerrit().Project("go.googlesource.com", "go").CL(92456)
	fmt.Println("cl.Created:", cl.Created.UTC().Format(time.UnixDate))
}

What did you expect to see?

maintner.GerritCL.Created is not documented, but given the field is named Created time.Time and it's inside the GerritCL struct, one would expect it to be the time the CL was created.

CL 92456 was created about 2 months ago.

So, I expected to see a date like Feb 6, 2018, 6:57 PM UTC.

What did you see instead?

A date like Apr 6, 2018, 7:42 PM UTC. Which is around 9~ hours ago.

It's the date the CL was last updated (merged).


I've looked at the code and it looks like the GerritCL.Created value is assigned to the CommitTime of the latest meta commit. That would explain the results I saw.

@gopherbot gopherbot added this to the Unreleased milestone Apr 7, 2018
@gopherbot gopherbot added the Builders x/build issues (builders, bots, dashboards) label Apr 7, 2018
@dmitshur dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Apr 7, 2018
@dmitshur
Copy link
Contributor Author

dmitshur commented Apr 7, 2018

I haven't looked too closely, but I suspect the fix might be as simple as using the oldest (rather than most recent) meta commit's commit time instead.

@dmitshur
Copy link
Contributor Author

dmitshur commented Apr 7, 2018

I tried it, that seems to work. Using the commit time of the root meta commit (instead of the head meta commit, as is currently done) gives me the same CL create time as via Gerrit API (and Gerrit UI).

This is what I tested (code needs to be cleaned up):

diff --git a/maintner/gerrit.go b/maintner/gerrit.go
index edee678..3efa3c8 100644
--- a/maintner/gerrit.go
+++ b/maintner/gerrit.go
@@ -803,7 +803,7 @@ func (gp *GerritProject) finishProcessingCL(cl *GerritCL) {
 	var backwardMessages []*GerritMessage
 	var backwardMeta []*GitCommit
 
-	gc, ok := c.gitCommit[cl.Meta.Hash]
+	_, ok := c.gitCommit[cl.Meta.Hash]
 	if !ok {
 		log.Printf("WARNING: GerritProject(%q).finishProcessingCL failed to find CL %v hash %s",
 			gp.ServerSlashProject(), cl.Number, cl.Meta.Hash)
@@ -839,8 +839,9 @@ func (gp *GerritProject) finishProcessingCL(cl *GerritCL) {
 		cl.Status = "new"
 	}
 
-	if cl.Created.IsZero() || gc.CommitTime.Before(cl.Created) {
-		cl.Created = gc.CommitTime
+	root := backwardMeta[len(backwardMeta)-1] // TODO: Is len(backwardMeta) >= 1 guaranteed? If not, this'll panic.
+	if cl.Created.IsZero() || root.CommitTime.Before(cl.Created) {
+		cl.Created = root.CommitTime
 	}
 	reverseGerritMessages(backwardMessages)
 	cl.Messages = backwardMessages

@dmitshur
Copy link
Contributor Author

/cc @bradfitz Do you agree this is a bug?

@bradfitz
Copy link
Contributor

Sounds like a bug.

It also needs proper docs: https://godoc.org/golang.org/x/build/maintner#GerritCL.Created

@dmitshur dmitshur added Documentation NeedsFix The path to resolution is known, but the work has not been done. help wanted and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Apr 12, 2018
@dmitshur dmitshur changed the title x/build/maintner: GerritCL.Created seemingly not accurate (it represents the "CL Updated" time instead) x/build/maintner: GerritCL.Created is not accurate (it equals to "CL Updated" time instead) May 7, 2018
@dmitshur dmitshur self-assigned this May 7, 2018
@dmitshur
Copy link
Contributor Author

dmitshur commented May 7, 2018

It was hard to fix this 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.

(The hacky diff I posted earlier worked as a prototype, but it was hard to be sure that it'd work all of the time, and hard to change finishProcessingCL without fearing it would break.)

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 allows 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 becomes 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, you need to keep checking if fields are non-nil everywhere before using them and think what to do if they are nil, and that's annoying.

@bradfitz I hope you're okay with that direction, but let me know if you prefer to do things differently.

It'll be a bigger CL, but I think the end result is worth it. Feel free to let me know if it's too hard to review and if there are ways I can make it easier (maybe by splitting it into 2-3 smaller independent changes). Edit: Sent CL 111877.

@gopherbot
Copy link

Change https://golang.org/cl/111877 mentions this issue: maintner: propagate and use internal invariants in finishProcessingCL

@dmitshur dmitshur removed their assignment May 8, 2018
@golang golang locked and limited conversation to collaborators Aug 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Builders x/build issues (builders, bots, dashboards) Documentation FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

3 participants