D6501: state: created new class statecheck to handle unfinishedstates

martinvonz (Martin von Zweigbergk) phabricator at mercurial-scm.org
Tue Jun 18 02:57:03 EDT 2019


martinvonz added inline comments.

INLINE COMMENTS

> shelve.py:1148
> +        allowcommit = False,
> +        cmdmsg = _('unshelve already in progress')
> +    )

A good patch to send right after this patch is one that removes this line and lets shelve use the default message. I understand why they picked "already" (because it makes sense when you run `hg shelve/unshelve` with an ongoing unshelve), but it doesn't make sense when e.g. running `hg commit`. There is already a test showing the bad example (and the good one) in `test-shelve.t`.

> state.py:100
> +
> +    def __init__(self, opname, fname, clearable=False, allowcommit=False,
> +                 cmdmsg="", cmdhint=""):

I'm not sure  if we should have a default for `clearable`. Not having a default would force callers to think about it, which may be good. OTOH, the default of `False` is pretty safe, so it's probably fine to have a default. But if we do have a default, I think you should update the callers to not pass `clearable` when they want the default.

> state.py:153
> +    clearable = True,
> +    allowcommit = False,
> +    cmdhint = _("use 'hg graft --continue' or 'hg graft --stop' to stop")

Don't specify `allowcommit` when it's the same as the default (the same applies in other places)

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6501/new/

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

To: taapas1128, durin42, martinvonz, #hg-reviewers
Cc: av6, Kwan, pulkit, mjpieters, mercurial-devel


More information about the Mercurial-devel mailing list