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

durham (Durham Goode) phabricator at mercurial-scm.org
Mon Sep 11 16:23:15 UTC 2017


durham accepted this revision.
durham added a comment.


  Looks good, except for my ui.allowemptycommit comment.

INLINE COMMENTS

> uncommit.py:59
> +    files = (initialfiles - exclude)
> +    allowempty = allowempty or repo.ui.configbool('ui', 'allowemptycommit')
> +    if not files and not allowempty:

If I read this right, it means a user having ui.allowemptycommit set to True will cause hg uncommit to always leave a commit behind?  I don't think that's what we want.  ui.allowemptycommit is to let a user make an empt commit if they want.  It's not really meant to say 'leave empty commits behind for me'.  I don't think it should affect the uncommit command.

I see this came from our original extension.  I'd still drop it.

> uncommit.py:61
> +    if not files and not allowempty:
> +        return ctx.parents()[0].node()
> +

Might be worth a comment that returning p1 here is special cased to not create an obsmarker later.

> uncommit.py:154
> +        if not old.mutable():
> +            raise error.Abort(_('cannot amend public changesets'))
> +        if len(old.parents()) > 1:

Should "amend" be "uncommit" here?

> uncommit.py:159
> +        if not allowunstable and old.children():
> +            raise error.Abort(_('cannot amend changeset with children'))
> +

Same here, "amend" -> "uncommit"

REPOSITORY
  rHG Mercurial

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

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


More information about the Mercurial-devel mailing list