D6005: uncommit: added interactive mode -i(issue6062)

[martinvonz (Martin von Zweigbergk)]

martinvonz added a comment.


  Here are some little comments to start with. I haven't even started reviewing the three new functions, but I need to take a break and work on other things a bit now.

INLINE COMMENTS

> uncommit.py:107
>  
> -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 _fixdirstate(repo, oldctx, newctx, match, interactive=False):
> +    """Fix the dirstate after switching the working directory from

No need to pass the `interactive` flag here, right?

> uncommit.py:109
> +    """Fix the dirstate after switching the working directory from
> +    oldctx to a copy of oldctx not containing changed files matched by
> +    match.

I preferred the old wording where it said "newctx" instead of "a copy of oldctx".

> uncommit.py:109-110
> +    """Fix the dirstate after switching the working directory from
> +    oldctx to a copy of oldctx not containing changed files matched by
> +    match.
>      """

I don't follow the "not containing changed files matched by match" part. It *only* fixes the parts matched by the matches, right? Also, I think the matcher is just an optimization -- it would be incorrect, I think, to pass a matcher that doesn't match all the changes between oldctx and newctx. I'd just leave the comment untouched for now. We may even want to remove the matcher argument, but we can do that in a separate patch, if at all.

> uncommit.py:148
>  @command('uncommit',
> -    [('', 'keep', None, _('allow an empty commit after uncommiting')),
> +    [('i', 'interactive', False, _('interactive mode to uncommit')),
> +     ('', 'keep', None, _('allow an empty commit after uncommiting')),

Similar existing flags and their descriptions:

commit: "use interactive mode"
amend: "use interactive mode"
forget: "use interactive mode"
revert: "interactively select the changes"
absorb: "interactively select which chunks to apply"
shelve: "interactive mode, only works while creating a shelve"

I'd prefer to be consistent with one of those. I personally think revert's and absorb's versions are clearest.

REPOSITORY
  rHG Mercurial

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

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