[PATCH 0 of 3 V2] move cache files to a collected cache folder .hg/caches/

Jason Harris jason.f.harris at gmail.com
Sun Jan 2 17:28:23 CST 2011


On Jan 2, 2011, at 2:04 PM, Adrian Buehlmann wrote:

> On 2011-01-02 06:03, Jason Harris wrote:
> 
> Ok. I just skimmed through this part of your texts again. See my
> comments below.
> 
>> One problem with adding a copener instead of just changing the paths "in situ",
>> is that if after setting up copener, changes are later made to the repo's opener
>> than the copener doesn't get these changes. Ie the copener later has to be
>> synchronized with the opener anytime the opener changes. Such synchronization
>> things *alway* break be it sooner or later. In fact the test suite caught one
>> such change in localrepo.py where in a place deep in the code
>> self.opener.createmode was changed, and one of the tests then failed with the
>> cache directories being created in the wrong mode. The fix was of course to make
>> the same change to self.copener.createmode. In any case we have taken something
>> very simple and abstracted it a bit into something that to me is not quite as
>> clear.
> 
> Not sure what changes your are referring to in there, but this is a
> non-issue and pretty much a red herring.
> 
> All that's needed for setting up copener is (as you demonstrated
> yourself in your patch 3):
> 
> diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
> --- a/mercurial/localrepo.py
> +++ b/mercurial/localrepo.py
> @@ -33,6 +33,7 @@
>         self.auditor = util.path_auditor(self.root, self._checknested)
>         self.opener = util.opener(self.path)
>         self.wopener = util.opener(self.root)
> +        self.copener = util.opener(os.path.join(self.path, "caches"))
>         self.baseui = baseui
>         self.ui = baseui.copy()
> 
> @@ -93,6 +94,7 @@
>         self.sopener = self.store.opener
>         self.sjoin = self.store.join
>         self.opener.createmode = self.store.createmode
> +        self.copener.createmode = self.store.createmode
>         self._applyrequirements(requirements)
>         if create:
>             self._writerequirements()
> 
> This is a whole lot less fragile than repeatedly inserting
> 
>   os.path.join("caches", ...)
> 
> wherever a cache file needs to be accessed.

Not really...  Whenever we are accessing a cache we either use a
os.path.join("caches", ...) or copener, but in both cases the change is right
there and we are explicitly accessing the caches. The syncronization problem is
implicit and unstable in that in some bit of the code somewhere, someone might
change the settings of opener, and the person changing that code won't change
copener at the same time since there is nothing explicitly indicating they need
to do this. Thus the settings on opener and copener become de-synchronized and
trouble ensues. I for one only picked up one such case due to the test suite
throwing up permissions errors. (Thus I found the other necessary change to
self.copener.createmode = self.store.createmode.) I wouldn't have found this
without the test suite. Whereas if the caches are using opener with a
os.path.join("caches",...) they automatically pick up any changes to opener...


> The copener approach has the benefit that the directory name of the
> cache directory is defined in *one place* -- something which is pretty
> much standard in programming.

We are spending too more time discussing this... What you say below is true in
that I would also of course go with a copener approach when you introduce the
specialization as you are saying below. But generally I would specialize *only*
when I have actually added code like you mention below... 

But I just want someone who can push this patch to comment and I'll fix the
patch the way they want and be done with it...


> Also the "knowledge" that the string "caches" must be path-joined to
> access "The Mercurial cache" is encapsulated in copener.
> 
> The opener approach taken by Mercurial is a bit unusual, but as soon as
> you have understood how it works, it is pretty simple. You can just call
> the opener like any function object and it will open the file in the
> right place for you, doing whatever is needed to open that file in that
> location. And it encapsulates the policy that needs to be followed to
> access files in that place.
> 
> You might even be able to have a special cache opener behavior in the
> future, possibly side-stepping things like the hardlink dance that is
> done in class util.opener. It might also be possible to speed up things
> further by side-stepping util.opener's makedirs dance: all files are in
> the caches directory itself, there are no subdirs anyway that would need
> to be created. All that needs to be created is the "caches" (or "cache"
> dir -- as I have proposed). These improvements would all be possible if
> you used a separate cache opener.
> 
> But as I said, you are free to wait for Matt or a crew member (or
> whoever would like to :) to chime in.
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel at selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel



More information about the Mercurial-devel mailing list