Patchwork [3,of,4,defaultdest,V2] destupdate: also include bookmark related logic

login
register
mail settings
Submitter Pierre-Yves David
Date Oct. 12, 2015, 5:22 p.m.
Message ID <330ed5e136273f627695.1444670575@marginatus.alto.octopoid.net>
Download mbox | patch
Permalink /patch/10966/
State Superseded
Commit 5c57d01fe64e32fb3b3e70943abdc40960851690
Headers show

Comments

Pierre-Yves David - Oct. 12, 2015, 5:22 p.m.
# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@fb.com>
# Date 1443513806 25200
#      Tue Sep 29 01:03:26 2015 -0700
# Node ID 330ed5e136273f627695777702a77ab695bc62e2
# Parent  976e836aa9fcc99d05a462afef5007351e5226a1
# EXP-Topic defaultdest
# Available At http://hg.netv6.net/marmoute-wip/mercurial/
#              hg pull http://hg.netv6.net/marmoute-wip/mercurial/ -r 330ed5e13627
destupdate: also include bookmark related logic

For the same reason, we move the bookmark related update logic into the
'destupdate' function. This requires to extend the returns of the function to
include the bookmark that needs to move (more or less) and the bookmark to
activate at the end of the function. See function documentation for details on
this returns.
Augie Fackler - Oct. 13, 2015, 3:28 p.m.
On Mon, Oct 12, 2015 at 10:22:55AM -0700, Pierre-Yves David wrote:
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@fb.com>
> # Date 1443513806 25200
> #      Tue Sep 29 01:03:26 2015 -0700
> # Node ID 330ed5e136273f627695777702a77ab695bc62e2
> # Parent  976e836aa9fcc99d05a462afef5007351e5226a1
> # EXP-Topic defaultdest
> # Available At http://hg.netv6.net/marmoute-wip/mercurial/
> #              hg pull http://hg.netv6.net/marmoute-wip/mercurial/ -r 330ed5e13627
> destupdate: also include bookmark related logic
>
> For the same reason, we move the bookmark related update logic into the
> 'destupdate' function. This requires to extend the returns of the function to
> include the bookmark that needs to move (more or less) and the bookmark to
> activate at the end of the function. See function documentation for details on
> this returns.
>
> diff --git a/mercurial/commands.py b/mercurial/commands.py
> --- a/mercurial/commands.py
> +++ b/mercurial/commands.py
> @@ -6547,10 +6547,11 @@ def update(ui, repo, node=None, rev=None
>
>      See :hg:`help dates` for a list of formats valid for -d/--date.
>
>      Returns 0 on success, 1 if there are unresolved files.
>      """
> +    movemarkfrom = None
>      if rev and node:
>          raise error.Abort(_("please specify just one revision"))
>
>      if rev is None or rev == '':
>          rev = node
> @@ -6562,13 +6563,10 @@ def update(ui, repo, node=None, rev=None
>          if date:
>              if rev is not None:
>                  raise error.Abort(_("you can't specify a revision and a date"))
>              rev = cmdutil.finddate(ui, repo, date)
>
> -        # with no argument, we also move the active bookmark, if any
> -        rev, movemarkfrom = bookmarks.calculateupdate(ui, repo, rev)
> -
>          # if we defined a bookmark, we have to remember the original name
>          brev = rev
>          rev = scmutil.revsingle(repo, rev, rev).rev()
>
>          if check and clean:
> @@ -6576,11 +6574,12 @@ def update(ui, repo, node=None, rev=None
>                               )
>
>          if check:
>              cmdutil.bailifchanged(repo, merge=False)
>          if rev is None:
> -            rev = destutil.destupdate(repo, clean=clean, check=check)
> +            updata =  destutil.destupdate(repo, clean=clean, check=check)
> +            rev, movemarkfrom, brev = updata
>
>          repo.ui.setconfig('ui', 'forcemerge', tool, 'update')
>
>          if clean:
>              ret = hg.clean(repo, rev)
> diff --git a/mercurial/destutil.py b/mercurial/destutil.py
> --- a/mercurial/destutil.py
> +++ b/mercurial/destutil.py
> @@ -5,29 +5,42 @@
>  # This software may be used and distributed according to the terms of the
>  # GNU General Public License version 2 or any later version.
>
>  from .i18n import _
>  from . import (
> +    bookmarks,
>      error,
>      obsolete,
>  )
>
>  def destupdate(repo, clean=False, check=False):
>      """destination for bare update operation
> +
> +    return (rev, ancestor, movemark, activemark)
> +
> +    - rev: the revision to update to,
> +    - a node to move the active bookmark from if it is there,
> +    - a bookmark to activate at the end of the update.

You've documented this as returning a 4-tuple, but it returns a 3-tuple.

I think, based on skimming the code, what should be here is this:

"""Destination for bare update operations.

Returns a tuple of (rev, movemark, activemark), where
  rev is the revision to update to
  movemark is ????
  activemark is always None
"""

Which leaves me with more questions than answers, I'm afraid. :(

>      """
> -    # Here is where we should consider bookmarks, divergent bookmarks, and tip
> -    # of current branch; but currently we are only checking the branch tips.
>      node = None
>      wc = repo[None]
>      p1 = wc.p1()
> -    try:
> -        node = repo.branchtip(wc.branch())
> -    except error.RepoLookupError:
> -        if wc.branch() == 'default': # no default branch!
> -            node = repo.lookup('tip') # update to tip
> -        else:
> -            raise error.Abort(_("branch %s not found") % wc.branch())
> +    activemark = None
> +
> +    # we also move the active bookmark, if any
> +    node, movemark = bookmarks.calculateupdate(repo.ui, repo, None)
> +    if node is not None:
> +        activemark = node
> +
> +    if node is None:
> +        try:
> +            node = repo.branchtip(wc.branch())
> +        except error.RepoLookupError:
> +            if wc.branch() == 'default': # no default branch!
> +                node = repo.lookup('tip') # update to tip
> +            else:
> +                raise error.Abort(_("branch %s not found") % wc.branch())
>
>      if p1.obsolete() and not p1.children():
>          # allow updating to successors
>          successors = obsolete.successorssets(repo, p1.node())
>
> @@ -74,6 +87,6 @@ def destupdate(repo, clean=False, check=
>                  elif not check:  # destination is not a descendant.
>                      msg = _("not a linear update")
>                      hint = _("merge or update --check to force update")
>                      raise error.Abort(msg, hint=hint)
>
> -    return rev
> +    return rev, movemark, activemark
> diff --git a/mercurial/merge.py b/mercurial/merge.py
> --- a/mercurial/merge.py
> +++ b/mercurial/merge.py
> @@ -998,11 +998,12 @@ def update(repo, node, branchmerge, forc
>          pas = [None]
>          if ancestor is not None:
>              pas = [repo[ancestor]]
>
>          if node is None:
> -            node = repo[destutil.destupdate(repo)].node()
> +            rev, _mark, _act = destutil.destupdate(repo)
> +            node = repo[rev].node()
>
>          overwrite = force and not branchmerge
>
>          p2 = repo[node]
>          if pas[0] is None:
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> https://selenic.com/mailman/listinfo/mercurial-devel
Pierre-Yves David - Oct. 13, 2015, 3:58 p.m.
On 10/13/2015 08:28 AM, Augie Fackler wrote:
> On Mon, Oct 12, 2015 at 10:22:55AM -0700, Pierre-Yves David wrote:
>> # HG changeset patch
>> # User Pierre-Yves David <pierre-yves.david@fb.com>
>> # Date 1443513806 25200
>> #      Tue Sep 29 01:03:26 2015 -0700
>> # Node ID 330ed5e136273f627695777702a77ab695bc62e2
>> # Parent  976e836aa9fcc99d05a462afef5007351e5226a1
>> # EXP-Topic defaultdest
>> # Available At http://hg.netv6.net/marmoute-wip/mercurial/
>> #              hg pull http://hg.netv6.net/marmoute-wip/mercurial/ -r 330ed5e13627
>> destupdate: also include bookmark related logic
>>
>> For the same reason, we move the bookmark related update logic into the
>> 'destupdate' function. This requires to extend the returns of the function to
>> include the bookmark that needs to move (more or less) and the bookmark to
>> activate at the end of the function. See function documentation for details on
>> this returns.
>>
>> diff --git a/mercurial/commands.py b/mercurial/commands.py
>> --- a/mercurial/commands.py
>> +++ b/mercurial/commands.py
>> @@ -6547,10 +6547,11 @@ def update(ui, repo, node=None, rev=None
>>
>>       See :hg:`help dates` for a list of formats valid for -d/--date.
>>
>>       Returns 0 on success, 1 if there are unresolved files.
>>       """
>> +    movemarkfrom = None
>>       if rev and node:
>>           raise error.Abort(_("please specify just one revision"))
>>
>>       if rev is None or rev == '':
>>           rev = node
>> @@ -6562,13 +6563,10 @@ def update(ui, repo, node=None, rev=None
>>           if date:
>>               if rev is not None:
>>                   raise error.Abort(_("you can't specify a revision and a date"))
>>               rev = cmdutil.finddate(ui, repo, date)
>>
>> -        # with no argument, we also move the active bookmark, if any
>> -        rev, movemarkfrom = bookmarks.calculateupdate(ui, repo, rev)
>> -
>>           # if we defined a bookmark, we have to remember the original name
>>           brev = rev
>>           rev = scmutil.revsingle(repo, rev, rev).rev()
>>
>>           if check and clean:
>> @@ -6576,11 +6574,12 @@ def update(ui, repo, node=None, rev=None
>>                                )
>>
>>           if check:
>>               cmdutil.bailifchanged(repo, merge=False)
>>           if rev is None:
>> -            rev = destutil.destupdate(repo, clean=clean, check=check)
>> +            updata =  destutil.destupdate(repo, clean=clean, check=check)
>> +            rev, movemarkfrom, brev = updata
>>
>>           repo.ui.setconfig('ui', 'forcemerge', tool, 'update')
>>
>>           if clean:
>>               ret = hg.clean(repo, rev)
>> diff --git a/mercurial/destutil.py b/mercurial/destutil.py
>> --- a/mercurial/destutil.py
>> +++ b/mercurial/destutil.py
>> @@ -5,29 +5,42 @@
>>   # This software may be used and distributed according to the terms of the
>>   # GNU General Public License version 2 or any later version.
>>
>>   from .i18n import _
>>   from . import (
>> +    bookmarks,
>>       error,
>>       obsolete,
>>   )
>>
>>   def destupdate(repo, clean=False, check=False):
>>       """destination for bare update operation
>> +
>> +    return (rev, ancestor, movemark, activemark)
>> +
>> +    - rev: the revision to update to,
>> +    - a node to move the active bookmark from if it is there,
>> +    - a bookmark to activate at the end of the update.
>
> You've documented this as returning a 4-tuple, but it returns a 3-tuple.
>
> I think, based on skimming the code, what should be here is this:
>
> """Destination for bare update operations.
>
> Returns a tuple of (rev, movemark, activemark), where
>    rev is the revision to update to
>    movemark is ????
>    activemark is always None
> """

Ha yes, the ancestors is the remain of an old memory.

I've documented movemark in this patch
(second item), not sure what you want more. You can check 
m.bookmarks.calculate update for more details about movemark.
I would be happy to be able to move forward here.
Augie Fackler - Oct. 13, 2015, 5:10 p.m.
On Tue, Oct 13, 2015 at 08:58:51AM -0700, Pierre-Yves David wrote:
>
>
> On 10/13/2015 08:28 AM, Augie Fackler wrote:
> >On Mon, Oct 12, 2015 at 10:22:55AM -0700, Pierre-Yves David wrote:

[snip]

> >>  def destupdate(repo, clean=False, check=False):
> >>      """destination for bare update operation
> >>+
> >>+    return (rev, ancestor, movemark, activemark)
> >>+
> >>+    - rev: the revision to update to,
> >>+    - a node to move the active bookmark from if it is there,
> >>+    - a bookmark to activate at the end of the update.
> >
> >You've documented this as returning a 4-tuple, but it returns a 3-tuple.
> >
> >I think, based on skimming the code, what should be here is this:
> >
> >"""Destination for bare update operations.
> >
> >Returns a tuple of (rev, movemark, activemark), where
> >   rev is the revision to update to
> >   movemark is ????
> >   activemark is always None
> >"""
>
> Ha yes, the ancestors is the remain of an old memory.
>
> I've documented movemark in this patch
> (second item), not sure what you want more. You can check
> m.bookmarks.calculate update for more details about movemark.
> I would be happy to be able to move forward here.

I have no idea how to interpret what you wrote. Please try again in
this thread so we can work it out before you do a v3. Perhaps the text
in the docstring should include a reference to whatever
m.bookmarks.calculate is.

>
> --
> Pierre-Yves David
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> https://selenic.com/mailman/listinfo/mercurial-devel
Pierre-Yves David - Oct. 13, 2015, 5:33 p.m.
On 10/13/2015 10:10 AM, Augie Fackler wrote:
> On Tue, Oct 13, 2015 at 08:58:51AM -0700, Pierre-Yves David wrote:
>>
>>
>> On 10/13/2015 08:28 AM, Augie Fackler wrote:
>>> On Mon, Oct 12, 2015 at 10:22:55AM -0700, Pierre-Yves David wrote:
>
> [snip]
>
>>>>   def destupdate(repo, clean=False, check=False):
>>>>       """destination for bare update operation
>>>> +
>>>> +    return (rev, ancestor, movemark, activemark)
>>>> +
>>>> +    - rev: the revision to update to,
>>>> +    - a node to move the active bookmark from if it is there,
>>>> +    - a bookmark to activate at the end of the update.
>>>
>>> You've documented this as returning a 4-tuple, but it returns a 3-tuple.
>>>
>>> I think, based on skimming the code, what should be here is this:
>>>
>>> """Destination for bare update operations.
>>>
>>> Returns a tuple of (rev, movemark, activemark), where
>>>    rev is the revision to update to
>>>    movemark is ????
>>>    activemark is always None
>>> """
>>
>> Ha yes, the ancestors is the remain of an old memory.
>>
>> I've documented movemark in this patch
>> (second item), not sure what you want more. You can check
>> m.bookmarks.calculate update for more details about movemark.
>> I would be happy to be able to move forward here.
>
> I have no idea how to interpret what you wrote. Please try again in
> this thread so we can work it out before you do a v3. Perhaps the text
> in the docstring should include a reference to whatever
> m.bookmarks.calculate is.

return (rev, ancestor, movemark, activemark)
- rev: the revision to update to,
- movemark: node to move the active bookmark from
            (cf bookmark.calculate update),
- activemark: a bookmark to activate at the end of the update.


#Iamjustmovingcodearound
Augie Fackler - Oct. 13, 2015, 6:24 p.m.
On Tue, Oct 13, 2015 at 1:33 PM, Pierre-Yves David
<pierre-yves.david@ens-lyon.org> wrote:
>
>
> On 10/13/2015 10:10 AM, Augie Fackler wrote:
>>
>> On Tue, Oct 13, 2015 at 08:58:51AM -0700, Pierre-Yves David wrote:
>>>
>>>
>>>
>>> On 10/13/2015 08:28 AM, Augie Fackler wrote:
>>>>
>>>> On Mon, Oct 12, 2015 at 10:22:55AM -0700, Pierre-Yves David wrote:
>>
>>
>> [snip]
>>
>>>>>   def destupdate(repo, clean=False, check=False):
>>>>>       """destination for bare update operation
>>>>> +
>>>>> +    return (rev, ancestor, movemark, activemark)
>>>>> +
>>>>> +    - rev: the revision to update to,
>>>>> +    - a node to move the active bookmark from if it is there,
>>>>> +    - a bookmark to activate at the end of the update.
>>>>
>>>>
>>>> You've documented this as returning a 4-tuple, but it returns a 3-tuple.
>>>>
>>>> I think, based on skimming the code, what should be here is this:
>>>>
>>>> """Destination for bare update operations.
>>>>
>>>> Returns a tuple of (rev, movemark, activemark), where
>>>>    rev is the revision to update to
>>>>    movemark is ????
>>>>    activemark is always None
>>>> """
>>>
>>>
>>> Ha yes, the ancestors is the remain of an old memory.
>>>
>>> I've documented movemark in this patch
>>> (second item), not sure what you want more. You can check
>>> m.bookmarks.calculate update for more details about movemark.
>>> I would be happy to be able to move forward here.
>>
>>
>> I have no idea how to interpret what you wrote. Please try again in
>> this thread so we can work it out before you do a v3. Perhaps the text
>> in the docstring should include a reference to whatever
>> m.bookmarks.calculate is.
>
>
> return (rev, ancestor, movemark, activemark)

You return a 3-tuple, not a 4-tuple. What is ancestor doing here?
Where is it coming from?

> - rev: the revision to update to,
> - movemark: node to move the active bookmark from
>            (cf bookmark.calculate update),
> - activemark: a bookmark to activate at the end of the update.

This looks fine.

> #Iamjustmovingcodearound

And you get to leave the campsite cleaner than you found it!

>
>
>
> --
> Pierre-Yves David
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> https://selenic.com/mailman/listinfo/mercurial-devel

Patch

diff --git a/mercurial/commands.py b/mercurial/commands.py
--- a/mercurial/commands.py
+++ b/mercurial/commands.py
@@ -6547,10 +6547,11 @@  def update(ui, repo, node=None, rev=None
 
     See :hg:`help dates` for a list of formats valid for -d/--date.
 
     Returns 0 on success, 1 if there are unresolved files.
     """
+    movemarkfrom = None
     if rev and node:
         raise error.Abort(_("please specify just one revision"))
 
     if rev is None or rev == '':
         rev = node
@@ -6562,13 +6563,10 @@  def update(ui, repo, node=None, rev=None
         if date:
             if rev is not None:
                 raise error.Abort(_("you can't specify a revision and a date"))
             rev = cmdutil.finddate(ui, repo, date)
 
-        # with no argument, we also move the active bookmark, if any
-        rev, movemarkfrom = bookmarks.calculateupdate(ui, repo, rev)
-
         # if we defined a bookmark, we have to remember the original name
         brev = rev
         rev = scmutil.revsingle(repo, rev, rev).rev()
 
         if check and clean:
@@ -6576,11 +6574,12 @@  def update(ui, repo, node=None, rev=None
                              )
 
         if check:
             cmdutil.bailifchanged(repo, merge=False)
         if rev is None:
-            rev = destutil.destupdate(repo, clean=clean, check=check)
+            updata =  destutil.destupdate(repo, clean=clean, check=check)
+            rev, movemarkfrom, brev = updata
 
         repo.ui.setconfig('ui', 'forcemerge', tool, 'update')
 
         if clean:
             ret = hg.clean(repo, rev)
diff --git a/mercurial/destutil.py b/mercurial/destutil.py
--- a/mercurial/destutil.py
+++ b/mercurial/destutil.py
@@ -5,29 +5,42 @@ 
 # This software may be used and distributed according to the terms of the
 # GNU General Public License version 2 or any later version.
 
 from .i18n import _
 from . import (
+    bookmarks,
     error,
     obsolete,
 )
 
 def destupdate(repo, clean=False, check=False):
     """destination for bare update operation
+
+    return (rev, ancestor, movemark, activemark)
+
+    - rev: the revision to update to,
+    - a node to move the active bookmark from if it is there,
+    - a bookmark to activate at the end of the update.
     """
-    # Here is where we should consider bookmarks, divergent bookmarks, and tip
-    # of current branch; but currently we are only checking the branch tips.
     node = None
     wc = repo[None]
     p1 = wc.p1()
-    try:
-        node = repo.branchtip(wc.branch())
-    except error.RepoLookupError:
-        if wc.branch() == 'default': # no default branch!
-            node = repo.lookup('tip') # update to tip
-        else:
-            raise error.Abort(_("branch %s not found") % wc.branch())
+    activemark = None
+
+    # we also move the active bookmark, if any
+    node, movemark = bookmarks.calculateupdate(repo.ui, repo, None)
+    if node is not None:
+        activemark = node
+
+    if node is None:
+        try:
+            node = repo.branchtip(wc.branch())
+        except error.RepoLookupError:
+            if wc.branch() == 'default': # no default branch!
+                node = repo.lookup('tip') # update to tip
+            else:
+                raise error.Abort(_("branch %s not found") % wc.branch())
 
     if p1.obsolete() and not p1.children():
         # allow updating to successors
         successors = obsolete.successorssets(repo, p1.node())
 
@@ -74,6 +87,6 @@  def destupdate(repo, clean=False, check=
                 elif not check:  # destination is not a descendant.
                     msg = _("not a linear update")
                     hint = _("merge or update --check to force update")
                     raise error.Abort(msg, hint=hint)
 
-    return rev
+    return rev, movemark, activemark
diff --git a/mercurial/merge.py b/mercurial/merge.py
--- a/mercurial/merge.py
+++ b/mercurial/merge.py
@@ -998,11 +998,12 @@  def update(repo, node, branchmerge, forc
         pas = [None]
         if ancestor is not None:
             pas = [repo[ancestor]]
 
         if node is None:
-            node = repo[destutil.destupdate(repo)].node()
+            rev, _mark, _act = destutil.destupdate(repo)
+            node = repo[rev].node()
 
         overwrite = force and not branchmerge
 
         p2 = repo[node]
         if pas[0] is None: