This adds interactive mode to uncommit option . This basically combines uncommit with hg commit -i as stated in the bug description.
Details
- Reviewers
durin42 - Group Reviewers
hg-reviewers - Commits
- rHG64d519ca44db: uncommit: added interactive mode(issue6062)
rHGedd5c8dc812e: uncommit: added interactive mode(issue6062)
Diff Detail
- Repository
- rHG Mercurial
- Lint
Lint Skipped - Unit
Unit Tests Skipped
Event Timeline
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 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.
@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. |
It will be nice if you send a patch for -n flag first, then rebase these patch on top of that.
@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.
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 | ||
---|---|---|
321 | Need an extra blank line here. |
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 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.
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 | ||
---|---|---|
144 | Much of this duplicates _fixdirstate(). We must be able to share some of this code. | |
276 | Unnecessary (done on line 269) | |
291 | 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 | ||
---|---|---|
276 | Will look into it. | |
291 | Oh that was a mistake made due to rebasing on top . I will shift that back. |
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.
Much of this duplicates _fixdirstate(). We must be able to share some of this code.