x/build/maintner: reports incorrect Hashtags for some CLs

[dmitshur]

CL https://go-review.googlesource.com/c/gddo/+/135536 (36 days old) has a no-owners hashtag:

However, the current maintner corpus reports that it does not have any hashtags:

package main

import (
    "context"
    "fmt"
    "log"

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

func main() {
    corpus, err := godata.Get(context.Background())
    if err != nil {
        log.Fatalln(err)
    }
    proj := corpus.Gerrit().Project("go.googlesource.com", "gddo")
    cl := proj.CL(135536)
    tags := cl.Meta.Hashtags()
    const tagNoOwners = "no-owners"
    fmt.Println(tags.Contains(tagNoOwners))

    // Output: false
}

It should print true.

This also affects some other CLs, including (not an exhaustive list):

• https://go-review.googlesource.com/c/gddo/+/135537 (36 days old)
• https://go-review.googlesource.com/c/go/+/120215 (5 days old)

So far, I haven't found the pattern of which CLs are affected. It seems to affect CLs in main repo and subrepo, and affects quite recent CLs as well as older ones. Many other CLs have correct hashtags.

/cc @bradfitz

[dmitshur]

This would be a good issue for anyone who wants to help out and do some investigation. It'd be very helpful for us to find out what's causing this, or what the pattern of affected CLs is.

[gopherbot]

Change https://golang.org/cl/145258 mentions this issue: maintner/cmd/maintserve: display Gerrit projects and CLs

[Skarlso]

@dmitshur Hi! :)

I'm a first time contributor here. I have experience with Go and would very much like to investigate, go down a rabbit hole and find out what's happening and fix it. :)

I've read the surrounding issues and have a vague idea of where to start. I have very little knowledge yet, but I seek to understand things better and be a contributor to Go in the long run. This is the first programming language I love to use for years and I would like to give back to it!

If you don't mind, I would like to take a stab at this. I've read the contribution guideline and did setup my environment.

[bradfitz]

@Skarlso, go for it. This is a nice self-contained problem so it'd be a good starter bug. Thanks!

[Skarlso]

Awesomesauce!

On it. :) Thanks!

[Skarlso]

Okay, I had time a little to play with this. What's happening is, that this thing is parsing the commit message to access the last entry like this:

// Footer returns the "key: value" lines at the base of the commit.
func (m *GerritMeta) Footer() string {
    i := strings.LastIndex(m.Commit.Msg, "\n\n")
    if i == -1 {
        return ""
    }
    return m.Commit.Msg[i+2:]
}

Looking for a thing called Hashtag in here.

What's happening is, that on some issues, the commit message is updated, and the LAST entry in the list of commit messages will not be something that contains a Hashtag entry, but rather a message like, Uploaded patch set 5: Commit message was updated..

Which in turn will no longer have this entry:

DEBUG: Message:  Update patch set 1

Hashtag added: wait-author

To see this on issues look at this where the commit message is updated:

https://go-review.googlesource.com/c/gddo/+/135536

And look at this issue where it's not and the hashtag is properly parsed out:

https://go-review.googlesource.com/c/go/+/95895

I'm not sure yet what the fix will be, but I'm on the right path, I think.

It's getting later however, so I'll be continuing this tomorrow.

[Skarlso]

The fix will probably involve parsing out the hashtag from somewhere else maybe, or something like, if it's encountered, save it and always display it unless a remove is encountered. Not sure yet.

Also, I need to fully understand what's happening in the display code under gerrit.go. :)

[Skarlso]

I have a failing unit test!

        commit: `Update patch set 1

Hashtag added:   bar

Patch-set: 1
Hashtags:
Tag: autogenerated:gerrit:setHashtag

Update patch set 2
`,
        wantAdd: "bar",
    },

The exact problem is that this line:

Tag: autogenerated:gerrit:setHashtag

... is missing from the consecutive update. Which makes things more interesting. If this line would be sent along the update, things would work.

[Skarlso]

To be accurate this is how it actually looks like:

        commit: `Update patch set 1

Hashtag added:   bar

Patch-set: 1
Hashtags:
Tag: autogenerated:gerrit:setHashtag

Create patch set 5

Uploaded patch set 5: Commit message was updated.

Patch-set: 5
Subject: gddo-server: fix expanding examples containing reserved characters
Commit: 9ef378a957aa4313a34acfdcd3d0bcdca2e7b9d1
Tag: autogenerated:gerrit:newPatchSet
Groups: 9ef378a957aa4313a34acfdcd3d0bcdca2e7b9d1

`,
        wantAdd: "bar",
    },

[Skarlso]

I think I understand what's going on. The parser parses the messages backwards then reverses the commits messages so we get the last one. But the last one in this case will be a newPatchSet and will not contain the setHashtag action.

So, I'm wondering. Is this something that you (@dmitshur) tried to address with your change set here: https://go-review.googlesource.com/c/build/+/145258/ ?

[Skarlso]

Yeah no. That only seems to be an update on displaying things. Doesn't change anything relevant to this issue if I understand it correctly

[Skarlso]

I have a working fix 🎉

Need to write some unit tests around it first and work out some quirks.

[Skarlso]

Yay! I'm looking forward to working together with you guys @dmitshur @bradfitz to get this solved. I hope the solution suits the needs and is in alignment with the rest of the project. If not, please let me know and I will address any issues that might come up.

Cheers, Gergely.

[gopherbot]

Change https://golang.org/cl/150075 mentions this issue: x/build/maintner: reports incorrect Hashtags for some CLs fixed by tag state machine

[gopherbot]

Change https://golang.org/cl/152779 mentions this issue: maintner: fix GerritMeta.Hashtags to look at earlier meta parents for answer

[gopherbot]

Change https://golang.org/cl/153441 mentions this issue: maintner: update TestLineValue to use new lineValueOK

[gopherbot]

Change https://golang.org/cl/153442 mentions this issue: maintner/godata: skip TestGerritHashtags when TEST_GERRIT_AUTH unset