D5398: narrow: keep bookmarks temporarily stripped for as long as commits are

[martinvonz (Martin von Zweigbergk)]

martinvonz created this revision.
Herald added a reviewer: durin42.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  The narrow extension also has support for shallowness and for
  inserting older commits on pull. It works by temporarily stripping
  newer commits, adding the older commits, then re-applying the stripped
  bundle. The regular Mercurial server uses that when you widen,
  although it shouldn't be necessary there. Our Google-internal server
  does it when the user requests an older commit.
  
  Our Google-internal tests fail since https://phab.mercurial-scm.org/rHG7caf632e30c3840e7ed5e67fe5127e7527046821 (filecache:
  unimplement __set__() and __delete__() (API), 2018-10-20). I haven't
  quite understood the problem, but it's related to the way we
  temporarily hide bookmarks while the commits they point to are
  stripped. When a transaction is started, Mercurial tries to read
  various things from the repo for the transaction summary. That leads
  to computation of hidden commits, which leads to an attempt to find
  commits pinned by bookmarks. This is the reason we temporarily hide
  the bookmarks. I think the aforementioned commit makes the restored
  bookmarks visible earlier than before (which seems like an
  improvement), so we can no longer incorrectly rely on the
  repo._bookmarks field being cached too long (IIUC).
  
  This patch makes it so we restore the temporarily hidden bookmarks
  only after the temporary bundle has been re-applied. It also adapts
  the code to update the repo.__bookmarks field using the pattern
  described in the aforementioned commit instead of writing directly to
  the fiels.
  
  I have spent many hours trying to understand what was going on here,
  but I still don't know if this can also happen without our custom
  server. So this patch unfortunately does not add any tests; I have
  only been able to test the fix using our Google-internal tests.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D5398

AFFECTED FILES
  hgext/narrow/narrowbundle2.py

CHANGE DETAILS

diff --git a/hgext/narrow/narrowbundle2.py b/hgext/narrow/narrowbundle2.py
--- a/hgext/narrow/narrowbundle2.py
+++ b/hgext/narrow/narrowbundle2.py
@@ -20,6 +20,7 @@
     changegroup,
     error,
     exchange,
+    localrepo,
     narrowspec,
     repair,
     repository,
@@ -179,14 +180,13 @@
 
     if clkills:
         # preserve bookmarks that repair.strip() would otherwise strip
-        bmstore = repo._bookmarks
+        op._bookmarksbackup = repo._bookmarks
         class dummybmstore(dict):
             def applychanges(self, repo, tr, changes):
                 pass
-        repo._bookmarks = dummybmstore()
+        localrepo.localrepository._bookmarks.set(repo, dummybmstore())
         chgrpfile = repair.strip(op.ui, repo, list(clkills), backup=True,
                                  topic='widen')
-        repo._bookmarks = bmstore
         if chgrpfile:
             op._widen_uninterr = repo.ui.uninterruptable()
             op._widen_uninterr.__enter__()
@@ -270,5 +270,10 @@
         origcghandler(op, inpart)
         if util.safehasattr(op, '_widen_bundle'):
             handlechangegroup_widen(op, inpart)
+        if util.safehasattr(op, '_bookmarksbackup'):
+            localrepo.localrepository._bookmarks.set(op.repo,
+                                                     op._bookmarksbackup)
+            del op._bookmarksbackup
+
     wrappedcghandler.params = origcghandler.params
     bundle2.parthandlermapping['changegroup'] = wrappedcghandler



To: martinvonz, durin42, #hg-reviewers
Cc: mercurial-devel