Patchwork D6490: branch: add --force flag to close branch from a not-head changeset

login
register
mail settings
Submitter phabricator
Date June 7, 2019, 1:57 p.m.
Message ID <differential-rev-PHID-DREV-qmwzsysqg5oijpgckl5t-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/40346/
State New
Headers show

Comments

phabricator - June 7, 2019, 1:57 p.m.
khanchi97 created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  While closing branch from a changeset which is not a branch head
  current implementation abort this action in every case but, there
  can be the situations where the changeset is not a local head but
  could be a remote head. This patch adds the functionality to bypass
  the "abort: can only close branch heads" by introducing --force flag.
  
  Added test case demonstrate the added behaviour.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6490

AFFECTED FILES
  mercurial/commands.py
  tests/test-alias.t
  tests/test-branches.t
  tests/test-completion.t
  tests/test-qrecord.t
  tests/test-record.t

CHANGE DETAILS




To: khanchi97, #hg-reviewers
Cc: mercurial-devel
phabricator - June 7, 2019, 5:26 p.m.
mharbison72 requested changes to this revision.
mharbison72 added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> commands.py:1680
> +        elif (branch == repo['.'].branch() and repo['.'].node() not in bheads
> +              and not opts.get('force')):
>              raise error.Abort(_('can only close branch heads'))

I didn't realize that `--force` wasn't already an option for commit, because it is an option to internal commit functions.  I still like this idea, but maybe you need to remove this from `opts` after completing the check here (and also in the `--no-close-branch` case).  Otherwise, it risks bypassing existing safeguards.

https://www.mercurial-scm.org/repo/hg/file/tip/mercurial/localrepo.py#l2498

(I have no idea why that's allowed at all.  MQ maybe?)

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6490

To: khanchi97, #hg-reviewers, mharbison72
Cc: mharbison72, mercurial-devel
phabricator - June 10, 2019, 1:02 p.m.
khanchi97 marked an inline comment as done.
khanchi97 added inline comments.

INLINE COMMENTS

> mharbison72 wrote in commands.py:1680
> I didn't realize that `--force` wasn't already an option for commit, because it is an option to internal commit functions.  I still like this idea, but maybe you need to remove this from `opts` after completing the check here (and also in the `--no-close-branch` case).  Otherwise, it risks bypassing existing safeguards.
> 
> https://www.mercurial-scm.org/repo/hg/file/tip/mercurial/localrepo.py#l2498
> 
> (I have no idea why that's allowed at all.  MQ maybe?)

Updated as per suggested changes. Also added a hint to use --force while aborting the user.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6490

To: khanchi97, #hg-reviewers, mharbison72
Cc: mharbison72, mercurial-devel
phabricator - June 10, 2019, 11:48 p.m.
mharbison72 accepted this revision.
mharbison72 added a comment.


  LGTM, thanks.
  
  Minor nit that I don't care too much about, but I'm wondering what others think of `not-head changeset` vs `non-head changeset`.  The latter sounds more natural to me, but I'm not sure if there's another example of this phrasing.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6490

To: khanchi97, #hg-reviewers, mharbison72
Cc: mharbison72, mercurial-devel
phabricator - June 11, 2019, 5:49 a.m.
khanchi97 marked an inline comment as done.
khanchi97 added a comment.


  In https://phab.mercurial-scm.org/D6490#94625, @mharbison72 wrote:
  
  > LGTM, thanks.
  >
  > Minor nit that I don't care too much about, but I'm wondering what others think of `not-head changeset` vs `non-head changeset`.  The latter sounds more natural to me, but I'm not sure if there's another example of this phrasing.
  
  
  Yes, I was also confused between these two phrasing when writing the patch. `non-head` sounds more natural to me too (not sure why I picked first one :P ). What others think about it?

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6490

To: khanchi97, #hg-reviewers, mharbison72
Cc: mharbison72, mercurial-devel
phabricator - June 11, 2019, 2:01 p.m.
pulkit added a comment.


  In https://phab.mercurial-scm.org/D6490#94627, @khanchi97 wrote:
  
  > In https://phab.mercurial-scm.org/D6490#94625, @mharbison72 wrote:
  >
  > > LGTM, thanks.
  > >
  > > Minor nit that I don't care too much about, but I'm wondering what others think of `not-head changeset` vs `non-head changeset`.  The latter sounds more natural to me, but I'm not sure if there's another example of this phrasing.
  >
  >
  > Yes, I was also confused between these two phrasing when writing the patch. `non-head` sounds more natural to me too (not sure why I picked first one :P ). What others think about it?
  
  
  Let's go with `non-head`.
  
  Also the commit title needs to be changed to `commit: add --force flag..` instead of `branch: add --force flag..`

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6490

To: khanchi97, #hg-reviewers, mharbison72
Cc: pulkit, mharbison72, mercurial-devel

Patch

diff --git a/tests/test-record.t b/tests/test-record.t
--- a/tests/test-record.t
+++ b/tests/test-record.t
@@ -52,6 +52,7 @@ 
       --amend               amend the parent of the working directory
    -s --secret              use the secret phase for committing
    -e --edit                invoke editor on commit messages
+   -f --force               forcibly close branch from a not-head changeset
    -I --include PATTERN [+] include names matching the given patterns
    -X --exclude PATTERN [+] exclude names matching the given patterns
    -m --message TEXT        use text as commit message
diff --git a/tests/test-qrecord.t b/tests/test-qrecord.t
--- a/tests/test-qrecord.t
+++ b/tests/test-qrecord.t
@@ -69,6 +69,7 @@ 
       --amend               amend the parent of the working directory
    -s --secret              use the secret phase for committing
    -e --edit                invoke editor on commit messages
+   -f --force               forcibly close branch from a not-head changeset
    -I --include PATTERN [+] include names matching the given patterns
    -X --exclude PATTERN [+] exclude names matching the given patterns
    -m --message TEXT        use text as commit message
diff --git a/tests/test-completion.t b/tests/test-completion.t
--- a/tests/test-completion.t
+++ b/tests/test-completion.t
@@ -245,7 +245,7 @@ 
   bundle: force, rev, branch, base, all, type, ssh, remotecmd, insecure
   cat: output, rev, decode, include, exclude, template
   clone: noupdate, updaterev, rev, branch, pull, uncompressed, stream, ssh, remotecmd, insecure
-  commit: addremove, close-branch, amend, secret, edit, interactive, include, exclude, message, logfile, date, user, subrepos
+  commit: addremove, close-branch, amend, secret, edit, force, interactive, include, exclude, message, logfile, date, user, subrepos
   config: untrusted, edit, local, global, template
   copy: after, force, include, exclude, dry-run
   debugancestor: 
diff --git a/tests/test-branches.t b/tests/test-branches.t
--- a/tests/test-branches.t
+++ b/tests/test-branches.t
@@ -972,3 +972,18 @@ 
   @  0: 9092f1db7931 added a
      	default
   
+Check --force to close a branch from a cset which is not a branch head
+
+  $ hg show stack --config extensions.show=
+    o  1553 added c
+    o  5f6d added b
+    @  9092 added a
+
+without --force
+  $ hg ci -m "branch closed" --close-branch
+  abort: can only close branch heads
+  [255]
+
+with --force
+  $ hg ci -m "branch closed" --close-branch --force
+  created new head
diff --git a/tests/test-alias.t b/tests/test-alias.t
--- a/tests/test-alias.t
+++ b/tests/test-alias.t
@@ -120,6 +120,7 @@ 
       --amend               amend the parent of the working directory
    -s --secret              use the secret phase for committing
    -e --edit                invoke editor on commit messages
+   -f --force               forcibly close branch from a not-head changeset
    -i --interactive         use interactive mode
    -I --include PATTERN [+] include names matching the given patterns
    -X --exclude PATTERN [+] exclude names matching the given patterns
diff --git a/mercurial/commands.py b/mercurial/commands.py
--- a/mercurial/commands.py
+++ b/mercurial/commands.py
@@ -1583,6 +1583,7 @@ 
     ('', 'amend', None, _('amend the parent of the working directory')),
     ('s', 'secret', None, _('use the secret phase for committing')),
     ('e', 'edit', None, _('invoke editor on commit messages')),
+    ('f', 'force', None, _('forcibly close branch from a not-head changeset')),
     ('i', 'interactive', None, _('use interactive mode')),
     ] + walkopts + commitopts + commitopts2 + subrepoopts,
     _('[OPTION]... [FILE]...'),
@@ -1675,7 +1676,8 @@ 
 
         if not bheads:
             raise error.Abort(_('can only close branch heads'))
-        elif branch == repo['.'].branch() and repo['.'].node() not in bheads:
+        elif (branch == repo['.'].branch() and repo['.'].node() not in bheads
+              and not opts.get('force')):
             raise error.Abort(_('can only close branch heads'))
         elif opts.get('amend'):
             if (repo['.'].p1().branch() != branch and