D6005: uncommit: added interactive mode -i(issue6062)

[martinvonz (Martin von Zweigbergk)]

martinvonz added subscribers: spectral, ryanmce.
martinvonz added a comment.


  In https://phab.mercurial-scm.org/D6005#89211, @pulkit wrote:
  
  > In https://phab.mercurial-scm.org/D6005#89182, @martinvonz 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.

INLINE COMMENTS

> uncommit.py:148
> +    [('i', 'interactive', None,
> +     _('interactively select which chunks to apply')),
> +     ('', 'keep', None, _('allow an empty commit after uncommiting')),

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

> uncommit.py:191
>                      keepcommit = ui.configbool('experimental', 'uncommit.keep')
>              newid = _commitfiltered(repo, old, match, keepcommit)
> +            if interactive:

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.

> uncommit.py:214-215
> +def _interactiveuncommit(ui, repo, old, match):
> +    """ The function which contains all the logic for interactively uncommiting
> +    a commit. This function makes a temporary commit with the chunks which user
> +    selected to uncommit. After that the diff of the parent and that commit is

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.

> uncommit.py:230
> +                                     opts=diffopts):
> +            fp.write(chunk)
> +

nit: indent by 4 spaces

> uncommit.py:235
> +    # creating obs marker temp -> ()
> +    obsolete.createmarkers(repo, [(repo[tempnode], ())], operation="uncommit")
> +    return newnode

Should probably use `scmutil.cleanupnodes()` here too?

> uncommit.py:253-254
> +    # uncommit a removed file partially.
> +    # TODO: wrap the operations in mercurial/patch.py and mercurial/crecord.py
> +    # to add uncommit as an operation taking care of BC.
> +    chunks, opts = cmdutil.recordfilter(repo.ui, originalchunks,

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.

> uncommit.py:261
> +    for c in chunks:
> +            c.write(fp)
> +

nit: indent by 4 spaces

> uncommit.py:270
> +def _patchtocommit(ui, repo, old, fp, message=None, extras=None):
> +    """ A function which will apply the patch to the working directory and
> +    make a commit whose parents are same as that of old argument. The message

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

> test-uncommit-interactive.t:110
> +   3
> +  discard change 1/3 to 'a'? [Ynesfdaq?] n
> +  

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 https://phab.mercurial-scm.org/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).

REPOSITORY
  rHG Mercurial

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

To: taapas1128, #hg-reviewers
Cc: ryanmce, spectral, pulkit, martinvonz, mercurial-devel