Patchwork D3672: retractboundary: add dryrun parameter

login
register
mail settings
Submitter phabricator
Date May 30, 2018, 10:25 a.m.
Message ID <differential-rev-PHID-DREV-4wa44uie4jeb5uhrancq-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/31903/
State New
Headers show

Comments

phabricator - May 30, 2018, 10:25 a.m.
khanchi97 created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  Added the logic to find those csets whose phase will be changed
  without dry-run while retracting boundary and return those csets.

REPOSITORY
  rHG Mercurial

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

AFFECTED FILES
  mercurial/phases.py

CHANGE DETAILS




To: khanchi97, #hg-reviewers
Cc: mercurial-devel
phabricator - June 1, 2018, 9:32 a.m.
pulkit added inline comments.

INLINE COMMENTS

> phases.py:395
>  
> -    def retractboundary(self, repo, tr, targetphase, nodes):
> +    def retractboundary(self, repo, tr, targetphase, nodes, dryrun=None):
>          oldroots = self.phaseroots[:targetphase + 1]

Add documentation about dry-run and the return value.

> phases.py:417
> +                old = oldroots[targetphase]
> +                affected = set(repo.revs('(%ln::) - (%ln::)', new, old))
> +

Why are we not using this affected set here to find the changesets whose phase is changed?

> phases.py:430
> +            repo.invalidatevolatilesets()
> +        return changes
>  

(Not sure which line I should put this comment on)

Just like https://phab.mercurial-scm.org/D3671, here also, the function should return the correct set of changes whose phase have been changed irrespective of the dryrun value passed.

> phases.py:511
> +
> +    If dryrun is true then it will not perform any action and only returns
> +    set of revision whose phase can be changed.

This should better be:

`If dryrun is True, no actions will be performed

returns a set of revs whose phase is changed or should be changed`

REPOSITORY
  rHG Mercurial

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

To: khanchi97, #hg-reviewers
Cc: pulkit, mercurial-devel
phabricator - June 1, 2018, 12:50 p.m.
khanchi97 added inline comments.

INLINE COMMENTS

> pulkit wrote in phases.py:395
> Add documentation about dry-run and the return value.

okay

> pulkit wrote in phases.py:417
> Why are we not using this affected set here to find the changesets whose phase is changed?

Because I think this affected set is calculated after performing actions, when phases are changed. And calculating affected using new phaseroots. Am I missing something here?

> pulkit wrote in phases.py:430
> (Not sure which line I should put this comment on)
> 
> Just like https://phab.mercurial-scm.org/D3671, here also, the function should return the correct set of changes whose phase have been changed irrespective of the dryrun value passed.

yeah, I got it.

> pulkit wrote in phases.py:511
> This should better be:
> 
> `If dryrun is True, no actions will be performed
> 
> returns a set of revs whose phase is changed or should be changed`

okay, will do this for 'advanceboundry' too

REPOSITORY
  rHG Mercurial

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

To: khanchi97, #hg-reviewers
Cc: pulkit, mercurial-devel

Patch

diff --git a/mercurial/phases.py b/mercurial/phases.py
--- a/mercurial/phases.py
+++ b/mercurial/phases.py
@@ -392,32 +392,44 @@ 
             self._retractboundary(repo, tr, targetphase, delroots)
         repo.invalidatevolatilesets()
 
-    def retractboundary(self, repo, tr, targetphase, nodes):
+    def retractboundary(self, repo, tr, targetphase, nodes, dryrun=None):
         oldroots = self.phaseroots[:targetphase + 1]
         if tr is None:
             phasetracking = None
         else:
             phasetracking = tr.changes.get('phases')
         repo = repo.unfiltered()
-        if (self._retractboundary(repo, tr, targetphase, nodes)
-            and phasetracking is not None):
-
-            # find the affected revisions
-            new = self.phaseroots[targetphase]
-            old = oldroots[targetphase]
-            affected = set(repo.revs('(%ln::) - (%ln::)', new, old))
+        changes = [set(), set(), set()]
+        if dryrun:
+            getphase = repo._phasecache.phase
+            nds = [n for n in nodes
+                   if getphase(repo, repo[n].rev()) < targetphase]
+            targetphroots = oldroots[-1]
+            affected = set(repo.revs('(%ln::) - (%ln::)', nds, targetphroots))
+            for rev in affected:
+                revphase = getphase(repo, rev)
+                changes[revphase].update((rev,))
+        else:
+            if (self._retractboundary(repo, tr, targetphase, nodes)
+                and phasetracking is not None):
 
-            # find the phase of the affected revision
-            for phase in xrange(targetphase, -1, -1):
-                if phase:
-                    roots = oldroots[phase]
-                    revs = set(repo.revs('%ln::%ld', roots, affected))
-                    affected -= revs
-                else: # public phase
-                    revs = affected
-                for r in revs:
-                    _trackphasechange(phasetracking, r, phase, targetphase)
-        repo.invalidatevolatilesets()
+                # find the affected revisions
+                new = self.phaseroots[targetphase]
+                old = oldroots[targetphase]
+                affected = set(repo.revs('(%ln::) - (%ln::)', new, old))
+
+                # find the phase of the affected revision
+                for phase in xrange(targetphase, -1, -1):
+                    if phase:
+                        roots = oldroots[phase]
+                        revs = set(repo.revs('%ln::%ld', roots, affected))
+                        affected -= revs
+                    else: # public phase
+                        revs = affected
+                    for r in revs:
+                        _trackphasechange(phasetracking, r, phase, targetphase)
+            repo.invalidatevolatilesets()
+        return changes
 
     def _retractboundary(self, repo, tr, targetphase, nodes):
         # Be careful to preserve shallow-copied values: do not update
@@ -489,17 +501,20 @@ 
     phcache.advanceboundary(repo, tr, targetphase, nodes)
     repo._phasecache.replace(phcache)
 
-def retractboundary(repo, tr, targetphase, nodes):
+def retractboundary(repo, tr, targetphase, nodes, dryrun=None):
     """Set nodes back to a phase changing other nodes phases if
     necessary.
 
     This function move boundary *backward* this means that all nodes
     are set in the target phase or kept in a *higher* phase.
 
     Simplify boundary to contains phase roots only."""
     phcache = repo._phasecache.copy()
-    phcache.retractboundary(repo, tr, targetphase, nodes)
-    repo._phasecache.replace(phcache)
+    changes = phcache.retractboundary(repo, tr, targetphase, nodes,
+                                      dryrun=dryrun)
+    if not dryrun:
+        repo._phasecache.replace(phcache)
+    return changes
 
 def registernew(repo, tr, targetphase, nodes):
     """register a new revision and its phase