D5792: uncommit: added interactive mode(issue6062)

[martinvonz (Martin von Zweigbergk)]

martinvonz added a comment.


  I'll drop this patch from the queue because I think there's a lot that can be cleaned up. I tried to re-unify `_fixdirstate()` and `_uncommitdirstate()` myself. This is my first step:
  
    diff --git a/hgext/uncommit.py b/hgext/uncommit.py
    --- a/hgext/uncommit.py
    +++ b/hgext/uncommit.py
    @@ -138,7 +138,7 @@ def _fixdirstate(repo, oldctx, newctx, m
             ds.copy(src, dst)
     
     
    -def _uncommitdirstate(repo, oldctx, match, interactive):
    +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.
    @@ -217,21 +217,13 @@ def _uncommitdirstate(repo, oldctx, matc
                     ds.remove(f)
     
         # Merge old parent and old working dir copies
    -    oldcopies = {}
    -    if interactive:
    -        # Interactive had different meaning of the variables so restoring the
    -        # original meaning to use them
    -        m, a, r = repo.status(oldctx.p1(), oldctx, match=match)[:3]
    -    for f in (m + a):
    -        src = oldctx[f].renamed()
    -        if src:
    -            oldcopies[f] = src[0]
    +    oldcopies = copiesmod.pathcopies(newctx, oldctx, match)
         oldcopies.update(copies)
         copies = dict((dst, oldcopies.get(src, src))
                       for dst, src in oldcopies.iteritems())
         # Adjust the dirstate copies
         for dst, src in copies.iteritems():
    -        if (src not in ctx or dst in ctx or ds[dst] != 'a'):
    +        if (src not in newctx or dst in newctx or ds[dst] != 'a'):
                 src = None
             ds.copy(src, dst)
     
    @@ -285,11 +277,11 @@ def uncommit(ui, repo, *pats, **opts):
                     # Fully removed the old commit
                     mapping[old.node()] = ()
     
    -            scmutil.cleanupnodes(repo, mapping, 'uncommit', fixphase=True)
    -
                 with repo.dirstate.parentchange():
                     repo.dirstate.setparents(newid, node.nullid)
    -                _uncommitdirstate(repo, old, match, interactive)
    +                _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
  
  Is there a reason that's broken? Tests seem to pass (test-uncommit.t passes). If not, could you try to continue in the direction of unifying the two method again?

INLINE COMMENTS

> uncommit.py:141
> +
> +def _uncommitdirstate(repo, oldctx, match, interactive):
> +    """Fix the dirstate after switching the working directory from

Much of this duplicates `_fixdirstate()`. We must be able to share some of this code.

> uncommit.py:273
> +            if interactive:
> +                match = scmutil.match(old, pats, opts)
> +                newid = _interactiveuncommit(ui, repo, old, match)

Unnecessary (done on line 269)

> uncommit.py:288
>  
> +            scmutil.cleanupnodes(repo, mapping, 'uncommit', fixphase=True)
> +            

Why did this moved here? I just moved it down in https://phab.mercurial-scm.org/D5660. I really should have written a better commit message explaining what the problem was. But it's also unclear why it should be moved back.

REPOSITORY
  rHG Mercurial

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

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