Patchwork [1,of,7,V3] fncache: remove the rewriting logic

login
register
mail settings
Submitter Durham Goode
Date April 1, 2014, 10:24 p.m.
Message ID <c3f57418b098edc80a69.1396391051@dev2000.prn2.facebook.com>
Download mbox | patch
Permalink /patch/4188/
State Accepted
Commit cd03854a2e060179eae490a00560dca9c5c4eba9
Headers show

Comments

Durham Goode - April 1, 2014, 10:24 p.m.
# HG changeset patch
# User Durham Goode <durham@fb.com>
# Date 1395700307 25200
#      Mon Mar 24 15:31:47 2014 -0700
# Node ID c3f57418b098edc80a693e1ed675f465b843f45a
# Parent  6500a2eebee805ec02eb0bf9c5cb54a3e911b1ca
fncache: remove the rewriting logic

The fncache could rewrite itself during a read operation if it noticed any
entries that were no longer on disk. This was problematic because it caused
Mercurial to perform write operations outside the scope of a lock or
transaction, which could interefere with any other pending writes.

This will be replaced in a future patch by logic that cleans up the fncache
as files are deleted during strips.
Adrian Buehlmann - April 2, 2014, 6:42 p.m.
On 2014-04-02 00:24, Durham Goode wrote:
> diff --git a/mercurial/store.py b/mercurial/store.py
> --- a/mercurial/store.py
> +++ b/mercurial/store.py

[..]

> @@ -476,7 +469,6 @@
>          return self.rawvfs.stat(path).st_size
>  
>      def datafiles(self):
> -        rewrite = False
>          existing = []
>          for f in sorted(self.fncache):
>              ef = self.encode(f)
> @@ -486,12 +478,6 @@
>              except OSError, err:
>                  if err.errno != errno.ENOENT:
>                      raise
> -                # nonexistent entry
> -                rewrite = True
> -        if rewrite:
> -            # rewrite fncache to remove nonexistent entries
> -            # (may be caused by rollback / strip)
> -            self.fncache.rewrite(existing)
>  
>      def copylist(self):
>          d = ('data dh fncache phaseroots obsstore'

This looks like local variable "existing" is now unused and can thus be
removed (no need to keep appending to it).
Matt Mackall - April 3, 2014, 8:30 p.m.
On Wed, 2014-04-02 at 20:42 +0200, Adrian Buehlmann wrote:
> On 2014-04-02 00:24, Durham Goode wrote:
> > diff --git a/mercurial/store.py b/mercurial/store.py
> > --- a/mercurial/store.py
> > +++ b/mercurial/store.py
> 
> [..]
> 
> > @@ -476,7 +469,6 @@
> >          return self.rawvfs.stat(path).st_size
> >  
> >      def datafiles(self):
> > -        rewrite = False
> >          existing = []
> >          for f in sorted(self.fncache):
> >              ef = self.encode(f)
> > @@ -486,12 +478,6 @@
> >              except OSError, err:
> >                  if err.errno != errno.ENOENT:
> >                      raise
> > -                # nonexistent entry
> > -                rewrite = True
> > -        if rewrite:
> > -            # rewrite fncache to remove nonexistent entries
> > -            # (may be caused by rollback / strip)
> > -            self.fncache.rewrite(existing)
> >  
> >      def copylist(self):
> >          d = ('data dh fncache phaseroots obsstore'
> 
> This looks like local variable "existing" is now unused and can thus be
> removed (no need to keep appending to it).

Good spotting. I've committed a fix, thanks.

Patch

diff --git a/mercurial/store.py b/mercurial/store.py
--- a/mercurial/store.py
+++ b/mercurial/store.py
@@ -402,20 +402,13 @@ 
                     raise util.Abort(t)
         fp.close()
 
-    def _write(self, files, atomictemp):
-        fp = self.vfs('fncache', mode='wb', atomictemp=atomictemp)
-        if files:
-            fp.write(encodedir('\n'.join(files) + '\n'))
-        fp.close()
-        self._dirty = False
-
-    def rewrite(self, files):
-        self._write(files, False)
-        self.entries = set(files)
-
     def write(self):
         if self._dirty:
-            self._write(self.entries, True)
+            fp = self.vfs('fncache', mode='wb', atomictemp=True)
+            if self.entries:
+                fp.write(encodedir('\n'.join(self.entries) + '\n'))
+            fp.close()
+            self._dirty = False
 
     def add(self, fn):
         if self.entries is None:
@@ -476,7 +469,6 @@ 
         return self.rawvfs.stat(path).st_size
 
     def datafiles(self):
-        rewrite = False
         existing = []
         for f in sorted(self.fncache):
             ef = self.encode(f)
@@ -486,12 +478,6 @@ 
             except OSError, err:
                 if err.errno != errno.ENOENT:
                     raise
-                # nonexistent entry
-                rewrite = True
-        if rewrite:
-            # rewrite fncache to remove nonexistent entries
-            # (may be caused by rollback / strip)
-            self.fncache.rewrite(existing)
 
     def copylist(self):
         d = ('data dh fncache phaseroots obsstore'