D748: cleanupnodes: separate out bookmark destination calculation from actual update

[martinvonz (Martin von Zweigbergk)]

martinvonz added a comment.


  In https://phab.mercurial-scm.org/D748#12953, @quark wrote:
  
  > In https://phab.mercurial-scm.org/D748#12948, @martinvonz 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.

REPOSITORY
  rHG Mercurial

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

To: martinvonz, #hg-reviewers, quark
Cc: quark, mercurial-devel