The HTTP response object contains an indication of how much (or until
when) to wait before the next successful API call.
This makes the code use this information to obey the rate limit
constraints.
Change-Id: I22cf851220db6f361f8663be28d49179f52dd623
Reviewed-on: https://go-review.googlesource.com/c/build/+/170959
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
This reverts commit 6265160dde.
Reason for revert: this incorrectly mirrors to the golang/protobuf repo, which is not intended.
Change-Id: Id75c3f9c82f98a0da0e6d33e3f617dae11354dea
Reviewed-on: https://go-review.googlesource.com/c/build/+/172279
Reviewed-by: Andrew Bonventre <andybons@golang.org>
Now that a pseudo-version of golang.org/x/build module with maintserve
carved out exists, the module golang.org/x/build/maintner/cmd/maintserve
can depend on it. Do so now.
This is analogous to CL 166857.
Updates golang/go#29935
Change-Id: I661f32336ede26fde5581527959d31d164a00b63
Reviewed-on: https://go-review.googlesource.com/c/build/+/167463
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
This change creates a new module with the module path
golang.org/x/build/maintner/cmd/maintserve, and moves the maintserve
command into it. This is done by adding a go.mod file in the
maintner/cmd/maintserve directory. As a result, the maintserve command
and its dependencies are removed from this and later pseudo-versions of
the golang.org/x/build module.
This is similar to how the h2demo command was carved out into a separate
module in CL 162822.
The motivation for doing this is that the maintserve command requires a
large number of dependencies for its own use, that no other package in
x/build needs. Even though authors of library modules should not be
requiring x/build, it still happens (see golang/go#29935). So, improve
the situation by moving maintserve into a module separate from x/build,
which in turn significantly reduces the dependencies in x/build.
Updates golang/go#29935
Change-Id: Ic64da32be142fedf6475a11fea787cdfe245e0ce
Reviewed-on: https://go-review.googlesource.com/c/build/+/167461
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
This README was an accident. I had an early prototype of maintner on
my local disk as slurpgit.go (before it was named maintner). Because
it was on my disk, when I ran x/build/update-readmes.go, it created
this directory.
Fixesgolang/go#30744
Change-Id: I8a7807a8c7c0faaf31124cba1401d7c96b0bdbdc
Reviewed-on: https://go-review.googlesource.com/c/build/+/166878
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
This removes a bunch of TODOs about scattered policy and moves it all
into builders.go.
While here,
* add a bunch of tests
* unexport some things
* rename some things
* document some things
* adjust FreeBSD policy as function of branch (per Go 1.12 being
last to support FreeBSD 10.x, and to unblock CL 165801)
* set GO_BUILDER_SET_GOPROXY=coordinator for reverse buildlets,
which is necessary to remove the oauth2 & build special cases
in the config
* change Elias Naur's mobile builder to how I think he wants it
(he was fighting the old system)
* add $HOME on the Solaris smartos builder, which was missing &
causing tests to fail lately
* make the (currently failing) longtest builder have GOPROXY set
* remove an allocation in version.ParseReleaseBranch
Fixesgolang/go#9603
Updates golang/go#14594
Change-Id: I50a23ad7cdf478c95b14bee9b3931ba361baacfa
Reviewed-on: https://go-review.googlesource.com/c/build/+/166218
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
The internal representation of the type implementing the error
interface returned by fmt.Errorf may change in new versions of Go,
and reflect.DeepEqual is not suitable for making stable comparisons.
For more complicated comparisons the go-cmp package should be used,
but since we're just comparing two error values, use a simple helper
instead.
Remove insignificant trailing whitespace from Go strings containing
JSON objects.
Fixesgolang/go#30442
Change-Id: Iecfb89ebcaf97b49fbe34b4fb17daa7fe70d6a7e
Reviewed-on: https://go-review.googlesource.com/c/164297
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Change https://dev.golang.org/reviews page to be more helpful
at displaying CLs that are in need of a general code review.
Don't hide CLs containing DO NOT SUBMIT. Per Interim Code Review
and Issue Tracker Conventions¹:
DO NOT SUBMIT means that while the CL is okay to review,
the author wants Gerrit to make sure the CL is not submitted
in its current form.
That means we should not be excluding CLs containing DO NOT SUBMIT
from the reviews page.
Remove a log line from GerritMeta.LabelVotes method. It's not serving
a helpful purpose at this time, but produces a large amount of output.
¹ https://groups.google.com/d/msg/golang-dev/YU56eKYiXJg/K1CkXo7Iy0gJ
Change-Id: Ibe40f756e6a0dab0a13c75fa53998e2d56d287e3
Reviewed-on: https://go-review.googlesource.com/c/163159
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
maintner represents Gerrit users via its underlying low-level
git author representation, such as:
Foo Bar <GerritUserID@GerritServerID>
The server ID represents the Gerrit instance and doesn't change.
The user ID uniquely identifies a Gerrit user on the Gerrit instance.
However, Gerrit is not consistent about the name it uses. Sometimes
it's the actual name, but other times it's "Gerrit User <NumericID>".
For example, both of these forms come up:
Dmitri Shuralyov <6005@62eb7196-b449-3ce5-99f1-c037f21e1705>
Gerrit User 6005 <6005@62eb7196-b449-3ce5-99f1-c037f21e1705>
Fix the author comparison logic in unwaitCLs task by comparing only
the git email of Gerrit users.
Fixesgolang/go#30172
Change-Id: Ib193de844ecc6212723344765fc920bc08d906a4
Reviewed-on: https://go-review.googlesource.com/c/161977
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
The githubRepoPoller type has a field c, which is a shortcut for
gr.github.c. Other githubRepoPoller methods use it, so start using
it in syncIssues as well.
This makes the code more consistent and easier to read.
Also remove a spurious period in GitHubRepo.newMutationFromIssue
documentation.
Change-Id: Iecebd2ba6c69e3479e8d627fea691fcf9415aa51
Reviewed-on: https://go-review.googlesource.com/c/160841
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
This is a followup to CL 161222, where I forgot to handle the case
where a commit message has a multi-line subject and no body.
Updates golang/go#30101
Change-Id: I01222e8cf783bc3b0631a332cf379717aa85cebc
Reviewed-on: https://go-review.googlesource.com/c/161797
Reviewed-by: Andrew Bonventre <andybons@golang.org>
In git, the text up to the first blank line in a commit message
is treated as the commit subject.
Most subjects are short and fit in one line (hopefully under
50 characters). In some cases, they end up being split into
multiple lines.
Report the entire subject rather than just the first line,
and join multiple lines with spaces.
Reference: https://git-scm.com/docs/git-commit#_discussion.
Fixesgolang/go#30101
Change-Id: I1ea8212202fce0033e5ac8075352d5d7e0ae823c
Reviewed-on: https://go-review.googlesource.com/c/161222
Reviewed-by: Andrew Bonventre <andybons@golang.org>
In CL 107296, the maintner API had undergone a change such that all
public endpoints would only return GerritCLs that are considered
complete. Complete CLs are defined to have certain properties that
make working with them more friendly. One of those properties is that
a complete GerritCL always has a non-nil Commit field.
Methods of GerritCL type are expected to operate on complete CLs
that were acquired via one of the public means (e.g., ForeachOpenCL,
ForeachCLUnsorted, CL), so it's a precondition that the CL must be
complete. (If the user makes a GerritCL type from scratch, they are
expected to follow the documented requirement that GerritCL.Commit
must be non-nil.)
The Footer, ChangeID methods were already relying on the fact that
Commit is non-nil. This change updates the GerritCL.Subject method
to also make use of that property, which makes the cl.Commit nil
check not necessary.
This leads to simpler code and higher confidence that our internal
invariants are maintained.
Change-Id: I5867fd248274b8dd94a0ca98a91a773d48b3bdec
Reviewed-on: https://go-review.googlesource.com/c/161221
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
The new BuildConfig.MinimumGoVersion field specifies the minimum
Go version the builder is allowed to use. It's useful when some
of the builders are too new, and do not support all of the supported
Go releases (e.g., openbsd-amd64-64 and freebsd-amd64-12_0 currently
require Go 1.11 and don't work on Go 1.10).
It only needs to be set when a builder doesn't support all supported
Go releases, since we don't typically test unsupported Go releases.
To allow cmd/coordinator to use this field and filter out work it
receives from maintner/maintnerd's GoFindTryWork RPC call,
we add a GoVersion slice to apipb.GerritTryWorkItem,
and populate it in maintapi.apiService.GoFindTryWork method.
For trybots on the Go repo, the GoVersion field is determined from
the branch name. For "release-branch.goX.Y" branches, it parses out
the major-minor Go version from the branch name. For master and
other branches, it assumes the latest Go release.
For trybots on subrepos, we already have the Go version information
available, so use it directly.
Afterwards, all that's left is to modify newTrySet in cmd/coordinator
to make use of BuildConfig.MinimumGoVersion and work.GoVersion to skip
builders that are too new for the Go version that needs to be tested.
Fixesgolang/go#29265
Change-Id: I50b01830647e33e37e9eb8b89e0f2518812fa44f
Reviewed-on: https://go-review.googlesource.com/c/155463
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Create a new ./maintner/maintnerd/maintapi/version package and move
the version parsing code there. Change the implementation to use
strings.Split on "." rather than a regexp. Make it more precise at
rejecting invalid names and increase test coverage of edge cases.
Having this code be in a separate package makes it easier to use it
elsewhere when needed.
Change commit hash in TestSupportedGoReleases for -security branches,
to make the test failure more prominent.
Remove an unneeded blank line at the end of a function body.
Change-Id: I3168b0b25a6b6433d17f83b01580f08208532daf
Reviewed-on: https://go-review.googlesource.com/c/157559
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
The website now has its own subrepo with gerrit mirroring set up.
Updates golang/go#29206
Change-Id: I6fb6fb62dfd50b48d2f78db2503641c521600ae7
Reviewed-on: https://go-review.googlesource.com/c/156217
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Katie Hockman <katie@golang.org>
Assign the first element to an empty slice, rather than appending it.
There is no behavior change. The goal of this CL is to improve code
readability only.
After this change, it should be easier for the reader to see what
the slice content will be, without having to verify that earlier code
has not assigned anything to that slice.
Change-Id: I2aaeaafafbdf206f57b0f85b42ccfa726e72ca8c
Reviewed-on: https://go-review.googlesource.com/c/155461
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>
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>
The Gerrit meta commit graph is a linear history. The most recent meta
with a "Hashtags: " footer line has the complete set. We just have to
go back and look for it.
Fixesgolang/go#28318
Updates golang/go#28510 (fixes after gopherbot re-deployed)
Updates golang/go#28320 (fixes after gopherbot re-deployed)
Change-Id: I43705075800ae3d353c1c8f60ab7685883ea5602
Reviewed-on: https://go-review.googlesource.com/c/152779
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
Previously, maintner would track only the Events
on an Issue/PullRequest, which missed out on the
discussions and approvals that the review
process generates.
This adds a new struct GitHubReviewEvent to represent
the results from the GitHub "pulls/reviews" API. This
struct follows the same pattern as the GitHubIssueEvent
struct.
To accomplish this, a new call has been added to the
sync() function: syncReviews, which follows the
pattern established with syncEvents().
Finally, this adds an additonal method: ForeachReview which
iterates over the GitHubReviewEvents in
the given GitHubIssue serially and in
chronological order.
GitHub API Reference: https://developer.github.com/v3/pulls/reviews/
Updates golang/go#21086
Change-Id: I4f59e154a4e4a8a4b8f2676dea932cf44354c288
Reviewed-on: https://go-review.googlesource.com/c/144699
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
The ClosedBy field is currently always nil due to the cause described
in the linked issue. Document it with a TODO comment so people don't
need to spend time on figuring that out for themselves.
Updates golang/go#28745
Change-Id: Icaa7b8fd5614dffbfd13a9783b9a71cb87e2af40
Reviewed-on: https://go-review.googlesource.com/c/149238
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Gerrit's NoteDB git-based metadata format changed recently to display
"Gerrit User 1234" as the display name for meta commit authors. Use
the Gerrit REST API to retrieve the proper display name and cache the
result to prevent superfluous API calls.
Also updates the maintner doc for the Author property of GerritMessage
to reflect NoteDB's format.
Fixesgolang/go#28663
Change-Id: I549474ad139e48c736d715414e82b6db83a9fdf3
Reviewed-on: https://go-review.googlesource.com/c/148560
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
This change deletes the OwnerName method on GerritCL, since Gerrit's
format for its NoteDB backing store changed to display "Gerrit User NNN"
instead of the proper display name in the meta commit.
Update golang/go#28663
Change-Id: Ifa64ca2b2694b17e888451582b9c63f3f37280a9
Reviewed-on: https://go-review.googlesource.com/c/148557
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
This change computes the supported Go release branches,
and adds them to the GerritTryWorkItem for trybots that
run on subrepos.
This should help significantly with trybots catching regressions
in subrepos that affect older, but supported Go releases.
Fixesgolang/go#17626
Change-Id: I4d88f6510c34d0c28266d44c341d5831a8514fce
Reviewed-on: https://go-review.googlesource.com/c/147198
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Previously, the list-Go-releases RPC endpoint was more flexible and
considered a Go release to exist as long as there's a git tag for it,
regardless of whether a corresponding release branch existed.
Such a situation is very unlikely to come up, and we don't want to cause
all users of this API to filter out releases without a release branch.
So just re-define a Go release to require a release branch, and
make that the API promise.
In practice, this should not have any effect because all go tags
have corresponding release branches.
Updates golang/go#17626
Change-Id: Ia7a8354000483c969e123f0f3605fd360846c40b
Reviewed-on: https://go-review.googlesource.com/c/147200
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
This change adds an RPC endpoint to maintnerd API server to list
the supported Go releases. This is needed and will be used by
cmd/coordinator to run subrepo trybots on previous Go release
(in addition to current).
It's implemented on top of the Gerrit project ref data that the
maintner corpus already tracks. A release is considered to be
exist for each git tag named "goX", "goX.Y", or "goX.Y.Z".
This functionality is also implemented in some other places using
data that is further away from the source of truth. Such places can
start to use this endpoint instead.
Add a subcommand to maintq to invoke the new list Go releases endpoint.
Its current output (against a local development maintnerd server):
$ go run .../maintner/maintq -server=localhost:6344 list-releases
major:1 minor:11 patch:1 tag_name:"go1.11.1" tag_commit:"26957168c4c0cdcc7ca4f0b19d0eb19474d224ac" branch_name:"release-branch.go1.11" branch_commit:"97781d2ed116d2cd9cb870d0b84fc0ec598c9abc"
major:1 minor:10 patch:4 tag_name:"go1.10.4" tag_commit:"2191fce26a7fd1cd5b4975e7bd44ab44b1d9dd78" branch_name:"release-branch.go1.10" branch_commit:"e97b7d68f107ff60152f5bd5701e0286f221ee93"
Updates golang/go#17626
Change-Id: Ia9ea7f49d421ce0c7a9e85c423aba31572cea52b
Reviewed-on: https://go-review.googlesource.com/c/146137
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
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
e712a6949f.
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>
CL 107296 has created a GerritCL.complete method and started
using it to enforce non-nil Meta and Commit fields in GerritCL
that are returned to the client.
It was used in ForeachCLUnsorted to skip incomplete CLs when
iterating over the gp.cls map, but due to an oversight, it
was not added to ForeachOpenCL. As a result, it was possible
for incomplete CLs to be iterated over via ForeachOpenCL.
This change fixes that by adding the missing complete check.
Update TestGerritMetaNonNil test to check ForeachOpenCL too.
Add missing period at the end of a sentence in ForeachCLUnsorted
documentation.
Fixesgolang/go#27536.
Change-Id: I453d0f2b2b2793ed1b8a55bae1c2254906d74792
Reviewed-on: https://go-review.googlesource.com/135677
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
In golang/go#23223, we've created a new subrepository:
https://go.googlesource.com/dl
Unlike other subrepositories, there isn't a mirror on GitHub yet:
https://github.com/golang/dl (404 at this moment)
We want to include it in the whitelist for mirroring to GitHub,
including mirroring its pull requests to Gerrit. The motivation is to
make this subrepository consistent with all others and prevent people
from running into an unexpected situation when some repos are mirrored
but some aren't.
We don't actually expect much contribution activity to the dl subrepo,
because it's very narrow in scope and mostly generated code.
Updates golang/go#26949.
Change-Id: Id995bb3d97b8eb328e6c6bba6714bc74fbefb942
Reviewed-on: https://go-review.googlesource.com/133896
Reviewed-by: Andrew Bonventre <andybons@golang.org>
Look for changes that haven’t had any human aside from the author
on them and assign reviewers/CCs based on entries in golang.org/s/owners.
If no owners can be found, the tag 'no-owners' is added to the
change to prevent gopherbot from making unneeded API calls.
Also updates gopherbot's Dockerfile to use 1.11 and update deps.
Change-Id: I2650a10dd324532d86bc902be419c5f29ae980db
Reviewed-on: https://go-review.googlesource.com/121018
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
When parsing commit messages for references to GitHub issues,
parseGithubRefs used to add entries to GerritCL.GitHubIssueRefs
slice whenever there was a reference, even if that reference has
already appeared before.
The GitHubIssueRef type only contains minimal information about
a reference: the repository and the issue number. As a result,
there is very little to gain from tracking multiple references
separately. Most users of the API would likely prefer a list
of unique issue references than the raw list. So, change it to
return unique issue references only, and document that.
If in the future there is a need to track issue references
at a more granular level, where it matters whether there was
one or multiple references, the API can be revisited then.
Fixesgolang/go#26113.
Change-Id: Ib51372736bcf49e3eae4cf111d0258733c742e61
Reviewed-on: https://go-review.googlesource.com/128119
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
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.
Fixesgolang/go#24744.
Change-Id: I07e362d52e30089a9ba03c30b04ad19b2c385722
Reviewed-on: https://go-review.googlesource.com/111877
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Explain how to keep the godata corpus up-to-date, and clarify the
requirements around locking and updating.
Fix a typo.
Change-Id: Ic9dbe203d1dee7ad3c7463ae932df5daadb51923
Reviewed-on: https://go-review.googlesource.com/47813
Reviewed-by: Andrew Bonventre <andybons@google.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
GitHub added some new field in their responses ("node_id") which
causes our maintnerd to start crashing.
The code was supposed to just log.Print, not log.Fatal.
Also, ignore that field. Not worth saving.
Change-Id: I3ecb20eb3c99c27baa147b226e1d21ce5ec592cb
Reviewed-on: https://go-review.googlesource.com/115335
Reviewed-by: Andrew Bonventre <andybons@golang.org>
Previously, the ids[i].Owner < ids[i].Owner condition would never
be true due to the typo.
Found with staticcheck tool by Dominik Honnef.
Change-Id: Ia067e68909c7f30968c927d4bfa68ca5f7653959
Reviewed-on: https://go-review.googlesource.com/111878
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
This change fixes a copy-paste mistake from CL 38137, where the wrong
time variable was set to UTC when cmut.Updated is non-nil.
Change-Id: I4dc62abf9b70b52b700e3b6b688fa8adbc502d98
Reviewed-on: https://go-review.googlesource.com/111645
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
GitHub reviews can be assigned to teams.
Change-Id: I675944c40619635e554d083cfa4fd6e332315f9b
Reviewed-on: https://go-review.googlesource.com/101955
Reviewed-by: Chris Broadfoot <cbro@golang.org>
From the "gcloud docker" output:
WARNING: `gcloud docker` will not be supported for Docker client
versions above 18.03. Please use `gcloud auth configure-docker` to
configure `docker` to use `gcloud` as a credential helper, then use
`docker` as you would for non-GCR registries.
Not adding "gcloud auth configure-docker" to the Makefiles as it spams
and there are implicit required authentication steps already anyway.
Change-Id: I5bd1177e82d30a6590126a307bee01d0acee9d6a
Reviewed-on: https://go-review.googlesource.com/108560
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Andrew
Damien Neil
Nikhil Benesch
Dmitri Shuralyov
Brad Fitzpatrick
Channing Kimble-Brown
Colin Nelson
Kevin Burke
Dmitri Shuralyov
Chris Broadfoot
Filippo Valsorda