Patchwork [6,of,6,RFC] destutil: choose non-closed branch head as default update destination (BC)

login
register
mail settings
Submitter Katsunori FUJIWARA
Date Feb. 24, 2016, 2:17 p.m.
Message ID <ae5d291a6dcb96683eae.1456323434@feefifofum>
Download mbox | patch
Permalink /patch/13346/
State Changes Requested
Delegated to: Pierre-Yves David
Headers show

Comments

Katsunori FUJIWARA - Feb. 24, 2016, 2:17 p.m.
# HG changeset patch
# User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
# Date 1456322433 -32400
#      Wed Feb 24 23:00:33 2016 +0900
# Node ID ae5d291a6dcb96683eae3c4e33575297fa113fbb
# Parent  1f9b3d38e380b018cbe10ed1acee882c014e3763
destutil: choose non-closed branch head as default update destination (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 branch head as default update
destination.

This patch changes not only normal lookup code path, but also "no
default branch" code path, for consistency.

If no appropriate default update destination is found, destupdate()
returns '.' as destination to work as no-op at updating. 'None' isn't
reasonable for this purpose, because:

  - repo[node] returns useless rev/node if node=None
  - merge.update() re-invokes destupdate() for None destination
Pierre-Yves David - Feb. 24, 2016, 11:19 p.m.
On 02/24/2016 03:17 PM, FUJIWARA Katsunori wrote:
> # HG changeset patch
> # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
> # Date 1456322433 -32400
> #      Wed Feb 24 23:00:33 2016 +0900
> # Node ID ae5d291a6dcb96683eae3c4e33575297fa113fbb
> # Parent  1f9b3d38e380b018cbe10ed1acee882c014e3763
> destutil: choose non-closed branch head as default update destination (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.

Excluding closed head from update makes a lot of sense to me. But we 
probably need some better messaging around the no-op update. I'm not 
exactly sure no-op update is the right choice if there is nothing but 
closed head above. However giving priority to non-closed head seems an 
obvious way to go here.

I need to think about the "no non-closed heads in .::" case a bit more.

Thanks for looking into that.

>
> This patch chooses non-closed branch head as default update
> destination.
>
> This patch changes not only normal lookup code path, but also "no
> default branch" code path, for consistency.
>
> If no appropriate default update destination is found, destupdate()
> returns '.' as destination to work as no-op at updating. 'None' isn't
> reasonable for this purpose, because:
>
>    - repo[node] returns useless rev/node if node=None
>    - merge.update() re-invokes destupdate() for None destination
>
> diff --git a/mercurial/destutil.py b/mercurial/destutil.py
> --- a/mercurial/destutil.py
> +++ b/mercurial/destutil.py
> @@ -93,14 +93,15 @@ def _destupdatebranch(repo, clean, check
>       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
> @@ -130,6 +131,11 @@ def destupdate(repo, clean=False, check=
>           node, movemark, activemark = destupdatestepmap[step](repo, clean, check)
>           if node is not None:
>               break
> +    else:
> +        # udate to current parent (= no-op), because no appropriate
> +        # default destination is found
> +        node = '.'
> +
>       rev = repo[node].rev()
>
>       _destupdatevalidate(repo, rev, clean, check)
> @@ -366,6 +372,16 @@ def _statusotherbranchheads(ui, repo):
>           if otherheads:
>               ui.status(_('%i other heads for branch "%s"\n') %
>                         (len(otherheads), currentbranch))
> +    elif len(repo): # omit redundant message in empty repository
> +        if not heads:
> +            if currentbranch not in repo.branchmap():
> +                # this is "update to the tipmost non-closed branch head" case
> +                ui.status(_('all branches are closed\n'))
> +            else:
> +                ui.status(_('all heads of branch "%s" are closed\n') %
> +                          currentbranch)
> +        elif not len(repo.revs('parents()::(%ln)', heads)):
> +            ui.status(_('all descendant branch heads are closed\n'))
>
>   def statusotherdests(ui, repo):
>       """Print message about other head"""
> 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
> @@ -196,6 +196,24 @@ if on the closed branch head:
>     1 other heads for branch "default"
>     parent=6
>
> +if only one descendant non-closed branch head exists:
> +- 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:
> +- updating is no-op
> +- "N other heads for ...." message isn't displayed
> +- "all descendant branch ...." is displayed
> +
> +  $ norevtest "all descendant branch heads are closed" clean 3
> +  0 files updated, 0 files merged, 0 files removed, 0 files unresolved
> +  all descendant branch heads are closed
> +  parent=3
> +
>   Test updating if all branch heads are closed
>
>   if on the closed branch head:
> @@ -208,6 +226,55 @@ if on the closed branch head:
>     0 files updated, 0 files merged, 0 files removed, 0 files unresolved
>     parent=6
>
> +if not on the closed branch head:
> +- updating is no-op
> +- "all heads of branch ...." message is displayed
> +
> +  $ norevtest "all heads of branch default are closed" clean 1
> +  0 files updated, 0 files merged, 0 files removed, 0 files unresolved
> +  all heads of branch "default" are closed
> +  parent=1
> +
> +  $ 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
> +- updating is no-op
> +- "all branches are closed" message is displayed
> +
> +  $ hg update -q -C 1
> +  $ hg commit --close-branch -m "#4"
> +
> +  $ norevtest "all branches are closed" clean null
> +  0 files updated, 0 files merged, 0 files removed, 0 files unresolved
> +  all branches are closed
> +
>     $ cd ../b1
>
>   Test obsolescence behavior
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
Pierre-Yves David - Feb. 26, 2016, 9:58 p.m.
On 02/25/2016 12:19 AM, Pierre-Yves David wrote:
>
>
> On 02/24/2016 03:17 PM, FUJIWARA Katsunori wrote:
>> # HG changeset patch
>> # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
>> # Date 1456322433 -32400
>> #      Wed Feb 24 23:00:33 2016 +0900
>> # Node ID ae5d291a6dcb96683eae3c4e33575297fa113fbb
>> # Parent  1f9b3d38e380b018cbe10ed1acee882c014e3763
>> destutil: choose non-closed branch head as default update destination
>> (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.
>
> Excluding closed head from update makes a lot of sense to me. But we
> probably need some better messaging around the no-op update. I'm not
> exactly sure no-op update is the right choice if there is nothing but
> closed head above. However giving priority to non-closed head seems an
> obvious way to go here.
>
> I need to think about the "no non-closed heads in .::" case a bit more.
>
> Thanks for looking into that.

I did not though too much about it, but here is some random ideas.

- update pick open heads over closed heads if you have multiple 
descendants head
- update still update to closed heads if there is only that
- the message is update to warn about 'hg update took you to a closed 
head, beware of ghost and other zombi"
Katsunori FUJIWARA - Feb. 27, 2016, 3:46 p.m.
At Fri, 26 Feb 2016 22:58:41 +0100,
Pierre-Yves David wrote:
> 
> 
> 
> On 02/25/2016 12:19 AM, Pierre-Yves David wrote:
> >
> >
> > On 02/24/2016 03:17 PM, FUJIWARA Katsunori wrote:
> >> # HG changeset patch
> >> # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
> >> # Date 1456322433 -32400
> >> #      Wed Feb 24 23:00:33 2016 +0900
> >> # Node ID ae5d291a6dcb96683eae3c4e33575297fa113fbb
> >> # Parent  1f9b3d38e380b018cbe10ed1acee882c014e3763
> >> destutil: choose non-closed branch head as default update destination
> >> (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.
> >
> > Excluding closed head from update makes a lot of sense to me. But we
> > probably need some better messaging around the no-op update. I'm not
> > exactly sure no-op update is the right choice if there is nothing but
> > closed head above. However giving priority to non-closed head seems an
> > obvious way to go here.
> >
> > I need to think about the "no non-closed heads in .::" case a bit more.
> >
> > Thanks for looking into that.
> 
> I did not though too much about it, but here is some random ideas.
> 
> - update pick open heads over closed heads if you have multiple 
> descendants head
> - update still update to closed heads if there is only that
> - the message is update to warn about 'hg update took you to a closed 
> head, beware of ghost and other zombi"

This specification seems reasonable.

BTW, should we advance active bookmark at updating to closed head ?

I expect answer YES, but NO isn't so surprised, because keeping
location of it is useful to easily back from closed head, which might
be unintentional destination for end users :-)


> -- 
> Pierre-Yves David
> 

----------------------------------------------------------------------
[FUJIWARA Katsunori]                             foozy@lares.dti.ne.jp
Pierre-Yves David - Feb. 27, 2016, 3:50 p.m.
On 02/27/2016 04:46 PM, FUJIWARA Katsunori wrote:
>
> At Fri, 26 Feb 2016 22:58:41 +0100,
> Pierre-Yves David wrote:
>>
>>
>>
>> On 02/25/2016 12:19 AM, Pierre-Yves David wrote:
>>>
>>>
>>> On 02/24/2016 03:17 PM, FUJIWARA Katsunori wrote:
>>>> # HG changeset patch
>>>> # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
>>>> # Date 1456322433 -32400
>>>> #      Wed Feb 24 23:00:33 2016 +0900
>>>> # Node ID ae5d291a6dcb96683eae3c4e33575297fa113fbb
>>>> # Parent  1f9b3d38e380b018cbe10ed1acee882c014e3763
>>>> destutil: choose non-closed branch head as default update destination
>>>> (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.
>>>
>>> Excluding closed head from update makes a lot of sense to me. But we
>>> probably need some better messaging around the no-op update. I'm not
>>> exactly sure no-op update is the right choice if there is nothing but
>>> closed head above. However giving priority to non-closed head seems an
>>> obvious way to go here.
>>>
>>> I need to think about the "no non-closed heads in .::" case a bit more.
>>>
>>> Thanks for looking into that.
>>
>> I did not though too much about it, but here is some random ideas.
>>
>> - update pick open heads over closed heads if you have multiple
>> descendants head
>> - update still update to closed heads if there is only that
>> - the message is update to warn about 'hg update took you to a closed
>> head, beware of ghost and other zombi"
>
> This specification seems reasonable.
>
> BTW, should we advance active bookmark at updating to closed head ?
>
> I expect answer YES, but NO isn't so surprised, because keeping
> location of it is useful to easily back from closed head, which might
> be unintentional destination for end users :-)

I think We should keep the behavior consistent, Tets not introduce a 
special case.

There is a perpendicular movement to have 'hg update' no longer move the 
active bookmark (and 'hg update -B .' do so instead (irrc). That will 
cover it.

I'm dropping this patch from patchwork and expect a new series in the 
future.

Patch

diff --git a/mercurial/destutil.py b/mercurial/destutil.py
--- a/mercurial/destutil.py
+++ b/mercurial/destutil.py
@@ -93,14 +93,15 @@  def _destupdatebranch(repo, clean, check
     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
@@ -130,6 +131,11 @@  def destupdate(repo, clean=False, check=
         node, movemark, activemark = destupdatestepmap[step](repo, clean, check)
         if node is not None:
             break
+    else:
+        # udate to current parent (= no-op), because no appropriate
+        # default destination is found
+        node = '.'
+
     rev = repo[node].rev()
 
     _destupdatevalidate(repo, rev, clean, check)
@@ -366,6 +372,16 @@  def _statusotherbranchheads(ui, repo):
         if otherheads:
             ui.status(_('%i other heads for branch "%s"\n') %
                       (len(otherheads), currentbranch))
+    elif len(repo): # omit redundant message in empty repository
+        if not heads:
+            if currentbranch not in repo.branchmap():
+                # this is "update to the tipmost non-closed branch head" case
+                ui.status(_('all branches are closed\n'))
+            else:
+                ui.status(_('all heads of branch "%s" are closed\n') %
+                          currentbranch)
+        elif not len(repo.revs('parents()::(%ln)', heads)):
+            ui.status(_('all descendant branch heads are closed\n'))
 
 def statusotherdests(ui, repo):
     """Print message about other head"""
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
@@ -196,6 +196,24 @@  if on the closed branch head:
   1 other heads for branch "default"
   parent=6
 
+if only one descendant non-closed branch head exists:
+- 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:
+- updating is no-op
+- "N other heads for ...." message isn't displayed
+- "all descendant branch ...." is displayed
+
+  $ norevtest "all descendant branch heads are closed" clean 3
+  0 files updated, 0 files merged, 0 files removed, 0 files unresolved
+  all descendant branch heads are closed
+  parent=3
+
 Test updating if all branch heads are closed
 
 if on the closed branch head:
@@ -208,6 +226,55 @@  if on the closed branch head:
   0 files updated, 0 files merged, 0 files removed, 0 files unresolved
   parent=6
 
+if not on the closed branch head:
+- updating is no-op
+- "all heads of branch ...." message is displayed
+
+  $ norevtest "all heads of branch default are closed" clean 1
+  0 files updated, 0 files merged, 0 files removed, 0 files unresolved
+  all heads of branch "default" are closed
+  parent=1
+
+  $ 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
+- updating is no-op
+- "all branches are closed" message is displayed
+
+  $ hg update -q -C 1
+  $ hg commit --close-branch -m "#4"
+
+  $ norevtest "all branches are closed" clean null
+  0 files updated, 0 files merged, 0 files removed, 0 files unresolved
+  all branches are closed
+
   $ cd ../b1
 
 Test obsolescence behavior