( )⚙ D5477 branches: add -r option to show branch name(s) of a given rev (issue5948)

This is an archive of the discontinued Mercurial Phabricator instance.

branches: add -r option to show branch name(s) of a given rev (issue5948)
AbandonedPublic

Authored by navaneeth.suresh on Dec 24 2018, 6:35 AM.

Details

Reviewers
None
Group Reviewers
hg-reviewers

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

Didn't looked at the code in detail yet, but this patch needs tests.

Also, @yuja how do you think of the following:

0. I move metaedit command to core

  1. I move branch changing functionality to metaedit
  2. and then we implement hg branch -r <rev> to show a branch name?
yuja added a comment.Dec 24 2018, 7:58 AM
fm.startitem()

+ rev = ctx.rev()
+ if opts.get('rev') and rev not in revs:
+ continue

ctx points to the tipmost branch head. so rev not in revs doesn't mean any
of the revs do not belong to the branch.

yuja added a comment.Dec 24 2018, 7:58 AM
0. I move metaedit command to core
1. I move branch changing functionality to metaedit

No opinion about 0 and 1. I've never used the metaedit.

  1. and then we implement hg branch -r <rev> to show a branch name?

I want to avoid it because no other namespace commands (i.e. bookmark and tag)
behave as such.

In D5477#81095, @yuja wrote:
fm.startitem()

+ rev = ctx.rev()
+ if opts.get('rev') and rev not in revs:
+ continue

ctx points to the tipmost branch head. so rev not in revs doesn't mean any
of the revs do not belong to the branch.

Is there any possibility of workaround within this iteration itself @yuja? I can do outside the current loop by creating a branches list and map it by iterating over scmutil.revrange() as suggested by you. But, that won't fetch all details. Just changeset and corresponding branch. We need to keep the same output format, right?

yuja added a comment.Dec 25 2018, 7:17 AM
> ctx points to the tipmost branch head. so `rev not in revs` doesn't mean any
>  of the `revs` do not belong to the branch.
Is there any possibility of workaround within this iteration itself @yuja? I can do outside the current loop by creating a branches list and map it by iterating over scmutil.revrange() as suggested by you. But, that won't fetch all details. Just changeset and corresponding branch. We need to keep the same output format, right?

Well, I meant you can build a set of branches to select.

selectedbranches = {repo[r].branch() for r in revs}
for tag, ... in repo.branchmap().iterbranches():
    if tag not in selectedbranches:
        continue
    ...

This is the simplest implementation. Since repo[r].branch() isn't a cheap
operation, it's probably better to query a revbranchcache.

getbi = repo.revbranchcache().branchinfo
selectedbranches = {getbi(r)[0] for r in revs}

There will be more optimization ideas (e.g. look up branchmap directly if
len(revs) == 1, but they won't be required to implement a minimal working
patch.

@yuja Thank you so much for the information. I've updated the revision. Please review when you're free.

yuja added a comment.Dec 26 2018, 8:02 AM

Looks mostly good.

Can you update the commit message to conform to our style?
https://www.mercurial-scm.org/wiki/ContributingChanges#Submission_checklist

You'll see some lint errors if you run test-check-*. And you'll probably
need to update test-completion.t.

opts = pycompat.byteskwargs(opts)

+ revs = opts.get('rev')
+ if revs:
+ revs = scmutil.revrange(repo, revs)
+ getbi = repo.revbranchcache().branchinfo
+ selectedbranches = {getbi(r)[0] for r in revs}
+

ui.pager('branches')
fm = ui.formatter('branches', opts)
hexfunc = fm.hexfunc

@@ -1165,6 +1172,8 @@

allheads = set(repo.heads())
branches = []
for tag, heads, tip, isclosed in repo.branchmap().iterbranches():

+ if revs and tag not in selectedbranches:
+ continue

I prefer initializing selectedbranches to None, and check if it is None
here. It makes sure that selectedbranches never be an undefined name, and
avoid weird behavior when revs resolved to an empty set.

navaneeth.suresh retitled this revision from branches: Added -r option to show branch name(s) of a given rev (Issue5948) to branches: add -r option to show branch name(s) of a given rev (issue5948).Dec 26 2018, 9:39 AM
navaneeth.suresh updated this revision to Diff 12980.
In D5477#81155, @yuja wrote:

Looks mostly good.
Can you update the commit message to conform to our style?
https://www.mercurial-scm.org/wiki/ContributingChanges#Submission_checklist

Updated.

You'll see some lint errors if you run test-check-*. And you'll probably
need to update test-completion.t.

I've updated test-completion.t. But, I didn't see any lint errors on running test-check-*.

opts = pycompat.byteskwargs(opts)

+ revs = opts.get('rev')
+ if revs:
+ revs = scmutil.revrange(repo, revs)
+ getbi = repo.revbranchcache().branchinfo
+ selectedbranches = {getbi(r)[0] for r in revs}
+

ui.pager('branches')
fm = ui.formatter('branches', opts)
hexfunc = fm.hexfunc

@@ -1165,6 +1172,8 @@

allheads = set(repo.heads())
branches = []
for tag, heads, tip, isclosed in repo.branchmap().iterbranches():

+ if revs and tag not in selectedbranches:
+ continue

I prefer initializing selectedbranches to None, and check if it is None
here. It makes sure that selectedbranches never be an undefined name, and
avoid weird behavior when revs resolved to an empty set.

I didn't think of this. Thanks for pointing that out. It's fixed in this patch.

yuja added a comment.Dec 27 2018, 7:13 AM

I tried to queue this, but the patch doesn't include any metadata (e.g. author
and date.) Please check your configuration.

https://www.mercurial-scm.org/wiki/Phabricator

+ revs = opts.get('rev')
+ selectedbranches = None
+ if revs:
+ revs = scmutil.revrange(repo, revs)
+ getbi = repo.revbranchcache().branchinfo
+ selectedbranches = {getbi(r)[0] for r in revs}
+

ui.pager('branches')
fm = ui.formatter('branches', opts)
hexfunc = fm.hexfunc

@@ -1165,6 +1173,8 @@

allheads = set(repo.heads())
branches = []
for tag, heads, tip, isclosed in repo.branchmap().iterbranches():

+ if selectedbranches and tag not in selectedbranches:

Please update this to selectedbranches is not None. You can see a difference
with -r 'none()'.

In D5477#81096, @yuja wrote:
0. I move metaedit command to core
1. I move branch changing functionality to metaedit

No opinion about 0 and 1. I've never used the metaedit.

  1. and then we implement hg branch -r <rev> to show a branch name?

I want to avoid it because no other namespace commands (i.e. bookmark and tag)
behave as such.

We like the UX of hg branch --rev R XXX for changing the branch of a commit, it's the same UX than bookmark and topic. It's also easier than hg branch --force XXX; hg amend.

yuja added a comment.Dec 27 2018, 8:46 AM
We like the UX of `hg branch --rev R XXX` for changing the branch of a commit, it's the same UX than bookmark and topic. It's also easier than `hg branch --force XXX; hg amend`.

What I don't like about hg branch --rev is that it actually rewrites
the commits, which is IMHO different in UX from hg branch and hg bookmark.

It's also different in that both hg branch (with no rev) and hg bookmark [-r]
specify a branch head (suppose the working change will eventually be new
branch head), whereas hg branch -r REV specifies a set of revisions to be
re-branched.

(Maybe we should move the discussion to the list if it continues. I hate Phab
emails.)

In D5477#81186, @yuja wrote:

I tried to queue this, but the patch doesn't include any metadata (e.g. author
and date.) Please check your configuration.
https://www.mercurial-scm.org/wiki/Phabricator

@yuja I'm really sorry about that. I was updating the diff within the phabricator UI itself without committing the changes. Everything is fixed in D5486.

@yuja Can this be closed?

pulkit added a comment.Jan 2 2019, 7:23 AM

@yuja Can this be closed?

Yep you can close this one.

navaneeth.suresh abandoned this revision.Jan 2 2019, 7:23 AM