Patchwork [2,of,2] localrepo: extensibility points for pull

login
register
mail settings
Submitter Gregory Szorc
Date July 8, 2014, 3:43 a.m.
Message ID <daa7b9b126b9ef4bbe66.1404790987@77.1.168.192.in-addr.arpa>
Download mbox | patch
Permalink /patch/5133/
State Changes Requested
Headers show

Comments

Gregory Szorc - July 8, 2014, 3:43 a.m.
# HG changeset patch
# User Gregory Szorc <gregory.szorc@gmail.com>
# Date 1404790424 25200
#      Mon Jul 07 20:33:44 2014 -0700
# Node ID daa7b9b126b9ef4bbe66e1a029e01eccdc57ac00
# Parent  30333e516874716b8d9b41450282d1f22b87eba1
localrepo: extensibility points for pull

To complement the pre and post push extensibility points, we offer
localrepo.checkpull() and localrepo.afterpull(), which each receive the
pulloperation instance for the in-flight pull. This gives extensions a
clear point from which to extend/adjust the behavior of pull.
Augie Fackler - July 9, 2014, 1:47 a.m.
On Mon, Jul 07, 2014 at 08:43:07PM -0700, Gregory Szorc wrote:
> # HG changeset patch
> # User Gregory Szorc <gregory.szorc@gmail.com>
> # Date 1404790424 25200
> #      Mon Jul 07 20:33:44 2014 -0700
> # Node ID daa7b9b126b9ef4bbe66e1a029e01eccdc57ac00
> # Parent  30333e516874716b8d9b41450282d1f22b87eba1
> localrepo: extensibility points for pull

Can you enlighten me on some of the (nefarious?) plans you have for
these functions?

I suspect it has to do with code review functions.

>
> To complement the pre and post push extensibility points, we offer
> localrepo.checkpull() and localrepo.afterpull(), which each receive the
> pulloperation instance for the in-flight pull. This gives extensions a
> clear point from which to extend/adjust the behavior of pull.
>
> diff --git a/mercurial/exchange.py b/mercurial/exchange.py
> --- a/mercurial/exchange.py
> +++ b/mercurial/exchange.py
> @@ -538,8 +538,9 @@ def pull(repo, remote, heads=None, force
>              raise util.Abort(msg)
>
>      lock = pullop.repo.lock()
>      try:
> +        pullop.repo.checkpull(pullop)
>          _pulldiscovery(pullop)
>          if (pullop.repo.ui.configbool('experimental', 'bundle2-exp', False)
>              and pullop.remote.capable('bundle2-exp')):
>              _pullbundle2(pullop)
> @@ -553,8 +554,9 @@ def pull(repo, remote, heads=None, force
>      finally:
>          pullop.releasetransaction()
>          lock.release()
>
> +    pullop.repo.afterpull(pullop)
>      return pullop.cgresult
>
>  def _pulldiscovery(pullop):
>      """discovery phase for the pull
> diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
> --- a/mercurial/localrepo.py
> +++ b/mercurial/localrepo.py
> @@ -1568,11 +1568,36 @@ class localrepository(object):
>              r.append(l)
>
>          return r
>
> +    def checkpull(self, pullop):
> +        """Extensibility point to perform work before pull.
> +
> +        Extensions may wish to alter the behavior of pull or to perform
> +        additional checks before a pull is attempted. This method provides
> +        a wrappable function that can intercept pull events easily.
> +
> +        This method receives an exchange.pulloperation instance. The instance
> +        can be modified and modified values will be used during the pull.
> +        """
> +        pass
> +
>      def pull(self, remote, heads=None, force=False):
>          return exchange.pull (self, remote, heads, force)
>
> +    def afterpull(self, pullop):
> +        """Extensibility point to do work after pull.
> +
> +        Extensions may wish to perform additional functionality after pull.
> +
> +        Merely wrapping pull() is not sufficient because pull() returns a
> +        status code for backwards compatibility.
> +
> +        Wrapping this method allows extensions to obtain the rich
> +        exchange.pulloperation instance after a pull has been completed.
> +        """
> +        pass
> +
>      def checkpush(self, pushop):
>          """Extensions can override this function if additional checks have
>          to be performed before pushing, or call it if they override push
>          command.
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
Gregory Szorc - July 9, 2014, 2:20 a.m.
On 7/8/14, 6:47 PM, Augie Fackler wrote:
> On Mon, Jul 07, 2014 at 08:43:07PM -0700, Gregory Szorc wrote:
>> # HG changeset patch
>> # User Gregory Szorc <gregory.szorc@gmail.com>
>> # Date 1404790424 25200
>> #      Mon Jul 07 20:33:44 2014 -0700
>> # Node ID daa7b9b126b9ef4bbe66e1a029e01eccdc57ac00
>> # Parent  30333e516874716b8d9b41450282d1f22b87eba1
>> localrepo: extensibility points for pull
>
> Can you enlighten me on some of the (nefarious?) plans you have for
> these functions?
>
> I suspect it has to do with code review functions.

I wrote 2 extension in the past few weeks that needed to hook into "post 
push" and needed access to the pushoperation. 1 was code review. I 
needed to see which changesets were pushed so I could initiate/update a 
code review against those changesets. The other was an extension that 
updated Bugzilla when a push was performed. Again, I needed access to 
the changesets that were pushed. In both cases, I had to wrap 
exchange._pushbookmark because it happens after the remote transaction 
has closed and has access to the pushop. Hacky.

For pull, I initially thought that you didn't need these hooks: you 
could wrap localrepo.pull and get what you need. But having access to 
pulloperation makes the lives of extensions easier. For example, if you 
want to perform post-pull processing of incoming changesets, your 
wrapped localrepo.pull must stow away a reference to the previous tip 
and then iterate oldtip:newtip. I'm not embarrassed to say I've 
introduced a bug or two due to an off-by-1 error in range(). IMO it's 
much easier to iterate pullop.remote. (It's worth noting that I /think/ 
I ran into issues hooking incoming and changegroup because these changes 
could get rolled back and the transaction API didn't (still doesn't?) 
offer a facility to intercept rollbacks to recover from this.)

I don't have any explicit use cases for pull extensibility in mind. 
Although I may retrofit an existing extension or two because I would 
rather look at a pulloperation instead instead of infer it from before 
and after state. pushoperation and pulloperation are definitely more 
pleasant to deal with. So much easier to understand than the full API of 
localrepo.

Patch

diff --git a/mercurial/exchange.py b/mercurial/exchange.py
--- a/mercurial/exchange.py
+++ b/mercurial/exchange.py
@@ -538,8 +538,9 @@  def pull(repo, remote, heads=None, force
             raise util.Abort(msg)
 
     lock = pullop.repo.lock()
     try:
+        pullop.repo.checkpull(pullop)
         _pulldiscovery(pullop)
         if (pullop.repo.ui.configbool('experimental', 'bundle2-exp', False)
             and pullop.remote.capable('bundle2-exp')):
             _pullbundle2(pullop)
@@ -553,8 +554,9 @@  def pull(repo, remote, heads=None, force
     finally:
         pullop.releasetransaction()
         lock.release()
 
+    pullop.repo.afterpull(pullop)
     return pullop.cgresult
 
 def _pulldiscovery(pullop):
     """discovery phase for the pull
diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
--- a/mercurial/localrepo.py
+++ b/mercurial/localrepo.py
@@ -1568,11 +1568,36 @@  class localrepository(object):
             r.append(l)
 
         return r
 
+    def checkpull(self, pullop):
+        """Extensibility point to perform work before pull.
+
+        Extensions may wish to alter the behavior of pull or to perform
+        additional checks before a pull is attempted. This method provides
+        a wrappable function that can intercept pull events easily.
+
+        This method receives an exchange.pulloperation instance. The instance
+        can be modified and modified values will be used during the pull.
+        """
+        pass
+
     def pull(self, remote, heads=None, force=False):
         return exchange.pull (self, remote, heads, force)
 
+    def afterpull(self, pullop):
+        """Extensibility point to do work after pull.
+
+        Extensions may wish to perform additional functionality after pull.
+
+        Merely wrapping pull() is not sufficient because pull() returns a
+        status code for backwards compatibility.
+
+        Wrapping this method allows extensions to obtain the rich
+        exchange.pulloperation instance after a pull has been completed.
+        """
+        pass
+
     def checkpush(self, pushop):
         """Extensions can override this function if additional checks have
         to be performed before pushing, or call it if they override push
         command.