This is an archive of the discontinued Mercurial Phabricator instance.

Differential D3671

advanceboundary: add dryrun parameter
ClosedPublic

Authored by khanchi97 on May 30 2018, 5:27 AM.

Details

Reviewers
pulkit
Group Reviewers
hg-reviewers
Commits
rHG36ba5dba372d: advanceboundary: add dryrun parameter
Summary

Added logic to find those csets whose phase will be changed (when
running without --dryrun) while advancing boundary 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, 5:27 AM
pulkit requested changes to this revision.May 30 2018, 5:44 PM
pulkit added a subscriber: pulkit.

Thanks for breaking this up. I have some put comments which are mostly doubts, can you explain by replying to them why you did so?

mercurial/phases.py
369

why are we maintaining this rejected list?

370

can you explain what this changes list means?

371

This conditional looks unnecessary unless I am mistaken. Can you explain why we need this?

391

We can prevent this else by returning the values early.

487

We should add documentation about the new dryrun argument and the new return values.

This revision now requires changes to proceed.May 30 2018, 5:44 PM
khanchi97 added inline comments.May 31 2018, 1:23 AM
mercurial/phases.py
369

rejected list will contain those csets which will be rejected during advancing boundary and then return this list so that we can report for rejected csets during dry_run of phase command.

370

yeah, this list is for storing sets of changed csets, ordered from minimum phase to maximum phase.
For example, changes[0] is a set of those csets whose phase will be changed from --public to --targetphase. Similarly, changes[1] for --draft csets.
Do we need to add some comments to explain this thing?
Or do you want any change here?

371

I thought we would calculate rejected only when we are in dryrun mode.

391

yeah, I will make this change.

487

okay, I will do this in other patches too.

khanchi97 added inline comments.May 31 2018, 3:00 AM
mercurial/phases.py
371

okay its fine. I will remove this unnecessary condition. Because I think rejected value can also be used in phase method of mercurial/commands.py even when we are not in dryrun.

390

I think I can add continue here and prevent else from next block.

391

But we are returning values after 'for loop' ends. As I am updating changes data in each iteration of loop.
I think we can add continue in if dryrun: condition and prevent else.

khanchi97 updated this revision to Diff 8940.May 31 2018, 3:43 AM
khanchi97 edited the summary of this revision. (Show Details)Jun 1 2018, 2:29 AM
khanchi97 updated this revision to Diff 8947.
pulkit added a comment.Jun 1 2018, 5:25 AM

Nice work! Returning a set of changesets whose phase is changed or can be changed is a better API than returning a rejected list and a list of sets.

mercurial/phases.py
358

Add docs about dryrun and return value here too.

382

We should populate the changes set and return that even if we are performing actions.

496

The return value should be set of revisions who phase is changed or can be changed irrespective of whether dry-run is passed or not.

pulkit added a comment.Jun 1 2018, 5:27 AM

Also, can you add about why we are adding a dryrun parameter to functions here and what will be the behavior in the commit message too?

khanchi97 updated this revision to Diff 8949.Jun 1 2018, 8:08 AM
khanchi97 updated this revision to Diff 8950.Jun 1 2018, 8:30 AM
pulkit accepted this revision.Jun 3 2018, 8:16 AM
pulkit added inline comments.Jun 3 2018, 8:25 AM
mercurial/phases.py
500

Fixed this indentation to align with opening brackets in flight.

Closed by commit rHG36ba5dba372d: advanceboundary: add dryrun parameter (authored by khanchi97). · Explain WhyJun 3 2018, 8:28 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mmercurial/phases.py (31 lines)

Diff 8947

mercurial/phases.py

torev = repo.changelog.rev torev = repo.changelog.rev
phase = self.phase phase = self.phase
for n in nodes: for n in nodes:
rev = torev(n) rev = torev(n)
revphase = phase(repo, rev) revphase = phase(repo, rev)
_trackphasechange(phasetracking, rev, None, revphase) _trackphasechange(phasetracking, rev, None, revphase)
repo.invalidatevolatilesets() repo.invalidatevolatilesets()
def advanceboundary(self, repo, tr, targetphase, nodes): def advanceboundary(self, repo, tr, targetphase, nodes, dryrun=None):
"""Set all 'nodes' to phase 'targetphase' """Set all 'nodes' to phase 'targetphase'
Nodes with a phase lower than 'targetphase' are not affected. Nodes with a phase lower than 'targetphase' are not affected.
pulkitUnsubmitted
Not Done

Add docs about dryrun and return value here too.

pulkit: Add docs about dryrun and return value here too.
""" """
# 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.
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 of revisions to be changed
pulkitUnsubmitted
Not Done

why are we maintaining this rejected list?

pulkit: why are we maintaining this rejected list?
khanchi97AuthorUnsubmitted
Not Done

rejected list will contain those csets which will be rejected during advancing boundary and then return this list so that we can report for rejected csets during dry_run of phase command.

khanchi97: rejected list will contain those csets which will be rejected during advancing boundary and…
delroots = [] # set of root deleted by this path delroots = [] # set of root deleted by this path
pulkitUnsubmitted
Not Done

can you explain what this changes list means?

pulkit: can you explain what this changes list means?
khanchi97AuthorUnsubmitted
Not Done

yeah, this list is for storing sets of changed csets, ordered from minimum phase to maximum phase.
For example, changes[0] is a set of those csets whose phase will be changed from --public to --targetphase. Similarly, changes[1] for --draft csets.
Do we need to add some comments to explain this thing?
Or do you want any change here?

khanchi97: yeah, this list is for storing sets of changed csets, ordered from minimum phase to maximum…
for phase in xrange(targetphase + 1, len(allphases)): for phase in xrange(targetphase + 1, len(allphases)):
pulkitUnsubmitted
Not Done

This conditional looks unnecessary unless I am mistaken. Can you explain why we need this?

pulkit: This conditional looks unnecessary unless I am mistaken. Can you explain why we need this?
khanchi97AuthorUnsubmitted
Not Done

I thought we would calculate rejected only when we are in dryrun mode.

khanchi97: I thought we would calculate `rejected` only when we are in dryrun mode.
khanchi97AuthorUnsubmitted
Not Done

okay its fine. I will remove this unnecessary condition. Because I think rejected value can also be used in phase method of mercurial/commands.py even when we are not in dryrun.

khanchi97: okay its fine. I will remove this unnecessary condition. Because I think `rejected` value can…
# filter nodes that are not in a compatible phase already # filter nodes that are not in a compatible phase already
nodes = [n for n in nodes nodes = [n for n in nodes
if self.phase(repo, repo[n].rev()) >= phase] if self.phase(repo, repo[n].rev()) >= phase]
if not nodes: if not nodes:
break # no roots to move anymore break # no roots to move anymore
olds = self.phaseroots[phase] olds = self.phaseroots[phase]
affected = repo.revs('%ln::%ln', olds, nodes) affected = repo.revs('%ln::%ln', olds, nodes)
if dryrun:
changes.update(affected)
pulkitUnsubmitted
Not Done

We should populate the changes set and return that even if we are performing actions.

pulkit: We should populate the changes set and return that even if we are performing actions.
continue
for r in affected: for r in affected:
_trackphasechange(phasetracking, r, self.phase(repo, r), _trackphasechange(phasetracking, r, self.phase(repo, r),
targetphase) targetphase)
roots = set(ctx.node() for ctx in repo.set( roots = set(ctx.node() for ctx in repo.set(
'roots((%ln::) - %ld)', olds, affected)) 'roots((%ln::) - %ld)', olds, affected))
if olds != roots: if olds != roots:
khanchi97AuthorUnsubmitted
Not Done

I think I can add continue here and prevent else from next block.

khanchi97: I think I can add `continue` here and prevent `else` from next block.
self._updateroots(phase, roots, tr) self._updateroots(phase, roots, tr)
pulkitUnsubmitted
Not Done

We can prevent this else by returning the values early.

pulkit: We can prevent this else by returning the values early.
khanchi97AuthorUnsubmitted
Not Done

yeah, I will make this change.

khanchi97: yeah, I will make this change.
khanchi97AuthorUnsubmitted
Not Done

But we are returning values after 'for loop' ends. As I am updating changes data in each iteration of loop.
I think we can add continue in if dryrun: condition and prevent else.

khanchi97: But we are returning values after 'for loop' ends. As I am updating `changes` data in each…
# 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)
if not dryrun:
# 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()
return changes
def retractboundary(self, repo, tr, targetphase, nodes): def retractboundary(self, repo, tr, targetphase, nodes):
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()
# have not proper way to check that). related to issue 3858. # have not proper way to check that). related to issue 3858.
# #
# The other caller is __init__ that have no _phasesets initialized # The other caller is __init__ that have no _phasesets initialized
# anyway. If this change we should consider adding a dedicated # anyway. If this change we should consider adding a dedicated
# "destroyed" function to phasecache or a proper cache key mechanism # "destroyed" function to phasecache or a proper cache key mechanism
# (see branchmap one) # (see branchmap one)
self.invalidate() self.invalidate()
def advanceboundary(repo, tr, targetphase, nodes): def advanceboundary(repo, tr, targetphase, nodes, dryrun=None):
pulkitUnsubmitted
Not Done

We should add documentation about the new dryrun argument and the new return values.

pulkit: We should add documentation about the new dryrun argument and the new return values.
khanchi97AuthorUnsubmitted
Not Done

okay, I will do this in other patches too.

khanchi97: okay, I will do this in other patches too.
"""Add nodes to a phase changing other nodes phases if necessary. """Add nodes to a phase changing other nodes phases if necessary.
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.
If dryrun is true then it will not perform any action and only returns
revisions whose phase could be changed.
pulkitUnsubmitted
Not Done

The return value should be set of revisions who phase is changed or can be changed irrespective of whether dry-run is passed or not.

pulkit: The return value should be set of revisions who phase is changed or can be changed irrespective…
"""
phcache = repo._phasecache.copy() phcache = repo._phasecache.copy()
phcache.advanceboundary(repo, tr, targetphase, nodes) changes = phcache.advanceboundary(repo, tr, targetphase, nodes,
dryrun=dryrun)
pulkitUnsubmitted
Not Done

Fixed this indentation to align with opening brackets in flight.

pulkit: Fixed this indentation to align with opening brackets in flight.
if not dryrun:
repo._phasecache.replace(phcache) repo._phasecache.replace(phcache)
return changes
def retractboundary(repo, tr, targetphase, nodes): def retractboundary(repo, tr, targetphase, nodes):
"""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.