uncommit: added interactive mode -i(issue6062)
Needs ReviewPublic

Authored by taapas1128 on Fri, Feb 22, 1:18 PM.

Details

Reviewers
None
Group Reviewers
hg-reviewers
Summary

This adds interactive option to uncommit . Part of this code has been
imported from evolve from commit hash da2fdaa3d97c. The basic
functionality of the code lies in _interactiveuncommit(). It first uncommits
everything then performs action similar to interactive amend .

The current implementation is not perfect and might result in Hunk Failed.
at some point.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped
There are a very large number of changes, so older changes are hidden. Show Older Changes
martinvonz added inline comments.Fri, Feb 22, 1:29 PM
hgext/uncommit.py
125

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)?

367

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.

368

Keep the match optional instead?

taapas1128 updated this revision to Diff 14192.Fri, Feb 22, 1:32 PM
taapas1128 retitled this revision from uncommit: added interactive mode and removed _fixdirstate()(issue6062) to uncommit: added interactive mode and removed _fixdirstate(issue6062).
taapas1128 updated this revision to Diff 14193.Fri, Feb 22, 2:44 PM
taapas1128 retitled this revision from uncommit: added interactive mode and removed _fixdirstate(issue6062) to uncommit: added interactive mode and modified fixdirstate(issue6062).
taapas1128 marked 4 inline comments as done.Fri, Feb 22, 2:51 PM

@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.

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

pulkit added inline comments.Thu, Feb 28, 4:27 PM
hgext/uncommit.py
107

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

taapas1128 updated this revision to Diff 14278.Thu, Feb 28, 4:44 PM
taapas1128 marked an inline comment as done.Thu, Feb 28, 4:46 PM

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

taapas1128 updated this revision to Diff 14385.Thu, Mar 7, 1:24 PM

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':
taapas1128 updated this revision to Diff 14386.Thu, Mar 7, 2:53 PM
taapas1128 updated this revision to Diff 14387.Thu, Mar 7, 2:59 PM
taapas1128 edited the summary of this revision. (Show Details)Thu, Mar 7, 3:01 PM
taapas1128 updated this revision to Diff 14388.Thu, Mar 7, 3:04 PM
taapas1128 retitled this revision from uncommit: added interactive mode and modified fixdirstate(issue6062) to uncommit: added interactive mode -i(issue6062).

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.

hgext/uncommit.py
108–110

No need to pass the interactive flag here, right?

110

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

110–111

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.

148

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.

taapas1128 updated this revision to Diff 14389.Thu, Mar 7, 4:00 PM
taapas1128 marked 2 inline comments as done and an inline comment as not done.
taapas1128 marked 3 inline comments as done.Thu, Mar 7, 4:02 PM
taapas1128 added inline comments.
hgext/uncommit.py
110–111

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

martinvonz added inline comments.Thu, Mar 7, 4:07 PM
hgext/uncommit.py
110–111

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

Are you referring to D971? I see that your change kind of backs out that change. But why?

taapas1128 marked an inline comment as done.Fri, Mar 8, 2:05 AM
taapas1128 added inline comments.
hgext/uncommit.py
110–111

In line number 115 a matcher is used . This is between oldctx and netctx now . In D971 a the matcher was removed so the comment was altered . But whenever the matcher was reused the comment wasn't changed .

martinvonz added inline comments.Fri, Mar 8, 10:03 AM
hgext/uncommit.py
110–111

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

taapas1128 added inline comments.Fri, Mar 8, 10:24 AM
hgext/uncommit.py
110–111

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

taapas1128 updated this revision to Diff 14399.Fri, Mar 8, 10:27 AM
taapas1128 added inline comments.Fri, Mar 8, 12:39 PM
hgext/uncommit.py
367

@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 ?

martinvonz added inline comments.Fri, Mar 8, 12:41 PM
hgext/uncommit.py
367

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.

taapas1128 added inline comments.Mon, Mar 11, 11:39 AM
hgext/uncommit.py
367

okay sending a separate patch right away

taapas1128 updated this revision to Diff 14464.Mon, Mar 11, 1:17 PM

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).

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.

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.

In D6005#89211, @pulkit 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.

hgext/uncommit.py
149

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.

191–194

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.

214–215

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.

230

nit: indent by 4 spaces

235

Should probably use scmutil.cleanupnodes() here too?

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.

261

nit: indent by 4 spaces

270

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

tests/test-uncommit-interactive.t
111

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 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).

taapas1128 marked 7 inline comments as done.
taapas1128 marked an inline comment as done.Thu, Mar 14, 10:39 AM
taapas1128 edited the summary of this revision. (Show Details)Thu, Mar 14, 10:50 AM

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

tests/test-uncommit-interactive.t
111

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

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.

hgext/uncommit.py
185–190

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).

214

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)

235

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.

239

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.

253–254

It doesn't look done to me...

263

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

268

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.

tests/test-uncommit-interactive.t
111

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.

taapas1128 updated this revision to Diff 14520.Sat, Mar 16, 5:24 AM
taapas1128 marked 4 inline comments as done.
taapas1128 edited the summary of this revision. (Show Details)
taapas1128 updated this revision to Diff 14521.Sat, Mar 16, 5:36 AM
taapas1128 marked an inline comment as done.
taapas1128 edited the summary of this revision. (Show Details)Sat, Mar 16, 5:41 AM
taapas1128 added inline comments.Sat, Mar 16, 6:12 AM
hgext/uncommit.py
239

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.

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 ?

martinvonz added inline comments.Sat, Mar 16, 10:18 AM
hgext/uncommit.py
239

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

253–254

I suggested addressing the todo now instead of waiting.

taapas1128 added inline comments.Sat, Mar 16, 11:52 AM
hgext/uncommit.py
239

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

253–254

okay sure.

pulkit added inline comments.Mon, Mar 18, 1:17 PM
hgext/uncommit.py
255

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

taapas1128 updated this revision to Diff 14546.Mon, Mar 18, 2:07 PM
taapas1128 edited the summary of this revision. (Show Details)
taapas1128 marked an inline comment as done.Mon, Mar 18, 2:10 PM
taapas1128 updated this revision to Diff 14547.Mon, Mar 18, 2:37 PM
taapas1128 edited the summary of this revision. (Show Details)Mon, Mar 18, 2:56 PM
pulkit added inline comments.Tue, Mar 19, 7:36 AM
mercurial/patch.py
1046 ↗(On Diff #14547)

this should be uncommit this change right?

1050 ↗(On Diff #14547)

uncommit instead of record here too.

1052 ↗(On Diff #14547)

here also, uncommit instead of record.

1053 ↗(On Diff #14547)

uncommitting instead of recording

taapas1128 updated this revision to Diff 14558.Tue, Mar 19, 9:01 AM
taapas1128 marked 4 inline comments as done.
taapas1128 edited the summary of this revision. (Show Details)
taapas1128 edited the summary of this revision. (Show Details)Tue, Mar 19, 9:05 AM