| baymax |
| hg-reviewers |
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.
| Lint Skipped |
| Unit Tests Skipped |
I haven't reviewed much of it, but here are some initial comments
| hgext/uncommit.py | ||
|---|---|---|
| 109 | 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.) | |
| 124 | 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)? | |
| 368 | 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. | |
| 369 | Keep the match optional instead? | |
@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 ?
| hgext/uncommit.py | ||
|---|---|---|
| 107 | I just skimmed through changes and looks like this is missing in the changed version. | |
@pulkit Thanks for pointing that out . That was extremely silly of me . I have corrected that . Is there anything else to be changed ?
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':
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 | ||
|---|---|---|
| 109 | No need to pass the interactive flag here, right? | |
| 111 | I preferred the old wording where it said "newctx" instead of "a copy of oldctx". | |
| 111–112 | 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. | |
| 147–149 | Similar existing flags and their descriptions: commit: "use interactive mode" I'd prefer to be consistent with one of those. I personally think revert's and absorb's versions are clearest. | |
| hgext/uncommit.py | ||
|---|---|---|
| 111–112 | It is the part of _uncommitdirstate() that has been left behind. okay I am not changing that | |
| hgext/uncommit.py | ||
|---|---|---|
| 111–112 | So you're fixing something that was missed in an old commit? Please put that fix in a separate patch. | |
| hgext/uncommit.py | ||
|---|---|---|
| 111–112 | Okay . Then I am reverting this back to what it was. | |
| hgext/uncommit.py | ||
|---|---|---|
| 368 | @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 ? | |
| hgext/uncommit.py | ||
|---|---|---|
| 368 | 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. | |
| hgext/uncommit.py | ||
|---|---|---|
| 368 | okay sending a separate patch right away | |
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.
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 | ||
|---|---|---|
| 148 | 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–196 | 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. | |
| 216–217 | 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. | |
| 232 | nit: indent by 4 spaces | |
| 237 | Should probably use scmutil.cleanupnodes() here too? | |
| 255–256 | 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. | |
| 263 | nit: indent by 4 spaces | |
| 272 | 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). | |
@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). | |
| 216 | 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) | |
| 237 | 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. | |
| 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. | |
| 255–256 | It doesn't look done to me... | |
| 265 | amend_source includes the full hash. I think it's better to do the same here. | |
| 270 | 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. | |
| hgext/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. | |
| 255–256 | 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 ? | |
| hgext/uncommit.py | ||
|---|---|---|
| 257 | This operation thing can be used to show select hunks to uncommit message. | |
There seems to have been no activities on this Diff for the past 3 Months.
By policy, we are automatically moving it out of the need-review state.
Please, move it back to need-review without hesitation if this diff should still be discussed.
:baymax:need-review-idle:
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.)