Patchwork [10,of,12] dirstate: refresh _branch cache entry after writing it

login
register
mail settings
Submitter Idan Kamara
Date Dec. 17, 2012, 3:35 p.m.
Message ID <9157ffe3d66d41bb7ea3.1355758535@idan>
Download mbox | patch
Permalink /patch/164/
State Superseded, archived
Commit 365fecd984c7beb451bd074585175a70f78d6765
Headers show

Comments

Idan Kamara - Dec. 17, 2012, 3:35 p.m.
# HG changeset patch
# User Idan Kamara <idankk86 at gmail.com>
# Date 1355682780 -7200
# Branch stable
# Node ID 9157ffe3d66d41bb7ea3cfa062d775a87731206a
# Parent  5046f500886a30970f1b7e40db9a372b51055748
dirstate: refresh _branch cache entry after writing it
Matt Mackall - Dec. 18, 2012, 10:40 p.m.
On Mon, 2012-12-17 at 17:35 +0200, Idan Kamara wrote:
> # HG changeset patch
> # User Idan Kamara <idankk86 at gmail.com>
> # Date 1355682780 -7200
> # Branch stable
> # Node ID 9157ffe3d66d41bb7ea3cfa062d775a87731206a
> # Parent  5046f500886a30970f1b7e40db9a372b51055748
> dirstate: refresh _branch cache entry after writing it
> 
> diff --git a/mercurial/dirstate.py b/mercurial/dirstate.py
> --- a/mercurial/dirstate.py
> +++ b/mercurial/dirstate.py
> @@ -266,6 +266,12 @@
>          try:
>              f.write(self._branch + '\n')
>              f.close()
> +
> +            # make sure filecache has the correct stat info for _branch after
> +            # replacing the underlying file
> +            ce = self._filecache['_branch']
> +            if ce:
> +                ce.refresh()

This seems rather unfortunate. I expect we'll eventually extend the
dirstate object with more auxiliary files like branch, so it might be
good to establish a cleaner pattern for this sort of thing.
Idan Kamara - Dec. 19, 2012, 8:42 a.m.
On Wed, Dec 19, 2012 at 12:40 AM, Matt Mackall <mpm at selenic.com> wrote:
> On Mon, 2012-12-17 at 17:35 +0200, Idan Kamara wrote:
>> # HG changeset patch
>> # User Idan Kamara <idankk86 at gmail.com>
>> # Date 1355682780 -7200
>> # Branch stable
>> # Node ID 9157ffe3d66d41bb7ea3cfa062d775a87731206a
>> # Parent  5046f500886a30970f1b7e40db9a372b51055748
>> dirstate: refresh _branch cache entry after writing it
>>
>> diff --git a/mercurial/dirstate.py b/mercurial/dirstate.py
>> --- a/mercurial/dirstate.py
>> +++ b/mercurial/dirstate.py
>> @@ -266,6 +266,12 @@
>>          try:
>>              f.write(self._branch + '\n')
>>              f.close()
>> +
>> +            # make sure filecache has the correct stat info for _branch after
>> +            # replacing the underlying file
>> +            ce = self._filecache['_branch']
>> +            if ce:
>> +                ce.refresh()
>
> This seems rather unfortunate. I expect we'll eventually extend the
> dirstate object with more auxiliary files like branch, so it might be
> good to establish a cleaner pattern for this sort of thing.

I agree. Currently the dirstate is acting a bit differently internally, and
the _branch file should probably be written when the wlock is released
through dirstate.write.

But this patch doesn't really belong with the rest of the series in the
sense that it doesn't seem to fix anything, even so I felt the need to
do this for correctness.
Matt Mackall - Dec. 19, 2012, 10:47 p.m.
On Wed, 2012-12-19 at 10:42 +0200, Idan Kamara wrote:
> On Wed, Dec 19, 2012 at 12:40 AM, Matt Mackall <mpm at selenic.com> wrote:
> > On Mon, 2012-12-17 at 17:35 +0200, Idan Kamara wrote:
> >> # HG changeset patch
> >> # User Idan Kamara <idankk86 at gmail.com>
> >> # Date 1355682780 -7200
> >> # Branch stable
> >> # Node ID 9157ffe3d66d41bb7ea3cfa062d775a87731206a
> >> # Parent  5046f500886a30970f1b7e40db9a372b51055748
> >> dirstate: refresh _branch cache entry after writing it
> >>
> >> diff --git a/mercurial/dirstate.py b/mercurial/dirstate.py
> >> --- a/mercurial/dirstate.py
> >> +++ b/mercurial/dirstate.py
> >> @@ -266,6 +266,12 @@
> >>          try:
> >>              f.write(self._branch + '\n')
> >>              f.close()
> >> +
> >> +            # make sure filecache has the correct stat info for _branch after
> >> +            # replacing the underlying file
> >> +            ce = self._filecache['_branch']
> >> +            if ce:
> >> +                ce.refresh()
> >
> > This seems rather unfortunate. I expect we'll eventually extend the
> > dirstate object with more auxiliary files like branch, so it might be
> > good to establish a cleaner pattern for this sort of thing.
> 
> I agree. Currently the dirstate is acting a bit differently internally, and
> the _branch file should probably be written when the wlock is released
> through dirstate.write.

That seems like a good idea.

> But this patch doesn't really belong with the rest of the series in the
> sense that it doesn't seem to fix anything, even so I felt the need to
> do this for correctness.

Ok. Any chance I can get you to whip up some wiki documentation for how
this all works, something like LockingDesign?
Idan Kamara - Dec. 20, 2012, 8:55 p.m.
On Thu, Dec 20, 2012 at 12:47 AM, Matt Mackall <mpm at selenic.com> wrote:
> On Wed, 2012-12-19 at 10:42 +0200, Idan Kamara wrote:
>> On Wed, Dec 19, 2012 at 12:40 AM, Matt Mackall <mpm at selenic.com> wrote:
>> > On Mon, 2012-12-17 at 17:35 +0200, Idan Kamara wrote:
>> >> # HG changeset patch
>> >> # User Idan Kamara <idankk86 at gmail.com>
>> >> # Date 1355682780 -7200
>> >> # Branch stable
>> >> # Node ID 9157ffe3d66d41bb7ea3cfa062d775a87731206a
>> >> # Parent  5046f500886a30970f1b7e40db9a372b51055748
>> >> dirstate: refresh _branch cache entry after writing it
>> >>
>> >> diff --git a/mercurial/dirstate.py b/mercurial/dirstate.py
>> >> --- a/mercurial/dirstate.py
>> >> +++ b/mercurial/dirstate.py
>> >> @@ -266,6 +266,12 @@
>> >>          try:
>> >>              f.write(self._branch + '\n')
>> >>              f.close()
>> >> +
>> >> +            # make sure filecache has the correct stat info for _branch after
>> >> +            # replacing the underlying file
>> >> +            ce = self._filecache['_branch']
>> >> +            if ce:
>> >> +                ce.refresh()
>> >
>> > This seems rather unfortunate. I expect we'll eventually extend the
>> > dirstate object with more auxiliary files like branch, so it might be
>> > good to establish a cleaner pattern for this sort of thing.
>>
>> I agree. Currently the dirstate is acting a bit differently internally, and
>> the _branch file should probably be written when the wlock is released
>> through dirstate.write.
>
> That seems like a good idea.

It should probably be done on default though, so we can either
postpone this patch, or fix it later on when we have the proper
thing in place.

>
>> But this patch doesn't really belong with the rest of the series in the
>> sense that it doesn't seem to fix anything, even so I felt the need to
>> do this for correctness.
>
> Ok. Any chance I can get you to whip up some wiki documentation for how
> this all works, something like LockingDesign?

Sure, now's the best time too before I forgot all the small details. I'll
probably add a page just for the filecache mechanism as well.
Idan Kamara - Dec. 22, 2012, 1 p.m.
On Thu, Dec 20, 2012 at 12:47 AM, Matt Mackall <mpm at selenic.com> wrote:
> On Wed, 2012-12-19 at 10:42 +0200, Idan Kamara wrote:
>> On Wed, Dec 19, 2012 at 12:40 AM, Matt Mackall <mpm at selenic.com> wrote:
>> > On Mon, 2012-12-17 at 17:35 +0200, Idan Kamara wrote:
>> >> # HG changeset patch
>> >> # User Idan Kamara <idankk86 at gmail.com>
>> >> # Date 1355682780 -7200
>> >> # Branch stable
>> >> # Node ID 9157ffe3d66d41bb7ea3cfa062d775a87731206a
>> >> # Parent  5046f500886a30970f1b7e40db9a372b51055748
>> >> dirstate: refresh _branch cache entry after writing it
>> >>
>> >> diff --git a/mercurial/dirstate.py b/mercurial/dirstate.py
>> >> --- a/mercurial/dirstate.py
>> >> +++ b/mercurial/dirstate.py
>> >> @@ -266,6 +266,12 @@
>> >>          try:
>> >>              f.write(self._branch + '\n')
>> >>              f.close()
>> >> +
>> >> +            # make sure filecache has the correct stat info for _branch after
>> >> +            # replacing the underlying file
>> >> +            ce = self._filecache['_branch']
>> >> +            if ce:
>> >> +                ce.refresh()
>> >
>> > This seems rather unfortunate. I expect we'll eventually extend the
>> > dirstate object with more auxiliary files like branch, so it might be
>> > good to establish a cleaner pattern for this sort of thing.
>>
>> I agree. Currently the dirstate is acting a bit differently internally, and
>> the _branch file should probably be written when the wlock is released
>> through dirstate.write.
>
> That seems like a good idea.
>
>> But this patch doesn't really belong with the rest of the series in the
>> sense that it doesn't seem to fix anything, even so I felt the need to
>> do this for correctness.
>
> Ok. Any chance I can get you to whip up some wiki documentation for how
> this all works, something like LockingDesign?

I started something a bit more wide here:
http://mercurial.selenic.com/wiki/Caching

Patch

diff --git a/mercurial/dirstate.py b/mercurial/dirstate.py
--- a/mercurial/dirstate.py
+++ b/mercurial/dirstate.py
@@ -266,6 +266,12 @@ 
         try:
             f.write(self._branch + '\n')
             f.close()
+
+            # make sure filecache has the correct stat info for _branch after
+            # replacing the underlying file
+            ce = self._filecache['_branch']
+            if ce:
+                ce.refresh()
         except: # re-raises
             f.discard()
             raise