advanceboundary: add dryrun parameter
ClosedPublic

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

Details

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
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
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
375

why are we maintaining this rejected list?

376

can you explain what this changes list means?

377

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

397

We can prevent this else by returning the values early.

493

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
375

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.

376

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?

377

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

397

yeah, I will make this change.

493

okay, I will do this in other patches too.

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

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.

396

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

397

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 updated this revision to Diff 8947.Jun 1 2018, 2:29 AM
khanchi97 edited the summary of this revision. (Show Details)
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
360–364

Add docs about dryrun and return value here too.

388

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

502

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
507

Fixed this indentation to align with opening brackets in flight.

This revision was automatically updated to reflect the committed changes.