1169834 - Record review URLs in commit messages and use them to track commits

Status: RESOLVED INVALID

(MozReview Graveyard :: General, defect, P3)

Description

[Gregory Szorc [:gps]]

+++ This bug was initially created as a clone of Bug #1160266 +++

Following up from bug 1160266 to add review URLs to commit messages so we can better track commits.

Comment 1

[Gregory Szorc [:gps]]

Attachment: MozReview Request: mozautomation: functions for parsing review URLs out of commit messages (bug 1169834); r?smacleod

mozautomation: functions for parsing review URLs out of commit messages (bug 1169834); r?smacleod

We're about to insert review URLs in commit messages. Let's create a
mechanism for parsing review URLs out of commit messages.

Comment 2

[Gregory Szorc [:gps]]

Attachment: MozReview Request: mozreview: add review URL to commit messages (bug 1169834); r?smacleod

mozreview: add review URL to commit messages (bug 1169834); r?smacleod

One of the reasons behind the popular "Bug N" commit message notation at
Mozilla is to give humans a link to follow to find out more context for
a commit. Code review is an integral part of code development. And now
that MozReview exists, code review can occur in a different venue from
bug tracking. Although, there is a link between MozReview and Bugzilla,
at least for now.

This commit enables automatic rewriting of commit messages on push to
review repositories to include the review URL that commit will be reviewed
at. This annotation will be forever engraved in the commit message. Humans
can follow the link to find more context around code review.

A URL was chosen instead of merely a short review request ID. URLs
are more descriptive and help build the hyperlinked web. Review request
IDs are merely fragments. They must be assembled into URLs for you to
derive much use from them. This is more work for humans and for
machines. URLs win here. URLs are longer than review request IDs.
But, since machines are writing them, the only significant penalty
for that should be a slight increase in per-changeset storage size
in the repository. For "https://reviewboard.mozilla.org/r/", that
penalty is 34 bytes. That's a few megabytes per 100,000 changesets
uncompressed. That is insignificant in the grand scheme of things.

In the future, machines can also leverage this new metadata. In
particular, the MozReview server can look for annotations to map
incoming changesets against existing review requests.

Since the Review Board server/URL is at a dynamic location in the test
environment, we had to invent a mechanism to use fake and static base
URLs in tests in order to ensure changeset SHA-1s don't change between
test runs. I am embarassed to admit how many hours it took me to realize
that dynamic URLs were the source of otherwise inexplainable test
output. This was partially masked by how Mercurial's test harness
substitutes variables before diffing output. So, the variable test ports
were not apparent.

Comment 3

[Gregory Szorc [:gps]]

Attachment: MozReview Request: mozreview: use review URL annotation to map commits to review requests (bug 1169834); r?smacleod

mozreview: use review URL annotation to map commits to review requests (bug 1169834); r?smacleod

Now that we store commit IDs in the extra fields of commits and review
URLs in commit messages automatically, the server can use this metadata
to better map submitted commits to existing review requests.

In this change, we enable the server to start looking at review URL
annotations as part of the commit to review request mapping. This
mapping takes precedence below exact commit/SHA-1 mapping but above
obsolescence data.

Comment 4

[Gregory Szorc [:gps]]

Attachment: MozReview Request: mozreview: support partially landed series through submitted review requests

mozreview: support partially landed series through submitted review requests

A goal of MozReview is to enable developers to move fast and be more
productive. Part of moving fast means landing commits as soon as they
are ready to land. Lingering commits are susceptible to bit rot and
addressing bit rot adds overhead.

Currently, MozReview operates an all-or-nothing mentality towards
pushes and series of commits: the series of commits on the DAG is what
is turned into the review series.

A significant problem with this approach is that once a commit lands, it
disappears from the review interface. And it doesn't disappear in a good
way: the parent review request will discard it when new changesets are
pushed. Unfortunately, discarded commits aren't easily found in the
interface. What's more, in some scenarios, old review requests could get
overwritten by new ones. This could lead to some very confusing
scenarios where a review was completed, landed, and then overwritten by
a new, unrelated commit!

This commit starts to address these problems.

This commit teaches the review request assignment algorithm how to be
aware of submitted review requests. If an individual review request
becomes submitted (there isn't currently an easy way of doing that -
this will be implemented later), this review request is treated
specially. It effectively becomes read-only. It is also moved to the
bottom of the stack of commits.

This code isn't perfect. There are numerous improvements that could be
made. But you have to start somewhere.

Comment 5

[Gregory Szorc [:gps]]

Comment on attachment 8613133 [details]
MozReview Request: mozautomation: functions for parsing review URLs out of commit messages (bug 1169834); r?smacleod

mozautomation: functions for parsing review URLs out of commit messages (bug 1169834); r?smacleod

We're about to insert review URLs in commit messages. Let's create a
mechanism for parsing review URLs out of commit messages.

Review-URL: https://reviewboard.mozilla.org/r/9715

Comment 6

[Gregory Szorc [:gps]]

Comment on attachment 8613134 [details]
MozReview Request: mozreview: add review URL to commit messages (bug 1169834); r?smacleod

mozreview: add review URL to commit messages (bug 1169834); r?smacleod

One of the reasons behind the popular "Bug N" commit message notation at
Mozilla is to give humans a link to follow to find out more context for
a commit. Code review is an integral part of code development. And now
that MozReview exists, code review can occur in a different venue from
bug tracking. Although, there is a link between MozReview and Bugzilla,
at least for now.

This commit enables automatic rewriting of commit messages on push to
review repositories to include the review URL that commit will be reviewed
at. This annotation will be forever engraved in the commit message. Humans
can follow the link to find more context around code review.

A URL was chosen instead of merely a short review request ID. URLs
are more descriptive and help build the hyperlinked web. Review request
IDs are merely fragments. They must be assembled into URLs for you to
derive much use from them. This is more work for humans and for
machines. URLs win here. URLs are longer than review request IDs.
But, since machines are writing them, the only significant penalty
for that should be a slight increase in per-changeset storage size
in the repository. For "https://reviewboard.mozilla.org/r/", that
penalty is 34 bytes. That's a few megabytes per 100,000 changesets
uncompressed. That is insignificant in the grand scheme of things.

In the future, machines can also leverage this new metadata. In
particular, the MozReview server can look for annotations to map
incoming changesets against existing review requests.

Since the Review Board server/URL is at a dynamic location in the test
environment, we had to invent a mechanism to use fake and static base
URLs in tests in order to ensure changeset SHA-1s don't change between
test runs. I am embarassed to admit how many hours it took me to realize
that dynamic URLs were the source of otherwise inexplainable test
output. This was partially masked by how Mercurial's test harness
substitutes variables before diffing output. So, the variable test ports
were not apparent.

Review-URL: https://reviewboard.mozilla.org/r/9717

Comment 7

[Gregory Szorc [:gps]]

Comment on attachment 8613135 [details]
MozReview Request: mozreview: use review URL annotation to map commits to review requests (bug 1169834); r?smacleod

mozreview: use review URL annotation to map commits to review requests (bug 1169834); r?smacleod

Now that we store commit IDs in the extra fields of commits and review
URLs in commit messages automatically, the server can use this metadata
to better map submitted commits to existing review requests.

In this change, we enable the server to start looking at review URL
annotations as part of the commit to review request mapping. This
mapping takes precedence below exact commit/SHA-1 mapping but above
obsolescence data.

Review-URL: https://reviewboard.mozilla.org/r/9719

Comment 8

[Gregory Szorc [:gps]]

Attachment: MozReview Request: mozreview: INCOMPLETE update Review-URL when changed (bug 1169834)

mozreview: INCOMPLETE update Review-URL when changed (bug 1169834)

Review request assignment is done by the server. There is no guarantee
that a Review-URL annotation in a commit submitted for review will
obtain the same review request as what it formerly was.

This commit rewrites commit messages on the client to account for
changed review requests.

I still want to add additional output when the review URL is updated. (I
think the user should be notified that something potentially unexpected
has occured.) I also want to think a bit about maintaining a list of
multiple review URLs. e.g. Previous-Review-URL. One reason is if we do a
push, rewrite the review url, realize we made a mistake, then push
again, we lost the record of the original review url. The "helpful"
rewriting in this case may not be so helpful. Definitely need to think
about this...

Comment 9

[Gregory Szorc [:gps]]

Comment on attachment 8613136 [details]
MozReview Request: mozreview: support partially landed series through submitted review requests

mozreview: support partially landed series through submitted review requests

A goal of MozReview is to enable developers to move fast and be more
productive. Part of moving fast means landing commits as soon as they
are ready to land. Lingering commits are susceptible to bit rot and
addressing bit rot adds overhead.

Currently, MozReview operates an all-or-nothing mentality towards
pushes and series of commits: the series of commits on the DAG is what
is turned into the review series.

A significant problem with this approach is that once a commit lands, it
disappears from the review interface. And it doesn't disappear in a good
way: the parent review request will discard it when new changesets are
pushed. Unfortunately, discarded commits aren't easily found in the
interface. What's more, in some scenarios, old review requests could get
overwritten by new ones. This could lead to some very confusing
scenarios where a review was completed, landed, and then overwritten by
a new, unrelated commit!

This commit starts to address these problems.

This commit teaches the review request assignment algorithm how to be
aware of submitted review requests. If an individual review request
becomes submitted (there isn't currently an easy way of doing that -
this will be implemented later), this review request is treated
specially. It effectively becomes read-only. It is also moved to the
bottom of the stack of commits.

This code isn't perfect. There are numerous improvements that could be
made. But you have to start somewhere.

Review-URL: https://reviewboard.mozilla.org/r/9721

Comment 10

[Steven MacLeod [:smacleod]]

Comment on attachment 8613133 [details]
MozReview Request: mozautomation: functions for parsing review URLs out of commit messages (bug 1169834); r?smacleod

https://reviewboard.mozilla.org/r/9715/#review8809

::: pylib/mozautomation/mozautomation/commitparser.py:72
(Diff revision 2)
> +    This simply looks for lines with "Review-URL" in them.

the first line starting with "Review-URL"

Comment 11

[Steven MacLeod [:smacleod]]

Comment on attachment 8613134 [details]
MozReview Request: mozreview: add review URL to commit messages (bug 1169834); r?smacleod

https://reviewboard.mozilla.org/r/9717/#review8815

LGTM. Only skimmed the tests but they seemed mostly fine.

Comment 12

[Steven MacLeod [:smacleod]]

Comment on attachment 8613135 [details]
MozReview Request: mozreview: use review URL annotation to map commits to review requests (bug 1169834); r?smacleod

https://reviewboard.mozilla.org/r/9719/#review8817

Just clearing review flag on this as we're not going to land this yet.

Comment 13

[Gregory Szorc [:gps]]

I'm not actively working on this.

Comment 14

[BMO Automation]

MozReview is now obsolete. Please use Phabricator instead. Closing this bug.