This is an archive of the discontinued Mercurial Phabricator instance.

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
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.

This revision was automatically updated to reflect the committed changes.