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
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 >
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"
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
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