Patchwork [02,of,12,V3] bookmarks: rewrite "updatefromremote()" by "compare()"

login
register
mail settings
Submitter Katsunori FUJIWARA
Date Oct. 2, 2013, 2:38 p.m.
Message ID <1559588706e842780f2f.1380724719@feefifofum>
Download mbox | patch
Permalink /patch/2694/
State Superseded
Headers show

Comments

Katsunori FUJIWARA - Oct. 2, 2013, 2:38 p.m.
# HG changeset patch
# User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
# Date 1380723659 -32400
#      Wed Oct 02 23:20:59 2013 +0900
# Node ID 1559588706e842780f2f54c488fde65bd1f188d2
# Parent  47300b7a97837455ff54fb6c0488b687eb230d65
bookmarks: rewrite "updatefromremote()" by "compare()"

To update entries in bmstore "localmarks", this patch uses
"bin(changesetid)" instead of "repo[changesetid].node()" used in
original "updatefromremote()" implementation, because the former is
cheaper than the latter.

This patch also changes order of updating bookmarks. Before this
patch, bookmarks are updated completely in order of their name. After
this patch, adding new bookmarks, updating existing bookmarks and
creating divergent bookmarks are executed in this order.
David Soria Parra - Oct. 2, 2013, 3:55 p.m.
On 10/02/2013 04:38 PM, FUJIWARA Katsunori wrote:
> # HG changeset patch
> # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
> # Date 1380723659 -32400
> #      Wed Oct 02 23:20:59 2013 +0900
> # Node ID 1559588706e842780f2f54c488fde65bd1f188d2
> # Parent  47300b7a97837455ff54fb6c0488b687eb230d65
> bookmarks: rewrite "updatefromremote()" by "compare()"
> 
> To update entries in bmstore "localmarks", this patch uses
> "bin(changesetid)" instead of "repo[changesetid].node()" used in
> original "updatefromremote()" implementation, because the former is
> cheaper than the latter.
> 
> This patch also changes order of updating bookmarks. Before this
> patch, bookmarks are updated completely in order of their name. After
> this patch, adding new bookmarks, updating existing bookmarks and
> creating divergent bookmarks are executed in this order.
> 
> diff --git a/mercurial/bookmarks.py b/mercurial/bookmarks.py
> --- a/mercurial/bookmarks.py
> +++ b/mercurial/bookmarks.py
> @@ -6,7 +6,7 @@
>  # GNU General Public License version 2 or any later version.
>  
>  from mercurial.i18n import _
> -from mercurial.node import hex
> +from mercurial.node import hex, bin
>  from mercurial import encoding, error, util, obsolete
>  import errno, os
>  
> @@ -325,46 +325,42 @@
>  
>      return results
>  
> +def _diverge(ui, b, path, localmarks):
> +    if b == '@':
> +        b = ''
> +    # find a unique @ suffix
> +    for x in range(1, 100):
> +        n = '%s@%d' % (b, x)
> +        if n not in localmarks:
> +            break
> +    # try to use an @pathalias suffix
> +    # if an @pathalias already exists, we overwrite (update) it
> +    for p, u in ui.configitems("paths"):
> +        if path == u:
> +            n = '%s@%s' % (b, p)
> +    return n

We prefer pathaliase over integers, so I think it's better to do.
If we have a path we never have to start looking for anything else.

def diverge(....):
    ....
    for p, u in ui.configitems("paths"):
        if path == u:
            return n = '%s@%s' % (b, p)

    for x in range(1, 100):
        n = '%s@%d' % (b, x):
        if n not in localmarks:
            return n
    ...error handling..

followed by error handling in case we cannot find a proper unique suffix.
Katsunori FUJIWARA - Oct. 15, 2013, 3:35 p.m.
At Wed, 02 Oct 2013 17:55:03 +0200,
David Soria Parra wrote:
> 
> On 10/02/2013 04:38 PM, FUJIWARA Katsunori wrote:

> > @@ -325,46 +325,42 @@
> >  
> >      return results
> >  
> > +def _diverge(ui, b, path, localmarks):
> > +    if b == '@':
> > +        b = ''
> > +    # find a unique @ suffix
> > +    for x in range(1, 100):
> > +        n = '%s@%d' % (b, x)
> > +        if n not in localmarks:
> > +            break
> > +    # try to use an @pathalias suffix
> > +    # if an @pathalias already exists, we overwrite (update) it
> > +    for p, u in ui.configitems("paths"):
> > +        if path == u:
> > +            n = '%s@%s' % (b, p)
> > +    return n
> 
> We prefer pathaliase over integers, so I think it's better to do.
> If we have a path we never have to start looking for anything else.
> 
> def diverge(....):
>     ....
>     for p, u in ui.configitems("paths"):
>         if path == u:
>             return n = '%s@%s' % (b, p)
> 
>     for x in range(1, 100):
>         n = '%s@%d' % (b, x):
>         if n not in localmarks:
>             return n
>     ...error handling..
> 
> followed by error handling in case we cannot find a proper unique suffix.

Thank you for your comments.

This patch just reused original implementation: for compatibility and
review-ability.

Now, I posted revised series including "_diverge()" improvements below:

    http://selenic.com/pipermail/mercurial-devel/2013-October/054292.html
    http://selenic.com/pipermail/mercurial-devel/2013-October/054295.html
    http://selenic.com/pipermail/mercurial-devel/2013-October/054293.html

----------------------------------------------------------------------
[FUJIWARA Katsunori]                             foozy@lares.dti.ne.jp

Patch

diff --git a/mercurial/bookmarks.py b/mercurial/bookmarks.py
--- a/mercurial/bookmarks.py
+++ b/mercurial/bookmarks.py
@@ -6,7 +6,7 @@ 
 # GNU General Public License version 2 or any later version.
 
 from mercurial.i18n import _
-from mercurial.node import hex
+from mercurial.node import hex, bin
 from mercurial import encoding, error, util, obsolete
 import errno, os
 
@@ -325,46 +325,42 @@ 
 
     return results
 
+def _diverge(ui, b, path, localmarks):
+    if b == '@':
+        b = ''
+    # find a unique @ suffix
+    for x in range(1, 100):
+        n = '%s@%d' % (b, x)
+        if n not in localmarks:
+            break
+    # try to use an @pathalias suffix
+    # if an @pathalias already exists, we overwrite (update) it
+    for p, u in ui.configitems("paths"):
+        if path == u:
+            n = '%s@%s' % (b, p)
+    return n
+
 def updatefromremote(ui, repo, remotemarks, path):
     ui.debug("checking for updated bookmarks\n")
+    localmarks = repo._bookmarks
+    (addsrc, adddst, advsrc, advdst, diverge, differ, invalid
+     ) = compare(repo, remotemarks, localmarks, dsthex=hex)
+
     changed = False
-    localmarks = repo._bookmarks
-    for k in sorted(remotemarks):
-        if k in localmarks:
-            nr, nl = remotemarks[k], localmarks[k]
-            if nr in repo:
-                cr = repo[nr]
-                cl = repo[nl]
-                if cl.rev() >= cr.rev():
-                    continue
-                if validdest(repo, cl, cr):
-                    localmarks[k] = cr.node()
-                    changed = True
-                    ui.status(_("updating bookmark %s\n") % k)
-                else:
-                    if k == '@':
-                        kd = ''
-                    else:
-                        kd = k
-                    # find a unique @ suffix
-                    for x in range(1, 100):
-                        n = '%s@%d' % (kd, x)
-                        if n not in localmarks:
-                            break
-                    # try to use an @pathalias suffix
-                    # if an @pathalias already exists, we overwrite (update) it
-                    for p, u in ui.configitems("paths"):
-                        if path == u:
-                            n = '%s@%s' % (kd, p)
-
-                    localmarks[n] = cr.node()
-                    changed = True
-                    ui.warn(_("divergent bookmark %s stored as %s\n") % (k, n))
-        elif remotemarks[k] in repo:
-            # add remote bookmarks for changes we already have
-            localmarks[k] = repo[remotemarks[k]].node()
+    for b, scid, dcid in addsrc:
+        if scid in repo: # add remote bookmarks for changes we already have
+            localmarks[b] = bin(scid)
+            ui.status(_("adding remote bookmark %s\n") % (b))
             changed = True
-            ui.status(_("adding remote bookmark %s\n") % k)
+    for b, scid, dcid in advsrc:
+        localmarks[b] = bin(scid)
+        ui.status(_("updating bookmark %s\n") % (b))
+        changed = True
+    for b, scid, dcid in diverge:
+        db = _diverge(ui, b, path, localmarks)
+        localmarks[db] = bin(scid)
+        ui.warn(_("divergent bookmark %s stored as %s\n") % (b, db))
+        changed = True
 
     if changed:
         localmarks.write()
diff --git a/tests/test-bookmarks-pushpull.t b/tests/test-bookmarks-pushpull.t
--- a/tests/test-bookmarks-pushpull.t
+++ b/tests/test-bookmarks-pushpull.t
@@ -42,8 +42,8 @@ 
   adding file changes
   added 1 changesets with 1 changes to 1 files
   adding remote bookmark X
+  adding remote bookmark Z
   updating bookmark Y
-  adding remote bookmark Z
   (run 'hg update' to get a working copy)
   $ hg bookmarks
      X                         0:4e3505fd9583
@@ -145,9 +145,9 @@ 
   adding manifests
   adding file changes
   added 1 changesets with 1 changes to 1 files (+1 heads)
+  updating bookmark Z
   divergent bookmark @ stored as @foo
   divergent bookmark X stored as X@foo
-  updating bookmark Z
   (run 'hg heads' to see heads, 'hg merge' to merge)
   $ hg book
      @                         1:9b140be10808
@@ -368,11 +368,11 @@ 
   $ hg pull -B Z http://localhost:$HGPORT/
   pulling from http://localhost:$HGPORT/
   no changes found
-  divergent bookmark @ stored as @1
-  divergent bookmark X stored as X@1
   adding remote bookmark Z
   adding remote bookmark foo
   adding remote bookmark foobar
+  divergent bookmark @ stored as @1
+  divergent bookmark X stored as X@1
   importing bookmark Z
   $ hg clone http://localhost:$HGPORT/ cloned-bookmarks
   requesting all changes