This is an archive of the discontinued Mercurial Phabricator instance.

Differential D3672

retractboundary: add dryrun parameter
Needs RevisionPublic

Authored by khanchi97 on May 30 2018, 6:25 AM.

Details

Reviewers
baymax
Group Reviewers
hg-reviewers
Summary

Added the logic to find those csets whose phase will be changed
without --dry-run and return those csets.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

khanchi97 created this revision.May 30 2018, 6:25 AM
khanchi97 edited the summary of this revision. (Show Details)Jun 1 2018, 2:59 AM
khanchi97 updated this revision to Diff 8948.
pulkit added a subscriber: pulkit.Jun 1 2018, 5:32 AM
pulkit added inline comments.
mercurial/phases.py
395

Add documentation about dry-run and the return value.

417

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

430

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

Just like D3671, here also, the function should return the correct set of changes whose phase have been changed irrespective of the dryrun value passed.

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`

khanchi97 added inline comments.Jun 1 2018, 8:50 AM
mercurial/phases.py
395

okay

417

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?

430

yeah, I got it.

511

okay, will do this for 'advanceboundry' too

khanchi97 updated this revision to Diff 8959.Jun 3 2018, 9:51 AM
khanchi97 updated this revision to Diff 8960.Jun 3 2018, 9:55 AM
pulkit added a subscriber: yuja.Jul 11 2018, 5:24 PM
pulkit added inline comments.
mercurial/phases.py
417

I think affected can be used here. Maybe @yuja might know better.

@yuja we want to return a set of revs whose phase is changed by this function. Does affected looks like the correct set to you?

baymax requested changes to this revision.Jan 24 2020, 12:32 PM

There seems to have been no activities on this Diff for the past 3 Months.

By policy, we are automatically moving it out of the need-review state.

Please, move it back to need-review without hesitation if this diff should still be discussed.

:baymax:need-review-idle:

This revision now requires changes to proceed.Jan 24 2020, 12:32 PM

Revision Contents
Changeset List

PathPackages
Mmercurial/phases.py (63 lines)

Diff 8948

mercurial/phases.py

self._updateroots(phase, roots, tr) self._updateroots(phase, roots, tr)
# some roots may need to be declared for lower phases # some roots may need to be declared for lower phases
delroots.extend(olds - roots) delroots.extend(olds - roots)
# declare deleted root in the target phase # declare deleted root in the target phase
if targetphase != 0: if targetphase != 0:
self._retractboundary(repo, tr, targetphase, delroots) self._retractboundary(repo, tr, targetphase, delroots)
repo.invalidatevolatilesets() repo.invalidatevolatilesets()
def retractboundary(self, repo, tr, targetphase, nodes): def retractboundary(self, repo, tr, targetphase, nodes, dryrun=None):
pulkitUnsubmitted
Not Done

Add documentation about dry-run and the return value.

pulkit: Add documentation about dry-run and the return value.
khanchi97AuthorUnsubmitted
Not Done

okay

khanchi97: okay
oldroots = self.phaseroots[:targetphase + 1] oldroots = self.phaseroots[:targetphase + 1]
if tr is None: if tr is None:
phasetracking = None phasetracking = None
else: else:
phasetracking = tr.changes.get('phases') phasetracking = tr.changes.get('phases')
repo = repo.unfiltered() repo = repo.unfiltered()
changes = set() # set or revisions whose phase can be changed.
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))
changes.update(affected)
else:
if (self._retractboundary(repo, tr, targetphase, nodes) if (self._retractboundary(repo, tr, targetphase, nodes)
and phasetracking is not None): and phasetracking is not None):
# find the affected revisions # find the affected revisions
new = self.phaseroots[targetphase] new = self.phaseroots[targetphase]
old = oldroots[targetphase] old = oldroots[targetphase]
affected = set(repo.revs('(%ln::) - (%ln::)', new, old)) affected = set(repo.revs('(%ln::) - (%ln::)', new, old))
pulkitUnsubmitted
Not Done

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

pulkit: Why are we not using this affected set here to find the changesets whose phase is changed?
khanchi97AuthorUnsubmitted
Not Done

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?

khanchi97: Because I think this affected set is calculated after performing actions, when phases are…
pulkitUnsubmitted
Not Done

I think affected can be used here. Maybe @yuja might know better.

@yuja we want to return a set of revs whose phase is changed by this function. Does affected looks like the correct set to you?

pulkit: I think affected can be used here. Maybe @yuja might know better. @yuja we want to return a…
# find the phase of the affected revision # find the phase of the affected revision
for phase in xrange(targetphase, -1, -1): for phase in xrange(targetphase, -1, -1):
if phase: if phase:
roots = oldroots[phase] roots = oldroots[phase]
revs = set(repo.revs('%ln::%ld', roots, affected)) revs = set(repo.revs('%ln::%ld', roots, affected))
affected -= revs affected -= revs
else: # public phase else: # public phase
revs = affected revs = affected
for r in revs: for r in revs:
_trackphasechange(phasetracking, r, phase, targetphase) _trackphasechange(phasetracking, r, phase, targetphase)
repo.invalidatevolatilesets() repo.invalidatevolatilesets()
return changes
pulkitUnsubmitted
Not Done

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

Just like D3671, here also, the function should return the correct set of changes whose phase have been changed irrespective of the dryrun value passed.

pulkit: (Not sure which line I should put this comment on) Just like D3671, here also, the function…
khanchi97AuthorUnsubmitted
Not Done

yeah, I got it.

khanchi97: yeah, I got it.
def _retractboundary(self, repo, tr, targetphase, nodes): def _retractboundary(self, repo, tr, targetphase, nodes):
# Be careful to preserve shallow-copied values: do not update # Be careful to preserve shallow-copied values: do not update
# phaseroots values, replace them. # phaseroots values, replace them.
repo = repo.unfiltered() repo = repo.unfiltered()
currentroots = self.phaseroots[targetphase] currentroots = self.phaseroots[targetphase]
finalroots = oldroots = set(currentroots) finalroots = oldroots = set(currentroots)
This function move boundary *forward* this means that all nodes This function move boundary *forward* this means that all nodes
are set in the target phase or kept in a *lower* phase. are set in the target phase or kept in a *lower* phase.
Simplify boundary to contains phase roots only.""" Simplify boundary to contains phase roots only."""
phcache = repo._phasecache.copy() phcache = repo._phasecache.copy()
phcache.advanceboundary(repo, tr, targetphase, nodes) phcache.advanceboundary(repo, tr, targetphase, nodes)
repo._phasecache.replace(phcache) 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 """Set nodes back to a phase changing other nodes phases if
necessary. necessary.
This function move boundary *backward* this means that all nodes This function move boundary *backward* this means that all nodes
are set in the target phase or kept in a *higher* phase. are set in the target phase or kept in a *higher* phase.
Simplify boundary to contains phase roots only.""" Simplify boundary to contains phase roots only.
If dryrun is true then it will not perform any action and only returns
pulkitUnsubmitted
Not Done

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`

pulkit: This should better be: `If dryrun is True, no actions will be performed returns a set of revs…
khanchi97AuthorUnsubmitted
Not Done

okay, will do this for 'advanceboundry' too

khanchi97: okay, will do this for 'advanceboundry' too
set of revision whose phase can be changed.
"""
phcache = repo._phasecache.copy() phcache = repo._phasecache.copy()
phcache.retractboundary(repo, tr, targetphase, nodes) changes = phcache.retractboundary(repo, tr, targetphase, nodes,
dryrun=dryrun)
if not dryrun:
repo._phasecache.replace(phcache) repo._phasecache.replace(phcache)
return changes
def registernew(repo, tr, targetphase, nodes): def registernew(repo, tr, targetphase, nodes):
"""register a new revision and its phase """register a new revision and its phase
Code adding revisions to the repository should use this function to Code adding revisions to the repository should use this function to
set new changeset in their target phase (or higher). set new changeset in their target phase (or higher).
""" """
phcache = repo._phasecache.copy() phcache = repo._phasecache.copy()