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
Comments
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. |
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 |
/cc @bradfitz Do you agree this is a bug? |
Sounds like a bug. It also needs proper docs: https://godoc.org/golang.org/x/build/maintner#GerritCL.Created |
It was hard to fix this bug with a short diff, because there were too few guarantees inside (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 We already established some guarantees for the user-facing CLs (i.e., that their 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 // 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 // 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 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. |
Change https://golang.org/cl/111877 mentions this issue: |
What version of Go are you using (
go version
)?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
What did you expect to see?
maintner.GerritCL.Created
is not documented, but given the field is namedCreated time.Time
and it's inside theGerritCL
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 theCommitTime
of the latest meta commit. That would explain the results I saw.The text was updated successfully, but these errors were encountered: