cmdutil: have statefile names in STATES instead of functions
Changes PlannedPublic

Authored by pulkit on May 29 2018, 6:22 PM.

Details

Reviewers
None
Group Reviewers
hg-reviewers
Summary

This will help us in unifying cmdutil.STATES and cmdutil.unfinishedstates as the
checking logic for STATES is moved to _getrepostate() and STATES now have just
filenames which is similar to unfinishedstates.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped
pulkit created this revision.May 29 2018, 6:22 PM
martinvonz added inline comments.
mercurial/cmdutil.py
649–650

The point of the STATES dict (as opposed to putting all the code here) seems to be to make it generic and extensible. Handling "merge" differently here seems like a step backwards. How about instead making unfinishedstates more generic by supporting a predicate there instead of the file? We could still allow files as well for BC for a little while.

pulkit added inline comments.Jun 1 2018, 6:50 AM
mercurial/cmdutil.py
3210

How about instead making unfinishedstates more generic by supporting a predicate there
instead of the file? We could still allow files as well for BC for a little while.

@martinvonz : because of above util.unlink() we have to keep the filenames in unfinishedstates. :(

martinvonz added inline comments.Jun 1 2018, 1:07 PM
mercurial/cmdutil.py
3210

We don't have to. We could add a clearstatefn function in addition to the statedetectionpredicate. Perhaps the if not clearable and ... stuff could move into the function in that case

pulkit added inline comments.Jun 3 2018, 8:08 AM
mercurial/cmdutil.py
3210

Don't you think it's getting a bit complicated?

martinvonz added inline comments.Jun 11 2018, 3:55 PM
mercurial/cmdutil.py
3210

It's a bit more complicated, I agree, but I think it's better than the alternative. If it removes the need for clearable, it's not that much more complicated.

An alternative would be to create an object with [is]dirty(), allowcommit(), and clear() methods and have a function that creates such an object for file-based states (all but .hg/merge?). That would reduce the duplication of fileexistspredicate(<name>) and clearfunction(<name>).

PS. Sorry about the delay. I've been away.

pulkit added inline comments.Sun, Jul 1, 5:27 PM
mercurial/cmdutil.py
3210

(Sorry, I missed this one too. I have turned off personal emails from phabricator and use mercurial-devel to keep track of people replying to me.)

The object sounds good API. I will try that.

pulkit planned changes to this revision.Wed, Jul 11, 5:35 PM

I won't have time before the upcoming freeze to get this done.