Closed Bug 1160266 Opened 8 years ago Closed 8 years ago

Generate and record a unique per-commit identifier

Categories

(MozReview Graveyard :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: gps, Assigned: gps)

References

Details

Attachments

(11 files, 1 obsolete file)

39 bytes, text/x-review-board-request
gps
: review+
Details
39 bytes, text/x-review-board-request
gps
: review+
gps
: review+
Details
39 bytes, text/x-review-board-request
gps
: review+
Details
39 bytes, text/x-review-board-request
gps
: review+
Details
39 bytes, text/x-review-board-request
gps
: review+
Details
39 bytes, text/x-review-board-request
gps
: review+
Details
39 bytes, text/x-review-board-request
gps
: review+
Details
39 bytes, text/x-review-board-request
Details
39 bytes, text/x-review-board-request
Details
39 bytes, text/x-review-board-request
Details
39 bytes, text/x-review-board-request
Details
As part of more robustly tracking commits across history editing and other complex workflows, we want to introduce a unique, per-commit identifier that can be used to reliably identity a commit.

This bug will track that work.
Attached file MozReview Request: bz://1160266/gps (obsolete) —
/r/7953 - mozreview: create per-commit identifiers (bug 1160266)

Pull down this commit:

hg pull -r ea0113923c162d4b5ffb459a9f7d7559f8d8d3e2 https://reviewboard-hg.mozilla.org/version-control-tools/
Attachment #8601816 - Flags: review?(smacleod)
My patch over at https://reviewboard.mozilla.org/r/3653/ had some initial work for a push time commit id which you might have some useful bits for you here.
https://reviewboard.mozilla.org/r/7953/#review6995

Overall this looks like a good start. I have one high level issue that comes to mind - we're treating the identifier as a single piece of information in the extra data, which will be problematic with some history rewriting operations. For example in the case where we have multiple commits which are folded together, which identifier becomes the new one? Should we maybe have a field which can contain many identifiers, almost like a history?

This would also solve the problem of divergent work, if you split a commit, or a new author takes your commit in a different direction we could generate a new identifier which is the main one, and keep the old for history and matching. Maybe I'm making things too complex, but I want to make sure you've thought about these cases.
Comment on attachment 8601816 [details]
MozReview Request: bz://1160266/gps

/r/7953 - mozreview: create per-commit identifiers (bug 1160266)
/r/8281 - mozhg: implement changeset rewriting API

Pull down these commits:

hg pull -r dd3ab77b32b621ac303a79cfdc7813c992642602 https://reviewboard-hg.mozilla.org/version-control-tools/
Attachment #8601816 - Flags: review?(dminor)
https://reviewboard.mozilla.org/r/8281/#review7013

I forgot to mention that this code doesn't work with Mercurial 3.0. Fortunately, I was already thinking about dropping 3.0 support from v-c-t. So I'll just write that patch.
Depends on: 1162304
https://reviewboard.mozilla.org/r/7953/#review7021

I did consider these cases. Fold, split (producing multiple commits from the same base commit), and divergent work are specific scenarios that came to mind.

Storing a list of identifiers is an interesting idea. I like it! Although, I'm not convinced it needs to be implemented now. As long as code reading the field plans for it, I *think* we can add this later, as it is needed.

That being said, something that came to mind is that at some point we'd be reinventing obsolescence markers. I'd rather not do that. In the new world order, I still think there is room for utilizing obsolescence markers. But, not everyone will be using obsolescence markers today or even in the next few months. We need something that works for more people *today*. I think these persistent (and hidden) commit IDs can be that. Furthermore, this commit identifier approach will be required for commit tracking in Git. So we can roll this out today, prove its value to Mercurial users, and have confidence when we implement Git support later.

Just so this is documented somewhere, I intend to use the time component of the commit ID for analytics purposes. If the ID is generated at initial commit time, we'll be able to derive a pretty accurate metrics for "time to develop patch:" we can use the difference between commit ID and landing time as a proxy for that (the "date" metadata in the commit is a horrible metric because some configurations bump it when they do things like amend or qrefresh). Commit ID, being immutable, is a slightly better metric. And, it can also be used to track local commits across grafts (i.e. uplifts). Lots of potential.

I could write more words on this. Maybe its best talked about in the MozReview meeting tomorrow?
https://reviewboard.mozilla.org/r/8281/#review7071

::: pylib/mozhg/mozhg/rewrite.py:246
(Diff revision 1)
> +            repair.strip(repo.ui, repo, stripnodes, topic=backuptopic)

I would prefer this backup to be optional. If we're doing a simple change to the description I think backups are unnecessarily and add clutter.

::: pylib/mozhg/mozhg/tests/testrewrite.py:23
(Diff revision 1)
> +

nit: should be an extra newline here

::: pylib/mozhg/mozhg/rewrite.py:71
(Diff revision 1)
> +        assert len(node) == 20

Please add a comment indicating what circumstances would cause this to fail. If it's because of caller/user error we should raise an Abort instead.
Attachment #8601816 - Flags: review?(dminor)
Comment on attachment 8601816 [details]
MozReview Request: bz://1160266/gps

/r/7953 - mozreview: create per-commit identifiers (bug 1160266)
/r/8281 - mozhg: implement changeset rewriting API
/r/8383 - mozreview: refactor code for determining what to review

Pull down these commits:

hg pull -r 67fc77a07283af6c1de64bbd8b61218bc8e4afac https://reviewboard-hg.mozilla.org/version-control-tools/
Attachment #8601816 - Flags: review?(dminor)
Comment on attachment 8601816 [details]
MozReview Request: bz://1160266/gps

/r/7953 - mozreview: create per-commit identifiers (bug 1160266)
/r/8281 - mozhg: implement changeset rewriting API
/r/8383 - mozreview: refactor code for determining what to review
/r/8395 - mozreview: generate commit IDs when pushing to review repo (bug 1160266)

Pull down these commits:

hg pull -r 46e7df17ff9b060f3509668f4ef70e8a8b193908 https://reviewboard-hg.mozilla.org/version-control-tools/
https://reviewboard.mozilla.org/r/8281/#review7103

> I would prefer this backup to be optional. If we're doing a simple change to the description I think backups are unnecessarily and add clutter.

I agree with the sentiment. However, I'm scared to offer a "no backup" option until I'm confident this code is safe. If we start stripping changesets without backup and we somehow lose user data, people will be upset. I'd rather not take the risk, at least not initially.
Comment on attachment 8601816 [details]
MozReview Request: bz://1160266/gps

/r/7953 - mozreview: create per-commit identifiers (bug 1160266)
/r/8281 - mozhg: implement changeset rewriting API
/r/8383 - mozreview: refactor code for determining what to review
/r/8395 - mozreview: generate commit IDs when pushing to review repo (bug 1160266)

Pull down these commits:

hg pull -r 55ee43650eaddeb9dd227a73543835cadc7a4dbd https://reviewboard-hg.mozilla.org/version-control-tools/
Comment on attachment 8601816 [details]
MozReview Request: bz://1160266/gps

/r/7953 - mozreview: create per-commit identifiers (bug 1160266)
/r/8281 - mozhg: implement changeset rewriting API
/r/8383 - mozreview: refactor code for determining what to review
/r/8395 - mozreview: generate commit IDs when pushing to review repo (bug 1160266)
/r/8447 - mozreview: abort push when working directory is dirty
/r/8449 - mozreview: update reviews after all data has been read
/r/8451 - mozreview: don't call doreview inside the push transaction
/r/8453 - mozreview: add Review URL to commit messages

Pull down these commits:

hg pull -r 2444159cb1cf8e6c526a9846d7d16fe925059c5f https://reviewboard-hg.mozilla.org/version-control-tools/
https://reviewboard.mozilla.org/r/8453/#review7129

If you look closely, this series was submitted with this commit and contains "Review-URL" annotations in the commit message :D
https://reviewboard.mozilla.org/r/8281/#review7161

::: pylib/mozhg/mozhg/rewrite.py:20
(Diff revision 5)
> +def newparents(repo, ctx, revmap):

I know this is pretty simple but it would still be nice to give it a docstring. I'm correct that this looks at the revmap to get the new nodes if the given ctx node's parents were re-written as part of the process?

::: pylib/mozhg/mozhg/rewrite.py:46
(Diff revision 5)
> +    as the memctx function for obtaining memfilectx when vanilla copying is
> +    to be performed (a common pattern). The function should return an

"When vanilla copying is to be performed" is a little ambiguous here with respect to file copies ("hg cp/mv") vs copying the filectx' information into a new memfilectx with the same information.

This is especially confusing when the actual implementation of copyfilectxfn is inspected - since a large portion of its body deals with the copied file ("hg cp/mv") meta-data.

Please clarify this comment to make it more obvious we're getting a memfilectx version copied from the filectx. Might also be helpful to stick a short docstring on the function implementation.

::: pylib/mozhg/mozhg/rewrite.py:183
(Diff revision 5)
> +            def filectxfn(repo, memctx, path):

It would be nice to avoid the duplication here - maybe have a factory for spitting out this function? (There code is identical, but the comments have already diverged.

::: pylib/mozhg/mozhg/rewrite.py:260
(Diff revision 5)
> +        # Move the working directory to the new node, if applicable.
> +        wdirrev = repo['.'].rev()
> +        if wdirrev in revmap:
> +            hg.updaterepo(repo, repo[revmap[wdirrev]].node(), True)

Should we be checking if the user is on a bookmark that was moved also, and updating to that bookmark rather than just the new node?
https://reviewboard.mozilla.org/r/8383/#review7175

::: hgext/reviewboard/client.py:255
(Diff revision 4)
>              if ctx.phase() == phases.public:
>                  break

We currently have a bug with respect to secret nodes here do we not?

If I "hg push -r ." with P1 <- D1 <- D2 <- S1(.) we'll mark S1 for review but it won't actually make it to the server since it's secret - or does hg abort pushes like this?

Might as well fix this up and account for secret while we're changing things?
https://reviewboard.mozilla.org/r/8281/#review7193

> I know this is pretty simple but it would still be nice to give it a docstring. I'm correct that this looks at the revmap to get the new nodes if the given ctx node's parents were re-written as part of the process?

Yes. I'll add a docstring.

> Should we be checking if the user is on a bookmark that was moved also, and updating to that bookmark rather than just the new node?

Yes. Good catch.
https://reviewboard.mozilla.org/r/8281/#review7195

> Yes. Good catch.

OK. So we already have test coverage for this and it appears to work.

Internally, there are separate APIs for "update working copy" from "update to bookmark." commands.update calls both of them.

Since we don't expect to change the active bookmark, we don't need to worry about calling APIs for adjusting the active bookmark. (The active bookmark is just a name stored in the .hg/bookmarks.current file.)
https://reviewboard.mozilla.org/r/8383/#review7197

> We currently have a bug with respect to secret nodes here do we not?
> 
> If I "hg push -r ." with P1 <- D1 <- D2 <- S1(.) we'll mark S1 for review but it won't actually make it to the server since it's secret - or does hg abort pushes like this?
> 
> Might as well fix this up and account for secret while we're changing things?

I wouldn't be surprised if there were a bug around secret phase handling. But, I'm inclined to push that off to a follow-up bug rather than worry about it here.
Comment on attachment 8601816 [details]
MozReview Request: bz://1160266/gps

/r/7953 - mozreview: create per-commit identifiers (bug 1160266); r=smacleod
/r/8281 - mozhg: implement changeset rewriting API
/r/8383 - mozreview: refactor code for determining what to review
/r/8395 - mozreview: generate commit IDs when pushing to review repo (bug 1160266)
/r/8447 - mozreview: abort push when working directory is dirty
/r/8449 - mozreview: update reviews after all data has been read
/r/8451 - mozreview: don't call doreview inside the push transaction
/r/8453 - mozreview: add Review URL to commit messages

Pull down these commits:

hg pull -r c22f24579c2863ee61c5f923dd2d3c9bdcfd794c https://reviewboard-hg.mozilla.org/version-control-tools/
Comment on attachment 8601816 [details]
MozReview Request: bz://1160266/gps

/r/7953 - mozreview: create per-commit identifiers (bug 1160266); r=smacleod
/r/8281 - mozhg: implement changeset rewriting API
/r/8383 - mozreview: refactor code for determining what to review
/r/8395 - mozreview: generate commit IDs when pushing to review repo (bug 1160266)
/r/8447 - mozreview: abort push when working directory is dirty
/r/8449 - mozreview: update reviews after all data has been read
/r/8451 - mozreview: don't call doreview inside the push transaction
/r/8453 - mozreview: add Review URL to commit messages

Pull down these commits:

hg pull -r 7ed52866396e3608e6156358900effe349311bad https://reviewboard-hg.mozilla.org/version-control-tools/
https://reviewboard.mozilla.org/r/8453/#review7209

Test output appears to have become unstable with this change. Fascinating.
https://reviewboard.mozilla.org/r/8281/#review7211

I think there's a bug somewhere in this code. With futures commits applied, if I have obsolescence enabled and rewrite commits on push, I get divergent changesets and my store is littered with a bunch of "temporary amend commit for XXX" changesets and my current revision is unstable. It's wonky.
Comment on attachment 8601816 [details]
MozReview Request: bz://1160266/gps

/r/7953 - mozreview: create per-commit identifiers (bug 1160266); r=smacleod
/r/8281 - mozhg: implement changeset rewriting API
/r/8383 - mozreview: refactor code for determining what to review
/r/8395 - mozreview: generate commit IDs when pushing to review repo (bug 1160266)
/r/8447 - mozreview: abort push when working directory is dirty
/r/8449 - mozreview: update reviews after all data has been read
/r/8451 - mozreview: don't call doreview inside the push transaction
/r/8609 - mozautomation: functions for parsing review URLs out of commit messages
/r/8453 - mozreview: add review URL to commit messages
/r/8611 - mozreview: use review URL annotation to map commits to review requests

Pull down these commits:

hg pull -r 64b9659236bf1975ec1fe74c678c9fce34374dfd https://reviewboard-hg.mozilla.org/version-control-tools/
https://reviewboard.mozilla.org/r/8281/#review7337

::: pylib/mozhg/mozhg/rewrite.py:25
(Diff revisions 5 - 7)
> +    return a the new parents for that changeset, taking any previous rewriting

"a the"
https://reviewboard.mozilla.org/r/8383/#review7339

> I wouldn't be surprised if there were a bug around secret phase handling. But, I'm inclined to push that off to a follow-up bug rather than worry about it here.

Fair enough. Drop this issue with a bug filed about investigating our secret changset handling.
Comment on attachment 8601816 [details]
MozReview Request: bz://1160266/gps

/r/7953 - mozreview: create per-commit identifiers (bug 1160266); r=smacleod
/r/8281 - mozhg: implement changeset rewriting API
/r/8383 - mozreview: refactor code for determining what to review
/r/8395 - mozreview: generate commit IDs when pushing to review repo (bug 1160266)
/r/8447 - mozreview: abort push when working directory is dirty
/r/8449 - mozreview: update reviews after all data has been read
/r/8451 - mozreview: don't call doreview inside the push transaction
/r/8609 - mozautomation: functions for parsing review URLs out of commit messages
/r/8453 - mozreview: add review URL to commit messages
/r/8611 - mozreview: use review URL annotation to map commits to review requests

Pull down these commits:

hg pull -r 5229e8808a6f9aea40997a886e14de3fc0fb720e https://reviewboard-hg.mozilla.org/version-control-tools/
https://reviewboard.mozilla.org/r/8611/#review7379

I'm still dealing with testing inconsistencies and can't verify that this or the patch before it are sane. I'd appreciate if you would run these locally and tell me if the tests pass.

As of this patch, about the only thing more I could do in this series is rewrite the Review-URL annotation on the client when the server assigns a new review request. There is tons of follow-up work to enable more advanced workflows. But I think that should be delegated to other bugs and series.
Comment on attachment 8601816 [details]
MozReview Request: bz://1160266/gps

/r/7953 - mozreview: create per-commit identifiers (bug 1160266); r=smacleod
/r/8281 - mozhg: implement changeset rewriting API
/r/8383 - mozreview: refactor code for determining what to review
/r/8395 - mozreview: generate commit IDs when pushing to review repo (bug 1160266)
/r/8447 - mozreview: abort push when working directory is dirty
/r/8449 - mozreview: update reviews after all data has been read
/r/8451 - mozreview: don't call doreview inside the push transaction
/r/8609 - mozautomation: functions for parsing review URLs out of commit messages
/r/8453 - mozreview: add review URL to commit messages
/r/8611 - mozreview: use review URL annotation to map commits to review requests

Pull down these commits:

hg pull -r d0ebe5f35b070e62cdb6d96eb25b7dc7553b65c0 https://reviewboard-hg.mozilla.org/version-control-tools/
Comment on attachment 8601816 [details]
MozReview Request: bz://1160266/gps

/r/7953 - mozreview: create per-commit identifiers (bug 1160266); r=smacleod
/r/8281 - mozhg: implement changeset rewriting API
/r/8383 - mozreview: refactor code for determining what to review
/r/8395 - mozreview: generate commit IDs when pushing to review repo (bug 1160266)
/r/8447 - mozreview: abort push when working directory is dirty
/r/8449 - mozreview: update reviews after all data has been read
/r/8451 - mozreview: don't call doreview inside the push transaction
/r/8609 - mozautomation: functions for parsing review URLs out of commit messages
/r/8453 - mozreview: add review URL to commit messages
/r/8611 - mozreview: use review URL annotation to map commits to review requests

Pull down these commits:

hg pull -r 792a908dbdc3a0e49c12bd00bf7596b208937255 https://reviewboard-hg.mozilla.org/version-control-tools/
https://reviewboard.mozilla.org/r/8281/#review7499

> I agree with the sentiment. However, I'm scared to offer a "no backup" option until I'm confident this code is safe. If we start stripping changesets without backup and we somehow lose user data, people will be upset. I'd rather not take the risk, at least not initially.

Fair enough.
Attachment #8601816 - Flags: review?(dminor) → review+
Comment on attachment 8601816 [details]
MozReview Request: bz://1160266/gps

/r/7953 - mozreview: create per-commit identifiers (bug 1160266); r=smacleod
/r/8281 - mozhg: implement changeset rewriting API
/r/8383 - mozreview: refactor code for determining what to review
/r/8395 - mozreview: generate commit IDs when pushing to review repo (bug 1160266)
/r/8447 - mozreview: abort push when working directory is dirty
/r/8449 - mozreview: update reviews after all data has been read
/r/8451 - mozreview: don't call doreview inside the push transaction
/r/8609 - mozautomation: functions for parsing review URLs out of commit messages
/r/8453 - mozreview: add review URL to commit messages
/r/8611 - mozreview: use review URL annotation to map commits to review requests
/r/8857 - mozreview: support partially landed series through submitted review requests

Pull down these commits:

hg pull -r ee2956c24907347b4869f92e8e3b8e612b11e022 https://reviewboard-hg.mozilla.org/version-control-tools/
Attachment #8601816 - Flags: review+ → review?(dminor)
https://reviewboard.mozilla.org/r/8395/#review7985

LGTM. I didn't actually go over every little test change - I did some inspection and I'll assume the rest is correct.

::: hgext/reviewboard/client.py:300
(Diff revision 8)
> +        nodes = [nodemap.get(node, node) for node in nodes]

This has to be some sort of record for how much node can fit on a line heh
https://reviewboard.mozilla.org/r/8449/#review7991

nit: it'd be nice to actually use "review request" or "request" rather than "review" due to the whole naming confusion thing. Since this is just a small code move I'll understand if you don't want to bother with it right now.
https://reviewboard.mozilla.org/r/8609/#review7995

::: pylib/mozautomation/mozautomation/commitparser.py:37
(Diff revision 3)
> +        # Followed by a numeric review id

review request id
Comment on attachment 8601816 [details]
MozReview Request: bz://1160266/gps

https://reviewboard.mozilla.org/r/7951/#review8055

Ship It!
Attachment #8601816 - Flags: review?(dminor) → review+
mozreview: create per-commit identifiers (bug 1160266); r=smacleod

MozReview needs a way to track commits across history rewriting
operations. This is a non-trivial problem because nothing is
guaranteed to be stable across history rewriting. Under normal
circumstances, the best you can do is look at the components of
a commit (author, message, changed files, diff content, etc) and
attempt a fuzzy match against other known commits. But no matter
how much work you put into this approach, it will always be based on
patterns and heuristics and therefore imperfect. It can also be
expensive to perform, as searching will require indexes and fast
access to the raw data.

This commit attempts to solve the problem of logical commit
tracking across history rewriting by introducing a persistent
identifier in commits themselves. When a commit is created, we
generate a 64-bit base62 encoded value and stuff it inside the
commit object as an "extra" field. This field is completely hidden
from Mercurial unless you do `hg log --debug` or unless a custom
template is used to render this field. However, this field is fully
visible to Mercurial's internals, which means it is available to
MozReview for commit mapping.

We ideally want to generate commit IDs at commit time because otherwise
you need to rewrite history to add them back in. Unfortunately, there
will always be users that don't generete commit IDs until review time,
so we must solve this problem. This patch starts with the simple
problem and only created commit IDs at commit time. Subsequent work
will implement commit ID generation at push time.

Review-URL: https://reviewboard.mozilla.org/r/7953
Attachment #8612653 - Flags: review?(smacleod)
mozhg: implement changeset rewriting API; r=smacleod, dminor

Mass modifying changesets and all their children is non-trivial because
there isn't an API in core Mercurial for this, at least not one that is
available to the versions of Mercurial we need to support. In Mercurial
3.4, histedit is kinda/sorta extensible and may work for this purpose.
In the future, apparently lots of histedit's functionality is going to
get rolled into core to make this easier. In the mean time, we need
to implementat functionality ourselves.

This commit introduces a generic changeset rewriting API. Pass in a list
of nodes and a function to produce a new commit and it takes care of
phases, bookmarks, obsolescence, stripping, working directory updates,
and everything else you want it to do.

Basic tests have been included.

Review-URL: https://reviewboard.mozilla.org/r/8281
Attachment #8612654 - Flags: review?(smacleod)
Attachment #8612654 - Flags: review?(dminor)
mozreview: refactor code for determining what to review; r=smacleod

The code for determining what to review was previously spread out over a
few functions and was difficult to comprehend and change.

We introduce a single function (which runs as part of discovery) that
determines which changesets are to be reviewed and it saves that list
inside the "pushoperation" instance. This allows all subsequent
consumers to easily see what is going to be reviewed. We plan to
leverage this work in a subsequent change to do more processing at push
time.

Functionality is mostly identical. Some refactoring has been performed
to clean up code that doesn't run anymore. This is most notable with
the usage of "pushop.outgoing.revs" to determine review nodes: this
never occurs because either the user specifies "-r" or "-r ." is
implied. This wasn't obvious before. But post code refactor it is pretty
clear.

Review-URL: https://reviewboard.mozilla.org/r/8383
Attachment #8612655 - Flags: review?(smacleod)
mozreview: generate commit IDs when pushing to review repo (bug 1160266); r?smacleod

Previously, we only generated commit IDs at commit time if the
repository was known to be associated with a review repository. Now that
we have support for mass rewriting changesets, start generating commit
IDs at push-to-review-repo time.

Since we now add commit IDs to every to-be-reviewed changesets, all
SHA-1s have been updated.

Review-URL: https://reviewboard.mozilla.org/r/8395
Attachment #8612656 - Flags: review?(smacleod)
mozreview: abort push when working directory is dirty; r?smacleod

Upcoming patches will perform more commit rewriting after push. We don't
want to get in a scenario where the push and/or review request succeeds
but the client can't update commits, as that would result in metadata
getting out of sync. With this change, we verify rewrites can be
performed *before* push and fail fast otherwise.

Review-URL: https://reviewboard.mozilla.org/r/8447
Attachment #8612657 - Flags: review?(smacleod)
mozreview: update reviews after all data has been read; r?smacleod

In a future patch, nodes may be rewritten after push. Defer writing
the node to review id mapping until later so we can inject a node
rewriting step.

Review-URL: https://reviewboard.mozilla.org/r/8449
Attachment #8612658 - Flags: review?(smacleod)
mozreview: don't call doreview inside the push transaction; r?smacleod

Mercurial 3.3 introduced a transaction around most parts of
exchange.push(). This interferes with our ability to perform
history rewriting after the push has effectively been completed.

This change hooks into the transaction callback registration
mechanism and moves review creation until after the transaction
has closed.

Review-URL: https://reviewboard.mozilla.org/r/8451
Attachment #8612659 - Flags: review?(smacleod)
mozautomation: functions for parsing review URLs out of commit messages; 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/8609
Attachment #8612660 - Flags: review?(smacleod)
mozreview: add review URL to commit messages; 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/8453
Attachment #8612661 - Flags: review?(smacleod)
mozreview: use review URL annotation to map commits to review requests; 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/8611
Attachment #8612662 - Flags: review?(smacleod)
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 patch starts to address these problems.

This patch 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/8857
https://reviewboard.mozilla.org/r/8609/#review8483

::: pylib/mozautomation/tests/test_commitparser.py:8
(Diff revisions 3 - 4)
> -    MozReviewInfo,
> +    MomzReviewInfo,

MomzReviewInfo sounds just like home, but you probable mean MozReviewInfo.
Since I have review for everything up to the Review-URL pieces, I'm going to land what's reviewed and move Review-URL elsewhere, probably to a new bug, as the scope warrants it.

Landing the commit id stuff sooner gives others access to the commit rewriting API and will buy us time to flesh out bugs in that API without jeopardizing rolling back Review-URL. #movefast.
Attachment #8612653 - Flags: review?(smacleod) → review+
Attachment #8612654 - Flags: review?(smacleod)
Attachment #8612654 - Flags: review?(dminor)
Attachment #8612654 - Flags: review+
Attachment #8612655 - Flags: review?(smacleod) → review+
Attachment #8612656 - Flags: review?(smacleod) → review+
Attachment #8612657 - Flags: review?(smacleod) → review+
Attachment #8612658 - Flags: review?(smacleod) → review+
Attachment #8612659 - Flags: review?(smacleod) → review+
Attachment #8612660 - Flags: review?(smacleod)
Attachment #8612661 - Flags: review?(smacleod)
Attachment #8612662 - Flags: review?(smacleod)
Attachment #8601816 - Flags: review?(smacleod)
First part landed. We now have a generic commit rewriting API. (We should use this for adding bug numbers on auto-filed bugs and for rewriting r? into r=.)
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Blocks: 1169834
Attachment #8601816 - Attachment is obsolete: true
Product: Developer Services → MozReview
You need to log in before you can comment on or make changes to this bug.