Patchwork D3669: cmdutil: have statefile names in STATES instead of functions

login
register
mail settings
Submitter phabricator
Date May 29, 2018, 10:22 p.m.
Message ID <differential-rev-PHID-DREV-umo6acq4p7sif3amccig-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/31898/
State New
Headers show

Comments

phabricator - May 29, 2018, 10:22 p.m.
pulkit created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION 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.

REPOSITORY
  rHG Mercurial

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

AFFECTED FILES
  mercurial/cmdutil.py

CHANGE DETAILS




To: pulkit, #hg-reviewers
Cc: mercurial-devel
phabricator - May 29, 2018, 10:59 p.m.
martinvonz added inline comments.

INLINE COMMENTS

> cmdutil.py:649-650
> +        detected = False
> +        if state == 'merge':
> +            detected = _mergepredicate(repo)
> +        else:

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.

REPOSITORY
  rHG Mercurial

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

To: pulkit, #hg-reviewers
Cc: martinvonz, mercurial-devel
phabricator - June 1, 2018, 10:50 a.m.
pulkit added inline comments.

INLINE COMMENTS

> cmdutil.py:3210
>          if clearable and repo.vfs.exists(f):
>              util.unlink(repo.vfs.join(f))
>  

> 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. :(

REPOSITORY
  rHG Mercurial

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

To: pulkit, #hg-reviewers
Cc: martinvonz, mercurial-devel
phabricator - June 1, 2018, 5:07 p.m.
martinvonz added inline comments.

INLINE COMMENTS

> pulkit wrote in 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. :(

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

REPOSITORY
  rHG Mercurial

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

To: pulkit, #hg-reviewers
Cc: martinvonz, mercurial-devel
phabricator - June 3, 2018, 12:08 p.m.
pulkit added inline comments.

INLINE COMMENTS

> martinvonz wrote in 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

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

REPOSITORY
  rHG Mercurial

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

To: pulkit, #hg-reviewers
Cc: martinvonz, mercurial-devel
phabricator - June 11, 2018, 7:55 p.m.
martinvonz added inline comments.

INLINE COMMENTS

> pulkit wrote in cmdutil.py:3210
> Don't you think it's getting a bit complicated?

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.

REPOSITORY
  rHG Mercurial

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

To: pulkit, #hg-reviewers
Cc: martinvonz, mercurial-devel
phabricator - July 1, 2018, 9:27 p.m.
pulkit added inline comments.

INLINE COMMENTS

> martinvonz wrote in 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.

(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.

REPOSITORY
  rHG Mercurial

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

To: pulkit, #hg-reviewers
Cc: martinvonz, mercurial-devel
phabricator - July 11, 2018, 9:35 p.m.
pulkit planned changes to this revision.
pulkit added a comment.


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

REPOSITORY
  rHG Mercurial

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

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

Patch

diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
--- a/mercurial/cmdutil.py
+++ b/mercurial/cmdutil.py
@@ -627,25 +627,30 @@ 
 
 STATES = (
     # (state, predicate to detect states, helpful message function)
-    ('histedit', fileexistspredicate('histedit-state'), _histeditmsg),
-    ('bisect', fileexistspredicate('bisect.state'), _bisectmsg),
-    ('graft', fileexistspredicate('graftstate'), _graftmsg),
-    ('unshelve', fileexistspredicate('unshelverebasestate'), _unshelvemsg),
-    ('rebase', fileexistspredicate('rebasestate'), _rebasemsg),
+    ('histedit', 'histedit-state', _histeditmsg),
+    ('bisect', 'bisect.state', _bisectmsg),
+    ('graft', 'graftstate', _graftmsg),
+    ('unshelve', 'unshelverebasestate', _unshelvemsg),
+    ('rebase', 'rebasestate', _rebasemsg),
     # The merge state is part of a list that will be iterated over.
     # They need to be last because some of the other unfinished states may also
     # be in a merge or update state (eg. rebase, histedit, graft, etc).
     # We want those to have priority.
-    ('merge', _mergepredicate, _mergemsg),
+    ('merge', None, _mergemsg),
 )
 
 def _getrepostate(repo):
     # experimental config: commands.status.skipstates
     skip = set(repo.ui.configlist('commands', 'status.skipstates'))
-    for state, statedetectionpredicate, msgfn in STATES:
+    for state, filename, msgfn in STATES:
         if state in skip:
             continue
-        if statedetectionpredicate(repo):
+        detected = False
+        if state == 'merge':
+            detected = _mergepredicate(repo)
+        else:
+            detected = repo.vfs.exists(filename)
+        if detected:
             return (state, msgfn)
 
 def morestatus(repo, fm):