This is an archive of the discontinued Mercurial Phabricator instance.

rebase: rewrite _computeobsoletenotrebased
ClosedPublic

Authored by quark on Aug 11 2017, 5:01 AM.

Details

Summary

The old code stores successors of all related nodes together, which works
fine if destination is unique. A future patch would make destination
non-unique so let's change the implementation to test successors for
rebaseset separately.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

quark created this revision.Aug 11 2017, 5:01 AM
quark updated this revision to Diff 835.Aug 13 2017, 12:28 AM
quark updated this revision to Diff 899.Aug 14 2017, 8:11 PM
quark updated this revision to Diff 1146.Aug 22 2017, 1:16 AM
martinvonz added inline comments.
hgext/rebase.py
1498

Oh, allsuccessors() includes "srcnode" in the output! It took me a while to figure that out. A comment would help.

How about something like this?

successors = list(obsutil.allsuccessors(repo.obsstore, [srcnode]))
successors = successors[1:] # allsuccessors() includes the start nodes
if not successors:
    # prune
...
quark added inline comments.Aug 29 2017, 3:11 PM
hgext/rebase.py
1498

That copies the list therefore slightly less performant. I'll add a comment.

quark updated this revision to Diff 1416.Aug 29 2017, 9:40 PM
martinvonz added inline comments.Aug 30 2017, 12:18 AM
hgext/rebase.py
1499

"returns node itself if it finds obsmarkers" is technically correct, but but also returns it if there are no obsmarkers :-)

However, "When the list only contains one element, it means there is at least one obsmarker with srcnode as predecessor" is not correct; it does not mean that there is at least one obsmarker, it means that there are no successors.

Or did I misunderstand either the text or what allsuccessors() does?

I'll rephrase it in flight unless you tell me soon that I misunderstood.

This revision was automatically updated to reflect the committed changes.