This is an archive of the discontinued Mercurial Phabricator instance.

Differential D3557

extensions: new closehead module for closing arbitrary heads
ClosedPublic

Authored by joerg.sonnenberger on May 13 2018, 7:42 PM.

Details

Reviewers
pulkit
Group Reviewers
hg-reviewers
Commits
rHGcd5f2e615262: extensions: new closehead module for closing arbitrary heads
Summary

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

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

pulkit added a subscriber: pulkit.May 20 2018, 3:52 PM

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?

joerg.sonnenberger added a comment.May 21 2018, 5:03 PM

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.

pulkit added a comment.May 25 2018, 7:30 AM

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
mercurial/commands.py
1508 ↗(On Diff #8680)

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

pulkit added a comment.May 25 2018, 7:31 AM
In 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?

joerg.sonnenberger added a comment.May 25 2018, 7:45 AM

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.

indygreg added a subscriber: indygreg.May 30 2018, 9:03 PM

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.

joerg.sonnenberger edited the summary of this revision. (Show Details)Jun 3 2018, 11:57 AM
joerg.sonnenberger retitled this revision from commit: add new close-branch command to extensions: new closehead module for closing arbitrary heads.
joerg.sonnenberger updated this revision to Diff 8963.
pulkit added a comment.Jun 11 2018, 2:45 PM

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?

hgext/closehead.py
33

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?

53

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

69

we don't start with uppercase in error messages

73

nit: how about using context manager?

79

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

tests/test-close-head.t
1

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

43

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

pulkit added a comment.Jul 23 2018, 2:46 PM

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.

hgext/closehead.py
56

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

joerg.sonnenberger marked 5 inline comments as done.Sep 21 2018, 6:25 AM
joerg.sonnenberger updated this revision to Diff 11254.
joerg.sonnenberger marked 2 inline comments as done.Sep 21 2018, 6:26 AM

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?

pulkit accepted this revision.Oct 3 2018, 8:23 AM
Closed by commit rHGcd5f2e615262: extensions: new closehead module for closing arbitrary heads (authored by joerg.sonnenberger). · Explain WhyOct 3 2018, 8:35 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
AMhgext/closehead.py (87 lines)
AMtests/test-close-head.t (51 lines)
Mtests/test-help.t (1 line)

Diff 8963

hgext/closehead.py

  • This file was added.
# closehead.py - Close arbitrary heads without checking them out first
#
# This software may be used and distributed according to the terms of the
# GNU General Public License version 2 or any later version.
'''close arbitrary heads without checking them out first'''
from __future__ import absolute_import
from mercurial.i18n import _
from mercurial import (
bookmarks,
cmdutil,
context,
error,
lock,
registrar,
scmutil,
)
release = lock.release
cmdtable = {}
command = registrar.command(cmdtable)
# Note for extension authors: ONLY specify testedwith = 'ships-with-hg-core' for
# extensions which SHIP WITH MERCURIAL. Non-mainline extensions should
# be specifying the version(s) of Mercurial they are tested with, or
# leave the attribute unspecified.
testedwith = 'ships-with-hg-core'
commitopts = cmdutil.commitopts
commitopts2 = cmdutil.commitopts2
@command('close|close-head|close-heads', commitopts + commitopts2,
pulkitUnsubmitted
Not Done

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?

pulkit: close is too generic, I don't feel confident enough to have this as a command name. Can we drop…
_('[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()
pulkitUnsubmitted
Done

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

pulkit: please add an `opts = pycompat.byteskwargs(opts)` call here for python 3 support.
if not revs:
raise error.Abort(_('no revisions specified'))
pulkitUnsubmitted
Done

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

pulkit: revs can be passed as opts['rev'] too now.
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"))
pulkitUnsubmitted
Done

we don't start with uppercase in error messages

pulkit: we don't start with uppercase in error messages
extra = { 'close': '1' }
wlock = lock = None
try:
pulkitUnsubmitted
Done

nit: how about using context manager?

pulkit: nit: how about using context manager?
wlock = repo.wlock()
lock = repo.lock()
for rev in revs:
r = repo[rev]
branch = r.branch()
if branch:
pulkitUnsubmitted
Done

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

pulkit: I don't think this will ever go the else part.
extra['branch'] = branch
else:
del extra['branch']
docommit(r)
finally:
release(lock, wlock)
return 0

tests/test-close-head.t

  • This file was added.
$ hg init
pulkitUnsubmitted
Done

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

pulkit: init a new repo and cd into that. don't work in top level $TESTTMP directory.
$ 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 --config extensions.closehead= close-head -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 --config extensions.closehead= close-head -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 --config extensions.closehead= close-head -m 'Not a head' 0
abort: revision is not an open head: 0
[255]
$ hg --config extensions.closehead= close-head -m 'Already closed head' 1
abort: revision is not an open head: 1
[255]
$ hg init test-empty
pulkitUnsubmitted
Done

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

pulkit: same, don't create a new inside existing repo, cd back and then create one.
$ cd test-empty
$ hg debugbuilddag '+1'
$ hg log -G --template '{rev}:{node|short}'
o 0:1ea73414a91b
$ hg --config extensions.closehead= close-head -m 'Close initial revision' 0
$ hg heads
[1]

tests/test-help.t

disabled extensions: disabled extensions:
acl hooks for controlling repository access acl hooks for controlling repository access
blackbox log repository events to a blackbox for debugging blackbox log repository events to a blackbox for debugging
bugzilla hooks for integrating with the Bugzilla bug tracker bugzilla hooks for integrating with the Bugzilla bug tracker
censor erase file content at a given revision censor erase file content at a given revision
churn command to display statistics about repository history churn command to display statistics about repository history
clonebundles advertise pre-generated bundles to seed clones clonebundles advertise pre-generated bundles to seed clones
closehead close arbitrary heads without checking them out first
convert import revisions from foreign VCS repositories into convert import revisions from foreign VCS repositories into
Mercurial Mercurial
eol automatically manage newlines in repository files eol automatically manage newlines in repository files
extdiff command to allow external programs to compare revisions extdiff command to allow external programs to compare revisions
factotum http authentication with factotum factotum http authentication with factotum
githelp try mapping git commands to Mercurial commands githelp try mapping git commands to Mercurial commands
gpg commands to sign and verify changesets gpg commands to sign and verify changesets
hgk browse the repository in a graphical way hgk browse the repository in a graphical way