1339689 - vcssync: wrong revision selected as destination after merge

Status: RESOLVED FIXED

(Developer Services :: Servo VCS Sync, defect)

Description

[:glob ✱]

the code walks the dest revision's ancestors and stops when it finds one that was overlaid.  however this code chooses the incorrect revision following the merge of faba0400cbe2.

this is because mercurial's ancestors() method doesn't traverse in purely reverse storage order when merges are present: it only guarantees that a parent is emitted after all its children.

we'd likely want to do something like
> sorted(changelog.ancestors(), reverse=True)

Comment 1

[:glob ✱]

Attachment: overlay: fix iteration of destination revision's ancestors (bug 1339689)

mercurial's ancestors() method doesn't traverse in purely reverse storage order
when merges are present: it only guarantees that a parent is emitted after all
its children.  switch to a sorted list.

Review commit: https://reviewboard.mozilla.org/r/112712/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/112712/

Comment 2

[:glob ✱]

Comment on attachment 8837587 [details]
overlay: fix iteration of destination revision's ancestors (bug 1339689)

found some issues with this.

Comment 3

[:glob ✱]

Comment on attachment 8837587 [details]
overlay: fix iteration of destination revision's ancestors (bug 1339689)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/112712/diff/1-2/

Comment 4

[Steven MacLeod [:smacleod]]

Comment on attachment 8837587 [details]
overlay: fix iteration of destination revision's ancestors (bug 1339689)

https://reviewboard.mozilla.org/r/112712/#review114178

::: hgext/overlay/__init__.py:183
(Diff revision 2)
>                           sourcecl.ancestors([sourcerevs[-1]], inclusive=True))
>  
>      # Attempt to find an incoming changeset in dest and prune already processed
>      # source revisions.
>      lastsourcectx = None
> -    for rev in destrepo.changelog.ancestors([destctx.rev()], inclusive=True):
> +    for rev in sorted(destrepo.changelog.ancestors([destctx.rev()],

I'm wondering, how fast is this operation on a repository of mozilla-central's size? the ancestors of our changeset is pretty much the entire repository history :/. Does this lazily sort or something?

Comment 5

[:glob ✱]

Comment on attachment 8837587 [details]
overlay: fix iteration of destination revision's ancestors (bug 1339689)

https://reviewboard.mozilla.org/r/112712/#review114178

> I'm wondering, how fast is this operation on a repository of mozilla-central's size? the ancestors of our changeset is pretty much the entire repository history :/. Does this lazily sort or something?

i didn't notice any performance difference between that and the original line on a slow ec2 instance.

Comment 6

[Steven MacLeod [:smacleod]]

Comment on attachment 8837587 [details]
overlay: fix iteration of destination revision's ancestors (bug 1339689)

https://reviewboard.mozilla.org/r/112712/#review114178

> i didn't notice any performance difference between that and the original line on a slow ec2 instance.

Okay, that's good to know, dropping.

Comment 7

[Steven MacLeod [:smacleod]]

Comment on attachment 8837587 [details]
overlay: fix iteration of destination revision's ancestors (bug 1339689)

https://reviewboard.mozilla.org/r/112712/#review114186

Tentative r+, please also provide a test - or if it's important to get things running, you can land but followup with a test.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/hgcustom/version-control-tools/rev/e01531f0d4e1
overlay: fix iteration of destination revision's ancestors r=smacleod

Comment 9

[:glob ✱]

landed and pushed to prod.
filed bug 1340032 for the tests, which i'll work on once some other prod issues are resolved.