Patchwork D3557: commit: add new close-branch command

login
register
mail settings
Submitter phabricator
Date May 13, 2018, 11:42 p.m.
Message ID <differential-rev-PHID-DREV-xc4qpx5at2fp5vs4hfcg-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/31580/
State Superseded
Headers show

Comments

phabricator - May 13, 2018, 11:42 p.m.
joerg.sonnenberger created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  ``hg close-branch`` allows closing arbitrary heads. It is equivalent to
  checking out the given revisions and commit an empty change with
  ``hg commit --close-branch``.

REPOSITORY
  rHG Mercurial

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

AFFECTED FILES
  mercurial/commands.py
  tests/test-close-branch.t
  tests/test-completion.t
  tests/test-globalopts.t
  tests/test-help.t
  tests/test-hgweb-json.t

CHANGE DETAILS




To: joerg.sonnenberger, #hg-reviewers
Cc: mercurial-devel
phabricator - May 20, 2018, 7:52 p.m.
pulkit added a comment.


  > `hg close-branch` allows closing arbitrary heads. It is equivalent to
  >  checking out the given revisions and commit an empty change with
  >  `hg commit --close-branch`.
  
  I am -1 on having a new command for this. How about adding this as a `--close` flag to `hg branch` command?

REPOSITORY
  rHG Mercurial

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

To: joerg.sonnenberger, #hg-reviewers
Cc: pulkit, mercurial-devel
phabricator - May 21, 2018, 9:03 p.m.
joerg.sonnenberger added a comment.


  It doesn't seem to fit the argument schema for `hg branch` at all. I'm open for better places, but I couldn't think of a consistent place.

REPOSITORY
  rHG Mercurial

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

To: joerg.sonnenberger, #hg-reviewers
Cc: pulkit, mercurial-devel
phabricator - May 25, 2018, 11:30 a.m.
pulkit added a comment.


  The code looks mostly good to me. Needs tests for following cases:
  
  - when working directory is dirty
  - when we are in an uncommitted merge state
  - there is some bookmark movement code, tests for that

INLINE COMMENTS

> commands.py:1508
> +    if not message:
> +        raise error.Abort(_("No commit message specified with -l or -m"))
> +    extra = { 'close': '1' }

we use all lowercase error messages, so this should be "no commit ..".

REPOSITORY
  rHG Mercurial

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

To: joerg.sonnenberger, #hg-reviewers
Cc: pulkit, mercurial-devel
phabricator - May 25, 2018, 11:31 a.m.
pulkit added a comment.


  In https://phab.mercurial-scm.org/D3557#57279, @joerg.sonnenberger wrote:
  
  > It doesn't seem to fit the argument schema for `hg branch` at all. I'm open for better places, but I couldn't think of a consistent place.
  
  
  The branch command has a `--rev` flag which we can use. Something like `hg branch --rev <revs> --close`. What do you think?

REPOSITORY
  rHG Mercurial

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

To: joerg.sonnenberger, #hg-reviewers
Cc: pulkit, mercurial-devel
phabricator - May 25, 2018, 11:45 a.m.
joerg.sonnenberger added a comment.


  If we want to go with a `--close` flag, I think the most natural place would actually be `hg heads`. This is not really a branch operation after all, but about cutting off heads. At the same time, none of those options would allow specifying a commit message naturally.

REPOSITORY
  rHG Mercurial

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

To: joerg.sonnenberger, #hg-reviewers
Cc: pulkit, mercurial-devel
phabricator - May 31, 2018, 1:03 a.m.
indygreg added a comment.


  I'm -0.25 on this.
  
  But I don't think we should add an argument to `hg heads` or `hg branch` because I don't like a) overloading commands to have multiple meanings b) making commands read-write and read-only. I think commands should do one thing and do them well. There should also be a set of //safe// commands that can be executed without meaningful side-effects.
  
  Following that logic, a dedicated command is justified.
  
  But I just don't feel like closing heads that aren't checked out is a frequent enough operation to justify a dedicated command.
  
  As a potential compromise, how about `hg commit --close-branch-rev <REV>` that does the branch closing without needing a working directory? But this seemingly violates the expectation of `hg commit`, which is that it operates on the working directory. Gah - now I think I talked myself into a dedicated command.
  
  Good UI design is hard.

REPOSITORY
  rHG Mercurial

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

To: joerg.sonnenberger, #hg-reviewers
Cc: indygreg, pulkit, mercurial-devel
phabricator - June 11, 2018, 6:45 p.m.
pulkit added a comment.


  Thanks for taking this out into a separate extension. Code looks mostly good to me. It feels to me that we need some more tests:
  
  - passing multiple revisions when some are head and some not
  - closing a head which has secret phase
  - checking for bookmark movement
  
  I know the behavior in couple of the above points but having tests will help us in future when we change some behavior, like showing warning for revs which are not heads and closing others.
  
  Also, wherever we pass revisions, we have support for `--rev` flag too. Can you add that too?

INLINE COMMENTS

> closehead.py:33
> +
> +@command('close|close-head|close-heads', commitopts + commitopts2,
> +    _('[OPTION]... [REV]...'), inferrepo=True)

close is too generic, I don't feel confident enough to have this as a command name. Can we drop that for now if you don't feel to strong?

> closehead.py:53
> +        tr.close()
> +
> +    if not revs:

please add an `opts = pycompat.byteskwargs(opts)` call here for python 3 support.

> closehead.py:69
> +    if not message:
> +        raise error.Abort(_("No commit message specified with -l or -m"))
> +    extra = { 'close': '1' }

we don't start with uppercase in error messages

> closehead.py:73
> +    wlock = lock = None
> +    try:
> +        wlock = repo.wlock()

nit: how about using context manager?

> closehead.py:79
> +            branch = r.branch()
> +            if branch:
> +                extra['branch'] = branch

I don't think this will ever go the else part.

> test-close-head.t:1
> +  $ hg init
> +  $ hg debugbuilddag '+2*2*3*4'

init a new repo and cd into that. don't work in top level $TESTTMP directory.

> test-close-head.t:43
> +
> +  $ hg init test-empty
> +  $ cd test-empty

same, don't create a new inside existing repo, cd back and then create one.

REPOSITORY
  rHG Mercurial

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

To: joerg.sonnenberger, #hg-reviewers
Cc: indygreg, pulkit, mercurial-devel
phabricator - July 23, 2018, 6:46 p.m.
pulkit added a comment.


  Sorry for getting late to review this. Found a minor nit and also looks like you missed adding tests for bookmark movement and phase preservation.

INLINE COMMENTS

> closehead.py:55
> +
> +    if not revs:
> +        raise error.Abort(_('no revisions specified'))

revs can be passed as opts['rev'] too now.

REPOSITORY
  rHG Mercurial

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

To: joerg.sonnenberger, #hg-reviewers
Cc: indygreg, pulkit, mercurial-devel
phabricator - Sept. 21, 2018, 10:26 a.m.
joerg.sonnenberger marked 2 inline comments as done.
joerg.sonnenberger added a comment.


  Adjusted for most of the review comments. Added a test that books are left alone, since they will not be active in the interesting cases. I'm not touching any existing phases, so I'm not sure if I should do anything about phase testing?

REPOSITORY
  rHG Mercurial

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

To: joerg.sonnenberger, #hg-reviewers
Cc: indygreg, pulkit, mercurial-devel

Patch

diff --git a/tests/test-hgweb-json.t b/tests/test-hgweb-json.t
--- a/tests/test-hgweb-json.t
+++ b/tests/test-hgweb-json.t
@@ -1800,6 +1800,10 @@ 
         "topic": "cat"
       },
       {
+        "summary": "close the given head revisions",
+        "topic": "close-branch"
+      },
+      {
         "summary": "show combined config settings from all hgrc files",
         "topic": "config"
       },
diff --git a/tests/test-help.t b/tests/test-help.t
--- a/tests/test-help.t
+++ b/tests/test-help.t
@@ -65,6 +65,7 @@ 
    bundle        create a bundle file
    cat           output the current or given revision of files
    clone         make a copy of an existing repository
+   close-branch  close the given head revisions
    commit        commit the specified files or all outstanding changes
    config        show combined config settings from all hgrc files
    copy          mark files as copied for the next commit
@@ -144,6 +145,7 @@ 
    bundle        create a bundle file
    cat           output the current or given revision of files
    clone         make a copy of an existing repository
+   close-branch  close the given head revisions
    commit        commit the specified files or all outstanding changes
    config        show combined config settings from all hgrc files
    copy          mark files as copied for the next commit
@@ -840,6 +842,7 @@ 
    bundle        create a bundle file
    cat           output the current or given revision of files
    clone         make a copy of an existing repository
+   close-branch  close the given head revisions
    commit        commit the specified files or all outstanding changes
    config        show combined config settings from all hgrc files
    copy          mark files as copied for the next commit
@@ -2356,6 +2359,13 @@ 
   output the current or given revision of files
   </td></tr>
   <tr><td>
+  <a href="/help/close-branch">
+  close-branch
+  </a>
+  </td><td>
+  close the given head revisions
+  </td></tr>
+  <tr><td>
   <a href="/help/config">
   config
   </a>
diff --git a/tests/test-globalopts.t b/tests/test-globalopts.t
--- a/tests/test-globalopts.t
+++ b/tests/test-globalopts.t
@@ -308,6 +308,7 @@ 
    bundle        create a bundle file
    cat           output the current or given revision of files
    clone         make a copy of an existing repository
+   close-branch  close the given head revisions
    commit        commit the specified files or all outstanding changes
    config        show combined config settings from all hgrc files
    copy          mark files as copied for the next commit
@@ -391,6 +392,7 @@ 
    bundle        create a bundle file
    cat           output the current or given revision of files
    clone         make a copy of an existing repository
+   close-branch  close the given head revisions
    commit        commit the specified files or all outstanding changes
    config        show combined config settings from all hgrc files
    copy          mark files as copied for the next commit
diff --git a/tests/test-completion.t b/tests/test-completion.t
--- a/tests/test-completion.t
+++ b/tests/test-completion.t
@@ -12,6 +12,7 @@ 
   bundle
   cat
   clone
+  close-branch
   commit
   config
   copy
@@ -252,6 +253,7 @@ 
   branches: active, closed, template
   bundle: force, rev, branch, base, all, type, ssh, remotecmd, insecure
   cat: output, rev, decode, include, exclude, template
+  close-branch: message, logfile, date, user
   config: untrusted, edit, local, global, template
   copy: after, force, include, exclude, dry-run
   debugancestor: 
diff --git a/tests/test-close-branch.t b/tests/test-close-branch.t
new file mode 100644
--- /dev/null
+++ b/tests/test-close-branch.t
@@ -0,0 +1,41 @@ 
+  $ hg init
+  $ hg debugbuilddag '+2*2*3*4'
+  $ hg log -G --template '{rev}:{node|short}'
+  o  4:e7bd5218ca15
+  |
+  | o  3:6100d3090acf
+  |/
+  | o  2:fa942426a6fd
+  |/
+  | o  1:66f7d451a68b
+  |/
+  o  0:1ea73414a91b
+  
+  $ hg close-branch -m 'Close old heads' 1 2
+  $ hg heads
+  changeset:   4:e7bd5218ca15
+  parent:      0:1ea73414a91b
+  user:        debugbuilddag
+  date:        Thu Jan 01 00:00:04 1970 +0000
+  summary:     r4
+  
+  changeset:   3:6100d3090acf
+  parent:      0:1ea73414a91b
+  user:        debugbuilddag
+  date:        Thu Jan 01 00:00:03 1970 +0000
+  summary:     r3
+  
+  $ hg close-branch -m 'Close more old heads' 4
+  $ hg heads
+  changeset:   3:6100d3090acf
+  parent:      0:1ea73414a91b
+  user:        debugbuilddag
+  date:        Thu Jan 01 00:00:03 1970 +0000
+  summary:     r3
+  
+  $ hg close-branch -m 'Not a head' 0
+  abort: revision is not an open head: 0
+  [255]
+  $ hg close-branch -m 'Already closed head' 1
+  abort: revision is not an open head: 1
+  [255]
diff --git a/mercurial/commands.py b/mercurial/commands.py
--- a/mercurial/commands.py
+++ b/mercurial/commands.py
@@ -26,6 +26,7 @@ 
     bundle2,
     changegroup,
     cmdutil,
+    context,
     copies,
     debugcommands as debugcommandsmod,
     destutil,
@@ -1468,6 +1469,55 @@ 
 
     return r is None
 
+@command('close-branch', commitopts + commitopts2,
+    _('[OPTION]... [REV]...'), inferrepo=True)
+def close_branch(ui, repo, *revs, **opts):
+    """close the given head revisions
+
+    This is equivalent to checking out each revision in a clean tree and running
+    ``hg commit --close-branch``, except that it doesn't change the working
+    directory.
+
+    The commit message must be specified with -l or -m.
+    """
+    def docommit(rev):
+        cctx = context.memctx(repo, parents=[rev, None], text=message,
+                              files=[], filectxfn=None, user=opts.get('user'),
+                              date=opts.get('date'), extra=extra)
+        tr = repo.transaction('commit')
+        ret = repo.commitctx(cctx, True)
+        bookmarks.update(repo, [rev, None], ret)
+        cctx.markcommitted(ret)
+        tr.close()
+
+    if not revs:
+        raise error.Abort(_('no revisions specified'))
+
+    revs = scmutil.revrange(repo, revs)
+
+    heads = []
+    for branch in repo.branchmap():
+        heads.extend(repo.branchheads(branch))
+    heads = set(repo[h].rev() for h in heads)
+    for rev in revs:
+        if rev not in heads:
+            raise error.Abort(_('revision is not an open head: %s') % rev)
+
+    message = cmdutil.logmessage(ui, opts)
+    if not message:
+        raise error.Abort(_("No commit message specified with -l or -m"))
+    extra = { 'close': '1' }
+
+    wlock = lock = None
+    try:
+        wlock = repo.wlock()
+        lock = repo.lock()
+        for rev in revs:
+            docommit(rev)
+    finally:
+        release(lock, wlock)
+    return 0
+
 @command('^commit|ci',
     [('A', 'addremove', None,
      _('mark new/missing files as added/removed before committing')),