Page MenuHomePhabricator

branch: add a --rev flag to change branch name of given revisions
ClosedPublic

Authored by pulkit on Oct 14 2017, 10:41 AM.

Details

Summary

This patch adds a new --rev flag to hg branch which can be used to change branch
of revisions. This is motivated from topic extension where you can change topic
on revisions but this one has few restrictions which are:

  1. You cannot change branch name in between the stack
  2. You cannot change branch name and set it to an existing name
  3. You cannot change branch of non-linear set of commits
  4. You cannot change branch of merge commits.

Tests are added for the same.

.. feature::

An experimental flag `--rev` to `hg branch` which can be used to change
branch of changesets.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

pulkit created this revision.Oct 14 2017, 10:41 AM
dlax added a subscriber: dlax.Oct 15 2017, 5:02 AM
dlax added inline comments.
mercurial/cmdutil.py
731

typo: betwen

Also, I'm not sure "between" is appropriate here. Maybe "cannot change branch in the middle of a stack"?

752

nit: with % on the continuation line and alignment with opening parenthesis, it'd be more readable.

pulkit updated this revision to Diff 2789.Oct 15 2017, 1:56 PM
dlax requested changes to this revision.Oct 16 2017, 3:57 AM

This looks correct to me overall.
I use this feature with topics a lot and find it pretty convenient so I guess it might make sense for named branches in some workflows.

mercurial/cmdutil.py
752

This message is not tested, maybe add a --debug call to one of the tests?

774

That comment isn't really helpful as is. Maybe explain how you handle phase instead?

782

Isn't rewrote == len(replacements)?

mercurial/commands.py
997

Nit: could you move the closing ] on the next line and add a , at the end of the new line? This way next time we add an option, there will be only a one-line + diff.

1036

Maybe _().

tests/test-branch-change.t
42

Not sure why, but many titles' underlines are either too long or too short. Could you make these consistent?

68

"in the middle of the stack"

256

typo: mutliple

This revision now requires changes to proceed.Oct 16 2017, 3:57 AM
pulkit updated this revision to Diff 2807.Oct 16 2017, 8:30 AM
pulkit marked 9 inline comments as done.Oct 16 2017, 8:31 AM
dlax accepted this revision.Oct 16 2017, 8:36 AM
ryanmce requested changes to this revision.Oct 17 2017, 8:53 AM
ryanmce added a subscriber: ryanmce.
ryanmce added inline comments.
mercurial/cmdutil.py
722–731

What about an unclean working copy? It looks like that's supported -- could we make sure it's tested?

733–734

In theory, the phase boundary could move after our check but before we take the repolock. Can we guard against that please?

Two ways to accomplish this:

1/ Take the repo lock earlier
2/ Check before repo lock (fast path) and again after the repo lock is held (in case something changed)

791–792

Seems like we need a refactor? (out of scope for this change though)

tests/test-branch-change.t
8

nit: I'd prefer no newline in the glog output so things are more compact

10

This test needs to include a case where we strip commits, especially with a merge, just so the behavior is clear in this change.

That will probably (in my understanding of the current code) expose an issue that needs to be fixed.

17–34

Not sure we need this many commits for the test to be useful.

304–305

Hm, I think I'd prefer this to be an error about public changesets rather than a no-op with a 0 return value. Trying to modify the branch of public changesets -- even if the change is a no-op, still seems like an error to me. Thoughts?

This revision now requires changes to proceed.Oct 17 2017, 8:53 AM
pulkit added inline comments.Oct 17 2017, 9:26 AM
mercurial/cmdutil.py
791–792

Yeah, I will be happy to do that in next cycle. Can you suggest a name for file in which we can split cmdutil.py.

tests/test-branch-change.t
304–305

Oh, no. They are not public changesets. The test for public changeset is just above it. I will plug the phase info in the glog output in next version.

pulkit edited the summary of this revision. (Show Details)Oct 17 2017, 4:52 PM
pulkit updated this revision to Diff 2944.
pulkit marked 4 inline comments as done.Oct 17 2017, 5:01 PM
pulkit added inline comments.
mercurial/cmdutil.py
722–731

It's not supported. Handled that case and added a test. Thanks!

733–734

Improved the implementation by taking lock earlier. Thanks!

tests/test-branch-change.t
10

Oh, I didn't know that. Just banned merge revisions for now.

pulkit added a comment.Nov 2 2017, 2:14 PM

Re-requesting review since the freeze is over.

Looking good, should we allow changing the branch if any changeset is unstable or obsolete?

mercurial/cmdutil.py
723

Small typo, missing space between in and case

774

For safety, I would add a check that p1 and p2 are not obsolete here.

Looking good, should we allow changing the branch if any changeset is unstable or obsolete?

For unstable yes, for obsolete no. Will add a check and test for obsolete case.

mercurial/cmdutil.py
774

A user may want to change branch of an unstable changeset which I think is okay. I am not sure why you want that check?

pulkit updated this revision to Diff 4082.Dec 2 2017, 1:26 PM
pulkit added a comment.Dec 4 2017, 9:38 AM

This was send before the code freeze of last major release but since it was late in the release, it was not taken. Let's iterate on it now.

@durin42 @yuja @indygreg @krbullock this one is lying here for months. Does this conflict with anything or is a BC or something else, or was just missed for the review?

Apart from my review, it looks good to me.

Also I don't think you have a test when trying to change the branch of several changesets on different branches. If not, could you add one?

mercurial/cmdutil.py
734

Might be helpful to precise which changeset is a merge commit.

736

Might be helpful to precise which changeset is am obsolete commit.

tests/test-branch-change.t
144

You should update the revset to use the same as the next glog call (.^)::

martinvonz added inline comments.
mercurial/cmdutil.py
723

"in case of uncommited merge or dirty wdir"? (note that there are 4 fixes there)

725

may want to check if revs is empty (e.g. hg branch -r 'draft() - all()' foo)

731

maybe if not root.mutable():

740–744

children() can be expensive because it iterates over all later revisions (in revlog order). So when you want to call children() for many revisions (especially old revisions), it can be better to do a single scan. This *might* be better:

if repo.revs('(%ld::head()) - %ld', heads, heads):
  raise error.Abort(_("cannot change branch in middle of a stack"))

I'm not sure it will be, or if it's even correct, so please test (or just tell me that this command will be used rarely enough that no one cares).

mercurial/commands.py
1046–1051

Does this need to be modified to work with --rev? I've never used hg branch, but it sounds like hg branch foo (without --force) is supposed to fail if foo is already taken unless one of the working directory parents already has that name. Should we do the corresponding check for --rev? If not, it seems like we should at least *not* do the check (in its current form) when --rev is given. If I'm doing hg branch --rev deadbeef foo, it doesn't seem relevant what the branch names in my working directory parents are. Would be good to add a test case or two for this too.

pulkit updated this revision to Diff 4934.Jan 19 2018, 2:44 AM
pulkit marked 5 inline comments as done.Jan 19 2018, 2:57 AM
pulkit removed subscribers: durin42, krbullock, yuja, indygreg.
pulkit added inline comments.
mercurial/commands.py
1046–1051

This is a very basic and experimental implementation, hence I have changed the logic to not accept a new name if any such name already exists. Test cases are also added.

lothiraldan accepted this revision.Jan 19 2018, 3:32 AM
pulkit added inline comments.Jan 19 2018, 9:31 AM
mercurial/cmdutil.py
742

After some more testing, I found this is not correct and raise the error even in cases when it should not. For example:

o 3

o 2
/

o 1
o 0

Here if I want to change the branch of 1::2, this will raise error however this should be completely fine if we have allowunstable enabled. So in next version, I am going to add a test for allowunstable and if that's enabled, allow to change branch of 1::2.

durin42 accepted this revision.Jan 19 2018, 2:54 PM

Accepting this since it seems generally liked and is marked experimental. We can always rip it out if it turns out to be a mistake.

This revision was automatically updated to reflect the committed changes.