[PATCH 1 of 3]: update: never move active bookmark

Matt Mackall mpm at selenic.com
Wed Nov 11 19:42:37 CST 2015


On Sun, 2015-11-08 at 01:17 +0100, Waldemar Kornewald wrote:
> # HG changeset patch
> # User Waldemar Kornewald <wkornewald>
> # Date 1446916310 -3600
> #      Sat Nov 07 18:11:50 2015 +0100
> # Node ID 7e94770574c7b1a62378c212c8fb338a4e0616b5
> # Parent  f9984f76fd90e439221425d751e29bae17bec995
> update: never move active bookmark
> 
> An "hg update" without args moved the active bookmark to tip, however
> tip has
> no useful meaning in the context of bookmarks.

This is not actually true. It currently tries to move to the tipmost
branch head of the current named branch. This was intentional and has a
meaning: "my bookmark is on a branch, the branch is ahead of me, catch
up with it".

That said, after a year-long debate about this behavior, we've actually
decided to move in the direction this patch is suggesting.

> Now we simply stay on the active bookmark (the "tip" of a bookmark is
> the
> bookmark itself).
> 
> This also fixes an issue with "hg pull --update":
> The active bookmark stayed active, but "hg id" would be tip.

Modulo your misusage of 'tip', this was also the currently intended
behavior.

> This was inconsistent with running "hg pull" followed by "hg update".

I believe you'll find that this is incorrect if you look more closely. 


However, this patch is too complex for our engineering best practices
and needs to be broken up into multiple simpler changes. For instance,
you've removed return parameters from multiple functions. These are
nice cleanups by themselves, but make it hard for a reviewer or
debugger or archaeologist to spot the chunk that actually does
$SUBJECT.

> diff -r f9984f76fd90 -r 7e94770574c7 mercurial/bookmarks.py
> --- a/mercurial/bookmarks.py Wed Nov 04 15:17:52 2015 -0600
> +++ b/mercurial/bookmarks.py Sat Nov 07 18:11:50 2015 +0100
> @@ -215,17 +215,13 @@
>      return deleted
> 
>  def calculateupdate(ui, repo, checkout):
> -    '''Return a tuple (targetrev, movemarkfrom) indicating the rev
> to
> -    check out and where to move the active bookmark from, if
> needed.'''
> -    movemarkfrom = None
> +    '''Return targetrev indicating the rev to check out.'''
>      if checkout is None:
>          activemark = repo._activebookmark
> -        if isactivewdirparent(repo):
> -            movemarkfrom = repo['.'].node()
> -        elif activemark:
> +        if activemark:
>              ui.status(_("updating to active bookmark %s\n") %
> activemark)
>              checkout = activemark
> -    return (checkout, movemarkfrom)
> +    return checkout
> 
>  def update(repo, parents, node):
>      deletefrom = parents
> diff -r f9984f76fd90 -r 7e94770574c7 mercurial/commands.py
> --- a/mercurial/commands.py Wed Nov 04 15:17:52 2015 -0600
> +++ b/mercurial/commands.py Sat Nov 07 18:11:50 2015 +0100
> @@ -5220,20 +5220,15 @@
>          return
>      if optupdate:
>          try:
> -            brev = checkout
> -            movemarkfrom = None
>              if not checkout:
> -                updata =  destutil.destupdate(repo)
> -                checkout, movemarkfrom, brev = updata
> +                updata = destutil.destupdate(repo)
> +                checkout, brev = updata
>              ret = hg.update(repo, checkout)
>          except error.UpdateAbort as inst:
>              ui.warn(_("not updating: %s\n") % str(inst))
>              if inst.hint:
>                  ui.warn(_("(%s)\n") % inst.hint)
>              return 0
> -        if not ret and not checkout:
> -            if bookmarks.update(repo, [movemarkfrom],
> repo['.'].node()):
> -                ui.status(_("updating bookmark %s\n") %
> repo._activebookmark)
>          return ret
>      if modheads > 1:
>          currentbranchheads = len(repo.branchheads())
> @@ -6613,9 +6608,8 @@
>      """update working directory (or switch revisions)
> 
>      Update the repository's working directory to the specified
> -    changeset. If no changeset is specified, update to the tip of
> the
> -    current named branch and move the active bookmark (see :hg:`help
> -    bookmarks`).
> +    changeset. If no changeset is specified, update to the active
> bookmark.
> +    If there is no active bookmark, update to the tip of the current
> named branch.
> 
>      Update sets the working directory's parent revision to the
> specified
>      changeset (see :hg:`help parents`).
> @@ -6659,7 +6653,6 @@
> 
>      Returns 0 on success, 1 if there are unresolved files.
>      """
> -    movemarkfrom = None
>      if rev and node:
>          raise error.Abort(_("please specify just one revision"))
> 
> @@ -6687,7 +6680,7 @@
>              cmdutil.bailifchanged(repo, merge=False)
>          if rev is None:
>              updata =  destutil.destupdate(repo, clean=clean,
> check=check)
> -            rev, movemarkfrom, brev = updata
> +            rev, brev = updata
> 
>          repo.ui.setconfig('ui', 'forcemerge', tool, 'update')
> 
> @@ -6696,17 +6689,7 @@
>          else:
>              ret = hg.update(repo, rev)
> 
> -        if not ret and movemarkfrom:
> -            if movemarkfrom == repo['.'].node():
> -                pass # no-op update
> -            elif bookmarks.update(repo, [movemarkfrom],
> repo['.'].node()):
> -                ui.status(_("updating bookmark %s\n") %
> repo._activebookmark)
> -            else:
> -                # this can happen with a non-linear update
> -                ui.status(_("(leaving bookmark %s)\n") %
> -                          repo._activebookmark)
> -                bookmarks.deactivate(repo)
> -        elif brev in repo._bookmarks:
> +        if brev in repo._bookmarks:
>              bookmarks.activate(repo, brev)
>              ui.status(_("(activating bookmark %s)\n") % brev)
>          elif brev:
> diff -r f9984f76fd90 -r 7e94770574c7 mercurial/destutil.py
> --- a/mercurial/destutil.py Wed Nov 04 15:17:52 2015 -0600
> +++ b/mercurial/destutil.py Sat Nov 07 18:11:50 2015 +0100
> @@ -45,7 +45,6 @@
>      node = None
>      wc = repo[None]
>      p1 = wc.p1()
> -    movemark = None
> 
>      if p1.obsolete() and not p1.children():
>          # allow updating to successors
> @@ -72,33 +71,23 @@
>              # get the max revision for the given successors set,
>              # i.e. the 'tip' of a set
>              node = repo.revs('max(%ln)', successors).first()
> -            if bookmarks.isactivewdirparent(repo):
> -                movemark = repo['.'].node()
> -    return node, movemark, None
> +    return node, None
> 
>  def _destupdatebook(repo, clean, check):
>      """decide on an update destination from active bookmark"""
> -    # we also move the active bookmark, if any
> -    activemark = None
> -    node, movemark = bookmarks.calculateupdate(repo.ui, repo, None)
> -    if node is not None:
> -        activemark = node
> -    return node, movemark, activemark
> +    return (bookmarks.calculateupdate(repo.ui, repo, None),) * 2
> 
>  def _destupdatebranch(repo, clean, check):
>      """decide on an update destination from current branch"""
>      wc = repo[None]
> -    movemark = node = None
>      try:
>          node = repo.branchtip(wc.branch())
> -        if bookmarks.isactivewdirparent(repo):
> -            movemark = repo['.'].node()
>      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())
> -    return node, movemark, None
> +    return node, None
> 
>  # order in which each step should be evalutated
>  # steps are run until one finds a destination
> @@ -115,21 +104,19 @@
>      return (rev, 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.
>      """
> -    node = movemark = activemark = None
> +    node = activemark = None
> 
>      for step in destupdatesteps:
> -        node, movemark, activemark = destupdatestepmap[step](repo,
> clean, check)
> +        node, activemark = destupdatestepmap[step](repo, clean,
> check)
>          if node is not None:
>              break
>      rev = repo[node].rev()
> 
>      _destupdatevalidate(repo, rev, clean, check)
> 
> -    return rev, movemark, activemark
> +    return rev, activemark
> 
>  def _destmergebook(repo):
>      """find merge destination in the active bookmark case"""
> diff -r f9984f76fd90 -r 7e94770574c7 mercurial/merge.py
> --- a/mercurial/merge.py Wed Nov 04 15:17:52 2015 -0600
> +++ b/mercurial/merge.py Sat Nov 07 18:11:50 2015 +0100
> @@ -1152,7 +1152,7 @@
>              if (repo.ui.configbool('devel', 'all-warnings')
>                      or repo.ui.configbool('devel', 'oldapi')):
>                  repo.ui.develwarn('update with no target')
> -            rev, _mark, _act = destutil.destupdate(repo)
> +            rev, _act = destutil.destupdate(repo)
>              node = repo[rev].node()
> 
>          overwrite = force and not branchmerge
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel at selenic.com
> https://selenic.com/mailman/listinfo/mercurial-devel
-- 
Mathematics is the supreme nostalgia of our time.



More information about the Mercurial-devel mailing list