D6005: uncommit: added interactive mode and removed _fixdirstate()(issue6062)

[martinvonz (Martin von Zweigbergk)]

martinvonz added a comment.


  I haven't reviewed much of it, but here are some initial comments

INLINE COMMENTS

> uncommit.py:101
>  
> -def _fixdirstate(repo, oldctx, newctx, match=None):
> -    """ fix the dirstate after switching the working directory from oldctx to
> -    newctx which can be result of either unamend or uncommit.
> +def _uncommitdirstate(repo, oldctx, newctx, match, interactive):
> +    """Fix the dirstate after switching the working directory from

I don't think we need to rename it from `_fixdirstate()`, especially since it's used for both uncommit and unamend. (Fix commit message too, of course, if you switch back to the old name.)

> uncommit.py:114
> +        # us in defining the exact behavior
> +        m, a, r = repo.status(oldctx, ctx, match=match)[:3]
> +        for f in m:

I've been trying to avoid accessing status fields by index because it's not very readable. Can you use the same pattern as below instead (i.e. `s.modifed` etc)?

> uncommit.py:399
> +            match=None
> +            interactive=0
> +            _uncommitdirstate(repo, curctx, newpredctx, match, interactive)

Instead of defining a value here, specify the argument by name when you call the function: `_uncommitdirstate(..., interactive=False)`. But it's probably better to just make `interactive` default to `False` and not pass it here.

> uncommit.py:400
> +            interactive=0
> +            _uncommitdirstate(repo, curctx, newpredctx, match, interactive)
>  

Keep the match optional instead?

REPOSITORY
  rHG Mercurial

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

To: taapas1128, #hg-reviewers
Cc: martinvonz, mercurial-devel