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

dmitshur · 2018-10-22T18:21:23Z

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

The text was updated successfully, but these errors were encountered:

dmitshur · 2018-10-22T18:26:20Z

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 · 2018-10-29T06:44:30Z

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

This change expands the scope of cmd/maintserve to visualize Gerrit CL maintner data, in addition to the GitHub repository issue tracker data. I've needed this recently when investigating golang/go#28318 to check maintner.GerritHashtags values of various CLs. They are shown as of https://dmitri.shuralyov.com/service/change/...$commit/e712a6949fbe7fe04b2f49fc22810f827b17f3f8. maintner doesn't have sufficient information to present Gerrit CLs in full detail, so this does a best effort and displays the available information. Inline review comments and diffs are not included. The downside of this change is that it adds new dependencies. However, they are actively maintained by me. Updates golang/go#28318 Change-Id: Ie6fe14f95f107e95371ea820af88563e03a6bb2a Reviewed-on: https://go-review.googlesource.com/c/145258 Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>

Skarlso · 2018-11-15T05:49:55Z

@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 · 2018-11-15T05:55:18Z

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

Skarlso · 2018-11-15T05:56:20Z

Awesomesauce!

On it. :) Thanks!

Skarlso · 2018-11-15T21:56:56Z

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 · 2018-11-15T22:08:16Z

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 · 2018-11-16T05:16:53Z

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 · 2018-11-16T05:21:26Z

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 · 2018-11-16T05:56:00Z

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 · 2018-11-16T07:03:52Z

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 · 2018-11-16T21:46:26Z

I have a working fix 🎉

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

for certain commits. Bug: The previous implementation was only looking at the last commit message in order to parse out the hashtags. However, the last commit message was not always a tag action. This function of displaying the correct hashtags was not at all aware of the state of hashtags. Fix: The current implementation looks at all commits regarding an issue and works like a state machine by parsing the parent commit messages as well as the current one. The states can be added, removed or no action at all. Once all messages are parsed the correct tags are joined together and returned correctly. Fixes golang/go#28318

Skarlso · 2018-11-17T14:16:33Z

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.

hashtags on a Gerrit CL using a tag state machine. In order to see all hashtags properly when running maintner, a tag state machine was introduced to track changes made to them during the life cycle of a Gerrit issue. Now tags are visible on each issue in the correct format as present in Gerrit. Fixes golang/go#28318

gopherbot · 2018-11-19T22:29:37Z

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

gopherbot · 2018-12-06T00:21:09Z

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

gopherbot · 2018-12-11T01:06:01Z

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

gopherbot · 2018-12-11T01:56:19Z

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

In CL 152779, lineValue was modified to return just the value, lineValueRest was added to do the same task as the old lineValue, and a new lineValueOK with a third bool return value was created. This change fixes a test build failure by adapting TestLineValue to use the new lineValueOK function, and renaming it to TestLineValueOK. Also add two test cases to cover the new bool return value. Updates golang/go#28318 Change-Id: I08106cc14f0c418a880ab139310830943a3a1ad9 Reviewed-on: https://go-review.googlesource.com/c/153441 Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>

TestGerritHashtags is documented to be off by default unless environment variable TEST_GERRIT_AUTH is set to "user:token", or we're running in the prod project. However, the previous code never skipped TestGerritHashtags, even when TEST_GERRIT_AUTH wasn't set, because strings.SplitN returns a slice with length 1 when the input string is empty. This change fixes that. Updates golang/go#28318 Change-Id: Ib2f113dbc3ccfd933f9944fe8efa105ff3ba5057 Reviewed-on: https://go-review.googlesource.com/c/153442 Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>

dmitshur added Builders x/build issues (builders, bots, dashboards) NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Oct 22, 2018

gopherbot added this to the Unreleased milestone Oct 22, 2018

dmitshur added the help wanted label Oct 22, 2018

dmitshur mentioned this issue Oct 22, 2018

x/build/cmd/gopherbot: keeps repeating some actions without effect #28320

Open

3 tasks

dmitshur mentioned this issue Oct 31, 2018

x/build/cmd/gopherbot: wait-author tag not being auto-removed from updated CLs #28510

Closed

Skarlso mentioned this issue Nov 17, 2018

maintner: correctly parse hashtags on a Gerrit CL using a tag state machine golang/build#15

Closed

gopherbot closed this as completed in golang/build@228e5d2 Dec 7, 2018

golang locked and limited conversation to collaborators Dec 11, 2019

gopherbot added the FrozenDueToAge label Dec 11, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

dmitshur commented Oct 22, 2018

dmitshur commented Oct 22, 2018 •

edited

gopherbot commented Oct 29, 2018

Skarlso commented Nov 15, 2018

bradfitz commented Nov 15, 2018

Skarlso commented Nov 15, 2018

Skarlso commented Nov 15, 2018 •

edited

Skarlso commented Nov 15, 2018

Skarlso commented Nov 16, 2018

Skarlso commented Nov 16, 2018

Skarlso commented Nov 16, 2018

Skarlso commented Nov 16, 2018

Skarlso commented Nov 16, 2018 •

edited

Skarlso commented Nov 17, 2018

gopherbot commented Nov 19, 2018

gopherbot commented Dec 6, 2018

gopherbot commented Dec 11, 2018

gopherbot commented Dec 11, 2018

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

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

Comments

dmitshur commented Oct 22, 2018

dmitshur commented Oct 22, 2018 • edited

gopherbot commented Oct 29, 2018

Skarlso commented Nov 15, 2018

bradfitz commented Nov 15, 2018

Skarlso commented Nov 15, 2018

Skarlso commented Nov 15, 2018 • edited

Skarlso commented Nov 15, 2018

Skarlso commented Nov 16, 2018

Skarlso commented Nov 16, 2018

Skarlso commented Nov 16, 2018

Skarlso commented Nov 16, 2018

Skarlso commented Nov 16, 2018 • edited

Skarlso commented Nov 17, 2018

gopherbot commented Nov 19, 2018

gopherbot commented Dec 6, 2018

gopherbot commented Dec 11, 2018

gopherbot commented Dec 11, 2018

dmitshur commented Oct 22, 2018 •

edited

Skarlso commented Nov 15, 2018 •

edited

Skarlso commented Nov 16, 2018 •

edited