Patchwork D152: repo: skip invalidation of changelog if it has 'delayed' changes (API)

login
register
mail settings
Submitter phabricator
Date July 19, 2017, 10:09 p.m.
Message ID <differential-rev-PHID-DREV-vpjqmvuhfnxdmpjiyieq-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/22530/
State Superseded, archived
Headers show

Comments

phabricator - July 19, 2017, 10:09 p.m.
martinvonz created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  The changelog object can store recently added revisions in memory
  until the transaction is committed. We don't want to lose those
  changes even if repo.invalidate(clearfilecache=True), so let's skip
  the changelog when it has such 'delayed' changes.

REPOSITORY
  rHG Mercurial

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

AFFECTED FILES
  mercurial/localrepo.py
  tests/test-context.py

CHANGE DETAILS




EMAIL PREFERENCES
  https://phab.mercurial-scm.org/settings/panel/emailpreferences/

To: martinvonz, #hg-reviewers
Cc: mercurial-devel
phabricator - July 19, 2017, 10:15 p.m.
quark added a comment.


  Could we call `changelog._writepending()` automatically to solve this problem?

REPOSITORY
  rHG Mercurial

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

EMAIL PREFERENCES
  https://phab.mercurial-scm.org/settings/panel/emailpreferences/

To: martinvonz, #hg-reviewers
Cc: quark, mercurial-devel
phabricator - July 20, 2017, 4:17 p.m.
martinvonz added a comment.


  In https://phab.mercurial-scm.org/D152#2246, @quark wrote:
  
  > Could we call `changelog._writepending()` automatically to solve this problem?
  
  
  Good question. I spent quite a lot of time trying to see if that would work. It was easy to get all tests to pass, but I couldn't get it to work with our extensions (and it didn't seem to be the extensions' faults). One problem was that cl.delayupdate() would happen again after repo.invalidate() and delayupdate() doesn't have a way of taking revisions already written to .a into account (it doesn't check if self._divert is set). Also, I think that writing to .a would mean that hooks would see changes they were not meant to see.

REPOSITORY
  rHG Mercurial

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

EMAIL PREFERENCES
  https://phab.mercurial-scm.org/settings/panel/emailpreferences/

To: martinvonz, #hg-reviewers
Cc: quark, mercurial-devel
phabricator - July 25, 2017, 4:40 p.m.
quark accepted this revision.
quark added a comment.


  I think this is fine as a workaround now. It's better than losing data.

INLINE COMMENTS

> test-context.py:185
> +    with open('4', 'wb') as f:
> +        f.write(i)
> +    repo.dirstate.normal('4')

Seems like `f.write(b'4')` is better.

REPOSITORY
  rHG Mercurial

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

To: martinvonz, #hg-reviewers, quark
Cc: quark, mercurial-devel
phabricator - July 26, 2017, 3:34 a.m.
indygreg accepted this revision as: indygreg.
indygreg added a comment.


  Aside from the test issue pointed out by @quark, I think this seems reasonable.
  
  @martinvonz is correct that writing the .i.a file has hooks implications. We definitely need to keep the changes in memory.
  
  I'm kinda surprised we haven't hit this bug in core. Unless you can find somewhere we can hit it, I'm leaning towards not taking this on stable.
  
  FWIW, these are the kinds of caching bugs that I think a refactor of the repo class into immutable and mutable components can help prevent. The whole property cache mechanism for various things on localrepo is a bug's nest.

REPOSITORY
  rHG Mercurial

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

To: martinvonz, #hg-reviewers, quark, indygreg
Cc: indygreg, quark, mercurial-devel
phabricator - Aug. 8, 2017, 4:31 a.m.
martinvonz added a comment.


  Can this be queued for default now?

INLINE COMMENTS

> quark wrote in test-context.py:185
> Seems like `f.write(b'4')` is better.

Oops, definitely. I also updated the line above for consistency. I don't know if other places should be updated, so I didn't.

REPOSITORY
  rHG Mercurial

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

To: martinvonz, #hg-reviewers, quark, indygreg
Cc: indygreg, quark, mercurial-devel

Patch

diff --git a/tests/test-context.py b/tests/test-context.py
--- a/tests/test-context.py
+++ b/tests/test-context.py
@@ -179,3 +179,14 @@ 
             print('data mismatch')
     except Exception as ex:
         print('cannot read data: %r' % ex)
+
+with repo.wlock(), repo.lock(), repo.transaction('test'):
+    with open('4', 'wb') as f:
+        f.write(i)
+    repo.dirstate.normal('4')
+    repo.commit('4')
+    revsbefore = len(repo.changelog)
+    repo.invalidate(clearfilecache=True)
+    revsafter = len(repo.changelog)
+    if revsbefore != revsafter:
+        print('changeset lost by repo.invalidate()')
diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
--- a/mercurial/localrepo.py
+++ b/mercurial/localrepo.py
@@ -1466,6 +1466,13 @@ 
             # dirstate is invalidated separately in invalidatedirstate()
             if k == 'dirstate':
                 continue
+            if (k == 'changelog' and
+                self.currenttransaction() and
+                self.changelog._delayed):
+                # The changelog object may store unwritten revisions. We don't
+                # want to lose them.
+                # TODO: Solve the problem instead of working around it.
+                continue
 
             if clearfilecache:
                 del self._filecache[k]