Patchwork [1,of,2] obsolete: move validedest function in obsolete module

login
register
mail settings
Submitter Pierre-Yves David
Date April 12, 2013, 4:55 p.m.
Message ID <5bd7702f8e9613a70b2d.1365785721@crater2.logilab.fr>
Download mbox | patch
Permalink /patch/1285/
State Deferred, archived
Delegated to: Augie Fackler
Headers show

Comments

Pierre-Yves David - April 12, 2013, 4:55 p.m.
# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@logilab.fr>
# Date 1365782993 -7200
#      Fri Apr 12 18:09:53 2013 +0200
# Node ID 5bd7702f8e9613a70b2dcf511aef85fd221eff78
# Parent  8c0a7eeda06d2773ec92b14527280db3e0167588
obsolete: move validedest function in obsolete module

Update related logic are going to use it too.
Pierre-Yves David - April 14, 2013, 2:41 a.m.
On 12 avr. 2013, at 23:48, Kevin Bullock wrote:

> On Apr 12, 2013, at 11:55 AM, pierre-yves.david@logilab.fr wrote:
> 
>> # HG changeset patch
>> # User Pierre-Yves David <pierre-yves.david@logilab.fr>
>> # Date 1365782993 -7200
>> #      Fri Apr 12 18:09:53 2013 +0200
>> # Node ID 5bd7702f8e9613a70b2dcf511aef85fd221eff78
>> # Parent  8c0a7eeda06d2773ec92b14527280db3e0167588
>> obsolete: move validedest function in obsolete module
>> 
>> Update related logic are going to use it too.
> 
> Needs a better explanation of what constitutes a "valid destination". Valid for what?
>  How do we determine what a "valid destination" is? (Answer: destination is the same or linear descendent of source, source is null rev, or <obsolescence-related hand-waving>.) This would also indicate why computing a "valid destination" is the same for both bookmarks and update-over-successors.

A valid destination (initially for bookmarks) are a descendants of origin. In the new evolution worlds it is something in the "foreground" of the origin. Something in the foreground is a changeset you can reachable through either "children" relation or successors relations (recursively).

A changeset foreground constitute its "linear future" as opposed to changeset in another topological branches. This is why bookmarks cleanly update to foreground changeset but create divergent one otherwise.

This logic applies to update too. We used to allows update to descendants, we now allows update to "foreground".

Notes that as we also allow dirty update to ancestors so we should allows dirty update to the background, that requires deeper changes not done in this changeset. That is also a much rarer usecase.

To conclude:

* Some of this explanation could go in the docstring
* we may rename this function "foreground"


> I'd also rather see it go into something like scmutil rather than obsolete (since it is indeed still used by bookmarks).

Meh, very few user yet.

Patch

diff --git a/mercurial/bookmarks.py b/mercurial/bookmarks.py
--- a/mercurial/bookmarks.py
+++ b/mercurial/bookmarks.py
@@ -231,11 +231,11 @@  def updatefromremote(ui, repo, remotemar
             if nr in repo:
                 cr = repo[nr]
                 cl = repo[nl]
                 if cl.rev() >= cr.rev():
                     continue
-                if validdest(repo, cl, cr):
+                if obsolete.validdest(repo, cl, cr):
                     localmarks[k] = cr.node()
                     changed = True
                     ui.status(_("updating bookmark %s\n") % k)
                 else:
                     if k == '@':
@@ -279,32 +279,5 @@  def diff(ui, dst, src):
     if len(diff) <= 0:
         ui.status(_("no changed bookmarks found\n"))
         return 1
     return 0
 
-def validdest(repo, old, new):
-    """Is the new bookmark destination a valid update from the old one"""
-    repo = repo.unfiltered()
-    if old == new:
-        # Old == new -> nothing to update.
-        return False
-    elif not old:
-        # old is nullrev, anything is valid.
-        # (new != nullrev has been excluded by the previous check)
-        return True
-    elif repo.obsstore:
-        # We only need this complicated logic if there is obsolescence
-        # XXX will probably deserve an optimised revset.
-        nm = repo.changelog.nodemap
-        validdests = set([old])
-        plen = -1
-        # compute the whole set of successors or descendants
-        while len(validdests) != plen:
-            plen = len(validdests)
-            succs = set(c.node() for c in validdests)
-            mutable = [c.node() for c in validdests if c.mutable()]
-            succs.update(obsolete.allsuccessors(repo.obsstore, mutable))
-            known = (n for n in succs if n in nm)
-            validdests = set(repo.set('%ln::', known))
-        return new in validdests
-    else:
-        return old.descendant(new)
diff --git a/mercurial/discovery.py b/mercurial/discovery.py
--- a/mercurial/discovery.py
+++ b/mercurial/discovery.py
@@ -5,11 +5,11 @@ 
 # This software may be used and distributed according to the terms of the
 # GNU General Public License version 2 or any later version.
 
 from node import nullid, short
 from i18n import _
-import util, setdiscovery, treediscovery, phases, obsolete, bookmarks
+import util, setdiscovery, treediscovery, phases, obsolete
 import branchmap
 
 def findcommonincoming(repo, remote, heads=None, force=False):
     """Return a tuple (common, anyincoming, heads) used to identify the common
     subset of nodes between repo and remote.
@@ -255,11 +255,11 @@  def checkheads(repo, remote, outgoing, r
     bookmarkedheads = set()
     for bm in localbookmarks:
         rnode = remotebookmarks.get(bm)
         if rnode and rnode in repo:
             lctx, rctx = repo[bm], repo[rnode]
-            if bookmarks.validdest(repo, rctx, lctx):
+            if obsolete.validdest(repo, rctx, lctx):
                 bookmarkedheads.add(lctx.node())
 
     # 3. Check for new heads.
     # If there are more heads after the push than before, a suitable
     # error message, depending on unsynced status, is displayed.
diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
--- a/mercurial/localrepo.py
+++ b/mercurial/localrepo.py
@@ -1933,11 +1933,11 @@  class localrepository(object):
             if k in unfi._bookmarks:
                 nr, nl = rb[k], hex(self._bookmarks[k])
                 if nr in unfi:
                     cr = unfi[nr]
                     cl = unfi[nl]
-                    if bookmarks.validdest(unfi, cr, cl):
+                    if obsolete.validdest(unfi, cr, cl):
                         r = remote.pushkey('bookmarks', k, nr, nl)
                         if r:
                             self.ui.status(_("updating bookmark %s\n") % k)
                         else:
                             self.ui.warn(_('updating bookmark %s'
diff --git a/mercurial/obsolete.py b/mercurial/obsolete.py
--- a/mercurial/obsolete.py
+++ b/mercurial/obsolete.py
@@ -401,10 +401,41 @@  def allsuccessors(obsstore, nodes, ignor
             for suc in mark[1]:
                 if suc not in seen:
                     seen.add(suc)
                     remaining.add(suc)
 
+def validdest(repo, old, new):
+    """Is <new> a valid destination from the <old> one
+
+    Used by bookmark logic.
+    """
+    repo = repo.unfiltered()
+    if old == new:
+        # Old == new -> nothing to update.
+        return False
+    elif not old:
+        # old is nullrev, anything is valid.
+        # (new != nullrev has been excluded by the previous check)
+        return True
+    elif repo.obsstore:
+        # We only need this complicated logic if there is obsolescence
+        # XXX will probably deserve an optimised revset.
+        nm = repo.changelog.nodemap
+        validdests = set([old])
+        plen = -1
+        # compute the whole set of successors or descendants
+        while len(validdests) != plen:
+            plen = len(validdests)
+            succs = set(c.node() for c in validdests)
+            mutable = [c.node() for c in validdests if c.mutable()]
+            succs.update(allsuccessors(repo.obsstore, mutable))
+            known = (n for n in succs if n in nm)
+            validdests = set(repo.set('%ln::', known))
+        return new in validdests
+    else:
+        return old.descendant(new)
+
 def successorssets(repo, initialnode, cache=None):
     """Return all set of successors of initial nodes
 
     Successors set of changeset A are a group of revision that succeed A. It
     succeed A as a consistent whole, each revision being only partial