[Differential] D6503: statecheck: added support for STATES

taapas1128 (Taapas Agrawal) phabricator at mercurial-scm.org
Tue Jun 11 13:19:38 EDT 2019


taapas1128 added inline comments.

INLINE COMMENTS

> martinvonz wrote in state.py:161
> 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)

In `getrepostate()` function merge must come last that is why I have filtered off update and merge and then checked them at the very end. In `checkunfinished()` there is no such thing.

> martinvonz wrote in state.py:174
> 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?)?

Actually in older code too there is no support for merge and bisect in `checkunfinished()` or `clearunfinished()` and is handled seperately. So I have bypassed merge and bisect over here too to comply with it.  As you can see later in `clearunfinished()` has `util.unlink()` and as fname is not present for merge it cant be passed here.

> martinvonz wrote in state.py:215
> 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?

These are hard-coded as we need to check update and merge last. Earlier they were hard coded in form of a list. here I have bypassed them earlier and then checked for them last .

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


More information about the Mercurial-devel mailing list