Page MenuHomePhabricator

pushrebase: check sources of renames when looking for conflicts
ClosedPublic

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

Details

Summary

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

Repository
rFBHGX Facebook Mercurial Extensions
Lint
Automatic diff as part of commit; lint not applicable.
Unit
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.

hgext3rd/pushrebase.py
549

This is fast because it just reads the changelog.

552–558

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

833

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.