Patchwork [10,of,22] obsstore: keep self._data updated with _addmarkers

login
register
mail settings
Submitter Jun Wu
Date June 4, 2017, 11:59 p.m.
Message ID <994e09024cdd5bbca80f.1496620762@x1c>
Download mbox | patch
Permalink /patch/21188/
State Accepted
Headers show

Comments

Jun Wu - June 4, 2017, 11:59 p.m.
# HG changeset patch
# User Jun Wu <quark@fb.com>
# Date 1496552183 25200
#      Sat Jun 03 21:56:23 2017 -0700
# Node ID 994e09024cdd5bbca80f1beb9dcbd2f1a19aa1bb
# Parent  5b83ced66577dc862594f85bcef2726a0d3fcfaf
# Available At https://bitbucket.org/quark-zju/hg-draft
#              hg pull https://bitbucket.org/quark-zju/hg-draft -r 994e09024cdd
obsstore: keep self._data updated with _addmarkers

This makes sure obsstore._data is still correct with added markers.
Gregory Szorc - June 6, 2017, 6:13 a.m.
On Sun, Jun 4, 2017 at 4:59 PM, Jun Wu <quark@fb.com> wrote:

> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1496552183 25200
> #      Sat Jun 03 21:56:23 2017 -0700
> # Node ID 994e09024cdd5bbca80f1beb9dcbd2f1a19aa1bb
> # Parent  5b83ced66577dc862594f85bcef2726a0d3fcfaf
> # Available At https://bitbucket.org/quark-zju/hg-draft
> #              hg pull https://bitbucket.org/quark-zju/hg-draft -r
> 994e09024cdd
> obsstore: keep self._data updated with _addmarkers
>
> This makes sure obsstore._data is still correct with added markers.
>
> diff --git a/mercurial/obsolete.py b/mercurial/obsolete.py
> --- a/mercurial/obsolete.py
> +++ b/mercurial/obsolete.py
> @@ -649,11 +649,11 @@ class obsstore(object):
>                  transaction.add('obsstore', offset)
>                  # offset == 0: new file - add the version header
> -                for bytes in encodemarkers(new, offset == 0,
> self._version):
> -                    f.write(bytes)
> +                data = b''.join(encodemarkers(new, offset == 0,
> self._version))
> +                f.write(data)
>              finally:
>                  # XXX: f.close() == filecache invalidation == obsstore
> rebuilt.
>                  # call 'filecacheentry.refresh()'  here
>                  f.close()
> -            self._addmarkers(new)
> +            self._addmarkers(new, data)
>              # new marker *may* have changed several set. invalidate the
> cache.
>              self.caches.clear()
> @@ -712,6 +712,7 @@ class obsstore(object):
>          return attr in self.__dict__
>
> -    def _addmarkers(self, markers):
> +    def _addmarkers(self, markers, rawdata):
>          markers = list(markers) # to allow repeated iteration
> +        self._data = self._data + rawdata
>

This scares me slightly because _data is a @propertycache. I think it is OK
because the self._data access will ensure the property cache getter is
replaced by self.__dict__['_data']. It may warrant an inline comment though.

(I really wish we used different types for representing mutable data
structures from read-only ones. It would make cases like this a bit easier
to reason about.)


>          self._all.extend(markers)
>          if self._cached('successors'):
>
Jun Wu - June 6, 2017, 5:23 p.m.
Excerpts from Gregory Szorc's message of 2017-06-05 23:13:52 -0700:
> This scares me slightly because _data is a @propertycache. I think it is OK
> because the self._data access will ensure the property cache getter is
> replaced by self.__dict__['_data']. It may warrant an inline comment though.

This got resolved in Patch 18 :)

> (I really wish we used different types for representing mutable data
> structures from read-only ones. It would make cases like this a bit easier
> to reason about.)

Patch

diff --git a/mercurial/obsolete.py b/mercurial/obsolete.py
--- a/mercurial/obsolete.py
+++ b/mercurial/obsolete.py
@@ -649,11 +649,11 @@  class obsstore(object):
                 transaction.add('obsstore', offset)
                 # offset == 0: new file - add the version header
-                for bytes in encodemarkers(new, offset == 0, self._version):
-                    f.write(bytes)
+                data = b''.join(encodemarkers(new, offset == 0, self._version))
+                f.write(data)
             finally:
                 # XXX: f.close() == filecache invalidation == obsstore rebuilt.
                 # call 'filecacheentry.refresh()'  here
                 f.close()
-            self._addmarkers(new)
+            self._addmarkers(new, data)
             # new marker *may* have changed several set. invalidate the cache.
             self.caches.clear()
@@ -712,6 +712,7 @@  class obsstore(object):
         return attr in self.__dict__
 
-    def _addmarkers(self, markers):
+    def _addmarkers(self, markers, rawdata):
         markers = list(markers) # to allow repeated iteration
+        self._data = self._data + rawdata
         self._all.extend(markers)
         if self._cached('successors'):