This is an archive of the discontinued Mercurial Phabricator instance.

extensions: new closehead module for closing arbitrary heads
ClosedPublic

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

Details

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?

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

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?

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.

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.

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
34

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?

54

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

70

we don't start with uppercase in error messages

74

nit: how about using context manager?

80

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

tests/test-close-head.t
2

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

44

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

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
55

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
This revision was automatically updated to reflect the committed changes.