This is an archive of the discontinued Mercurial Phabricator instance.

commands: correctly show inactive multiheaded branches
ClosedPublic

Authored by the31k on Aug 31 2017, 1:12 PM.

Details

Summary

Issue being fixed here: hg branches incorrectly renders inactive multiheaded
branches as active if they have closed heads.

Example:

$ hg log --template '{rev}:{node|short} "{desc}" ({branch}) [parents: {parents}]\n'
4:2e2fa7af8357 "merge" (default) [parents: 0:c94e548c8c7d 3:7be622ae5832 ]
3:7be622ae5832 "2" (somebranch) [parents: 1:81c1d9458987 ]
2:be82cf30409c "close" (somebranch) [parents: ]
1:81c1d9458987 "1" (somebranch) [parents: ]
0:c94e548c8c7d "initial" (default) [parents: ]

$ hg branches
default                        4:2e2fa7af8357
somebranch                     3:7be622ae5832

Branch somebranch have two heads, the 1st one being closed (rev 2) and
the other one being merged into default (rev 3). This branch should be shown as
inactive one.

This happens because we intersect branch heads with repo heads to check branch
activity. In this case intersection in a set with one node (rev 2). This head
is closed but the branch is marked as active nevertheless.

Fix is to check branch activity by intersecting only open heads set.

Fixed output:

$ hg branches
default                        4:2e2fa7af8357
somebranch                     3:7be622ae5832 (inactive)

Relevant tests for multihead branches added to test-branches suite.

Implentation note about adding iteropen method:

At first I have tried to modify iterbranches is such a way that it would
filter out closed heads itself. For example it could have closed=False
parameter. But in this case we would have to filter closed tips as well.
Reasoning in terms of hg branches we actually are not allowed to do this.

Also, we need to do heads filtering only if tip is not closed itself. But if it
is - we are ok to skip filtering, because branch is already known to be inactive.

So we can't implement heads filtering in iterbranches in elegant way, because
we will end up with something like closed_heads=False or even
closed_heads_is_tip_is_open. Finally I decided to move this logic to the
branches function, adding iteropen helper method.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

the31k created this revision.Aug 31 2017, 1:12 PM
the31k edited the summary of this revision. (Show Details)Aug 31 2017, 1:24 PM
yuja requested changes to this revision.Sep 1 2017, 9:49 AM
yuja added a subscriber: yuja.

The logic looks correct. Can you add some tests?

mercurial/branchmap.py
225

iterbranches() could have closed=False option to filter out
closed heads, but in which case, the tip would have to follow
it as well.

I'm not sure which will be a better API.

This revision now requires changes to proceed.Sep 1 2017, 9:49 AM
the31k planned changes to this revision.Sep 1 2017, 10:09 AM
In D583#9714, @yuja wrote:

The logic looks correct. Can you add some tests?

Ok, I will try to.

mercurial/branchmap.py
225

I have thought about it, but reasoning in terms of hg branches code we actually are not allowed to filter out closed tip. Command needs to get closed tip if it is there.

Also, we need to do heads filtering only if tip is not closed itself. But if it is - we are ok to skip filtering, because branch is already known to be inactive.

We can't do this in iterbranches code in elegant way, because we will end up with something like closed_heads=False or even closed_heads_is_tip_is_open. I tried to go with closed_heads=False, but later decided to move this logic to branches function.

yuja added inline comments.Sep 1 2017, 10:23 AM
mercurial/branchmap.py
225

Fair enough. Thanks for elaborating that. Can you include it in
the commit message?

the31k edited the summary of this revision. (Show Details)Sep 3 2017, 7:42 AM
the31k edited edge metadata.Sep 3 2017, 9:06 AM
the31k edited the summary of this revision. (Show Details)
the31k updated this revision to Diff 1584.
the31k added inline comments.Sep 3 2017, 9:10 AM
tests/test-branches.t
432

I decided to move this commit before multihead branches testing takes place to keep this commit's revision number. Named the section "Reclose branch".

the31k marked an inline comment as done.Sep 3 2017, 9:11 AM
yuja accepted this revision.Sep 4 2017, 9:39 AM

Queued, thanks.

This revision is now accepted and ready to land.Sep 4 2017, 9:39 AM
This revision was automatically updated to reflect the committed changes.

Thank you. Should I propose this to stable branch as a bugfix?

yuja added a comment.Sep 4 2017, 10:47 AM

Maybe no. This is old bug and not serious.