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

[ryanmce (Ryan McElroy)]

ryanmce requested changes to this revision.
ryanmce added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> cmdutil.py:722-731
> +    revs = scmutil.revrange(repo, revs)
> +    roots = repo.revs('roots(%ld)', revs)
> +    if len(roots) > 1:
> +        raise error.Abort(_("cannot change branch of non-linear revisions"))
> +    root = repo[roots.first()]
> +    if root.phase() <= phases.public:
> +        raise error.Abort(_("cannot change branch of public revisions"))

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

> cmdutil.py:733-734
> +
> +    replacements = {}
> +    with repo.wlock(), repo.lock(), repo.transaction('branches'):
> +        # avoid import cycle mercurial.cmdutil -> mercurial.context ->

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)

> cmdutil.py:791-792
> +            if newid is not None:
> +                # avoid import cycle mercurial.cmdutil -> mercurial.hg ->
> +                # mercurial.cmdutil
> +                from . import hg

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

> test-branch-change.t:8
> +  > [alias]
> +  > glog = log -G -T "{rev}:{node|short} {desc}\n{branch} ({bookmarks})"
> +  > [experimental]

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

> test-branch-change.t:10
> +  > [experimental]
> +  > evolution = createmarkers
> +  > [extensions]

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.

> test-branch-change.t:17-34
> +  $ for ch in a b c d e f g h; do echo foo >> $ch; hg ci -Aqm "Added "$ch; done
> +  $ hg glog
> +  @  7:ec2426147f0e Added h
> +  |  default ()
> +  o  6:87d6d6676308 Added g
> +  |  default ()
> +  o  5:825660c69f0c Added f

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

> test-branch-change.t:304-305
> +
> +  $ hg branch -r 23::28 wat
> +  changed branch on 0 changesets
> +  $ hg glog

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?

REPOSITORY
  rHG Mercurial

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

To: pulkit, #hg-reviewers, dlax, ryanmce
Cc: ryanmce, dlax, mercurial-devel