Patchwork [V3] destutil: choose non-closed branch head at first (BC)

login
register
mail settings
Submitter Katsunori FUJIWARA
Date March 8, 2016, 10:59 a.m.
Message ID <4c53d1402e9d641f9b9f.1457434788@feefifofum>
Download mbox | patch
Permalink /patch/13672/
State Accepted
Headers show

Comments

Katsunori FUJIWARA - March 8, 2016, 10:59 a.m.
# HG changeset patch
# User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
# Date 1457288059 -32400
#      Mon Mar 07 03:14:19 2016 +0900
# Node ID 4c53d1402e9d641f9b9f99ac6114e1f13e51fc45
# Parent  ffd3ac07b1d79dda7f57bd826208fdaf92a76717
destutil: choose non-closed branch head at first (BC)

Before this patch, destupdate() returns the tipmost (descendant)
branch head regardless of closed or not. But updating to closed branch
head isn't reasonable for ordinary workflow, because:

  - "hg heads" doesn't show closed heads (= updated parent itself) by
    default

  - subsequent committing on it re-opens closed branch

    even if inactivation of closed head is needed, update destination
    isn't it, because it should be merged into to another branch in
    such case.

This patch chooses non-closed descendant branch head as default update
destination at first. If all descendant branch heads are closed,
destupdate() returns the tipmost closed branch head.

For simplicity, this patch chooses adding _destupdatebranchfallback()
instead largely changing _destupdatebranch().

This patch changes not only normal lookup code path, but also "no
default branch" code path, for consistency.
Augie Fackler - March 8, 2016, 3:05 p.m.
On Tue, Mar 08, 2016 at 07:59:48PM +0900, FUJIWARA Katsunori wrote:
> # HG changeset patch
> # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
> # Date 1457288059 -32400
> #      Mon Mar 07 03:14:19 2016 +0900
> # Node ID 4c53d1402e9d641f9b9f99ac6114e1f13e51fc45
> # Parent  ffd3ac07b1d79dda7f57bd826208fdaf92a76717
> destutil: choose non-closed branch head at first (BC)

Queued with some spelling fixes I spotted. Thanks!
Matt Mackall - March 22, 2016, 8:54 p.m.
On Tue, 2016-03-08 at 19:59 +0900, FUJIWARA Katsunori wrote:
> # HG changeset patch
> # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
> # Date 1457288059 -32400
> #      Mon Mar 07 03:14:19 2016 +0900
> # Node ID 4c53d1402e9d641f9b9f99ac6114e1f13e51fc45
> # Parent  ffd3ac07b1d79dda7f57bd826208fdaf92a76717
> destutil: choose non-closed branch head at first (BC)
> 
> Before this patch, destupdate() returns the tipmost (descendant)
> branch head regardless of closed or not. But updating to closed branch
> head isn't reasonable for ordinary workflow, because:
> 
>   - "hg heads" doesn't show closed heads (= updated parent itself) by
>     default
> 
>   - subsequent committing on it re-opens closed branch
> 
>     even if inactivation of closed head is needed, update destination
>     isn't it, because it should be merged into to another branch in
>     such case.
> 
> This patch chooses non-closed descendant branch head as default update
> destination at first. If all descendant branch heads are closed,
> destupdate() returns the tipmost closed branch head.
> 
> For simplicity, this patch chooses adding _destupdatebranchfallback()
> instead largely changing _destupdatebranch().
> 
> This patch changes not only normal lookup code path, but also "no
> default branch" code path, for consistency.

A couple notes here:

- test-convert-mtn.t needs updating

> +  updated to a closed branch head, because all descendant heads are closed.
> +  beware of re-opening closed head by subsequent commit here.
> +  1 other heads for branch "default"

This is a really long message and not really in our normal format. We should be
aiming for:

 one line description of the problem with no period
 (a suggestion about how to move forward or get more info)

Something like:

 no open branch heads, updating to a closed head
 (committing will reopen the branch)

We also might consider making it a warning.

-- 
Mathematics is the supreme nostalgia of our time.
Katsunori FUJIWARA - March 24, 2016, 7:44 a.m.
At Tue, 22 Mar 2016 13:54:32 -0700,
Matt Mackall wrote:
> 
> On Tue, 2016-03-08 at 19:59 +0900, FUJIWARA Katsunori wrote:
> > # HG changeset patch
> > # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
> > # Date 1457288059 -32400
> > #      Mon Mar 07 03:14:19 2016 +0900
> > # Node ID 4c53d1402e9d641f9b9f99ac6114e1f13e51fc45
> > # Parent  ffd3ac07b1d79dda7f57bd826208fdaf92a76717
> > destutil: choose non-closed branch head at first (BC)
> > 
> > Before this patch, destupdate() returns the tipmost (descendant)
> > branch head regardless of closed or not. But updating to closed branch
> > head isn't reasonable for ordinary workflow, because:
> > 
> >   - "hg heads" doesn't show closed heads (= updated parent itself) by
> >     default
> > 
> >   - subsequent committing on it re-opens closed branch
> > 
> >     even if inactivation of closed head is needed, update destination
> >     isn't it, because it should be merged into to another branch in
> >     such case.
> > 
> > This patch chooses non-closed descendant branch head as default update
> > destination at first. If all descendant branch heads are closed,
> > destupdate() returns the tipmost closed branch head.
> > 
> > For simplicity, this patch chooses adding _destupdatebranchfallback()
> > instead largely changing _destupdatebranch().
> > 
> > This patch changes not only normal lookup code path, but also "no
> > default branch" code path, for consistency.
> 
> A couple notes here:
> 
> - test-convert-mtn.t needs updating

Oh, I overlooked it, sorry. I'll send patch for it.


> > +  updated to a closed branch head, because all descendant heads are closed.
> > +  beware of re-opening closed head by subsequent commit here.
> > +  1 other heads for branch "default"
> 
> This is a really long message and not really in our normal format. We should be
> aiming for:
> 
>  one line description of the problem with no period
>  (a suggestion about how to move forward or get more info)
> 
> Something like:
> 
>  no open branch heads, updating to a closed head
>  (committing will reopen the branch)
>
> We also might consider making it a warning.

I see. I'll send the series, which will revise message and make it a
warning.


> -- 
> Mathematics is the supreme nostalgia of our time.
> 
> 

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

Patch

diff --git a/mercurial/destutil.py b/mercurial/destutil.py
--- a/mercurial/destutil.py
+++ b/mercurial/destutil.py
@@ -88,30 +88,55 @@  def _destupdatebook(repo, clean, check):
     return node, movemark, activemark
 
 def _destupdatebranch(repo, clean, check):
-    """decide on an update destination from current branch"""
+    """decide on an update destination from current branch
+
+    This ignores closd branch head.
+    """
     wc = repo[None]
     movemark = node = None
     currentbranch = wc.branch()
     if currentbranch in repo.branchmap():
-        heads = repo.branchheads(currentbranch, closed=True)
+        heads = repo.branchheads(currentbranch)
         if heads:
             node = repo.revs('max(.::(%ln))', heads).first()
         if bookmarks.isactivewdirparent(repo):
             movemark = repo['.'].node()
     else:
         if currentbranch == 'default': # no default branch!
-            node = repo.lookup('tip') # update to tip
+            # update to the tipmost non-closed branch head
+            node = repo.revs('max(head() and not closed())').first()
         else:
             raise error.Abort(_("branch %s not found") % currentbranch)
     return node, movemark, None
 
+def _destupdatebranchfallback(repo, clean, check):
+    """decide on an update destination from closed heads in current branch"""
+    wc = repo[None]
+    currentbranch = wc.branch()
+    movemark = None
+    if currentbranch in repo.branchmap():
+        # here, all descendant branch heads are closed
+        heads = repo.branchheads(currentbranch, closed=True)
+        assert heads, "any branch has at least one head"
+        node = repo.revs('max(.::(%ln))', heads).first()
+        assert node is not None, ("any revision has at least "
+                                  "one descendant branch head")
+        if bookmarks.isactivewdirparent(repo):
+            movemark = repo['.'].node()
+    else:
+        # here, no "default" branch, and all branches are closed
+        node = repo.lookup('tip')
+        assert node is not None, "'tip' exists even in empty repository"
+    return node, movemark, None
+
 # order in which each step should be evalutated
 # steps are run until one finds a destination
-destupdatesteps = ['evolution', 'bookmark', 'branch']
+destupdatesteps = ['evolution', 'bookmark', 'branch', 'branchfallback']
 # mapping to ease extension overriding steps.
 destupdatestepmap = {'evolution': _destupdateobs,
                      'bookmark': _destupdatebook,
                      'branch': _destupdatebranch,
+                     'branchfallback': _destupdatebranchfallback,
                      }
 
 def destupdate(repo, clean=False, check=False):
@@ -362,8 +387,27 @@  def _statusotherbranchheads(ui, repo):
     heads = repo.branchheads(currentbranch)
     if repo.revs('%ln and parents()', allheads):
         # we are on a head, even though it might be closed
+        #
+        #  on closed otherheads
+        #  ========= ==========
+        #      o        0       all heads for current branch are closed
+        #               N       only descendant branch heads are closed
+        #      x        0       there is only one non-closed branch head
+        #               N       there are some non-closed branch heads
+        #  ========= ==========
         otherheads = repo.revs('%ln - parents()', heads)
-        if otherheads:
+        if repo['.'].closesbranch():
+            ui.status(_('updated to a closed branch head, '
+                        'because all descendant heads are closed.\n'
+                        'beware of re-opening closed head '
+                        'by subsequent commit here.\n'))
+            if otherheads:
+                ui.status(_('%i other heads for branch "%s"\n') %
+                          (len(otherheads), currentbranch))
+            else:
+                ui.status(_('all heads for branch "%s" are closed.\n') %
+                          currentbranch)
+        elif otherheads:
             ui.status(_('%i other heads for branch "%s"\n') %
                       (len(otherheads), currentbranch))
 
diff --git a/tests/test-bookmarks-current.t b/tests/test-bookmarks-current.t
--- a/tests/test-bookmarks-current.t
+++ b/tests/test-bookmarks-current.t
@@ -202,3 +202,22 @@  issue 4552 -- simulate a pull moving the
   Z
   $ hg log -T '{bookmarks % "{active}\n"}' -r Z
   Z
+
+test that updating to closed branch head also advances active bookmark
+
+  $ hg commit --close-branch -m "closed"
+  $ hg update -q ".^1"
+  $ hg bookmark Y
+  $ hg bookmarks
+     X                         3:4d6bd4bfb1ae
+   * Y                         3:4d6bd4bfb1ae
+     Z                         0:719295282060
+  $ hg update
+  0 files updated, 0 files merged, 0 files removed, 0 files unresolved
+  updating bookmark Y
+  $ hg bookmarks
+     X                         3:4d6bd4bfb1ae
+   * Y                         4:8fa964221e8e
+     Z                         0:719295282060
+  $ hg parents -q
+  4:8fa964221e8e
diff --git a/tests/test-update-branches.t b/tests/test-update-branches.t
--- a/tests/test-update-branches.t
+++ b/tests/test-update-branches.t
@@ -186,28 +186,111 @@  Test updating with closed head
 Test updating if at least one non-closed branch head exists
 
 if on the closed branch head:
-- updating is no-op
+- update to "."
+- "updated to a closed branch head ...." message is displayed
 - "N other heads for ...." message is displayed
 
   $ hg update -q -C 3
   $ hg commit --close-branch -m 6
   $ norevtest "on closed branch head" clean 6
   0 files updated, 0 files merged, 0 files removed, 0 files unresolved
+  updated to a closed branch head, because all descendant heads are closed.
+  beware of re-opening closed head by subsequent commit here.
+  1 other heads for branch "default"
+  parent=6
+
+if descendant non-closed branch head exists, and it is only one branch head:
+- update to it, even if its revision is less than closed one
+- "N other heads for ...." message isn't displayed
+
+  $ norevtest "non-closed 2 should be chosen" clean 1
+  1 files updated, 0 files merged, 0 files removed, 0 files unresolved
+  parent=2
+
+if all descendant branch heads are closed, but there is another branch head:
+- update to the tipmost descendant head
+- "updated to a closed branch head ...." message is displayed
+- "N other heads for ...." message is displayed
+
+  $ norevtest "all descendant branch heads are closed" clean 3
+  0 files updated, 0 files merged, 0 files removed, 0 files unresolved
+  updated to a closed branch head, because all descendant heads are closed.
+  beware of re-opening closed head by subsequent commit here.
   1 other heads for branch "default"
   parent=6
 
 Test updating if all branch heads are closed
 
 if on the closed branch head:
-- updating is no-op
-- "N other heads for ...." message isn't displayed
+- update to "."
+- "updated to a closed branch head ...." message is displayed
+- "all heads of branch ...." message is displayed
 
   $ hg update -q -C 2
   $ hg commit --close-branch -m 7
   $ norevtest "all heads of branch default are closed" clean 6
   0 files updated, 0 files merged, 0 files removed, 0 files unresolved
+  updated to a closed branch head, because all descendant heads are closed.
+  beware of re-opening closed head by subsequent commit here.
+  all heads for branch "default" are closed.
   parent=6
 
+if not on the closed branch head:
+- update to the tipmost descendant (closed) head
+- "updated to a closed branch head ...." message is displayed
+- "all heads of branch ...." message is displayed
+
+  $ norevtest "all heads of branch default are closed" clean 1
+  1 files updated, 0 files merged, 0 files removed, 0 files unresolved
+  updated to a closed branch head, because all descendant heads are closed.
+  beware of re-opening closed head by subsequent commit here.
+  all heads for branch "default" are closed.
+  parent=7
+
+  $ cd ..
+
+Test updating if "default" branch doesn't exist and no revision is
+checked out (= "default" is used as current branch)
+
+  $ hg init no-default-branch
+  $ cd no-default-branch
+
+  $ hg branch foobar
+  marked working directory as branch foobar
+  (branches are permanent and global, did you want a bookmark?)
+  $ echo a > a
+  $ hg commit -m "#0" -A
+  adding a
+  $ echo 1 >> a
+  $ hg commit -m "#1"
+  $ hg update -q 0
+  $ echo 3 >> a
+  $ hg commit -m "#2"
+  created new head
+  $ hg commit --close-branch -m "#3"
+
+if there is at least one non-closed branch head:
+- update to the tipmost branch head
+
+  $ norevtest "non-closed 1 should be chosen" clean null
+  1 files updated, 0 files merged, 0 files removed, 0 files unresolved
+  parent=1
+
+if all branch heads are closed
+- update to "tip"
+- "updated to a closed branch head ...." message is displayed
+- "all heads for branch "XXXX" are closed" message is displayed
+
+  $ hg update -q -C 1
+  $ hg commit --close-branch -m "#4"
+
+  $ norevtest "all branches are closed" clean null
+  1 files updated, 0 files merged, 0 files removed, 0 files unresolved
+  updated to a closed branch head, because all descendant heads are closed.
+  beware of re-opening closed head by subsequent commit here.
+  all heads for branch "foobar" are closed.
+  parent=4
+
   $ cd ../b1
 
 Test obsolescence behavior