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

login
register
mail settings
Submitter phabricator
Date Feb. 22, 2019, 6:18 p.m.
Message ID <differential-rev-PHID-DREV-xmmgsmwxjysvrkcckrxr-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/38874/
State New
Headers show

Comments

phabricator - Feb. 22, 2019, 6:18 p.m.
taapas1128 created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REPOSITORY
  rHG Mercurial

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

AFFECTED FILES
  hgext/uncommit.py
  tests/test-unamend.t
  tests/test-uncommit-interactive.t
  tests/test-uncommit.t

CHANGE DETAILS




To: taapas1128, #hg-reviewers
Cc: mercurial-devel
phabricator - Feb. 22, 2019, 6:29 p.m.
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
phabricator - Feb. 22, 2019, 7:51 p.m.
taapas1128 marked 4 inline comments as done.
taapas1128 added a comment.


  @martinvonz I made the changes you asked . However I am not sure why `test-unamend.t` is changing because when `interactive` is false so it should behave exactly like the previous `_fixdirstate()` functioned . Can you provide some help with this ? I will work further based on your review.

REPOSITORY
  rHG Mercurial

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

To: taapas1128, #hg-reviewers
Cc: martinvonz, mercurial-devel
phabricator - Feb. 28, 2019, 7:19 p.m.
taapas1128 added a subscriber: pulkit.
taapas1128 added a comment.


  @pulkit I am unable to figure out why the output for `test-unamend.t` is changing . Any hint on that ?

REPOSITORY
  rHG Mercurial

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

To: taapas1128, #hg-reviewers
Cc: pulkit, martinvonz, mercurial-devel
phabricator - Feb. 28, 2019, 9:27 p.m.
pulkit added inline comments.

INLINE COMMENTS

> uncommit.py:101
>      ds = repo.dirstate
> -    ds.setparents(newctx.node(), node.nullid)
>      copies = dict(ds.copies())

I just skimmed through changes and looks like this is missing in the changed version.

REPOSITORY
  rHG Mercurial

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

To: taapas1128, #hg-reviewers
Cc: pulkit, martinvonz, mercurial-devel
phabricator - Feb. 28, 2019, 9:46 p.m.
taapas1128 marked an inline comment as done.
taapas1128 added a comment.


  @pulkit Thanks for pointing that out . That was extremely silly of me . I have corrected that . Is there anything else to be changed ?

REPOSITORY
  rHG Mercurial

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

To: taapas1128, #hg-reviewers
Cc: pulkit, martinvonz, mercurial-devel
phabricator - March 7, 2019, 7:04 p.m.
martinvonz added a comment.


  As I said a while ago on IRC, it feels like `_fixdirstate()` should not need an `interactive` flag, and it should not even need to be specifically for uncommit. It feels like it should work the same for any case where we move to a different commit without changing the working copy, such as uncommit, unamend, amend, fix (though we may want optimized code paths for some of them). So I took this patch and tried the most naive attempt at unifying the interactive version with the non-interactive version, namely to just drop the code for the interactive path (see patch below). Surprisingly, all tests pass :) So maybe you can just apply the below patch and clean it up (drop the `if True`), or maybe you need to add tests.
  
    diff --git a/hgext/uncommit.py b/hgext/uncommit.py
    --- a/hgext/uncommit.py
    +++ b/hgext/uncommit.py
    @@ -109,59 +109,10 @@ def _fixdirstate(repo, oldctx, newctx, m
         oldctx to a copy of oldctx not containing changed files matched by
         match.
         """
    -    ctx = repo['.']
         ds = repo.dirstate
         ds.setparents(newctx.node(), node.nullid)
         copies = dict(ds.copies())
    -    if interactive:
    -        # In interactive cases, we will find the status between oldctx and ctx
    -        # and considering only the files which are changed between oldctx and
    -        # ctx, and the status of what changed between oldctx and ctx will help
    -        # us in defining the exact behavior
    -        s = oldctx.status(ctx, match=match)
    -        for f in s.modified:
    -            # These are files which are modified between oldctx and ctx which
    -            # contains two cases: 1) Were modified in oldctx and some
    -            # modifications are uncommitted
    -            # 2) Were added in oldctx but some part is uncommitted (this cannot
    -            # contain the case when added files are uncommitted completely as
    -            # that will result in status as removed not modified.)
    -            # Also any modifications to a removed file will result the status as
    -            # added, so we have only two cases. So in either of the cases, the
    -            # resulting status can be modified or clean.
    -            if ds[f] == 'r':
    -                # But the file is removed in the working directory, leaving that
    -                # as removed
    -                continue
    -            ds.normallookup(f)
    -
    -        for f in s.added:
    -            # These are the files which are added between oldctx and ctx(new
    -            # one), which means the files which were removed in oldctx
    -            # but uncommitted completely while making the ctx
    -            # This file should be marked as removed if the working directory
    -            # does not adds it back. If it's adds it back, we do a normallookup.
    -            # The file can't be removed in working directory, because it was
    -            # removed in oldctx
    -            if ds[f] == 'a':
    -                ds.normallookup(f)
    -                continue
    -            ds.remove(f)
    -
    -        for f in s.removed:
    -            # These are files which are removed between oldctx and ctx, which
    -            # means the files which were added in oldctx and were completely
    -            # uncommitted in ctx. If a added file is partially uncommitted, that
    -            # would have resulted in modified status, not removed.
    -            # So a file added in a commit, and uncommitting that addition must
    -            # result in file being stated as unknown.
    -            if ds[f] == 'r':
    -                # The working directory say it's removed, so lets make the file
    -                # unknown
    -                ds.drop(f)
    -                continue
    -            ds.add(f)
    -    else:
    +    if True:
             s = newctx.status(oldctx, match=match)
             for f in s.modified:
                 if ds[f] == 'r':

REPOSITORY
  rHG Mercurial

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

To: taapas1128, #hg-reviewers
Cc: pulkit, martinvonz, mercurial-devel
phabricator - March 7, 2019, 8:01 p.m.
taapas1128 added a comment.


  @martinvonz cleaned it up .

REPOSITORY
  rHG Mercurial

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

To: taapas1128, #hg-reviewers
Cc: pulkit, martinvonz, mercurial-devel
phabricator - March 7, 2019, 8:16 p.m.
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
phabricator - March 7, 2019, 9:02 p.m.
taapas1128 marked 3 inline comments as done.
taapas1128 added inline comments.

INLINE COMMENTS

> martinvonz wrote in uncommit.py:109-110
> 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.

It is the  part of _uncommitdirstate() that has been left behind. okay I am not changing that

REPOSITORY
  rHG Mercurial

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

To: taapas1128, #hg-reviewers
Cc: pulkit, martinvonz, mercurial-devel
phabricator - March 7, 2019, 9:07 p.m.
martinvonz added inline comments.

INLINE COMMENTS

> taapas1128 wrote in uncommit.py:109-110
> It is the  part of _uncommitdirstate() that has been left behind. okay I am not changing that

> It is the part of _uncommitdirstate() that has been left behind. okay I am not changing that

Are you referring to https://phab.mercurial-scm.org/D971? I see that your change kind of backs out that change. But why?

REPOSITORY
  rHG Mercurial

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

To: taapas1128, #hg-reviewers
Cc: pulkit, martinvonz, mercurial-devel
phabricator - March 8, 2019, 7:05 a.m.
taapas1128 marked an inline comment as done.
taapas1128 added inline comments.

INLINE COMMENTS

> martinvonz wrote in uncommit.py:109-110
> > It is the part of _uncommitdirstate() that has been left behind. okay I am not changing that
> 
> Are you referring to https://phab.mercurial-scm.org/D971? I see that your change kind of backs out that change. But why?

In line number `115` a matcher is used . This is between oldctx and netctx now . In https://phab.mercurial-scm.org/D971 a the matcher was removed so the comment was altered . But whenever the matcher was reused the comment wasn't changed .

REPOSITORY
  rHG Mercurial

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

To: taapas1128, #hg-reviewers
Cc: pulkit, martinvonz, mercurial-devel
phabricator - March 8, 2019, 3:03 p.m.
martinvonz added inline comments.

INLINE COMMENTS

> taapas1128 wrote in uncommit.py:109-110
> In line number `115` a matcher is used . This is between oldctx and netctx now . In https://phab.mercurial-scm.org/D971 a the matcher was removed so the comment was altered . But whenever the matcher was reused the comment wasn't changed .

So you're fixing something that was missed in an old commit? Please put that fix in a separate patch.

REPOSITORY
  rHG Mercurial

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

To: taapas1128, #hg-reviewers
Cc: pulkit, martinvonz, mercurial-devel
phabricator - March 8, 2019, 3:24 p.m.
taapas1128 added inline comments.

INLINE COMMENTS

> martinvonz wrote in uncommit.py:109-110
> So you're fixing something that was missed in an old commit? Please put that fix in a separate patch.

Okay . Then I am reverting this back to what it was.

REPOSITORY
  rHG Mercurial

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

To: taapas1128, #hg-reviewers
Cc: pulkit, martinvonz, mercurial-devel
phabricator - March 8, 2019, 5:39 p.m.
taapas1128 added inline comments.

INLINE COMMENTS

> martinvonz wrote in uncommit.py:399
> 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.

@martinvonz this part of the patch too (`323,329,366`) has no relation with interactive uncommit but it makes `match` optional for `unamend`. Should I remove is and send it as a seperate patch too ?

REPOSITORY
  rHG Mercurial

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

To: taapas1128, #hg-reviewers
Cc: pulkit, martinvonz, mercurial-devel
phabricator - March 8, 2019, 5:41 p.m.
martinvonz added inline comments.

INLINE COMMENTS

> taapas1128 wrote in uncommit.py:399
> @martinvonz this part of the patch too (`323,329,366`) has no relation with interactive uncommit but it makes `match` optional for `unamend`. Should I remove is and send it as a seperate patch too ?

Yes, please do. Then we can consider while reviewing that patch if we even need to pass the matcher. It's better to have that discussion separate from this patch.

REPOSITORY
  rHG Mercurial

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

To: taapas1128, #hg-reviewers
Cc: pulkit, martinvonz, mercurial-devel
phabricator - March 11, 2019, 3:39 p.m.
taapas1128 added inline comments.

INLINE COMMENTS

> martinvonz wrote in uncommit.py:399
> Yes, please do. Then we can consider while reviewing that patch if we even need to pass the matcher. It's better to have that discussion separate from this patch.

okay sending a separate patch right away

REPOSITORY
  rHG Mercurial

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

To: taapas1128, #hg-reviewers
Cc: pulkit, martinvonz, mercurial-devel
phabricator - March 12, 2019, 6:20 p.m.
martinvonz added a comment.


  Can you update the commit message to explain how this works? I think I saw somewhere else that it first uncommits everything and then does an interactive amend. Is that correct?
  
  Also, I just tried to use this feature and it failed (depending on how many lines were added). I added two lines to a file, committed, then ran `hg uncommit -i` and selected the first line. Then I got this:
  
    starting interactive selection
    patching file z
    Hunk #1 succeeded at 2 with fuzz 2 (offset -1 lines).

REPOSITORY
  rHG Mercurial

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

To: taapas1128, #hg-reviewers
Cc: pulkit, martinvonz, mercurial-devel
phabricator - March 12, 2019, 6:48 p.m.
taapas1128 added a comment.


  I suppose no .. because in the very initial patch I combined `hg uncommit` with `hg commit -i`(as was stated in the bug description(https://bz.mercurial-scm.org/show_bug.cgi?id=6062)) . But later on I was asked to import it from `evolve` . The basic functionality lies in `_interactiveuncommit()` and the other two functions are imported as dependencies to that.

REPOSITORY
  rHG Mercurial

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

To: taapas1128, #hg-reviewers
Cc: pulkit, martinvonz, mercurial-devel
phabricator - March 13, 2019, 8:32 a.m.
pulkit added a comment.


  In https://phab.mercurial-scm.org/D6005#89182, @martinvonz wrote:
  
  > Can you update the commit message to explain how this works? I think I saw somewhere else that it first uncommits everything and then does an interactive amend. Is that correct?
  
  
  Yes, this is correct.
  
  > Also, I just tried to use this feature and it failed (depending on how many lines were added). I added two lines to a file, committed, then ran `hg uncommit -i` and selected the first line. Then I got this:
  > 
  >   starting interactive selection
  >   patching file z
  >   Hunk #1 succeeded at 2 with fuzz 2 (offset -1 lines).
  
  Yes this is also expected with the current implementation. Sometimes uncommit can fail and it will report `Hunk Failed` like this. There is definitely lot to improve in this implementation and I am fine with having this in core first.

REPOSITORY
  rHG Mercurial

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

To: taapas1128, #hg-reviewers
Cc: pulkit, martinvonz, mercurial-devel
phabricator - March 13, 2019, 5:13 p.m.
martinvonz added subscribers: spectral, ryanmce.
martinvonz added a comment.


  In https://phab.mercurial-scm.org/D6005#89211, @pulkit wrote:
  
  > In https://phab.mercurial-scm.org/D6005#89182, @martinvonz wrote:
  >
  > > Can you update the commit message to explain how this works? I think I saw somewhere else that it first uncommits everything and then does an interactive amend. Is that correct?
  >
  >
  > Yes, this is correct.
  >
  > > Also, I just tried to use this feature and it failed (depending on how many lines were added). I added two lines to a file, committed, then ran `hg uncommit -i` and selected the first line. Then I got this:
  > > 
  > >   starting interactive selection
  > >   patching file z
  > >   Hunk #1 succeeded at 2 with fuzz 2 (offset -1 lines).
  >
  > Yes this is also expected with the current implementation. Sometimes uncommit can fail and it will report `Hunk Failed` like this. There is definitely lot to improve in this implementation and I am fine with having this in core first.
  
  
  That definitely should be in the commit message.

INLINE COMMENTS

> uncommit.py:148
> +    [('i', 'interactive', None,
> +     _('interactively select which chunks to apply')),
> +     ('', 'keep', None, _('allow an empty commit after uncommiting')),

Add " (EXPERIMENTAL)" to the end of this description since the feature doesn't work with curses. That way it won't be visible when the user runs `hg help uncommit`.

> uncommit.py:191
>                      keepcommit = ui.configbool('experimental', 'uncommit.keep')
>              newid = _commitfiltered(repo, old, match, keepcommit)
> +            if interactive:

I think you're relying on this empty commit to not be saved, but I think it will be if the user has set `experiemtal.uncommit.keep` (or if they passed `--keep`). It seems better to not even attempt to create it if we don't want it.

> uncommit.py:214-215
> +def _interactiveuncommit(ui, repo, old, match):
> +    """ The function which contains all the logic for interactively uncommiting
> +    a commit. This function makes a temporary commit with the chunks which user
> +    selected to uncommit. After that the diff of the parent and that commit is

nit: No need to say that it's a function, and the "for interactively uncommiting a commit" seems pretty obvious from the name. Just start with "Makes a temporary commit..." instead.

> uncommit.py:230
> +                                     opts=diffopts):
> +            fp.write(chunk)
> +

nit: indent by 4 spaces

> uncommit.py:235
> +    # creating obs marker temp -> ()
> +    obsolete.createmarkers(repo, [(repo[tempnode], ())], operation="uncommit")
> +    return newnode

Should probably use `scmutil.cleanupnodes()` here too?

> uncommit.py:253-254
> +    # uncommit a removed file partially.
> +    # TODO: wrap the operations in mercurial/patch.py and mercurial/crecord.py
> +    # to add uncommit as an operation taking care of BC.
> +    chunks, opts = cmdutil.recordfilter(repo.ui, originalchunks,

I think it's fine to do it directly there, without wrapping. We're shipping this extension with core, so it shouldn't be a problem. It will have no effect unless you've enabled the extension, as far as I can tell.

> uncommit.py:261
> +    for c in chunks:
> +            c.write(fp)
> +

nit: indent by 4 spaces

> uncommit.py:270
> +def _patchtocommit(ui, repo, old, fp, message=None, extras=None):
> +    """ A function which will apply the patch to the working directory and
> +    make a commit whose parents are same as that of old argument. The message

Same nit here: No need to say that it's a function, just start with "Applies the patch..."

> test-uncommit-interactive.t:110
> +   3
> +  discard change 1/3 to 'a'? [Ynesfdaq?] n
> +  

I think at least @spectral and @ryanmce had previously suggested that `hg revert -i` should ask you what pieces to keep, not what to discard. Your patch reminded me of that discussion, so I sent https://phab.mercurial-scm.org/D6125. I'm curious to hear what others think about `hg uncommit -i`. Should it ask you what to discard or what to keep? I can see arguments both ways.

Perhaps the strongest argument in favor if showing what to keep is that it makes uncommitting part of a line *much* easier. Let's say you changed a line from "a" to "c" in the commit and you meant to change it to "b" in the commit and leave the change to "c" in the working copy. You'd be presented with this:

  -a
  +c

If what you select is what to keep in the commit, then it seems pretty obvious that you should change that to:

  -a
  +b

It's much less obvious what to do if you select what to discard.

I think it would also make it easier to implement this feature if the user selected what to keep (we would create a commit based on that and then move the dirstate there).

REPOSITORY
  rHG Mercurial

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

To: taapas1128, #hg-reviewers
Cc: ryanmce, spectral, pulkit, martinvonz, mercurial-devel
phabricator - March 14, 2019, 4:08 p.m.
taapas1128 added a comment.


  @martinvonz I have made the necessary modifications and updated the description.

INLINE COMMENTS

> martinvonz wrote in test-uncommit-interactive.t:110
> I think at least @spectral and @ryanmce had previously suggested that `hg revert -i` should ask you what pieces to keep, not what to discard. Your patch reminded me of that discussion, so I sent https://phab.mercurial-scm.org/D6125. I'm curious to hear what others think about `hg uncommit -i`. Should it ask you what to discard or what to keep? I can see arguments both ways.
> 
> Perhaps the strongest argument in favor if showing what to keep is that it makes uncommitting part of a line *much* easier. Let's say you changed a line from "a" to "c" in the commit and you meant to change it to "b" in the commit and leave the change to "c" in the working copy. You'd be presented with this:
> 
>   -a
>   +c
> 
> If what you select is what to keep in the commit, then it seems pretty obvious that you should change that to:
> 
>   -a
>   +b
> 
> It's much less obvious what to do if you select what to discard.
> 
> I think it would also make it easier to implement this feature if the user selected what to keep (we would create a commit based on that and then move the dirstate there).

Can I work on this as a part as a follow up ? or should i make amends in this patch .

REPOSITORY
  rHG Mercurial

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

To: taapas1128, #hg-reviewers
Cc: ryanmce, spectral, pulkit, martinvonz, mercurial-devel
phabricator - March 15, 2019, 7:40 p.m.
martinvonz added a comment.


  Can you wrap the commit message to 80 columns? I didn't find anything on https://www.mercurial-scm.org/wiki/ContributingChanges about it, but that's how almost everyone else does it.

INLINE COMMENTS

> uncommit.py:185-190
>              keepcommit = pats
>              if not keepcommit:
>                  if opts.get('keep') is not None:
>                      keepcommit = opts.get('keep')
>                  else:
>                      keepcommit = ui.configbool('experimental', 'uncommit.keep')

It looks like this is only useful in the non-interactive case. It's generally preferred to avoid unnecessary work, both because it's wasting computer resources (not really an issue here) and because it makes it clearer how the code works. In this case, it will make it clear that the `--keep` option does not matter with `--interactive`. However, it seems reasonable for `--keep` (and the corresponding config option) to be respected with `--interactive`. Maybe you should pass `keep` it into `_interactiveuncommit()` and use it there? I'd be fine with just leaving a TODO about that (and leave the `keepcommit` code here).

> uncommit.py:216
> +def _interactiveuncommit(ui, repo, old, match):
> +    """ Makes a temporary commit with the chunks which user
> +    selected to uncommit. After that the diff of the parent and that commit is

nit: remove the leading space here and in other docstrings for consistency (we seem to have about 60 instances with a leading space and 2900 without)

> uncommit.py:241
> +
> +def _createtempcommit(ui, repo, old, match):
> +    """ Creates a temporary commit for `uncommit --interative` which contains

Do we need to create the temporary commit? I found it hard to reason about (the temporary commit contains the changes that should be removed, which confused me) and we should ideally not leave that commit in the repo. I tried to rewrite it to not write the temporary commit. You can see my patch at http://paste.debian.net/1073278/. As you can see in the changed test case there, it doesn't work with added files. I don't know if that's because of the crecord bug that you mention on line 254 of this version or something else. Hopefully the patch is still a good start and maybe you can fix that bug. It would be great if you can even fix it in crecord (if that's where it is), so all users of crecord can benefit.

> uncommit.py:265
> +    fp.seek(0)
> +    oldnode = node.hex(old.node())[:12]
> +    message = 'temporary commit for uncommiting %s' % oldnode

`amend_source` includes the full hash. I think it's better to do the same here.

> uncommit.py:270
> +
> +def _patchtocommit(ui, repo, old, fp, message=None, extras=None):
> +    """ Applies the patch to the working directory and

`extras` is a confusing name for a node (it sounds too much like it's a changeset extras dict). I suggest renaming it to `oldnode`.

> martinvonz wrote in uncommit.py:235
> Should probably use `scmutil.cleanupnodes()` here too?

I didn't mean "use `cleanupnodes()` too", I meant "here too" :) I.e., use `cleanupnodes()` here just like we do elsewhere in this file, not in addition to using `createmarkers()`. So just delete the `createmarkers()` call.

> martinvonz wrote in uncommit.py:253-254
> I think it's fine to do it directly there, without wrapping. We're shipping this extension with core, so it shouldn't be a problem. It will have no effect unless you've enabled the extension, as far as I can tell.

It doesn't look done to me...

> taapas1128 wrote in test-uncommit-interactive.t:110
> Can I work on this as a part as a follow up ? or should i make amends in this patch .

I think it's still unclear if we even want to make that change (I think I would prefer it, but I'd like to hear from others), so let's leave it as is for now.

REPOSITORY
  rHG Mercurial

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

To: taapas1128, #hg-reviewers
Cc: ryanmce, spectral, pulkit, martinvonz, mercurial-devel
phabricator - March 16, 2019, 10:12 a.m.
taapas1128 added inline comments.

INLINE COMMENTS

> martinvonz wrote in uncommit.py:253-254
> It doesn't look done to me...

I am a little puzzled about what is to be done here . the TODO states that operations are currently not wrapped . how do you want me to continue ?

> martinvonz wrote in uncommit.py:241
> Do we need to create the temporary commit? I found it hard to reason about (the temporary commit contains the changes that should be removed, which confused me) and we should ideally not leave that commit in the repo. I tried to rewrite it to not write the temporary commit. You can see my patch at http://paste.debian.net/1073278/. As you can see in the changed test case there, it doesn't work with added files. I don't know if that's because of the crecord bug that you mention on line 254 of this version or something else. Hopefully the patch is still a good start and maybe you can fix that bug. It would be great if you can even fix it in crecord (if that's where it is), so all users of crecord can benefit.

do you want me to remove the temporary commit part in this patch or a follow up of this because that will require fixing it up for added files.

REPOSITORY
  rHG Mercurial

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

To: taapas1128, #hg-reviewers
Cc: ryanmce, spectral, pulkit, martinvonz, mercurial-devel
phabricator - March 16, 2019, 2:18 p.m.
martinvonz added inline comments.

INLINE COMMENTS

> taapas1128 wrote in uncommit.py:253-254
> I am a little puzzled about what is to be done here . the TODO states that operations are currently not wrapped . how do you want me to continue ?

I suggested addressing the todo now instead of waiting.

> taapas1128 wrote in uncommit.py:241
> do you want me to remove the temporary commit part in this patch or a follow up of this because that will require fixing it up for added files.

Yes, please try to fix it. Did you try?

REPOSITORY
  rHG Mercurial

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

To: taapas1128, #hg-reviewers
Cc: ryanmce, spectral, pulkit, martinvonz, mercurial-devel
phabricator - March 16, 2019, 3:52 p.m.
taapas1128 added inline comments.

INLINE COMMENTS

> martinvonz wrote in uncommit.py:253-254
> I suggested addressing the todo now instead of waiting.

okay sure.

> martinvonz wrote in uncommit.py:241
> Yes, please try to fix it. Did you try?

yes I applied your patch and I m trying to figure out what is going wrong . I will send a fix soon.

REPOSITORY
  rHG Mercurial

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

To: taapas1128, #hg-reviewers
Cc: ryanmce, spectral, pulkit, martinvonz, mercurial-devel
phabricator - March 18, 2019, 5:17 p.m.
pulkit added inline comments.

INLINE COMMENTS

> uncommit.py:258
> +    chunks, opts = cmdutil.recordfilter(repo.ui, originalchunks,
> +                                        operation='discard')
> +    if not chunks:

This operation thing can be used to show `select hunks to uncommit` message.

REPOSITORY
  rHG Mercurial

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

To: taapas1128, #hg-reviewers
Cc: ryanmce, spectral, pulkit, martinvonz, mercurial-devel
phabricator - March 19, 2019, 11:38 a.m.
pulkit added inline comments.

INLINE COMMENTS

> patch.py:1046
> +            'uncommit': _('[Ynesfdaq?]'
> +                        '$$ &Yes, record this change'
> +                        '$$ &No, skip this change'

this should be `uncommit this change` right?

> patch.py:1050
> +                        '$$ &Skip remaining changes to this file'
> +                        '$$ Record remaining changes to this &file'
> +                        '$$ &Done, skip remaining changes and files'

`uncommit` instead of `record` here too.

> patch.py:1052
> +                        '$$ &Done, skip remaining changes and files'
> +                        '$$ Record &all changes to all remaining files'
> +                        '$$ &Quit, recording no changes'

here also, uncommit instead of record.

> patch.py:1053
> +                        '$$ Record &all changes to all remaining files'
> +                        '$$ &Quit, recording no changes'
> +                        '$$ &? (display help)'),

uncommitting instead of recording

REPOSITORY
  rHG Mercurial

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

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

Patch

diff --git a/tests/test-uncommit.t b/tests/test-uncommit.t
--- a/tests/test-uncommit.t
+++ b/tests/test-uncommit.t
@@ -34,6 +34,7 @@ 
   
   options ([+] can be repeated):
   
+   -i --interactive         interactive mode to uncommit
       --keep                allow an empty commit after uncommiting
    -I --include PATTERN [+] include names matching the given patterns
    -X --exclude PATTERN [+] exclude names matching the given patterns
diff --git a/tests/test-uncommit-interactive.t b/tests/test-uncommit-interactive.t
new file mode 100644
--- /dev/null
+++ b/tests/test-uncommit-interactive.t
@@ -0,0 +1,969 @@ 
+================================================
+||  The test for `hg uncommit --interactive`  ||
+================================================
+
+Repo Setup
+============
+
+  $ cat >> $HGRCPATH <<EOF
+  > [ui]
+  > interactive = true
+  > [experimental]
+  > evolution.createmarkers=True
+  > evolution.allowunstable=True
+  > uncommitondirtywdir = true
+  > [extensions]
+  > uncommit =
+  > amend =
+  > drawdag=$TESTDIR/drawdag.py
+  > EOF
+  $ glog() {
+  >   hg log -G --template '{rev}:{node|short}@{branch}({separate("/", obsolete, phase)}) {desc|firstline}\n' "$@"
+  > }
+
+  $ hg init repo
+  $ cd repo
+
+  $ touch a
+  $ cat >> a << EOF
+  > 1
+  > 2
+  > 3
+  > 4
+  > 5
+  > EOF
+
+  $ hg add a
+  $ hg ci -m "The base commit"
+
+Make sure aborting the interactive selection does no magic
+----------------------------------------------------------
+
+  $ hg status
+  $ hg uncommit -i<<EOF
+  > q
+  > EOF
+  diff --git a/a b/a
+  new file mode 100644
+  examine changes to 'a'? [Ynesfdaq?] q
+  
+  abort: user quit
+  [255]
+  $ hg status
+
+Make a commit with multiple hunks
+---------------------------------
+
+  $ cat > a << EOF
+  > -2
+  > -1
+  > 0
+  > 1
+  > 2
+  > 3
+  > foo
+  > bar
+  > 4
+  > 5
+  > babar
+  > EOF
+
+  $ hg diff
+  diff -r 7733902a8d94 a
+  --- a/a	Thu Jan 01 00:00:00 1970 +0000
+  +++ b/a	Thu Jan 01 00:00:00 1970 +0000
+  @@ -1,5 +1,11 @@
+  +-2
+  +-1
+  +0
+   1
+   2
+   3
+  +foo
+  +bar
+   4
+   5
+  +babar
+
+  $ hg ci -m "another one"
+
+Not selecting anything to uncommit
+==================================
+
+  $ hg uncommit -i<<EOF
+  > y
+  > n
+  > n
+  > n
+  > EOF
+  diff --git a/a b/a
+  3 hunks, 6 lines changed
+  examine changes to 'a'? [Ynesfdaq?] y
+  
+  @@ -1,3 +1,6 @@
+  +-2
+  +-1
+  +0
+   1
+   2
+   3
+  discard change 1/3 to 'a'? [Ynesfdaq?] n
+  
+  @@ -1,5 +4,7 @@
+   1
+   2
+   3
+  +foo
+  +bar
+   4
+   5
+  discard change 2/3 to 'a'? [Ynesfdaq?] n
+  
+  @@ -4,2 +9,3 @@
+   4
+   5
+  +babar
+  discard change 3/3 to 'a'? [Ynesfdaq?] n
+  
+  abort: nothing selected to uncommit
+  [255]
+  $ hg status
+
+Uncommit a chunk
+================
+
+  $ hg uncommit -i<<EOF
+  > y
+  > y
+  > n
+  > n
+  > EOF
+  diff --git a/a b/a
+  3 hunks, 6 lines changed
+  examine changes to 'a'? [Ynesfdaq?] y
+  
+  @@ -1,3 +1,6 @@
+  +-2
+  +-1
+  +0
+   1
+   2
+   3
+  discard change 1/3 to 'a'? [Ynesfdaq?] y
+  
+  @@ -1,5 +4,7 @@
+   1
+   2
+   3
+  +foo
+  +bar
+   4
+   5
+  discard change 2/3 to 'a'? [Ynesfdaq?] n
+  
+  @@ -4,2 +9,3 @@
+   4
+   5
+  +babar
+  discard change 3/3 to 'a'? [Ynesfdaq?] n
+  
+
+  $ hg log -G --hidden
+  @  changeset:   3:678a59e5ff90
+  |  tag:         tip
+  |  parent:      0:7733902a8d94
+  |  user:        test
+  |  date:        Thu Jan 01 00:00:00 1970 +0000
+  |  summary:     another one
+  |
+  | x  changeset:   2:e9635f4beaf1
+  |/   parent:      0:7733902a8d94
+  |    user:        test
+  |    date:        Thu Jan 01 00:00:00 1970 +0000
+  |    obsolete:    pruned using uncommit
+  |    summary:     temporary commit for uncommiting f70fb463d5bf
+  |
+  | x  changeset:   1:f70fb463d5bf
+  |/   user:        test
+  |    date:        Thu Jan 01 00:00:00 1970 +0000
+  |    obsolete:    rewritten using uncommit as 3:678a59e5ff90
+  |    summary:     another one
+  |
+  o  changeset:   0:7733902a8d94
+     user:        test
+     date:        Thu Jan 01 00:00:00 1970 +0000
+     summary:     The base commit
+  
+The unselected part should be in the diff
+-----------------------------------------
+
+  $ hg diff
+  diff -r 678a59e5ff90 a
+  --- a/a	Thu Jan 01 00:00:00 1970 +0000
+  +++ b/a	Thu Jan 01 00:00:00 1970 +0000
+  @@ -1,3 +1,6 @@
+  +-2
+  +-1
+  +0
+   1
+   2
+   3
+
+The commit should contain the rest of part
+------------------------------------------
+
+  $ hg exp
+  # HG changeset patch
+  # User test
+  # Date 0 0
+  #      Thu Jan 01 00:00:00 1970 +0000
+  # Node ID 678a59e5ff90754d5e94719bd82ad169be773c21
+  # Parent  7733902a8d94c789ca81d866bea1893d79442db6
+  another one
+  
+  diff -r 7733902a8d94 -r 678a59e5ff90 a
+  --- a/a	Thu Jan 01 00:00:00 1970 +0000
+  +++ b/a	Thu Jan 01 00:00:00 1970 +0000
+  @@ -1,5 +1,8 @@
+   1
+   2
+   3
+  +foo
+  +bar
+   4
+   5
+  +babar
+
+Uncommiting on dirty working directory
+======================================
+
+  $ hg status
+  M a
+  $ hg diff
+  diff -r 678a59e5ff90 a
+  --- a/a	Thu Jan 01 00:00:00 1970 +0000
+  +++ b/a	Thu Jan 01 00:00:00 1970 +0000
+  @@ -1,3 +1,6 @@
+  +-2
+  +-1
+  +0
+   1
+   2
+   3
+
+  $ hg uncommit -i<<EOF
+  > y
+  > n
+  > y
+  > EOF
+  diff --git a/a b/a
+  2 hunks, 3 lines changed
+  examine changes to 'a'? [Ynesfdaq?] y
+  
+  @@ -1,5 +1,7 @@
+   1
+   2
+   3
+  +foo
+  +bar
+   4
+   5
+  discard change 1/2 to 'a'? [Ynesfdaq?] n
+  
+  @@ -4,2 +6,3 @@
+   4
+   5
+  +babar
+  discard change 2/2 to 'a'? [Ynesfdaq?] y
+  
+  patching file a
+  Hunk #1 succeeded at 2 with fuzz 1 (offset 0 lines).
+
+  $ hg diff
+  diff -r 46e35360be47 a
+  --- a/a	Thu Jan 01 00:00:00 1970 +0000
+  +++ b/a	Thu Jan 01 00:00:00 1970 +0000
+  @@ -1,3 +1,6 @@
+  +-2
+  +-1
+  +0
+   1
+   2
+   3
+  @@ -5,3 +8,4 @@
+   bar
+   4
+   5
+  +babar
+
+  $ hg exp
+  # HG changeset patch
+  # User test
+  # Date 0 0
+  #      Thu Jan 01 00:00:00 1970 +0000
+  # Node ID 46e35360be473bf761bedf3d05de4a68ffd9d9f8
+  # Parent  7733902a8d94c789ca81d866bea1893d79442db6
+  another one
+  
+  diff -r 7733902a8d94 -r 46e35360be47 a
+  --- a/a	Thu Jan 01 00:00:00 1970 +0000
+  +++ b/a	Thu Jan 01 00:00:00 1970 +0000
+  @@ -1,5 +1,7 @@
+   1
+   2
+   3
+  +foo
+  +bar
+   4
+   5
+
+Checking the obsolescence history
+
+  $ hg log -G --hidden
+  @  changeset:   5:46e35360be47
+  |  tag:         tip
+  |  parent:      0:7733902a8d94
+  |  user:        test
+  |  date:        Thu Jan 01 00:00:00 1970 +0000
+  |  summary:     another one
+  |
+  | x  changeset:   4:7ca9935a62f1
+  |/   parent:      0:7733902a8d94
+  |    user:        test
+  |    date:        Thu Jan 01 00:00:00 1970 +0000
+  |    obsolete:    pruned using uncommit
+  |    summary:     temporary commit for uncommiting 678a59e5ff90
+  |
+  | x  changeset:   3:678a59e5ff90
+  |/   parent:      0:7733902a8d94
+  |    user:        test
+  |    date:        Thu Jan 01 00:00:00 1970 +0000
+  |    obsolete:    rewritten using uncommit as 5:46e35360be47
+  |    summary:     another one
+  |
+  | x  changeset:   2:e9635f4beaf1
+  |/   parent:      0:7733902a8d94
+  |    user:        test
+  |    date:        Thu Jan 01 00:00:00 1970 +0000
+  |    obsolete:    pruned using uncommit
+  |    summary:     temporary commit for uncommiting f70fb463d5bf
+  |
+  | x  changeset:   1:f70fb463d5bf
+  |/   user:        test
+  |    date:        Thu Jan 01 00:00:00 1970 +0000
+  |    obsolete:    rewritten using uncommit as 3:678a59e5ff90
+  |    summary:     another one
+  |
+  o  changeset:   0:7733902a8d94
+     user:        test
+     date:        Thu Jan 01 00:00:00 1970 +0000
+     summary:     The base commit
+  
+Push the changes back to the commit and more commits for more testing
+
+  $ hg amend
+  $ glog
+  @  6:905eb2a23ea2@default(draft) another one
+  |
+  o  0:7733902a8d94@default(draft) The base commit
+  
+  $ touch foo
+  $ echo "hey" >> foo
+  $ hg ci -Am "Added foo"
+  adding foo
+
+Testing uncommiting a whole changeset and also for a file addition
+==================================================================
+
+  $ hg uncommit -i<<EOF
+  > y
+  > y
+  > EOF
+  diff --git a/foo b/foo
+  new file mode 100644
+  examine changes to 'foo'? [Ynesfdaq?] y
+  
+  @@ -0,0 +1,1 @@
+  +hey
+  discard this change to 'foo'? [Ynesfdaq?] y
+  
+
+  $ hg status
+  A foo
+  $ hg diff
+  diff -r 857367499298 foo
+  --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+  +++ b/foo	Thu Jan 01 00:00:00 1970 +0000
+  @@ -0,0 +1,1 @@
+  +hey
+
+  $ hg exp
+  # HG changeset patch
+  # User test
+  # Date 0 0
+  #      Thu Jan 01 00:00:00 1970 +0000
+  # Node ID 857367499298e999b5841bb01df65f73088b5d3b
+  # Parent  905eb2a23ea2d92073419d0e19165b90d36ea223
+  Added foo
+  
+  $ hg amend
+
+Testing to uncommit removed files completely
+============================================
+
+  $ hg rm a
+  $ hg ci -m "Removed a"
+  $ hg exp
+  # HG changeset patch
+  # User test
+  # Date 0 0
+  #      Thu Jan 01 00:00:00 1970 +0000
+  # Node ID 219cfe20964e93f8bb9bd82ceaa54d3b776046db
+  # Parent  42cc15efbec26c14d96d805dee2766ba91d1fd31
+  Removed a
+  
+  diff -r 42cc15efbec2 -r 219cfe20964e a
+  --- a/a	Thu Jan 01 00:00:00 1970 +0000
+  +++ /dev/null	Thu Jan 01 00:00:00 1970 +0000
+  @@ -1,11 +0,0 @@
+  --2
+  --1
+  -0
+  -1
+  -2
+  -3
+  -foo
+  -bar
+  -4
+  -5
+  -babar
+
+Not examining the file
+----------------------
+
+  $ hg uncommit -i<<EOF
+  > n
+  > EOF
+  diff --git a/a b/a
+  deleted file mode 100644
+  examine changes to 'a'? [Ynesfdaq?] n
+  
+  abort: nothing selected to uncommit
+  [255]
+
+Examining the file
+------------------
+XXX: there is a bug in interactive selection as it is not letting to examine the
+file. Tried with curses too. In the curses UI, if you just unselect the hunks
+and the not file mod thing at the top, it will show the same "nothing unselected
+to uncommit" message which is a bug in interactive selection.
+
+  $ hg uncommit -i<<EOF
+  > y
+  > EOF
+  diff --git a/a b/a
+  deleted file mode 100644
+  examine changes to 'a'? [Ynesfdaq?] y
+  
+
+  $ hg diff
+  diff -r 737487f1e5f8 a
+  --- a/a	Thu Jan 01 00:00:00 1970 +0000
+  +++ /dev/null	Thu Jan 01 00:00:00 1970 +0000
+  @@ -1,11 +0,0 @@
+  --2
+  --1
+  -0
+  -1
+  -2
+  -3
+  -foo
+  -bar
+  -4
+  -5
+  -babar
+  $ hg status
+  R a
+  $ hg exp
+  # HG changeset patch
+  # User test
+  # Date 0 0
+  #      Thu Jan 01 00:00:00 1970 +0000
+  # Node ID 737487f1e5f853e55decb73ea31522c63e7f5980
+  # Parent  42cc15efbec26c14d96d805dee2766ba91d1fd31
+  Removed a
+  
+
+  $ hg prune .
+  hg: unknown command 'prune'
+  (use 'hg help' for a list of commands)
+  [255]
+  $ hg revert --all
+  undeleting a
+
+  $ glog
+  @  13:737487f1e5f8@default(draft) Removed a
+  |
+  o  10:42cc15efbec2@default(draft) Added foo
+  |
+  o  6:905eb2a23ea2@default(draft) another one
+  |
+  o  0:7733902a8d94@default(draft) The base commit
+  
+
+Testing when a new file is added in the last commit
+===================================================
+
+  $ echo "foo" >> foo
+  $ touch x
+  $ echo "abcd" >> x
+  $ hg add x
+  $ hg ci -m "Added x"
+  $ hg uncommit -i<<EOF
+  > y
+  > y
+  > y
+  > n
+  > EOF
+  diff --git a/foo b/foo
+  1 hunks, 1 lines changed
+  examine changes to 'foo'? [Ynesfdaq?] y
+  
+  @@ -1,1 +1,2 @@
+   hey
+  +foo
+  discard change 1/2 to 'foo'? [Ynesfdaq?] y
+  
+  diff --git a/x b/x
+  new file mode 100644
+  examine changes to 'x'? [Ynesfdaq?] y
+  
+  @@ -0,0 +1,1 @@
+  +abcd
+  discard change 2/2 to 'x'? [Ynesfdaq?] n
+  
+
+  $ hg exp
+  # HG changeset patch
+  # User test
+  # Date 0 0
+  #      Thu Jan 01 00:00:00 1970 +0000
+  # Node ID f7b39cf595081f8e63fe1119953cc7f669663720
+  # Parent  737487f1e5f853e55decb73ea31522c63e7f5980
+  Added x
+  
+  diff -r 737487f1e5f8 -r f7b39cf59508 x
+  --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+  +++ b/x	Thu Jan 01 00:00:00 1970 +0000
+  @@ -0,0 +1,1 @@
+  +abcd
+
+  $ hg diff
+  diff -r f7b39cf59508 foo
+  --- a/foo	Thu Jan 01 00:00:00 1970 +0000
+  +++ b/foo	Thu Jan 01 00:00:00 1970 +0000
+  @@ -1,1 +1,2 @@
+   hey
+  +foo
+
+  $ hg status
+  M foo
+
+  $ hg revert --all
+  reverting foo
+
+Testing between the stack and with dirty working copy
+=====================================================
+
+  $ glog
+  @  16:f7b39cf59508@default(draft) Added x
+  |
+  o  13:737487f1e5f8@default(draft) Removed a
+  |
+  o  10:42cc15efbec2@default(draft) Added foo
+  |
+  o  6:905eb2a23ea2@default(draft) another one
+  |
+  o  0:7733902a8d94@default(draft) The base commit
+  
+  $ hg up 905eb2a23ea2
+  0 files updated, 0 files merged, 2 files removed, 0 files unresolved
+
+  $ touch bar
+  $ echo "foo" >> bar
+  $ hg add bar
+  $ hg status
+  A bar
+  ? foo.orig
+
+  $ hg exp
+  # HG changeset patch
+  # User test
+  # Date 0 0
+  #      Thu Jan 01 00:00:00 1970 +0000
+  # Node ID 905eb2a23ea2d92073419d0e19165b90d36ea223
+  # Parent  7733902a8d94c789ca81d866bea1893d79442db6
+  another one
+  
+  diff -r 7733902a8d94 -r 905eb2a23ea2 a
+  --- a/a	Thu Jan 01 00:00:00 1970 +0000
+  +++ b/a	Thu Jan 01 00:00:00 1970 +0000
+  @@ -1,5 +1,11 @@
+  +-2
+  +-1
+  +0
+   1
+   2
+   3
+  +foo
+  +bar
+   4
+   5
+  +babar
+
+  $ hg uncommit -i<<EOF
+  > y
+  > n
+  > n
+  > y
+  > EOF
+  diff --git a/a b/a
+  3 hunks, 6 lines changed
+  examine changes to 'a'? [Ynesfdaq?] y
+  
+  @@ -1,3 +1,6 @@
+  +-2
+  +-1
+  +0
+   1
+   2
+   3
+  discard change 1/3 to 'a'? [Ynesfdaq?] n
+  
+  @@ -1,5 +4,7 @@
+   1
+   2
+   3
+  +foo
+  +bar
+   4
+   5
+  discard change 2/3 to 'a'? [Ynesfdaq?] n
+  
+  @@ -4,2 +9,3 @@
+   4
+   5
+  +babar
+  discard change 3/3 to 'a'? [Ynesfdaq?] y
+  
+  patching file a
+  Hunk #1 succeeded at 1 with fuzz 1 (offset -1 lines).
+  3 new orphan changesets
+
+  $ hg diff
+  diff -r 676366511f95 a
+  --- a/a	Thu Jan 01 00:00:00 1970 +0000
+  +++ b/a	Thu Jan 01 00:00:00 1970 +0000
+  @@ -8,3 +8,4 @@
+   bar
+   4
+   5
+  +babar
+  diff -r 676366511f95 bar
+  --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+  +++ b/bar	Thu Jan 01 00:00:00 1970 +0000
+  @@ -0,0 +1,1 @@
+  +foo
+
+  $ hg exp
+  # HG changeset patch
+  # User test
+  # Date 0 0
+  #      Thu Jan 01 00:00:00 1970 +0000
+  # Node ID 676366511f95ca4122413dcf79b45eaab61fb387
+  # Parent  7733902a8d94c789ca81d866bea1893d79442db6
+  another one
+  
+  diff -r 7733902a8d94 -r 676366511f95 a
+  --- a/a	Thu Jan 01 00:00:00 1970 +0000
+  +++ b/a	Thu Jan 01 00:00:00 1970 +0000
+  @@ -1,5 +1,10 @@
+  +-2
+  +-1
+  +0
+   1
+   2
+   3
+  +foo
+  +bar
+   4
+   5
+  $ hg status
+  M a
+  A bar
+  ? foo.orig
+
+More uncommit on the same dirty working copy
+=============================================
+
+  $ hg uncommit -i<<EOF
+  > y
+  > y
+  > n
+  > EOF
+  diff --git a/a b/a
+  2 hunks, 5 lines changed
+  examine changes to 'a'? [Ynesfdaq?] y
+  
+  @@ -1,3 +1,6 @@
+  +-2
+  +-1
+  +0
+   1
+   2
+   3
+  discard change 1/2 to 'a'? [Ynesfdaq?] y
+  
+  @@ -1,5 +4,7 @@
+   1
+   2
+   3
+  +foo
+  +bar
+   4
+   5
+  discard change 2/2 to 'a'? [Ynesfdaq?] n
+  
+
+  $ hg exp
+  # HG changeset patch
+  # User test
+  # Date 0 0
+  #      Thu Jan 01 00:00:00 1970 +0000
+  # Node ID 62d907d0c4fa13b4b8bfeed05f13751035daf963
+  # Parent  7733902a8d94c789ca81d866bea1893d79442db6
+  another one
+  
+  diff -r 7733902a8d94 -r 62d907d0c4fa a
+  --- a/a	Thu Jan 01 00:00:00 1970 +0000
+  +++ b/a	Thu Jan 01 00:00:00 1970 +0000
+  @@ -1,5 +1,7 @@
+   1
+   2
+   3
+  +foo
+  +bar
+   4
+   5
+
+  $ hg diff
+  diff -r 62d907d0c4fa a
+  --- a/a	Thu Jan 01 00:00:00 1970 +0000
+  +++ b/a	Thu Jan 01 00:00:00 1970 +0000
+  @@ -1,3 +1,6 @@
+  +-2
+  +-1
+  +0
+   1
+   2
+   3
+  @@ -5,3 +8,4 @@
+   bar
+   4
+   5
+  +babar
+  diff -r 62d907d0c4fa bar
+  --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+  +++ b/bar	Thu Jan 01 00:00:00 1970 +0000
+  @@ -0,0 +1,1 @@
+  +foo
+
+  $ hg status
+  M a
+  A bar
+  ? foo.orig
+
+Interactive uncommit with a pattern
+-----------------------------------
+
+(more setup)
+
+  $ hg ci -m 'roaming changes'
+  $ cat > b << EOF
+  > a
+  > b
+  > c
+  > d
+  > e
+  > f
+  > h
+  > EOF
+  $ hg add b
+  $ hg ci -m 'add b'
+  $ echo 'celeste' >> a
+  $ echo 'i' >> b
+  $ hg ci -m 'some more changes'
+  $ hg export
+  # HG changeset patch
+  # User test
+  # Date 0 0
+  #      Thu Jan 01 00:00:00 1970 +0000
+  # Node ID be5c67225e80b050867862bbd9f4755c4e9207c5
+  # Parent  c280a907fddcef2ffe9fadcc2d87f29998e22b2f
+  some more changes
+  
+  diff -r c280a907fddc -r be5c67225e80 a
+  --- a/a	Thu Jan 01 00:00:00 1970 +0000
+  +++ b/a	Thu Jan 01 00:00:00 1970 +0000
+  @@ -9,3 +9,4 @@
+   4
+   5
+   babar
+  +celeste
+  diff -r c280a907fddc -r be5c67225e80 b
+  --- a/b	Thu Jan 01 00:00:00 1970 +0000
+  +++ b/b	Thu Jan 01 00:00:00 1970 +0000
+  @@ -5,3 +5,4 @@
+   e
+   f
+   h
+  +i
+
+  $ hg uncommit -i a << DONE
+  > y
+  > y
+  > DONE
+  diff --git a/a b/a
+  1 hunks, 1 lines changed
+  examine changes to 'a'? [Ynesfdaq?] y
+  
+  @@ -9,3 +9,4 @@
+   4
+   5
+   babar
+  +celeste
+  discard this change to 'a'? [Ynesfdaq?] y
+  
+  $ hg status
+  M a
+  ? foo.orig
+  $ hg diff
+  diff -r c701d7c8d18b a
+  --- a/a	Thu Jan 01 00:00:00 1970 +0000
+  +++ b/a	Thu Jan 01 00:00:00 1970 +0000
+  @@ -9,3 +9,4 @@
+   4
+   5
+   babar
+  +celeste
+  $ hg export
+  # HG changeset patch
+  # User test
+  # Date 0 0
+  #      Thu Jan 01 00:00:00 1970 +0000
+  # Node ID c701d7c8d18be55a92688f4458c26bd74fb1f525
+  # Parent  c280a907fddcef2ffe9fadcc2d87f29998e22b2f
+  some more changes
+  
+  diff -r c280a907fddc -r c701d7c8d18b b
+  --- a/b	Thu Jan 01 00:00:00 1970 +0000
+  +++ b/b	Thu Jan 01 00:00:00 1970 +0000
+  @@ -5,3 +5,4 @@
+   e
+   f
+   h
+  +i
+
+(reset)
+
+  $ cat << EOF  > a
+  > -3
+  > -2
+  > -1
+  > 0
+  > 1
+  > 2
+  > 3
+  > foo
+  > bar
+  > 4
+  > 5
+  > babar
+  > celeste
+  > EOF
+  $ hg amend 
+
+Same but do not select some change in 'a'
+
+  $ hg uncommit -i a << DONE
+  > y
+  > y
+  > n
+  > DONE
+  diff --git a/a b/a
+  2 hunks, 2 lines changed
+  examine changes to 'a'? [Ynesfdaq?] y
+  
+  @@ -1,3 +1,4 @@
+  +-3
+   -2
+   -1
+   0
+  discard change 1/2 to 'a'? [Ynesfdaq?] y
+  
+  @@ -9,3 +10,4 @@
+   4
+   5
+   babar
+  +celeste
+  discard change 2/2 to 'a'? [Ynesfdaq?] n
+  
+  $ hg status
+  M a
+  ? foo.orig
+
+  $ hg diff
+  diff -r 28d5de12b225 a
+  --- a/a	Thu Jan 01 00:00:00 1970 +0000
+  +++ b/a	Thu Jan 01 00:00:00 1970 +0000
+  @@ -1,3 +1,4 @@
+  +-3
+   -2
+   -1
+   0
+
+  $ hg export
+  # HG changeset patch
+  # User test
+  # Date 0 0
+  #      Thu Jan 01 00:00:00 1970 +0000
+  # Node ID 28d5de12b225d1e0951110cced8d8994227be026
+  # Parent  c280a907fddcef2ffe9fadcc2d87f29998e22b2f
+  some more changes
+  
+  diff -r c280a907fddc -r 28d5de12b225 a
+  --- a/a	Thu Jan 01 00:00:00 1970 +0000
+  +++ b/a	Thu Jan 01 00:00:00 1970 +0000
+  @@ -9,3 +9,4 @@
+   4
+   5
+   babar
+  +celeste
+  diff -r c280a907fddc -r 28d5de12b225 b
+  --- a/b	Thu Jan 01 00:00:00 1970 +0000
+  +++ b/b	Thu Jan 01 00:00:00 1970 +0000
+  @@ -5,3 +5,4 @@
+   e
+   f
+   h
+  +i
+
+  $ cat b
+  a
+  b
+  c
+  d
+  e
+  f
+  h
+  i
diff --git a/tests/test-unamend.t b/tests/test-unamend.t
--- a/tests/test-unamend.t
+++ b/tests/test-unamend.t
@@ -83,9 +83,9 @@ 
   
   $ hg unamend
   $ hg glog --hidden
-  @  9:46d02d47eec6  Added h
+  o  9:46d02d47eec6  Added h
   |
-  | x  8:c9fa1a715c1b  Added h
+  | @  8:c9fa1a715c1b  Added h
   |/
   | x  7:ec2426147f0e  Added h
   |/
@@ -104,34 +104,28 @@ 
   o  0:18d04c59bb5d  Added a
   
   $ hg diff
-  diff -r 46d02d47eec6 h
-  --- a/h	Thu Jan 01 00:00:00 1970 +0000
-  +++ b/h	Thu Jan 01 00:00:00 1970 +0000
-  @@ -1,1 +1,2 @@
-   foo
-  +bar
 
   $ hg exp
   # HG changeset patch
   # User test
   # Date 0 0
   #      Thu Jan 01 00:00:00 1970 +0000
-  # Node ID 46d02d47eec6ca096b8dcab3f8f5579c40c3dd9a
+  # Node ID c9fa1a715c1b7661c0fafb362a9f30bd75878d7d
   # Parent  87d6d66763085b629e6d7ed56778c79827273022
   Added h
   
-  diff -r 87d6d6676308 -r 46d02d47eec6 h
+  diff -r 87d6d6676308 -r c9fa1a715c1b h
   --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
   +++ b/h	Thu Jan 01 00:00:00 1970 +0000
-  @@ -0,0 +1,1 @@
+  @@ -0,0 +1,2 @@
   +foo
+  +bar
 
   $ hg status
-  M h
 
   $ hg log -r . -T '{extras % "{extra}\n"}' --config alias.log=log
+  amend_source=ec2426147f0e39dbc9cef599b066be6035ce691d
   branch=default
-  unamend_source=c9fa1a715c1b7661c0fafb362a9f30bd75878d7d
 
 Using unamend to undo an unamed (intentional)
 
@@ -141,11 +135,11 @@ 
   # User test
   # Date 0 0
   #      Thu Jan 01 00:00:00 1970 +0000
-  # Node ID 850ddfc1bc662997ec6094ada958f01f0cc8070a
+  # Node ID c9fa1a715c1b7661c0fafb362a9f30bd75878d7d
   # Parent  87d6d66763085b629e6d7ed56778c79827273022
   Added h
   
-  diff -r 87d6d6676308 -r 850ddfc1bc66 h
+  diff -r 87d6d6676308 -r c9fa1a715c1b h
   --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
   +++ b/h	Thu Jan 01 00:00:00 1970 +0000
   @@ -0,0 +1,2 @@
@@ -157,6 +151,7 @@ 
 
   $ echo "bar" >> a
   $ hg amend
+  2 new content-divergent changesets
   $ echo "foobar" >> a
   $ echo "bar" >> b
   $ hg status
@@ -170,14 +165,14 @@ 
   M b
 
   $ hg diff
-  diff -r ec338db45d51 a
+  diff -r 924a1bde06b4 a
   --- a/a	Thu Jan 01 00:00:00 1970 +0000
   +++ b/a	Thu Jan 01 00:00:00 1970 +0000
-  @@ -1,1 +1,3 @@
+  @@ -1,2 +1,3 @@
    foo
-  +bar
+   bar
   +foobar
-  diff -r ec338db45d51 b
+  diff -r 924a1bde06b4 b
   --- a/b	Thu Jan 01 00:00:00 1970 +0000
   +++ b/b	Thu Jan 01 00:00:00 1970 +0000
   @@ -1,1 +1,2 @@
@@ -187,6 +182,7 @@ 
 Unamending an added file
 
   $ hg ci -m "Added things to a and b"
+  1 new orphan changesets
   $ echo foo > bar
   $ hg add bar
   $ hg amend
@@ -202,45 +198,61 @@ 
 
   $ hg remove a
   $ hg amend
+  1 new orphan changesets
+  2 new content-divergent changesets
 
   $ hg unamend
   $ hg status
   R a
   ? bar
 
   $ hg revert --all
   undeleting a
+  abort: a@a7d3ff363911: not found in manifest!
+  [255]
 
 Unamending an added file with dirty wdir status
 
   $ hg add bar
   $ hg amend
+  1 new orphan changesets
+  1 new content-divergent changesets
   $ echo bar >> bar
   $ hg status
   M bar
 
   $ hg unamend
   $ hg status
-  A bar
+  M bar
   $ hg diff
-  diff -r 7f79409af972 bar
-  --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+  diff -r cfcfb29e2ff7 bar
+  --- a/bar	Thu Jan 01 00:00:00 1970 +0000
   +++ b/bar	Thu Jan 01 00:00:00 1970 +0000
-  @@ -0,0 +1,2 @@
-  +foo
+  @@ -1,1 +1,2 @@
+   foo
   +bar
 
   $ hg revert --all
-  forgetting bar
+  reverting bar
   $ rm bar
 
 Unamending in middle of a stack
 
   $ hg glog
-  @  19:7f79409af972  Added things to a and b
+  *  18:93b9747d736b  Added things to a and b
   |
-  o  12:ec338db45d51  Added h
-  |
+  | @  17:cfcfb29e2ff7  Added things to a and b
+  |/
+  | *  16:f3e8006b11ed  Added things to a and b
+  |/
+  | *  14:61096993d6f1  Added things to a and b
+  |/
+  | *  11:f392ea0dfe43  Added h
+  | |
+  x |  10:924a1bde06b4  Added h
+  |/
+  | *  9:46d02d47eec6  Added h
+  |/
   o  6:87d6d6676308  Added g
   |
   o  5:825660c69f0c  Added f
@@ -263,14 +275,26 @@ 
   $ hg rebase -s 6 -d . -q
 
   $ hg glog
-  o  23:03ddd6fc5af1  Added things to a and b
+  *  22:744f14403439  Added h
   |
-  o  22:3e7b64ee157b  Added h
+  | *  21:cf05c9d36833  Added h
+  |/
+  o  20:49635b68477e  Added g
+  |
+  @  19:93f0e8ffab32  Added f
   |
-  o  21:49635b68477e  Added g
-  |
-  @  20:93f0e8ffab32  Added f
-  |
+  | *  18:93b9747d736b  Added things to a and b
+  | |
+  | | *  16:f3e8006b11ed  Added things to a and b
+  | |/
+  | | *  14:61096993d6f1  Added things to a and b
+  | |/
+  | x  10:924a1bde06b4  Added h
+  | |
+  | x  6:87d6d6676308  Added g
+  | |
+  | x  5:825660c69f0c  Added f
+  |/
   o  4:aa98ab95a928  Added e
   |
   o  3:62615734edd5  Added d
@@ -292,17 +316,17 @@ 
 Trying to unamend a public changeset
 
   $ hg up -C 23
-  5 files updated, 0 files merged, 0 files removed, 0 files unresolved
+  1 files updated, 0 files merged, 0 files removed, 0 files unresolved
   $ hg phase -r . -p
-  1 new phase-divergent changesets
   $ hg unamend
   abort: cannot unamend public changesets
   (see 'hg help phases' for details)
   [255]
 
 Testing whether unamend retains copies or not
 
   $ hg status
+  ? bar.orig
 
   $ hg mv a foo
 
@@ -312,8 +336,8 @@ 
   # User test
   # Date 0 0
   #      Thu Jan 01 00:00:00 1970 +0000
-  # Node ID cfef290346fbee5126313d7e1aab51d877679b09
-  # Parent  03ddd6fc5af19e028c44a2fd6d790dd22712f231
+  # Node ID 801c2513f166634bfae97bad18df32b999050c12
+  # Parent  7a9352c9ddcedbf857d38c13bca4679f7a3ba78a
   Moved a to foo
   
   diff --git a/a b/foo
@@ -332,8 +356,8 @@ 
   # User test
   # Date 0 0
   #      Thu Jan 01 00:00:00 1970 +0000
-  # Node ID eca050985275bb271ce3092b54e56ea5c85d29a3
-  # Parent  03ddd6fc5af19e028c44a2fd6d790dd22712f231
+  # Node ID 7b9f94d173c7d15a9e32e33afb654de21dbc16bf
+  # Parent  7a9352c9ddcedbf857d38c13bca4679f7a3ba78a
   Moved a to foo
   
   diff --git a/a b/foo
@@ -353,24 +377,32 @@ 
   # User test
   # Date 0 0
   #      Thu Jan 01 00:00:00 1970 +0000
-  # Node ID 552e3af4f01f620f88ca27be1f898316235b736a
-  # Parent  03ddd6fc5af19e028c44a2fd6d790dd22712f231
+  # Node ID 7b9f94d173c7d15a9e32e33afb654de21dbc16bf
+  # Parent  7a9352c9ddcedbf857d38c13bca4679f7a3ba78a
   Moved a to foo
   
   diff --git a/a b/foo
   rename from a
   rename to foo
+  diff --git a/b b/foobar
+  rename from b
+  rename to foobar
 
 Retained copies in working directoy
 
   $ hg diff --git
-  diff --git a/b b/foobar
-  rename from b
-  rename to foobar
+  diff --git a/foobar b/foobar
+  new file mode 100644
+  --- /dev/null
+  +++ b/foobar
+  @@ -0,0 +1,1 @@
+  +foo
   diff --git a/c b/wat
   rename from c
   rename to wat
   $ hg revert -qa
+  abort: b@7b9f94d173c7: not found in manifest!
+  [255]
   $ rm foobar wat
 
 Rename a->b, then amend b->c. After unamend, should look like b->c.
@@ -382,14 +414,17 @@ 
   $ hg amend
   $ hg unamend
   $ hg st --copies --change .
-  A b
+  A c
     a
   R a
   $ hg st --copies
   A c
     b
   R b
+  ? bar.orig
   $ hg revert -qa
+  abort: b@a629a52c2105: not found in manifest!
+  [255]
   $ rm c
 
 Rename a->b, then amend b->c, and working copy change c->d. After unamend, should look like b->d
@@ -402,10 +437,11 @@ 
   $ hg mv c d
   $ hg unamend
   $ hg st --copies --change .
-  A b
+  A c
     a
   R a
   $ hg st --copies
   A d
     b
   R b
+  ? bar.orig
diff --git a/hgext/uncommit.py b/hgext/uncommit.py
--- a/hgext/uncommit.py
+++ b/hgext/uncommit.py
@@ -28,11 +28,14 @@ 
     copies as copiesmod,
     error,
     node,
+    obsolete,
     obsutil,
+    patch,
     pycompat,
     registrar,
     rewriteutil,
     scmutil,
+    util,
 )
 
 cmdtable = {}
@@ -45,6 +48,8 @@ 
     default=False,
 )
 
+stringio = util.stringio
+
 # Note for extension authors: ONLY specify testedwith = 'ships-with-hg-core' for
 # extensions which SHIP WITH MERCURIAL. Non-mainline extensions should
 # be specifying the version(s) of Mercurial they are tested with, or
@@ -93,33 +98,83 @@ 
                          extra=ctx.extra())
     return repo.commitctx(new)
 
-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
+    oldctx to a copy of oldctx not containing changed files matched by
+    match.
     """
+    ctx = repo['.']
     ds = repo.dirstate
-    ds.setparents(newctx.node(), node.nullid)
     copies = dict(ds.copies())
-    s = newctx.status(oldctx, match=match)
-    for f in s.modified:
-        if ds[f] == 'r':
-            # modified + removed -> removed
-            continue
-        ds.normallookup(f)
+    if interactive:
+        # In interactive cases, we will find the status between oldctx and ctx
+        # and considering only the files which are changed between oldctx and
+        # ctx, and the status of what changed between oldctx and ctx will help
+        # us in defining the exact behavior
+        m, a, r = repo.status(oldctx, ctx, match=match)[:3]
+        for f in m:
+            # These are files which are modified between oldctx and ctx which
+            # contains two cases: 1) Were modified in oldctx and some
+            # modifications are uncommitted
+            # 2) Were added in oldctx but some part is uncommitted (this cannot
+            # contain the case when added files are uncommitted completely as
+            # that will result in status as removed not modified.)
+            # Also any modifications to a removed file will result the status as
+            # added, so we have only two cases. So in either of the cases, the
+            # resulting status can be modified or clean.
+            if ds[f] == 'r':
+                # But the file is removed in the working directory, leaving that
+                # as removed
+                continue
+            ds.normallookup(f)
 
-    for f in s.added:
-        if ds[f] == 'r':
-            # added + removed -> unknown
-            ds.drop(f)
-        elif ds[f] != 'a':
-            ds.add(f)
+        for f in a:
+            # These are the files which are added between oldctx and ctx(new
+            # one), which means the files which were removed in oldctx
+            # but uncommitted completely while making the ctx
+            # This file should be marked as removed if the working directory
+            # does not adds it back. If it's adds it back, we do a normallookup.
+            # The file can't be removed in working directory, because it was
+            # removed in oldctx
+            if ds[f] == 'a':
+                ds.normallookup(f)
+                continue
+            ds.remove(f)
 
-    for f in s.removed:
-        if ds[f] == 'a':
-            # removed + added -> normal
+        for f in r:
+            # These are files which are removed between oldctx and ctx, which
+            # means the files which were added in oldctx and were completely
+            # uncommitted in ctx. If a added file is partially uncommitted, that
+            # would have resulted in modified status, not removed.
+            # So a file added in a commit, and uncommitting that addition must
+            # result in file being stated as unknown.
+            if ds[f] == 'r':
+                # The working directory say it's removed, so lets make the file
+                # unknown
+                ds.drop(f)
+                continue
+            ds.add(f)
+    else:
+        s = newctx.status(oldctx, match=match)
+        for f in s.modified:
+            if ds[f] == 'r':
+                # modified + removed -> removed
+                continue
             ds.normallookup(f)
-        elif ds[f] != 'r':
-            ds.remove(f)
+
+        for f in s.added:
+            if ds[f] == 'r':
+                # added + removed -> unknown
+                ds.drop(f)
+            elif ds[f] != 'a':
+                ds.add(f)
+
+        for f in s.removed:
+            if ds[f] == 'a':
+                # removed + added -> normal
+                ds.normallookup(f)
+            elif ds[f] != 'r':
+                ds.remove(f)
 
     # Merge old parent and old working dir copies
     oldcopies = copiesmod.pathcopies(newctx, oldctx, match)
@@ -133,7 +188,8 @@ 
         ds.copy(src, dst)
 
 @command('uncommit',
-    [('', 'keep', False, _('allow an empty commit after uncommiting')),
+    [('i', 'interactive', False, _('interactive mode to uncommit')),
+    ('', 'keep', False, _('allow an empty commit after uncommiting')),
     ] + commands.walkopts,
     _('[OPTION]... [FILE]...'),
     helpcategory=command.CATEGORY_CHANGE_MANAGEMENT)
@@ -149,6 +205,7 @@ 
     given.
     """
     opts = pycompat.byteskwargs(opts)
+    interactive = opts.get('interactive')
 
     with repo.wlock(), repo.lock():
 
@@ -164,6 +221,9 @@ 
             match = scmutil.match(old, pats, opts)
             keepcommit = opts.get('keep') or pats
             newid = _commitfiltered(repo, old, match, keepcommit)
+            if interactive:
+                newid = _interactiveuncommit(ui, repo, old, match)
+
             if newid is None:
                 ui.status(_("nothing to uncommit\n"))
                 return 1
@@ -177,10 +237,106 @@ 
                 mapping[old.node()] = ()
 
             with repo.dirstate.parentchange():
-                _fixdirstate(repo, old, repo[newid], match)
+                repo.dirstate.setparents(newid, node.nullid)
+                _uncommitdirstate(repo, old, repo[newid], match, interactive)
 
             scmutil.cleanupnodes(repo, mapping, 'uncommit', fixphase=True)
 
+def _interactiveuncommit(ui, repo, old, match):
+    """ The function which contains all the logic for interactively uncommiting
+    a commit. This function makes a temporary commit with the chunks which user
+    selected to uncommit. After that the diff of the parent and that commit is
+    applied to the working directory and committed again which results in the
+    new commit which should be one after uncommitted.
+    """
+
+    # create a temporary commit with hunks user selected
+    tempnode = _createtempcommit(ui, repo, old, match)
+
+    diffopts = patch.difffeatureopts(repo.ui, whitespace=True)
+    diffopts.nodates = True
+    diffopts.git = True
+    fp = stringio()
+    for chunk, label in patch.diffui(repo, tempnode, old.node(), None,
+                                     opts=diffopts):
+            fp.write(chunk)
+
+    fp.seek(0)
+    newnode = _patchtocommit(ui, repo, old, fp)
+    # creating obs marker temp -> ()
+    obsolete.createmarkers(repo, [(repo[tempnode], ())], operation="uncommit")
+    return newnode
+
+def _createtempcommit(ui, repo, old, match):
+    """ Creates a temporary commit for `uncommit --interative` which contains
+    the hunks which were selected by the user to uncommit.
+    """
+
+    pold = old.p1()
+    # The logic to interactively selecting something copied from
+    # cmdutil.revert()
+    diffopts = patch.difffeatureopts(repo.ui, whitespace=True)
+    diffopts.nodates = True
+    diffopts.git = True
+    diff = patch.diff(repo, pold.node(), old.node(), match, opts=diffopts)
+    originalchunks = patch.parsepatch(diff)
+    # XXX: The interactive selection is buggy and does not let you
+    # uncommit a removed file partially.
+    # TODO: wrap the operations in mercurial/patch.py and mercurial/crecord.py
+    # to add uncommit as an operation taking care of BC.
+    chunks, opts = cmdutil.recordfilter(repo.ui, originalchunks,
+                                        operation='discard')
+    if not chunks:
+        raise error.Abort(_("nothing selected to uncommit"))
+    fp = stringio()
+    for c in chunks:
+            c.write(fp)
+
+    fp.seek(0)
+    oldnode = node.hex(old.node())[:12]
+    message = 'temporary commit for uncommiting %s' % oldnode
+    tempnode = _patchtocommit(ui, repo, old, fp, message, oldnode)
+    return tempnode
+
+def _patchtocommit(ui, repo, old, fp, message=None, extras=None):
+    """ A function which will apply the patch to the working directory and
+    make a commit whose parents are same as that of old argument. The message
+    argument tells us whether to use the message of the old commit or a
+    different message which is passed. Returns the node of new commit made.
+    """
+    pold = old.p1()
+    parents = (old.p1().node(), old.p2().node())
+    date = old.date()
+    branch = old.branch()
+    user = old.user()
+    extra = old.extra()
+    if extras:
+        extra['uncommit_source'] = extras
+    if not message:
+        message = old.description()
+    store = patch.filestore()
+    try:
+        files = set()
+        try:
+            patch.patchrepo(ui, repo, pold, store, fp, 1, '',
+                            files=files, eolmode=None)
+        except patch.PatchError as err:
+            raise error.Abort(str(err))
+
+        finally:
+            del fp
+
+        memctx = context.memctx(repo, parents, message, files=files,
+                                filectxfn=store,
+                                user=user,
+                                date=date,
+                                branch=branch,
+                                extra=extra)
+        newcm = memctx.commit()
+    finally:
+        store.close()
+    return newcm
+
 def predecessormarkers(ctx):
     """yields the obsolete markers marking the given changeset as a successor"""
     for data in ctx.repo().obsstore.predecessors.get(ctx.node(), ()):
@@ -239,7 +395,9 @@ 
         dirstate = repo.dirstate
 
         with dirstate.parentchange():
-            _fixdirstate(repo, curctx, newpredctx)
+            match=None
+            interactive=0
+            _uncommitdirstate(repo, curctx, newpredctx, match, interactive)
 
         mapping = {curctx.node(): (newprednode,)}
         scmutil.cleanupnodes(repo, mapping, 'unamend', fixphase=True)