dmitri.shuralyov.com/service/change/...

gerritapi: determine merged commit safely

Use the CurrentRevision of the CL to determine the merge commit,
rather than unsafely parsing it out of the human-readable message.

Background

Up until recently, all "autogenerated:gerrit:merged"-tagged messages
that I've encountered on the Gerrit instance at https://go-review.googlesource.com
have started with the 46-byte-long "Change has been successfully
cherry-picked as " string, followed by a 40-byte-long commit hash.

I didn't originally see a way to retrieve the merged commit from the
API response directly, so I implemented an unsafe string slice
operation that assumed the familiar message format.

A CL that used merge submit type rather than cherry-pick was submitted
at https://go-review.googlesource.com/c/go/+/164238/1#message-e75ad5d4bec9069a005bdc9efad6fbe8af8eb935
and generated a "autogenerated:gerrit:merged"-tagged message with
a different format, causing a panic.

By now, I've found a way to use the Gerrit API to determine the merge
commit. When a CL has an "autogenerated:gerrit:merged"-tagged messaged,
the CurrentRevision of the CL is the merge commit. Use it, since it's
more robust and safe.

dmitshur committed 5 years ago commit c25fb47d71b3e3fdf73798c58fffb56fa8be2921

Collapse all

gerritapi/gerritapi.go

@@ -255,11 +255,13 @@ func (s service) GetDiff(ctx context.Context, repo string, id uint64, opt *chang

func (s service) ListTimeline(ctx context.Context, repo string, id uint64, opt *change.ListTimelineOptions) ([]interface{}, error) {
	// TODO: Pagination. Respect opt.Start and opt.Length, if given.

	project := project(repo)
	chg, resp, err := s.cl.Changes.GetChangeDetail(fmt.Sprintf("%s~%d", project, id), nil)
	chg, resp, err := s.cl.Changes.GetChangeDetail(fmt.Sprintf("%s~%d", project, id), &gerrit.ChangeOptions{
		AdditionalFields: []string{"CURRENT_REVISION"},
	})
	if err != nil {
		if resp != nil && resp.StatusCode == http.StatusNotFound {
			return nil, os.ErrNotExist
		}
		return nil, err
@@ -282,11 +284,11 @@ func (s service) ListTimeline(ctx context.Context, repo string, id uint64, opt *
			case "gerrit:merged":
				timeline = append(timeline, change.TimelineItem{
					Actor:     s.gerritUser(message.Author),
					CreatedAt: message.Date.Time,
					Payload: change.MergedEvent{
						CommitID: message.Message[46:86], // TODO: Make safer.
						CommitID: chg.CurrentRevision,
						RefName:  chg.Branch,
					},
				})
			case "gerrit:newPatchSet":
				// Parse a new patchset message, check if it has comments.