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

[martinvonz (Martin von Zweigbergk)]

martinvonz added a comment.


  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.

INLINE COMMENTS

> uncommit.py:185-190
>              keepcommit = pats
>              if not keepcommit:
>                  if opts.get('keep') is not None:
>                      keepcommit = opts.get('keep')
>                  else:
>                      keepcommit = ui.configbool('experimental', 'uncommit.keep')

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

> uncommit.py:216
> +def _interactiveuncommit(ui, repo, old, match):
> +    """ Makes a temporary commit with the chunks which user
> +    selected to uncommit. After that the diff of the parent and that commit is

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)

> uncommit.py:241
> +
> +def _createtempcommit(ui, repo, old, match):
> +    """ Creates a temporary commit for `uncommit --interative` which contains

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.

> uncommit.py:265
> +    fp.seek(0)
> +    oldnode = node.hex(old.node())[:12]
> +    message = 'temporary commit for uncommiting %s' % oldnode

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

> uncommit.py:270
> +
> +def _patchtocommit(ui, repo, old, fp, message=None, extras=None):
> +    """ Applies the patch to the working directory and

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

> martinvonz wrote in uncommit.py:235
> Should probably use `scmutil.cleanupnodes()` here too?

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.

> martinvonz wrote in uncommit.py: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.

It doesn't look done to me...

> taapas1128 wrote in test-uncommit-interactive.t:110
> Can I work on this as a part as a follow up ? or should i make amends in this patch .

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.

REPOSITORY
  rHG Mercurial

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

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