Patchwork [6,of,9,V6] bundle2: add `bookmarks` part handler

login
register
mail settings
Submitter Stanislau Hlebik
Date Oct. 11, 2016, 4:25 p.m.
Message ID <b9e71247c1b68ce1fac7.1476203148@dev1918.lla1.facebook.com>
Download mbox | patch
Permalink /patch/17037/
State Changes Requested
Delegated to: Pierre-Yves David
Headers show

Comments

Stanislau Hlebik - Oct. 11, 2016, 4:25 p.m.
# HG changeset patch
# User Stanislau Hlebik <stash@fb.com>
# Date 1476197535 25200
#      Tue Oct 11 07:52:15 2016 -0700
# Node ID b9e71247c1b68ce1fac7dd69cf26d106f88f9372
# Parent  f781756b8de11a6f3e7dd5fd6354e9778defd8c3
bundle2: add `bookmarks` part handler

Applies bookmarks part to the local repo. `processbookmarksmode` determines
how remote bookmarks are handled. They are either ignored ('ignore' mode),
diverged ('diverge' mode) or applied ('apply' mode).
Pierre-Yves David - Oct. 14, 2016, 1:28 a.m.
On 10/11/2016 06:25 PM, Stanislau Hlebik wrote:
> # HG changeset patch
> # User Stanislau Hlebik <stash@fb.com>
> # Date 1476197535 25200
> #      Tue Oct 11 07:52:15 2016 -0700
> # Node ID b9e71247c1b68ce1fac7dd69cf26d106f88f9372
> # Parent  f781756b8de11a6f3e7dd5fd6354e9778defd8c3
> bundle2: add `bookmarks` part handler
>
> Applies bookmarks part to the local repo. `processbookmarksmode` determines
> how remote bookmarks are handled. They are either ignored ('ignore' mode),
> diverged ('diverge' mode) or applied ('apply' mode).
>
> diff --git a/mercurial/bundle2.py b/mercurial/bundle2.py
> --- a/mercurial/bundle2.py
> +++ b/mercurial/bundle2.py
> @@ -154,7 +154,9 @@
>  import sys
>
>  from .i18n import _
> +from .node import hex
>  from . import (
> +    bookmarks as bookmod,
>      changegroup,
>      error,
>      obsolete,
> @@ -287,13 +289,18 @@
>      * a way to construct a bundle response when applicable.
>      """
>
> -    def __init__(self, repo, transactiongetter, captureoutput=True):
> +    def __init__(self, repo, transactiongetter, captureoutput=True,
> +                 processbookmarksmode='ignore', explicitbookmarks=(),
> +                 remotepath=''):
>          self.repo = repo
>          self.ui = repo.ui
>          self.records = unbundlerecords()
>          self.gettransaction = transactiongetter
>          self.reply = None
>          self.captureoutput = captureoutput
> +        self.processbookmarksmode = processbookmarksmode
> +        self.explicitbookmarks = explicitbookmarks
> +        self.remotepath = remotepath

Hum, now that we see this change in practice, it seems a bit odd. The 
unbundle operation do not have business specific logic yet 
(pull/pushoperation does but they are higher level).

The best way here is probably to introduce a "input" attribut on the 
object (or "config" but the name is meh, or anything else good).

That input should probably be a simple dictionnary where higher order 
logic will be able to stick data to be used by part handler.

What do you think ?

>
>  class TransactionUnavailable(RuntimeError):
>      pass
> @@ -1620,3 +1627,28 @@
>
>      cache.write()
>      op.ui.debug('applied %i hgtags fnodes cache entries\n' % count)
> +
> +@parthandler('bookmarks')
> +def handlebookmarks(op, inpart):
> +    """Applies bookmarks to the local repo.

"Applies" does not seems to be the best wording as some of the process 
mode do not apply anything.

> +
> +    `processbookmarksmode` determines how remote bookmarks are handled. They are
> +    either ignored ('ignore' mode), diverged ('diverge' mode) or applied
> +    ('apply' mode).
> +    """

We should probably explain what use case each more is aimed at.

> +
> +    bookmarks = {}
> +    bookmarks = bookmod.decodebookmarks(inpart.read())
> +    if op.processbookmarksmode == 'apply':
> +        for bookmark, node in bookmarks.items():
> +            if node:
> +                op.repo._bookmarks[bookmark] = node
> +            else:
> +                del op.repo._bookmarks[bookmark]
> +        op.repo._bookmarks.recordchange(op.gettransaction())

What is your plan regarding hooks execution here?


> +    elif op.processbookmarksmode == 'diverge':
> +        bookmod.updatefromremote(op.ui, op.repo, bookmarks,
> +                                 op.remotepath, op.gettransaction,
> +                                 explicit=op.explicitbookmarks,
> +                                 srchex=hex)
> +    op.records.add('bookmarks', bookmarks)

How do you handle cases where the part exist multiple time in the bundle ?


cheers
Stanislau Hlebik - Nov. 2, 2016, 10:56 a.m.
On 10/14/16, 2:28 AM, "Pierre-Yves David" <pierre-yves.david@ens-lyon.org> wrote:

    What is your plan regarding hooks execution here?
    
I planned to add it when I’m going to work on push    

    How do you handle cases where the part exist multiple time in the bundle ?

Is there a problem with multiple parts? `op.records.add('bookmarks', bookmarks)` appends bookmarks from each part so it should work fine.
Pierre-Yves David - Nov. 10, 2016, 5:39 p.m.
On 11/02/2016 10:56 AM, Stanislau Hlebik wrote:
>
>
> On 10/14/16, 2:28 AM, "Pierre-Yves David" <pierre-yves.david@ens-lyon.org> wrote:
>
>     What is your plan regarding hooks execution here?
>
> I planned to add it when I’m going to work on push
>
>     How do you handle cases where the part exist multiple time in the bundle ?
>
> Is there a problem with multiple parts? `op.records.add('bookmarks', bookmarks)` appends bookmarks from each part so it should work fine.

Yes but it adds them as multiple items instead of a unified dictionnary, 
this seems strange. Could we have them as a single dict instead?
Stanislau Hlebik - Nov. 11, 2016, 11:50 a.m.
I think we can but it would require a bit of rewriting in unbundlerecords.

On 11/10/16, 5:39 PM, "Pierre-Yves David" <pierre-yves.david@ens-lyon.org> wrote:

    
    
    On 11/02/2016 10:56 AM, Stanislau Hlebik wrote:
    >

    >

    > On 10/14/16, 2:28 AM, "Pierre-Yves David" <pierre-yves.david@ens-lyon.org> wrote:

    >

    >     What is your plan regarding hooks execution here?

    >

    > I planned to add it when I’m going to work on push

    >

    >     How do you handle cases where the part exist multiple time in the bundle ?

    >

    > Is there a problem with multiple parts? `op.records.add('bookmarks', bookmarks)` appends bookmarks from each part so it should work fine.

    
    Yes but it adds them as multiple items instead of a unified dictionnary, 
    this seems strange. Could we have them as a single dict instead?
    
    -- 
    Pierre-Yves David
Stanislau Hlebik - Nov. 11, 2016, 12:04 p.m.
BTW, how do you think it should look like?
Do you want to introduce another type of categories in unbundlerecords or just rewrite categories whenever a new bookmark category is added?

On 11/11/16, 11:50 AM, "Stanislau Hlebik" <stash@fb.com> wrote:

    I think we can but it would require a bit of rewriting in unbundlerecords.
    
    On 11/10/16, 5:39 PM, "Pierre-Yves David" <pierre-yves.david@ens-lyon.org> wrote:
    
        
        
        On 11/02/2016 10:56 AM, Stanislau Hlebik wrote:
        >

        >

        > On 10/14/16, 2:28 AM, "Pierre-Yves David" <pierre-yves.david@ens-lyon.org> wrote:

        >

        >     What is your plan regarding hooks execution here?

        >

        > I planned to add it when I’m going to work on push

        >

        >     How do you handle cases where the part exist multiple time in the bundle ?

        >

        > Is there a problem with multiple parts? `op.records.add('bookmarks', bookmarks)` appends bookmarks from each part so it should work fine.

        
        Yes but it adds them as multiple items instead of a unified dictionnary, 
        this seems strange. Could we have them as a single dict instead?
        
        -- 
        Pierre-Yves David
Pierre-Yves David - Nov. 11, 2016, 12:09 p.m.
On 11/11/2016 11:50 AM, Stanislau Hlebik wrote:
> I think we can but it would require a bit of rewriting in unbundlerecords.

I don't think we need to change anything in the API, you can just access 
the previously recorded data and updated them.

Note that this is probably minor and we could move forward without that. 
We probably need bookmark movement tracking on the transaction level 
(for the hooks, etc) anyway and that might be enough.

What do you think ? on my part I think having a unified bookmark record 
would be more useful but keeping thing simple is also valuable.

Cheers,

> On 11/10/16, 5:39 PM, "Pierre-Yves David" <pierre-yves.david@ens-lyon.org> wrote:
>
>
>
>     On 11/02/2016 10:56 AM, Stanislau Hlebik wrote:
>     >
>     >
>     > On 10/14/16, 2:28 AM, "Pierre-Yves David" <pierre-yves.david@ens-lyon.org> wrote:
>     >
>     >     What is your plan regarding hooks execution here?
>     >
>     > I planned to add it when I’m going to work on push
>     >
>     >     How do you handle cases where the part exist multiple time in the bundle ?
>     >
>     > Is there a problem with multiple parts? `op.records.add('bookmarks', bookmarks)` appends bookmarks from each part so it should work fine.
>
>     Yes but it adds them as multiple items instead of a unified dictionnary,
>     this seems strange. Could we have them as a single dict instead?
>
>     --
>     Pierre-Yves David
>
>
Stanislau Hlebik - Nov. 11, 2016, 12:18 p.m.
I’d prefer to keep it simpler and implement bookmark unifying only if we need it

On 11/11/16, 12:09 PM, "Pierre-Yves David" <pierre-yves.david@ens-lyon.org> wrote:

    On 11/11/2016 11:50 AM, Stanislau Hlebik wrote:
    > I think we can but it would require a bit of rewriting in unbundlerecords.

    
    I don't think we need to change anything in the API, you can just access 
    the previously recorded data and updated them.
    
    Note that this is probably minor and we could move forward without that. 
    We probably need bookmark movement tracking on the transaction level 
    (for the hooks, etc) anyway and that might be enough.
    
    What do you think ? on my part I think having a unified bookmark record 
    would be more useful but keeping thing simple is also valuable.
    
    Cheers,
    
    > On 11/10/16, 5:39 PM, "Pierre-Yves David" <pierre-yves.david@ens-lyon.org> wrote:

    >

    >

    >

    >     On 11/02/2016 10:56 AM, Stanislau Hlebik wrote:

    >     >

    >     >

    >     > On 10/14/16, 2:28 AM, "Pierre-Yves David" <pierre-yves.david@ens-lyon.org> wrote:

    >     >

    >     >     What is your plan regarding hooks execution here?

    >     >

    >     > I planned to add it when I’m going to work on push

    >     >

    >     >     How do you handle cases where the part exist multiple time in the bundle ?

    >     >

    >     > Is there a problem with multiple parts? `op.records.add('bookmarks', bookmarks)` appends bookmarks from each part so it should work fine.

    >

    >     Yes but it adds them as multiple items instead of a unified dictionnary,

    >     this seems strange. Could we have them as a single dict instead?

    >

    >     --

    >     Pierre-Yves David

    >

    >

    
    -- 
    Pierre-Yves David

Patch

diff --git a/mercurial/bundle2.py b/mercurial/bundle2.py
--- a/mercurial/bundle2.py
+++ b/mercurial/bundle2.py
@@ -154,7 +154,9 @@ 
 import sys
 
 from .i18n import _
+from .node import hex
 from . import (
+    bookmarks as bookmod,
     changegroup,
     error,
     obsolete,
@@ -287,13 +289,18 @@ 
     * a way to construct a bundle response when applicable.
     """
 
-    def __init__(self, repo, transactiongetter, captureoutput=True):
+    def __init__(self, repo, transactiongetter, captureoutput=True,
+                 processbookmarksmode='ignore', explicitbookmarks=(),
+                 remotepath=''):
         self.repo = repo
         self.ui = repo.ui
         self.records = unbundlerecords()
         self.gettransaction = transactiongetter
         self.reply = None
         self.captureoutput = captureoutput
+        self.processbookmarksmode = processbookmarksmode
+        self.explicitbookmarks = explicitbookmarks
+        self.remotepath = remotepath
 
 class TransactionUnavailable(RuntimeError):
     pass
@@ -1620,3 +1627,28 @@ 
 
     cache.write()
     op.ui.debug('applied %i hgtags fnodes cache entries\n' % count)
+
+@parthandler('bookmarks')
+def handlebookmarks(op, inpart):
+    """Applies bookmarks to the local repo.
+
+    `processbookmarksmode` determines how remote bookmarks are handled. They are
+    either ignored ('ignore' mode), diverged ('diverge' mode) or applied
+    ('apply' mode).
+    """
+
+    bookmarks = {}
+    bookmarks = bookmod.decodebookmarks(inpart.read())
+    if op.processbookmarksmode == 'apply':
+        for bookmark, node in bookmarks.items():
+            if node:
+                op.repo._bookmarks[bookmark] = node
+            else:
+                del op.repo._bookmarks[bookmark]
+        op.repo._bookmarks.recordchange(op.gettransaction())
+    elif op.processbookmarksmode == 'diverge':
+        bookmod.updatefromremote(op.ui, op.repo, bookmarks,
+                                 op.remotepath, op.gettransaction,
+                                 explicit=op.explicitbookmarks,
+                                 srchex=hex)
+    op.records.add('bookmarks', bookmarks)