Page MenuHomePhabricator

uncommit: added interactive mode(issue6062)
ClosedPublic

Authored by taapas1128 on Feb 1 2019, 2:39 AM.

Details

Summary

This adds interactive mode to uncommit option . This basically combines uncommit with hg commit -i as stated in the bug description.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

taapas1128 created this revision.Feb 1 2019, 2:39 AM
taapas1128 edited the summary of this revision. (Show Details)Feb 1 2019, 2:40 AM
taapas1128 updated this revision to Diff 13669.Feb 1 2019, 2:42 AM

Hi, thanks for the patch. Your patch is a good start at implementing uncommit -i. It misses multiple things like dirstate handling.

The good news is that you don't need to implement all that. There is an evolve extension which has already implemented that: https://www.mercurial-scm.org/repo/evolve/file/8494015ec24b/hgext3rd/evolve/cmdrewrite.py#l460. You can import that from there.

If you plan to import that from there, I suggest notifying @lothiraldan about that too so that they can manage things in evolve.

Thanks for the review @pulkit . By importing do you suggest importing the code for interactive or someway is there is someway to link evolve to hg-stable ?

pulkit added a comment.Feb 1 2019, 6:31 PM

Thanks for the review @pulkit . By importing do you suggest importing the code for interactive or someway is there is someway to link evolve to hg-stable ?

I mean importing the code.

taapas1128 updated this revision to Diff 13701.Feb 2 2019, 1:11 PM

@pulkit I have made an attempt to import the code please review. @lothiraldan can you please review and help me if I have missed out any portion of interactive from evolve

taapas1128 edited the summary of this revision. (Show Details)Feb 2 2019, 1:16 PM
pulkit added a comment.Feb 3 2019, 7:20 AM

@pulkit I have made an attempt to import the code please review. @lothiraldan can you please review and help me if I have missed out any portion of interactive from evolve

@taapas1128 haven't looked at the code in detail yet, but there are tests in evolve which should also be moved.

taapas1128 edited the summary of this revision. (Show Details)Feb 4 2019, 9:12 AM
taapas1128 updated this revision to Diff 13737.
taapas1128 updated this revision to Diff 13738.Feb 4 2019, 9:19 AM

@pulkit I was able to import the tests and make them compatible with hg-stable . Some tests which include commands like hg obslog , hg amend --extract , and hg uncommit -n which are not part of hg-stable but evolve are not running else all tests are now running smoothly .

pulkit added a comment.Feb 7 2019, 1:15 PM

@pulkit I was able to import the tests and make them compatible with hg-stable . Some tests which include commands like hg obslog , hg amend --extract , and hg uncommit -n which are not part of hg-stable but evolve are not running else all tests are now running smoothly .

hg amend --extract is just another name for hg uncommit.
You can move --note flag to core uncommit as a part of different patch.
Related to obslog, you can do hg log -r . -r precursor(.) or hg diff -r . -r precursor(.) but I don't think that will be sufficient.

@pulkit I have added some inline comments . So what do you suggest one way can be merging this and I will send a follow up for integrating -n to hg uncommitand then amending the tests. The other way is I should send a request for -n flag and then make all these changes again as a follow up pr for that request.

tests/test-uncommit-interactive.t
136

hg amend --extract uses -n flag here so it cant be completed with hg uncommit alone -n integration will be necessary. Also hg obslog is the same test so without this it can also not be replaced.

209

Same problem here.

pulkit added a comment.Feb 7 2019, 3:28 PM

@pulkit I have added some inline comments . So what do you suggest one way can be merging this and I will send a follow up for integrating -n to hg uncommitand then amending the tests. The other way is I should send a request for -n flag and then make all these changes again as a follow up pr for that request.

It will be nice if you send a patch for -n flag first, then rebase these patch on top of that.

taapas1128 updated this revision to Diff 13915.Feb 8 2019, 9:29 AM
taapas1128 added a comment.EditedFeb 8 2019, 9:42 AM

@pulkit Apparently it seems removing -n flag altogether will not interfere with the tests and they can be completed without that flag too . I have added inline comments please review. Also I am unable to use hg diff -r . -r precursor(.) so I used hg log -G --hidden in that place.

tests/test-uncommit-interactive.t
135

replaced hg amend --extract by hg uncommit -i

254

Removed -n flag here

@pulkit Can you please review this . I did not send a patch regarding -n flag because tests could be completed without it and also it would mean i would have to import large chunks of code.

The import looks fine to me. I will let someone else review the code because I authored this code in first place in evolve extension. Also it will be nice if you specify that the code is imported from evolve extension and also specify the commit hash from which you imported.

taapas1128 edited the summary of this revision. (Show Details)Feb 15 2019, 10:34 AM

@pulkit I have updated the description .

@pulkit I have updated the description .

Great thanks! As mentioned before I will let someone else review this :)

durin42 requested changes to this revision.Feb 15 2019, 10:56 PM
durin42 added a subscriber: durin42.

I'd fix this one style nit in flight (this actually looks good), but it no longer applies - could you rebase it for me?

hgext/uncommit.py
317

Need an extra blank line here.

This revision now requires changes to proceed.Feb 15 2019, 10:56 PM
taapas1128 edited the summary of this revision. (Show Details)Feb 16 2019, 2:31 AM
taapas1128 updated this revision to Diff 14132.
taapas1128 marked an inline comment as done.Feb 16 2019, 2:32 AM

@durin42 I have made the change. Please review.

Patch still fails to apply. Would you please rebase it as requested?

(I get patch application problems in uncommit.py, and the parent of the diff isn't in my repo so I can't do the rebase easily locally.)

@durin42 to which rev should I rebase it to ?

Anything recent. @ on https://mercurial-scm.org/hg is fine.

taapas1128 added a comment.EditedFeb 17 2019, 11:14 PM

@durin42 I have sent the patch for hg-stable repo (https://www.mercurial-scm.org/repo/hg-stable) . Maybe it is this the reason you are unable to apply the patch. Should I send a patch for hg repo ?

@durin42 I have sent the patch for hg-stable repo (https://www.mercurial-scm.org/repo/hg-stable) . Maybe it is this the reason you are unable to apply the patch. Should I send a patch for hg repo ?

Yes, you should rebase your patch on default branch of hg repo. You don't need to clone again, you can pull from https://mercurial-scm.org/hg and then rebase.

taapas1128 updated this revision to Diff 14139.Feb 18 2019, 7:29 AM
taapas1128 added a comment.EditedFeb 18 2019, 7:30 AM

Thank you for your help @pulkit . I have rebased it to the top and updated the diff.

@durin42 please review this whenever you are free.

This revision was automatically updated to reflect the committed changes.

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?

hgext/uncommit.py
141

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

273

Unnecessary (done on line 269)

288

Why did this moved here? I just moved it down in 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.

@martinvonz Sure I will try to unify _fixdirstate() and _uncommitdirstate(). I will follow on the advancements you made. Should I send the new patch as a follow up or and altogether new patch making the amends in this?

hgext/uncommit.py
273

Will look into it.

288

Oh that was a mistake made due to rebasing on top . I will shift that back.

@martinvonz Sure I will try to unify _fixdirstate() and _uncommitdirstate(). I will follow on the advancements you made. Should I send the new patch as a follow up or and altogether new patch making the amends in this?

A new patch please (I've dropped this version). Just pull from the hg-commited repo and make sure it applies there. Note that you'll probably get an obsmarker when you pull, so you may need to hg touch some version of this commit in your repo to revive it.