This is an archive of the discontinued Mercurial Phabricator instance.

cleanupnodes: separate out bookmark destination calculation from actual update
ClosedPublic

Authored by martinvonz on Sep 20 2017, 12:55 PM.

Details

Summary

We will soon want to pass in overrides for bookmark movements and this
will make that patch simpler. I also think this makes the code easier
to follow regardless of the later patch.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

martinvonz created this revision.Sep 20 2017, 12:55 PM
quark accepted this revision.Sep 21 2017, 6:06 PM
quark added a subscriber: quark.

I noticed the change from repo to unfi. That actually looks better. Since we are not querying "descendants", it won't cause correctness issues.

mercurial/scmutil.py
646

This could be removed then.

martinvonz marked an inline comment as done.Sep 21 2017, 6:11 PM
In D748#12944, @quark wrote:

I noticed the change from repo to unfi. That actually looks better. Since we are not querying "descendants", it won't cause correctness issues.

Yes, that was necessary because oldnode could be obsolete and that was previously protected by "if not oldbmarks", which I didn't think would belong in the new loop.

martinvonz updated this revision to Diff 1985.Sep 21 2017, 6:14 PM
quark requested changes to this revision.Sep 21 2017, 6:19 PM

Yes, that was necessary because oldnode could be obsolete and that was previously protected by "if not oldbmarks", which I didn't think would belong in the new loop.

Ah, I didn't realize that minor difference. So the new code could have unnecessary computation - it calculates "moves" for every node even if there is no move needed for it.

I think we still want an extra moves argument, but use it inside the for loop (so the if ...: continue shortcut works).

This revision now requires changes to proceed.Sep 21 2017, 6:19 PM
quark added a comment.Sep 21 2017, 6:25 PM

I think either you or me can update the code. If you're busy with other things, I can update (and split) the original fix directly.

In D748#12953, @quark wrote:

Yes, that was necessary because oldnode could be obsolete and that was previously protected by "if not oldbmarks", which I didn't think would belong in the new loop.

Ah, I didn't realize that minor difference. So the new code could have unnecessary computation - it calculates "moves" for every node even if there is no move needed for it.

Yes. I simply didn't think it would be a noticeable extra computation. For working copy movement that's coming in your later patch, I also assumed that would just do "newwc = moves[oldwc]" (simplified), but I now see that it looks more complex.

I think we still want an extra moves argument, but use it inside the for loop (so the if ...: continue shortcut works).

I'd rather move the " if not oldbmarks:" to the new loop if you think that's necessary.

quark accepted this revision.Sep 21 2017, 6:53 PM
quark added inline comments.
mercurial/scmutil.py
604

I thought this could be O(changelog), but it's O(1) given generatorset implicitly requires the generator to be sorted.

durin42 accepted this revision.Sep 30 2017, 9:09 AM
This revision is now accepted and ready to land.Sep 30 2017, 9:09 AM
martinvonz updated this revision to Diff 2196.Sep 30 2017, 10:16 AM
This revision was automatically updated to reflect the committed changes.