D529: uncommit: move fb-extension to core which uncommits a changeset

[yuja (Yuya Nishihara)]

yuja added a comment.


  Overall, the patch looks good, but I don't have expertise in this area.
  
  > The uncommit extension in fb-hgext does not creates an empty commit like the one
  >  in evolve extension unless user has specified ui.alllowemptycommit to True.
  
  Maybe we'll need a command option to keep the empty commit? ui.allowemptycommit
  is an internal flag,

INLINE COMMENTS

> uncommit.py:49
> +
> +def _commitfiltered(repo, ctx, match):
> +    """Recommit ctx with changed files not in match. Return the new

Can't we make this share some bits with cmdutil.amend?

> uncommit.py:153
> +
> +        if len(wctx.parents()) <= 0 or not wctx.parents()[0]:
> +            raise error.Abort(_("cannot uncommit null changeset"))

Nit: wctx.parents() should return at least one ctx even if it is null
revision.

> uncommit.py:159
> +        oldphase = old.phase()
> +        if oldphase == phases.public:
> +            raise error.Abort(_("cannot rewrite immutable changeset"))

Nit: amend uses old.mutable().

> uncommit.py:168
> +            if newid is None:
> +                raise error.Abort(_('nothing to uncommit'))
> +

Perhaps this should exit with 1 as "hg commit" would do.

> uncommit.py:173
> +                obsolete.createmarkers(repo, [(old, (repo[newid],))])
> +                phases.retractboundary(repo, tr, oldphase, [newid])
> +            else:

I'm not sure if this `retractboundary()` is valid. cmdutil.amend()
appears to use phases.new-commit config instead.

> uncommit.py:176
> +                # Fully removed the old commit
> +                obsolete.createmarkers(repo, [(old, ())])
> +

Can't we use scmutil.cleanupnodes()?

REPOSITORY
  rHG Mercurial

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

To: pulkit, #hg-reviewers
Cc: yuja, mercurial-devel