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

login
register
mail settings
Submitter Pierre-Yves David
Date Oct. 8, 2015, 9:42 p.m.
Message ID <725812283ac94ac10c38.1444340537@marginatus.alto.octopoid.net>
Download mbox | patch
Permalink /patch/10895/
State Superseded
Commit 5c57d01fe64e32fb3b3e70943abdc40960851690
Headers show

Comments

Pierre-Yves David - Oct. 8, 2015, 9:42 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 725812283ac94ac10c38ba1112b0857191a1f621
# Parent  18cba2544597d954fde95ee36f6450faab57fcbc
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.
Augie Fackler - Oct. 9, 2015, 3:06 p.m.
On Thu, Oct 08, 2015 at 02:42:17PM -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 725812283ac94ac10c38ba1112b0857191a1f621
> # Parent  18cba2544597d954fde95ee36f6450faab57fcbc
> 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.

I'm not sure what this sentence is trying to say (the "more or less"
is confusing me). I think what you mean is:

This requires destupdate also returning the name of any bookmark that
needs to move, and the bookmark to activiate after the function.

Is that right?

>
> diff --git a/mercurial/commands.py b/mercurial/commands.py
> --- a/mercurial/commands.py
> +++ b/mercurial/commands.py
> @@ -6543,10 +6543,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 util.Abort(_("please specify just one revision"))
>
>      if rev is None or rev == '':
>          rev = node
> @@ -6558,24 +6559,22 @@ def update(ui, repo, node=None, rev=None
>          if date:
>              if rev is not None:
>                  raise util.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:
>              raise util.Abort(_("cannot specify both -c/--check and -C/--clean"))
>
>          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,30 +5,43 @@
>  # 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,
>      util,
>      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 bookmark moved by the update
> +    - a bookmark activated by 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 util.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 util.Abort(_("branch %s not found") % wc.branch())
>
>      if p1.obsolete() and not p1.children():
>          # allow updating to successors
>          successors = obsolete.successorssets(repo, p1.node())
>
> @@ -75,6 +88,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 util.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
> @@ -993,11 +993,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. 9, 2015, 7:52 p.m.
On 10/09/2015 08:06 AM, Augie Fackler wrote:
> On Thu, Oct 08, 2015 at 02:42:17PM -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 725812283ac94ac10c38ba1112b0857191a1f621
>> # Parent  18cba2544597d954fde95ee36f6450faab57fcbc
>> 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.
>
> I'm not sure what this sentence is trying to say (the "more or less"
> is confusing me). I think what you mean is:
>
> This requires destupdate also returning the name of any bookmark that
> needs to move, and the bookmark to activiate after the function.
>
> Is that right?

No, What we actually return here is:

1) A node
2) A bookmark name

If after the update, the active bookmark is on the node (1), we move it 
to the new location.

And if after the update the bookmark name is under the current location, 
it should be activated.

Or in summary: http://imgur.com/ES8wIja

This is how bookmark have been doing it for the last 4 years and it is 
not related to the goal to this series so I did not looked into clearing 
this up.

I tried to stay simple in the description so I describe something 
sensible with a (more or less) tune.
Augie Fackler - Oct. 12, 2015, 5:02 p.m.
On Fri, Oct 9, 2015 at 3:52 PM, Pierre-Yves David
<pierre-yves.david@ens-lyon.org> wrote:
>
>
> On 10/09/2015 08:06 AM, Augie Fackler wrote:
>>
>> On Thu, Oct 08, 2015 at 02:42:17PM -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 725812283ac94ac10c38ba1112b0857191a1f621
>>> # Parent  18cba2544597d954fde95ee36f6450faab57fcbc
>>> 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.
>>
>>
>> I'm not sure what this sentence is trying to say (the "more or less"
>> is confusing me). I think what you mean is:
>>
>> This requires destupdate also returning the name of any bookmark that
>> needs to move, and the bookmark to activiate after the function.
>>
>> Is that right?
>
>
> No, What we actually return here is:
>
> 1) A node
> 2) A bookmark name
>
> If after the update, the active bookmark is on the node (1), we move it to
> the new location.
>
> And if after the update the bookmark name is under the current location, it
> should be activated.
>
> Or in summary: http://imgur.com/ES8wIja

Please document this madness for future readers.

>
> This is how bookmark have been doing it for the last 4 years and it is not
> related to the goal to this series so I did not looked into clearing this
> up.
>
> I tried to stay simple in the description so I describe something sensible
> with a (more or less) tune.
>
>
>
> --
> Pierre-Yves David
Pierre-Yves David - Oct. 12, 2015, 5:22 p.m.
On 10/12/2015 10:02 AM, Augie Fackler wrote:
> On Fri, Oct 9, 2015 at 3:52 PM, Pierre-Yves David
> <pierre-yves.david@ens-lyon.org> wrote:
>>
>>
>> On 10/09/2015 08:06 AM, Augie Fackler wrote:
>>>
>>> On Thu, Oct 08, 2015 at 02:42:17PM -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 725812283ac94ac10c38ba1112b0857191a1f621
>>>> # Parent  18cba2544597d954fde95ee36f6450faab57fcbc
>>>> 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.
>>>
>>>
>>> I'm not sure what this sentence is trying to say (the "more or less"
>>> is confusing me). I think what you mean is:
>>>
>>> This requires destupdate also returning the name of any bookmark that
>>> needs to move, and the bookmark to activiate after the function.
>>>
>>> Is that right?
>>
>>
>> No, What we actually return here is:
>>
>> 1) A node
>> 2) A bookmark name
>>
>> If after the update, the active bookmark is on the node (1), we move it to
>> the new location.
>>
>> And if after the update the bookmark name is under the current location, it
>> should be activated.
>>
>> Or in summary: http://imgur.com/ES8wIja
>
> Please document this madness for future readers.

I've updated the function docstring and pointed to it in the changesets 
description. V2 coming.

Patch

diff --git a/mercurial/commands.py b/mercurial/commands.py
--- a/mercurial/commands.py
+++ b/mercurial/commands.py
@@ -6543,10 +6543,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 util.Abort(_("please specify just one revision"))
 
     if rev is None or rev == '':
         rev = node
@@ -6558,24 +6559,22 @@  def update(ui, repo, node=None, rev=None
         if date:
             if rev is not None:
                 raise util.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:
             raise util.Abort(_("cannot specify both -c/--check and -C/--clean"))
 
         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,30 +5,43 @@ 
 # 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,
     util,
     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 bookmark moved by the update
+    - a bookmark activated by 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 util.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 util.Abort(_("branch %s not found") % wc.branch())
 
     if p1.obsolete() and not p1.children():
         # allow updating to successors
         successors = obsolete.successorssets(repo, p1.node())
 
@@ -75,6 +88,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 util.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
@@ -993,11 +993,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: