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>
When git has no http cookies, the request for http cookies
will fail because git will exit(1). Ignore this failure
because the output is properly tested either way. This allows
authentication to fallthrough to the netrc lookup.
Correct the netrc lookup under windows. git reads the netrc
file as "_netrc" in the users home directory.
Add a warning at the end of the function that no authentication
was set.
Fixesgolang/go#26782
Change-Id: I0ba94ff7fa4b6038d6117156dcc729ddf4616fdc
Reviewed-on: https://go-review.googlesource.com/127855
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
This will allow for zero-downtime deployments, which will be
important for when services start relying on the /owners endpoint.
Change-Id: I4671dbbbf473ba07dbab585222d12832c3193fac
Reviewed-on: https://go-review.googlesource.com/127036
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
QueryChangesOpt has now a new pagination field Start for the number of first changes to skip, as it was already implemented for QueryAccountsOpt.
Fixesgolang/go#24838
Change-Id: If779a404f256aca1924ce2412c79b821f4f9f639
GitHub-Last-Rev: aa79a753b6
GitHub-Pull-Request: golang/build#8
Reviewed-on: https://go-review.googlesource.com/122584
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
We don't need to run all the tests in a trybot. Run just enough to get
good coverage, without going over 5 minutes.
Any regressions elsewhere will be caught by the build.golang.org
(slower) runs.
Change-Id: I32f1fc17681bfb509844d2bd35b05c950806d283
Reviewed-on: https://go-review.googlesource.com/121938
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Windows Defender scans all new files written to disk which slows down
build performance.
- Uninstall Windows Defender as part of image building
- Refactor/Cleanup registry section for consistency
- Promote new image version to dashboard (built in staging/prod)
fixesgolang/go#26055
Change-Id: I3e16b9a9581824c20abed5e8ffae1efd46c6dd09
Reviewed-on: https://go-review.googlesource.com/121937
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Go 1.9 is the latest offered on App Engine, which at least is new
enough to get us type aliases.
Fixesgolang/go#26115
Change-Id: I4de3062cf2f7df70216536a05b7150324c1a5e91
Reviewed-on: https://go-review.googlesource.com/121418
Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>
This change introduces a tiered model for making changes to labels:
+ If a label exists (or does not exist in the case of removal) on an
issue according to maintner, then no action is taken to add (or
remove) that label.
+ Before making any requests to remove a set of labels, the GitHub
API is checked to see which ones are already not present on the issue.
Then it only removes the labels that exist on the issue.
Change-Id: If6693db71ad9dfc3537c462bcdbc5af8f33e5b16
Reviewed-on: https://go-review.googlesource.com/120043
Reviewed-by: Bryan C. Mills <bcmills@google.com>
See tests for the various acceptable range of commands.
Fixesgolang/go#24785
Change-Id: If3dfded60db4879f6d7e2281a3ba014a003a2b81
Reviewed-on: https://go-review.googlesource.com/119538
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Jakob Borg kindly supplied a new iPhone 6 to replace the ailing
iPhone 5 (arm) and iPhone 6 (arm64) that currenctly runs the iOS
builders.
The new phone runs iOS 10 that can run both arm and arm64, so
instead of replacing the device ids specified with GOIOS_DEVICE_ID,
simply remove them altogether.
Change-Id: I36cf574a902b953a3d2bf2a5de50741dff1b940d
Reviewed-on: https://go-review.googlesource.com/119415
Run-TryBot: Elias Naur <elias.naur@gmail.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Split the process in two parts with a manual +2 and submit of the
release commit in the middle. This way the release manager doesn't need
to force submit anything, and the first stage stops before serious
write operations. This also made dry-run mode more complete and the
process easier to resume if something breaks.
Completely removed the beta and rc code as it got messy with the partial
rewrite and it would have just been broken untested code waiting to break.
Fixesgolang/go#24902
Change-Id: I2cbd9bdf88e283d2ca527e5c91c620617d7e068e
Reviewed-on: https://go-review.googlesource.com/116357
Reviewed-by: Andrew Bonventre <andybons@golang.org>
The nacl image hadn't been updated in 2+ years and it needed to be
updated as part of rolling out the new COS-based builders.
But no released version works for us yet; we were getting the same
errors as in golang/go#23836 ("Signal 11 from untrusted code")
We were getting lucky that it was working with an ancient (pepper_34?)
version, but I was unable to get those working again either.
Rolling forward is better anyway, as we haven't had a Dockerfile
reflecting reality for this builder for 2+ years.
This is the same version used in playground in CL 101735, which said:
> playground: update NaCl to trunk.544461
>
> This pulls in https://crrev.com/c/962675, which fixes the
> underlying issue of NaCl mishandling signals during a SIGSEGV.
Updates golang/go#23836
Updates golang/go#25108
Change-Id: I187042af71a1249e84ce2070aa8039a88d2c02c2
Reviewed-on: https://go-review.googlesource.com/112735
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Andrew Bonventre <andybons@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>
Using global API keys or user managed service account keys for
"personal" actions (as opposed to application ones) is discouraged, as
it does not map to a specific user identity.
Change-Id: I945cfc3ad581a9c1df2289c6b77891164258802d
Reviewed-on: https://go-review.googlesource.com/117315
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
In https://golang.org/cl/115375 I had neglected to test against a
non-empty README file, and accidentally wrote a regexp that never
matches.
Change-Id: I676cb11abea7e0f5bf337aab640ebc2478295a9b
Reviewed-on: https://go-review.googlesource.com/115377
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Use a known version of MinGW on Windows. (On other platforms, we use
whatever compiler is already installed on the system image.)
Test at a well-defined Go commit.
Record the Go and LLVM commits for each platform independently.
Extracting the C++ toolchain version is left for future work, but it
should be recoverable from the resulting .syso file anyway.
Change-Id: I9af1d2a6f540a4d276b87074864564bf989e4731
Reviewed-on: https://go-review.googlesource.com/115375
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>
CL 114595 added git and hg to the stretch builder to fix cmd/go tests,
but I missed that one test is using svn. Add svn too.
Change-Id: I8c6555c1c71cecd4012136ca413008025deeef32
Reviewed-on: https://go-review.googlesource.com/114676
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Dmitri Shuralyov
Kevin Burke
Daniel Theophanes
Andrew Bonventre
Brad Fitzpatrick
Martin Möhrmann
Quentin Perez
Dmitri Shuralyov
Daniel Martí
Robert-André Mauchin
Hana Kim
Michael Dorner
Akhil Indurti
Jeff Johnson
Filippo Valsorda
Elias Naur
He Liu
Chris Broadfoot
Bryan C. Mills
Alberto Donizetti