This is an archive of the discontinued Mercurial Phabricator instance.

infinitepush: don't use only() to compute not-backed-up revs
ClosedPublic

Authored by mbthomas on Sep 11 2017, 7:42 AM.
Tags
None
Subscribers

Details

Summary

The infinitepush backup smartlog summary uses only(a, b) to determine which
revisions have not been backed up. However, if the set of backup heads is
empty, only() behaves as if it were called as only(a), which is wrong and slow.

Instead, use ::a - ::b, which is always correct, even if b is empty.

Test Plan

Re-run UTs. Test perf on a repo with no backup heads.

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.Sep 11 2017, 7:42 AM
simonfar accepted this revision.Sep 11 2017, 9:21 AM
This revision is now accepted and ready to land.Sep 11 2017, 9:21 AM
durham added a subscriber: durham.Sep 11 2017, 12:00 PM

This sounds like a bug in core. I don't think only(X, []) should be equal to only(X).

On closer examination, it's not actually equivalent to only(a), but it's not helpful either as it seems to scan through all ancestor revisions looking for ones that are members of the empty set. There's probably scope for improving the performance of only(a, []) in core, but ::a - ::b just works.

quark requested changes to this revision.Sep 11 2017, 2:58 PM
quark added a subscriber: quark.

I believe ::a - ::b will be optimized to only(a, b):

$ hg debugrevspec -p optimized '(::3 - ::(2-2))'
* optimized:
(func
  (symbol 'only')
  (list
    (symbol '3')
    (difference
      (symbol '2')
      (symbol '2'))))

So send back to your queue for further investigation.

infinitepush/backupcommands.py
327

This is expanded to (draft() and ::%ls) - ::%ls.

This revision now requires changes to proceed.Sep 11 2017, 2:58 PM
This revision was automatically updated to reflect the committed changes.