[Differential] D6503: statecheck: added support for STATES

[martinvonz (Martin von Zweigbergk)]

martinvonz added a comment.


  > Results of tests are shown.
  
  Are there other functional changes that are not shown? For example, was it previously allowed to do some operations (update?) during an interrupted rebase that are no longer allowed?

INLINE COMMENTS

> taapas1128 wrote in state.py:147
> mergecheck is introduced in this patch now so now this change must be okay I suppose.

Yes, I agree with doing it here. It wasn't used before, so it wasn't clear then why it would be needed or how it would be used.

But actually, it still doesn't seem to be used. Should it be removed?

> state.py:161
> +unfinishedstates.append(
> +    statecheck('merge', 'None', clearable=True, allowcommit=False,
> +               cmdhint=_('To continue:    hg commit\n'

Did you mean None instead of 'None'?

> state.py:161
> +unfinishedstates.append(
> +    statecheck('merge', 'None', clearable=True, allowcommit=False,
> +               cmdhint=_('To continue:    hg commit\n'

There was a comment somewhere saying that merge has to come last. It seems merge is no longer last and you've solved it by checking specifically for 'merge' in many places. I think it would be better to check for unfinished merges last like we used to. That can be achieved by using a helper that returns the states in the desired order, or it can be done by hiding the registration behind a function that knows to insert new states before 'merge'. Or perhaps it can be generalized to some "priority" option for the states where the lowest priority is checked last (and rebase, histedit and similar would all have the same priority)

> state.py:174
> +        if (state._clearable or (commit and state._allowcommit)
> +            or state._cmdname == 'merge'or state._cmdname == 'bisect'):
>              continue

Why is merge not considered? The caller is expected to call bailifchanged() right after. I think that will report the unfinished merge state. What happens if we report it here? Would it result in a different message? Or would it fail when it shouldn't when bailifchanged() is called with merge=True (or False?)?

> state.py:215
> +            continue
> +        if s._cmdname == 'update' and s.isunfinished(repo):
> +            return (s._cmdname, s.hint())

Hard-coded values like this goes against the idea of a generic check for unfinished operations. What is it that we really want to check here? The same applies in lots of places above, of course. This really needs to be fixed.

It seems bisect state is quite different from the others. We want to allow any (?) operation with an unfinished bisect state. We just want to report it as an unfinished operation in morestatus. So maybe we need another flag like reportonly=True?

REPOSITORY
  rHG Mercurial

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

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

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