D3671: advanceboundary: add dryrun parameter

pulkit (Pulkit Goyal) phabricator at mercurial-scm.org
Wed May 30 17:45:02 EDT 2018


pulkit requested changes to this revision.
pulkit added a comment.
This revision now requires changes to proceed.


  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?

INLINE COMMENTS

> phases.py:369
>  
> +        rejected = list()
> +        changes = [set(), set(), set()]

why are we maintaining this rejected list?

> phases.py:370
> +        rejected = list()
> +        changes = [set(), set(), set()]
> +        if dryrun:

can you explain what this changes list means?

> phases.py:371
> +        changes = [set(), set(), set()]
> +        if dryrun:
> +            getphase = repo._phasecache.phase

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

> phases.py:392
> +                changes[phase].update(faffected)
> +            else:
> +                for r in affected:

We can prevent this else by returning the values early.

> phases.py:496
>  
> -def advanceboundary(repo, tr, targetphase, nodes):
> +def advanceboundary(repo, tr, targetphase, nodes, dryrun=None):
>      """Add nodes to a phase changing other nodes phases if necessary.

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

REPOSITORY
  rHG Mercurial

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

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


More information about the Mercurial-devel mailing list