Patchwork [1,of,2] bundlerevlog: extract 'baserevision' method

login
register
mail settings
Submitter Wojciech Lopata
Date Aug. 27, 2013, 12:47 a.m.
Message ID <05999156c4f06d3ec4da.1377564453@dev1179.prn1.facebook.com>
Download mbox | patch
Permalink /patch/2272/
State Accepted
Commit 81241f978fd211cfdccf20485bdbeeb9c4b995a3
Headers show

Comments

Wojciech Lopata - Aug. 27, 2013, 12:47 a.m.
# HG changeset patch
# User Wojciech Lopata <lopek@fb.com>
# Date 1377561031 25200
#      Mon Aug 26 16:50:31 2013 -0700
# Node ID 05999156c4f06d3ec4da91303d5d6952523b038d
# Parent  1c7cf12674ecf5a8561a94cf93828ace0cd63027
bundlerevlog: extract 'baserevision' method

This makes possible to use bundlerevlog class with subclasses of revlog
that override revlog's 'revision' method. In particular this change is necessary
to implement manifest compression, as it allows extension to replace manifest
class and override 'revision' method there.
Augie Fackler - Aug. 27, 2013, 3:18 p.m.
On Mon, Aug 26, 2013 at 05:47:33PM -0700, Wojciech Lopata wrote:
> # HG changeset patch
> # User Wojciech Lopata <lopek@fb.com>
> # Date 1377561031 25200
> #      Mon Aug 26 16:50:31 2013 -0700
> # Node ID 05999156c4f06d3ec4da91303d5d6952523b038d
> # Parent  1c7cf12674ecf5a8561a94cf93828ace0cd63027
> bundlerevlog: extract 'baserevision' method
>
> This makes possible to use bundlerevlog class with subclasses of revlog
> that override revlog's 'revision' method. In particular this change is necessary
> to implement manifest compression, as it allows extension to replace manifest
> class and override 'revision' method there.
>
> diff --git a/mercurial/bundlerepo.py b/mercurial/bundlerepo.py
> --- a/mercurial/bundlerepo.py
> +++ b/mercurial/bundlerepo.py
> @@ -120,7 +120,7 @@
>              chain.append(iterrev)
>              iterrev = self.index[iterrev][3]
>          if text is None:
> -            text = revlog.revlog.revision(self, iterrev)
> +            text = self.baserevision(iterrev)
>
>          while chain:
>              delta = self._chunk(chain.pop())
> @@ -130,6 +130,12 @@
>          self._cache = (node, rev, text)
>          return text
>
> +    def baserevision(self, nodeorrev):
> +        # Revlog subclasses may override 'revision' method to modify format of
> +        # content retrieved from revlog. To use bundlerevlog with such class one
> +        # needs to override 'baserevision' and make more specific call here.
> +        return revlog.revlog.revision(self, nodeorrev)
> +
>      def addrevision(self, text, transaction, link, p1=None, p2=None, d=None):
>          raise NotImplementedError
>      def addgroup(self, revs, linkmapper, transaction):
> @@ -146,12 +152,21 @@
>          bundlerevlog.__init__(self, opener, self.indexfile, bundle,
>                                linkmapper)
>
> +    def baserevision(self, nodeorrev):
> +        # Although changelog doesn't override 'revision' method, some extensions
> +        # may replace this class with another that does. Same story with
> +        # manifest and filelog classes.
> +        return changelog.changelog.revision(self, nodeorrev)
> +
>  class bundlemanifest(bundlerevlog, manifest.manifest):
>      def __init__(self, opener, bundle, linkmapper):
>          manifest.manifest.__init__(self, opener)
>          bundlerevlog.__init__(self, opener, self.indexfile, bundle,
>                                linkmapper)
>
> +    def baserevision(self, nodeorrev):
> +        return manifest.manifest.revision(self, nodeorrev)
> +
>  class bundlefilelog(bundlerevlog, filelog.filelog):
>      def __init__(self, opener, path, bundle, linkmapper, repo):
>          filelog.filelog.__init__(self, opener, path)
> @@ -159,6 +174,9 @@
>                                linkmapper)
>          self._repo = repo
>
> +    def baserevision(self, nodeorrev):
> +        return filelog.filelog.revision(self, nodeorrev)

Is this change (and the other ones like it) actually necessary? Is filelog.filelog.revision not currently the same as revlog.revlog.revision?

(That is: couldn't this use the method from the base class for now?)

> +
>      def _file(self, f):
>          self._repo.file(f)
>
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
Wojciech Lopata - Aug. 27, 2013, 5:20 p.m.
> From: Augie Fackler [mailto:raf@durin42.com]
> Sent: Tuesday, August 27, 2013 8:18 AM
> To: Wojciech Lopata
> Cc: mercurial-devel@selenic.com
> Subject: Re: [PATCH 1 of 2] bundlerevlog: extract 'baserevision' 
> method
> 
> On Mon, Aug 26, 2013 at 05:47:33PM -0700, Wojciech Lopata wrote:
> > # HG changeset patch
> > # User Wojciech Lopata <lopek@fb.com> # Date 1377561031 25200
> > #      Mon Aug 26 16:50:31 2013 -0700
> > # Node ID 05999156c4f06d3ec4da91303d5d6952523b038d
> > # Parent  1c7cf12674ecf5a8561a94cf93828ace0cd63027
> > bundlerevlog: extract 'baserevision' method
> >
> > This makes possible to use bundlerevlog class with subclasses of 
> > revlog that override revlog's 'revision' method. In particular this 
> > change is necessary to implement manifest compression, as it allows 
> > extension to replace manifest class and override 'revision' method there.
> >
> > diff --git a/mercurial/bundlerepo.py b/mercurial/bundlerepo.py
> > --- a/mercurial/bundlerepo.py
> > +++ b/mercurial/bundlerepo.py
> > @@ -120,7 +120,7 @@
> >              chain.append(iterrev)
> >              iterrev = self.index[iterrev][3]
> >          if text is None:
> > -            text = revlog.revlog.revision(self, iterrev)
> > +            text = self.baserevision(iterrev)
> >
> >          while chain:
> >              delta = self._chunk(chain.pop()) @@ -130,6 +130,12 @@
> >          self._cache = (node, rev, text)
> >          return text
> >
> > +    def baserevision(self, nodeorrev):
> > +        # Revlog subclasses may override 'revision' method to modify format of
> > +        # content retrieved from revlog. To use bundlerevlog with such class one
> > +        # needs to override 'baserevision' and make more specific call here.
> > +        return revlog.revlog.revision(self, nodeorrev)
> > +
> >      def addrevision(self, text, transaction, link, p1=None, p2=None, d=None):
> >          raise NotImplementedError
> >      def addgroup(self, revs, linkmapper, transaction):
> > @@ -146,12 +152,21 @@
> >          bundlerevlog.__init__(self, opener, self.indexfile, bundle,
> >                                linkmapper)
> >
> > +    def baserevision(self, nodeorrev):
> > +        # Although changelog doesn't override 'revision' method, some extensions
> > +        # may replace this class with another that does. Same story with
> > +        # manifest and filelog classes.
> > +        return changelog.changelog.revision(self, nodeorrev)
> > +
> >  class bundlemanifest(bundlerevlog, manifest.manifest):
> >      def __init__(self, opener, bundle, linkmapper):
> >          manifest.manifest.__init__(self, opener)
> >          bundlerevlog.__init__(self, opener, self.indexfile, bundle,
> >                                linkmapper)
> >
> > +    def baserevision(self, nodeorrev):
> > +        return manifest.manifest.revision(self, nodeorrev)
> > +
> >  class bundlefilelog(bundlerevlog, filelog.filelog):
> >      def __init__(self, opener, path, bundle, linkmapper, repo):
> >          filelog.filelog.__init__(self, opener, path) @@ -159,6 
> > +174,9 @@
> >                                linkmapper)
> >          self._repo = repo
> >
> > +    def baserevision(self, nodeorrev):
> > +        return filelog.filelog.revision(self, nodeorrev)
> 
> Is this change (and the other ones like it) actually necessary? Is 
> filelog.filelog.revision not currently the same as revlog.revlog.revision?
> 
> (That is: couldn't this use the method from the base class for now?)

First of all, I need this change only in bundlemanifest (in bundlefilelog and bundlechangelog I added it only for consistency)

My extension replaces 'manifest' class with 'tinymanifest' class. 'tinymanifest' overrides 'revision' method of revlog. Bundlerevlog's 'revision' method calls explicitly revlog.revlog.revision which is wrong in my case (it should probably call the next 'revision' method in MRO).

In other words, manifest.manifest.revlog is the same as revlog.revlog.revision, as long as manifest.manifest is not replaced with tinymanifest class.

Extracting 'baserevision' is cleanest solution among simple ones we thought of, although I know it feels quite dirty.

> 
> > +
> >      def _file(self, f):
> >          self._repo.file(f)
> >
> > _______________________________________________
> > Mercurial-devel mailing list
> > Mercurial-devel@selenic.com
> > http://selenic.com/mailman/listinfo/mercurial-devel
Augie Fackler - Aug. 27, 2013, 5:25 p.m.
On Tue, Aug 27, 2013 at 1:20 PM, Wojciech Lopata <lopek@fb.com> wrote:
>> Is this change (and the other ones like it) actually necessary? Is
>> filelog.filelog.revision not currently the same as revlog.revlog.revision?
>>
>> (That is: couldn't this use the method from the base class for now?)
>
> First of all, I need this change only in bundlemanifest (in bundlefilelog and bundlechangelog I added it only for consistency)


What I meant was, "do you actually need to do this explicit forwarding
of the method to the superclass, since that's what would happen if you
didn't add this particular line?" - I understand adding the hook for a
tinymanifest extension :)
Wojciech Lopata - Aug. 27, 2013, 6:34 p.m.
> From: Augie Fackler [mailto:raf@durin42.com]
> Sent: Tuesday, August 27, 2013 10:26 AM
> To: Wojciech Lopata
> Cc: mercurial-devel@selenic.com
> Subject: Re: [PATCH 1 of 2] bundlerevlog: extract 'baserevision' method
> 
> On Tue, Aug 27, 2013 at 1:20 PM, Wojciech Lopata <lopek@fb.com> wrote:
> >> Is this change (and the other ones like it) actually necessary? Is
> >> filelog.filelog.revision not currently the same as revlog.revlog.revision?
> >>
> >> (That is: couldn't this use the method from the base class for now?)
> >
> > First of all, I need this change only in bundlemanifest (in bundlefilelog and bundlechangelog I added it only for consistency)
> 
> 
> What I meant was, "do you actually need to do this explicit forwarding
> of the method to the superclass, since that's what would happen if you
> didn't add this particular line?" - I understand adding the hook for a
> tinymanifest extension :)

To make sure it is clear - http://pastebin.com/MEkgDDzs - that's what I mean by 'replacing class'.

Why overriding ' baserevision ' in bundlemanifest class in the way I do it makes sense? When my extension is loaded and manifest class is replaced, semantic of

class bundlemanifest(bundlerevlog, manifest.manifest)

changes to

class bundlemanifest(bundlerevlog, tinymanifest)

Similarly, manifest.manifest.revision means now tinymanifest.revision. And this is not revlog.revlog.revision.

If we go with only creating 'baserevision' hook, without overriding it in bundlemanifest, I will need to put something like this in my extension's code: http://pastebin.com/BXLGWFET. Same with unionrepo. This is basically a copy&paste, so I wanted to avoid it, but if we still disagree on overriding 'baserevision' in bundlemanifest, I can go with it.
Augie Fackler - Aug. 27, 2013, 7:05 p.m.
On Mon, Aug 26, 2013 at 05:47:33PM -0700, Wojciech Lopata wrote:
> # HG changeset patch
> # User Wojciech Lopata <lopek@fb.com>
> # Date 1377561031 25200
> #      Mon Aug 26 16:50:31 2013 -0700
> # Node ID 05999156c4f06d3ec4da91303d5d6952523b038d
> # Parent  1c7cf12674ecf5a8561a94cf93828ace0cd63027
> bundlerevlog: extract 'baserevision' method

Closing this thread: I'm not good at reading, and lopek has convinced
me (in IRC) that this is the right change. I'd kind of like some other
crew member to eyeball it since I'm evidently unable to read code
today, however.

>
> This makes possible to use bundlerevlog class with subclasses of revlog
> that override revlog's 'revision' method. In particular this change is necessary
> to implement manifest compression, as it allows extension to replace manifest
> class and override 'revision' method there.
>
> diff --git a/mercurial/bundlerepo.py b/mercurial/bundlerepo.py
> --- a/mercurial/bundlerepo.py
> +++ b/mercurial/bundlerepo.py
> @@ -120,7 +120,7 @@
>              chain.append(iterrev)
>              iterrev = self.index[iterrev][3]
>          if text is None:
> -            text = revlog.revlog.revision(self, iterrev)
> +            text = self.baserevision(iterrev)
>
>          while chain:
>              delta = self._chunk(chain.pop())
> @@ -130,6 +130,12 @@
>          self._cache = (node, rev, text)
>          return text
>
> +    def baserevision(self, nodeorrev):
> +        # Revlog subclasses may override 'revision' method to modify format of
> +        # content retrieved from revlog. To use bundlerevlog with such class one
> +        # needs to override 'baserevision' and make more specific call here.
> +        return revlog.revlog.revision(self, nodeorrev)
> +
>      def addrevision(self, text, transaction, link, p1=None, p2=None, d=None):
>          raise NotImplementedError
>      def addgroup(self, revs, linkmapper, transaction):
> @@ -146,12 +152,21 @@
>          bundlerevlog.__init__(self, opener, self.indexfile, bundle,
>                                linkmapper)
>
> +    def baserevision(self, nodeorrev):
> +        # Although changelog doesn't override 'revision' method, some extensions
> +        # may replace this class with another that does. Same story with
> +        # manifest and filelog classes.
> +        return changelog.changelog.revision(self, nodeorrev)
> +
>  class bundlemanifest(bundlerevlog, manifest.manifest):
>      def __init__(self, opener, bundle, linkmapper):
>          manifest.manifest.__init__(self, opener)
>          bundlerevlog.__init__(self, opener, self.indexfile, bundle,
>                                linkmapper)
>
> +    def baserevision(self, nodeorrev):
> +        return manifest.manifest.revision(self, nodeorrev)
> +
>  class bundlefilelog(bundlerevlog, filelog.filelog):
>      def __init__(self, opener, path, bundle, linkmapper, repo):
>          filelog.filelog.__init__(self, opener, path)
> @@ -159,6 +174,9 @@
>                                linkmapper)
>          self._repo = repo
>
> +    def baserevision(self, nodeorrev):
> +        return filelog.filelog.revision(self, nodeorrev)
> +
>      def _file(self, f):
>          self._repo.file(f)
>
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel

Patch

diff --git a/mercurial/bundlerepo.py b/mercurial/bundlerepo.py
--- a/mercurial/bundlerepo.py
+++ b/mercurial/bundlerepo.py
@@ -120,7 +120,7 @@ 
             chain.append(iterrev)
             iterrev = self.index[iterrev][3]
         if text is None:
-            text = revlog.revlog.revision(self, iterrev)
+            text = self.baserevision(iterrev)
 
         while chain:
             delta = self._chunk(chain.pop())
@@ -130,6 +130,12 @@ 
         self._cache = (node, rev, text)
         return text
 
+    def baserevision(self, nodeorrev):
+        # Revlog subclasses may override 'revision' method to modify format of
+        # content retrieved from revlog. To use bundlerevlog with such class one
+        # needs to override 'baserevision' and make more specific call here.
+        return revlog.revlog.revision(self, nodeorrev)
+
     def addrevision(self, text, transaction, link, p1=None, p2=None, d=None):
         raise NotImplementedError
     def addgroup(self, revs, linkmapper, transaction):
@@ -146,12 +152,21 @@ 
         bundlerevlog.__init__(self, opener, self.indexfile, bundle,
                               linkmapper)
 
+    def baserevision(self, nodeorrev):
+        # Although changelog doesn't override 'revision' method, some extensions
+        # may replace this class with another that does. Same story with
+        # manifest and filelog classes.
+        return changelog.changelog.revision(self, nodeorrev)
+
 class bundlemanifest(bundlerevlog, manifest.manifest):
     def __init__(self, opener, bundle, linkmapper):
         manifest.manifest.__init__(self, opener)
         bundlerevlog.__init__(self, opener, self.indexfile, bundle,
                               linkmapper)
 
+    def baserevision(self, nodeorrev):
+        return manifest.manifest.revision(self, nodeorrev)
+
 class bundlefilelog(bundlerevlog, filelog.filelog):
     def __init__(self, opener, path, bundle, linkmapper, repo):
         filelog.filelog.__init__(self, opener, path)
@@ -159,6 +174,9 @@ 
                               linkmapper)
         self._repo = repo
 
+    def baserevision(self, nodeorrev):
+        return filelog.filelog.revision(self, nodeorrev)
+
     def _file(self, f):
         self._repo.file(f)