Page MenuHomePhabricator

pushrebase: check sources of renames when looking for conflicts

Authored by mbthomas on Oct 20 2017, 6:10 AM.



When checking the bundle contents against the revisions it is being rebased
over, include the sources of renames, as changes made in those files also
conflict with the bundle.

Diff Detail

rFBHGX Facebook Mercurial Extensions
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

mbthomas created this revision.Oct 20 2017, 6:10 AM
Herald added a reviewer: Restricted Project. · View Herald TranscriptOct 20 2017, 6:10 AM
mbthomas updated this revision to Diff 3046.Oct 20 2017, 6:15 AM
ryanmce accepted this revision.Oct 20 2017, 3:07 PM
ryanmce added a subscriber: ryanmce.

Sweet! Thanks for the quick fix.

This revision is now accepted and ready to land.Oct 20 2017, 3:07 PM
quark added a subscriber: quark.Oct 20 2017, 8:38 PM

Since this is sensitive to push throughput, we need to be more careful here.


This is fast because it just reads the changelog.


This is not that fast since it looks up file from manifest and extracts the file content.


The below (including _getrevs) is in a critical section (taking the SQL lock) that affects push throughput.

We can pre-calcualte the renamed information before entering the critical section, and cache them per revision.

durham requested changes to this revision.Oct 24 2017, 11:30 AM
durham added a subscriber: durham.

+1 Jun's comment.

This revision now requires changes to proceed.Oct 24 2017, 11:30 AM
mbthomas updated this revision to Diff 3084.Oct 25 2017, 7:14 AM
mbthomas marked 3 inline comments as done.Oct 25 2017, 8:05 AM
quark accepted this revision.EditedOct 25 2017, 2:27 PM

LGTM. Thanks for solving this issue!

This looks like it broke test-pushrebase-fastmanifest.t for me?

I fixed the failing test. Looks like it was always a bad test, and this diff just exposed it.

It also broke test-treemanifest-server.t though. Did we run all the tests on this?

This revision was automatically updated to reflect the committed changes.