Patchwork [4,of,4,V2] localrepo: make _refreshfilecachestats unfiltered method to refresh correctly

login
register
mail settings
Submitter Katsunori FUJIWARA
Date Sept. 11, 2016, 6:11 p.m.
Message ID <59f8a0e41e092819b48f.1473617516@feefifofum>
Download mbox | patch
Permalink /patch/16588/
State Accepted
Headers show

Comments

Katsunori FUJIWARA - Sept. 11, 2016, 6:11 p.m.
# HG changeset patch
# User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
# Date 1473617189 -32400
#      Mon Sep 12 03:06:29 2016 +0900
# Node ID 59f8a0e41e092819b48fff468795c96a464d22a4
# Parent  0f6ba83a54fab382452faf389265ef636d0c42a1
localrepo: make _refreshfilecachestats unfiltered method to refresh correctly

Before this patch, if transaction is started via "filtered repo"
object, _refreshfilecachestats() at closing transaction doesn't
refresh file stat of any @filecache properties correctly, because:

  - _refreshfilecachestats() omits refreshing file stat of a
    @filecache property, if it doesn't appear in self.__dict__

  - if transaction is started via "filtered repo",
    _refreshfilecachestats() is applied on "filtered repo"

    because repo.transaction() adds "self._refreshfilecachestats" to
    post close procedures. repo.transaction() isn't unfiltered method,
    and "self" in it means "filtered repo" in this case.

    Transactions started by explicit repo.transaction() easily causes
    this situation.

  - _refreshfilecachestats() applied on "filtered repo" omits whole
    refreshing

    because @filecache properties are stored into "unfiltered repo",
    and appear only in self.__dict__ of "unfiltered repo".

This incorrect refreshing causes unnecessary reloading from files.

To refresh file stat of @filecache properties at closing transaction
correctly, this patch makes _refreshfilecachestats() unfiltered
method.

This patch chooses making _refreshfilecachestats() unfiltered method
instead of making transaction() unfiltered method, to reduce
unexpected side effect.
Augie Fackler - Sept. 12, 2016, 1:34 p.m.
On Mon, Sep 12, 2016 at 03:11:56AM +0900, FUJIWARA Katsunori wrote:
> # HG changeset patch
> # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
> # Date 1473617189 -32400
> #      Mon Sep 12 03:06:29 2016 +0900
> # Node ID 59f8a0e41e092819b48fff468795c96a464d22a4
> # Parent  0f6ba83a54fab382452faf389265ef636d0c42a1
> localrepo: make _refreshfilecachestats unfiltered method to refresh correctly

Very nice work. Queued.

>
> Before this patch, if transaction is started via "filtered repo"
> object, _refreshfilecachestats() at closing transaction doesn't
> refresh file stat of any @filecache properties correctly, because:
>
>   - _refreshfilecachestats() omits refreshing file stat of a
>     @filecache property, if it doesn't appear in self.__dict__
>
>   - if transaction is started via "filtered repo",
>     _refreshfilecachestats() is applied on "filtered repo"
>
>     because repo.transaction() adds "self._refreshfilecachestats" to
>     post close procedures. repo.transaction() isn't unfiltered method,
>     and "self" in it means "filtered repo" in this case.
>
>     Transactions started by explicit repo.transaction() easily causes
>     this situation.
>
>   - _refreshfilecachestats() applied on "filtered repo" omits whole
>     refreshing
>
>     because @filecache properties are stored into "unfiltered repo",
>     and appear only in self.__dict__ of "unfiltered repo".
>
> This incorrect refreshing causes unnecessary reloading from files.
>
> To refresh file stat of @filecache properties at closing transaction
> correctly, this patch makes _refreshfilecachestats() unfiltered
> method.
>
> This patch chooses making _refreshfilecachestats() unfiltered method
> instead of making transaction() unfiltered method, to reduce
> unexpected side effect.
>
> diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
> --- a/mercurial/localrepo.py
> +++ b/mercurial/localrepo.py
> @@ -1279,6 +1279,7 @@ class localrepository(object):
>          self.invalidate()
>          self.invalidatedirstate()
>
> +    @unfilteredmethod
>      def _refreshfilecachestats(self, tr):
>          """Reload stats of cached files so that they are flagged as valid"""
>          for k, ce in self._filecache.items():
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Pierre-Yves David - Sept. 13, 2016, 12:13 a.m.
On 09/11/2016 08:11 PM, FUJIWARA Katsunori wrote:
> # HG changeset patch
> # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
> # Date 1473617189 -32400
> #      Mon Sep 12 03:06:29 2016 +0900
> # Node ID 59f8a0e41e092819b48fff468795c96a464d22a4
> # Parent  0f6ba83a54fab382452faf389265ef636d0c42a1
> localrepo: make _refreshfilecachestats unfiltered method to refresh correctly
>
> Before this patch, if transaction is started via "filtered repo"
> object, _refreshfilecachestats() at closing transaction doesn't
> refresh file stat of any @filecache properties correctly, because:
>
>   - _refreshfilecachestats() omits refreshing file stat of a
>     @filecache property, if it doesn't appear in self.__dict__
>
>   - if transaction is started via "filtered repo",
>     _refreshfilecachestats() is applied on "filtered repo"
>
>     because repo.transaction() adds "self._refreshfilecachestats" to
>     post close procedures. repo.transaction() isn't unfiltered method,
>     and "self" in it means "filtered repo" in this case.
>
>     Transactions started by explicit repo.transaction() easily causes
>     this situation.
>
>   - _refreshfilecachestats() applied on "filtered repo" omits whole
>     refreshing
>
>     because @filecache properties are stored into "unfiltered repo",
>     and appear only in self.__dict__ of "unfiltered repo".
>
> This incorrect refreshing causes unnecessary reloading from files.
>
> To refresh file stat of @filecache properties at closing transaction
> correctly, this patch makes _refreshfilecachestats() unfiltered
> method.
>
> This patch chooses making _refreshfilecachestats() unfiltered method
> instead of making transaction() unfiltered method, to reduce
> unexpected side effect.

We can probably unfilter transaction too.
(but this patch is correct in itself)

Patch

diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
--- a/mercurial/localrepo.py
+++ b/mercurial/localrepo.py
@@ -1279,6 +1279,7 @@  class localrepository(object):
         self.invalidate()
         self.invalidatedirstate()
 
+    @unfilteredmethod
     def _refreshfilecachestats(self, tr):
         """Reload stats of cached files so that they are flagged as valid"""
         for k, ce in self._filecache.items():