1160266 - Generate and record a unique per-commit identifier

Status: RESOLVED FIXED

(MozReview Graveyard :: General, defect)

Description

[Gregory Szorc [:gps]]

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.

Comment 2

[Gregory Szorc [:gps]]

Attachment: MozReview Request: bz://1160266/gps

/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/

Comment 3

[Dan Minor [:dminor]]

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.

Comment 4

[Steven MacLeod [:smacleod]]

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 5

[Gregory Szorc [:gps]]

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/

Comment 6

[Gregory Szorc [:gps]]

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.

Comment 7

[Gregory Szorc [:gps]]

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?

Comment 8

[Dan Minor [:dminor]]

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.

Comment 9

[Gregory Szorc [:gps]]

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/

Comment 10

[Gregory Szorc [:gps]]

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/

Comment 11

[Gregory Szorc [:gps]]

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 12

[Gregory Szorc [:gps]]

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 13

[Gregory Szorc [:gps]]

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/

Comment 14

[Gregory Szorc [:gps]]

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

Comment 15

[Steven MacLeod [:smacleod]]

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?

Comment 16

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/7953/#review7173

Ship It!

Comment 17

[Steven MacLeod [:smacleod]]

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?

Comment 18

[Gregory Szorc [:gps]]

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.

Comment 19

[Gregory Szorc [:gps]]

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.)

Comment 20

[Gregory Szorc [:gps]]

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 21

[Gregory Szorc [:gps]]

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 22

[Gregory Szorc [:gps]]

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/

Comment 23

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/8453/#review7209

Test output appears to have become unstable with this change. Fascinating.

Comment 24

[Gregory Szorc [:gps]]

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 25

[Gregory Szorc [:gps]]

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/

Comment 26

[Steven MacLeod [:smacleod]]

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"

Comment 27

[Steven MacLeod [:smacleod]]

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 28

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/8383/#review7341

Ship It!

Comment 29

[Gregory Szorc [:gps]]

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/

Comment 30

[Gregory Szorc [:gps]]

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 31

[Gregory Szorc [:gps]]

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 32

[Gregory Szorc [:gps]]

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/

Comment 33

[Dan Minor [:dminor]]

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.

Comment 34

[Dan Minor [:dminor]]

https://reviewboard.mozilla.org/r/8281/#review7501

Ship It!

Comment 35

[Gregory Szorc [:gps]]

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/

Comment 36

[Steven MacLeod [:smacleod]]

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

Comment 37

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/8447/#review7987

Ship It!

Comment 38

[Steven MacLeod [:smacleod]]

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.

Comment 39

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/8451/#review7993

Ship It!

Comment 40

[Steven MacLeod [:smacleod]]

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 41

[Dan Minor [:dminor]]

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

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

Ship It!

Comment 42

[Gregory Szorc [:gps]]

Attachment: MozReview Request: mozreview: create per-commit identifiers (bug 1160266); r=smacleod

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

Comment 43

[Gregory Szorc [:gps]]

Attachment: MozReview Request: mozhg: implement changeset rewriting API; r=smacleod, dminor

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

Comment 44

[Gregory Szorc [:gps]]

Attachment: MozReview Request: mozreview: refactor code for determining what to review; r=smacleod

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

Comment 45

[Gregory Szorc [:gps]]

Attachment: MozReview Request: mozreview: generate commit IDs when pushing to review repo (bug 1160266); r?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

Comment 46

[Gregory Szorc [:gps]]

Attachment: MozReview Request: mozreview: abort push when working directory is dirty; r?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

Comment 47

[Gregory Szorc [:gps]]

Attachment: MozReview Request: mozreview: update reviews after all data has been read; r?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

Comment 48

[Gregory Szorc [:gps]]

Attachment: MozReview Request: mozreview: don't call doreview inside the push transaction; r?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

Comment 49

[Gregory Szorc [:gps]]

Attachment: MozReview Request: mozautomation: functions for parsing review URLs out of commit messages; r?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

Comment 50

[Gregory Szorc [:gps]]

Attachment: MozReview Request: mozreview: add review URL to commit messages; r?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

Comment 51

[Gregory Szorc [:gps]]

Attachment: MozReview Request: mozreview: use review URL annotation to map commits to review requests; r?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

Comment 52

[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 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

Comment 53

[Dan Minor [:dminor]]

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.

Comment 54

[Gregory Szorc [:gps]]

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.

Comment 55

[Gregory Szorc [:gps]]

https://hg.mozilla.org/hgcustom/version-control-tools/rev/69f8fd878631
https://hg.mozilla.org/hgcustom/version-control-tools/rev/7644daad0b5c

Comment 56

[Gregory Szorc [:gps]]

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=.)