Patchwork D7595: status: outputting structured unfinished-operation information

login
register
mail settings
Submitter phabricator
Date Dec. 10, 2019, 6:40 a.m.
Message ID <differential-rev-PHID-DREV-vul5s2lsfchitwmciful-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/43663/
State Superseded
Headers show

Comments

phabricator - Dec. 10, 2019, 6:40 a.m.
rdamazio created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  This adds a new item in the json/template output for morestatus. For backwards
  compatibility, I put this behind a config option (but I'd be happy to remove
  that if nobody thinks it's a concern), and added item types to all entries.

REPOSITORY
  rHG Mercurial

BRANCH
  default

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

AFFECTED FILES
  mercurial/cmdutil.py
  mercurial/commands.py
  mercurial/configitems.py
  tests/test-conflict.t
  tests/test-status.t

CHANGE DETAILS




To: rdamazio, #hg-reviewers
Cc: mercurial-devel
phabricator - Dec. 10, 2019, 1:19 p.m.
pulkit added inline comments.
pulkit added subscribers: yuja, pulkit.

INLINE COMMENTS

> configitems.py:247
>  coreconfigitem(
> +    b'commands', b'status.morestatus-item', default=False,
> +)

IMO the whole morestatus functionality is already behind a config option, so lets not have a config option just for that.

But I am not sure whether this change is a BC or not. I believe @yuja might have thoughts here.

REPOSITORY
  rHG Mercurial

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

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

To: rdamazio, #hg-reviewers
Cc: pulkit, yuja, mercurial-devel
phabricator - Dec. 10, 2019, 6:07 p.m.
rdamazio added inline comments.

INLINE COMMENTS

> pulkit wrote in configitems.py:247
> IMO the whole morestatus functionality is already behind a config option, so lets not have a config option just for that.
> 
> But I am not sure whether this change is a BC or not. I believe @yuja might have thoughts here.

I'm fine with removing the config option, but will let yuja reply first.
About being BC or not - it depends on how people are parsing the JSON output :) If they're either turning off morestatus for -Tjson calls, or they're ok with an entry with no path, then it should work, but I won't be surprised if it breaks some callers.

REPOSITORY
  rHG Mercurial

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

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

To: rdamazio, #hg-reviewers
Cc: pulkit, yuja, mercurial-devel
phabricator - Dec. 11, 2019, 5:35 p.m.
durin42 added inline comments.

INLINE COMMENTS

> rdamazio wrote in configitems.py:247
> I'm fine with removing the config option, but will let yuja reply first.
> About being BC or not - it depends on how people are parsing the JSON output :) If they're either turning off morestatus for -Tjson calls, or they're ok with an entry with no path, then it should work, but I won't be surprised if it breaks some callers.

I also support not adding a second config option and just adding content that's from the morestatus config.

REPOSITORY
  rHG Mercurial

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

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

To: rdamazio, #hg-reviewers
Cc: durin42, pulkit, yuja, mercurial-devel
phabricator - Dec. 12, 2019, 12:27 p.m.
pulkit added a comment.


  Applying this on top of D7605 <https://phab.mercurial-scm.org/D7605> failed. So unfortunately you have to rebase again and resend. Also, I think you are `pip install black` away from preventing such conflicts.

REPOSITORY
  rHG Mercurial

BRANCH
  default

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

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

To: rdamazio, #hg-reviewers, pulkit
Cc: durin42, pulkit, yuja, mercurial-devel
phabricator - Dec. 12, 2019, 5:33 p.m.
martinvonz added a comment.


  In D7595#111986 <https://phab.mercurial-scm.org/D7595#111986>, @pulkit wrote:
  
  > Applying this on top of D7605 <https://phab.mercurial-scm.org/D7605> failed. So unfortunately you have to rebase again and resend. Also, I think you are `pip install black` away from preventing such conflicts.
  
  I'll queue this based on Pulkit's approval.

REPOSITORY
  rHG Mercurial

BRANCH
  default

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

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

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

Patch

diff --git a/tests/test-status.t b/tests/test-status.t
--- a/tests/test-status.t
+++ b/tests/test-status.t
@@ -254,35 +254,43 @@ 
   $ hg status -A -Tjson
   [
    {
+    "itemtype": "file",
     "path": "added",
     "status": "A"
    },
    {
+    "itemtype": "file",
     "path": "copied",
     "source": "modified",
     "status": "A"
    },
    {
+    "itemtype": "file",
     "path": "removed",
     "status": "R"
    },
    {
+    "itemtype": "file",
     "path": "deleted",
     "status": "!"
    },
    {
+    "itemtype": "file",
     "path": "unknown",
     "status": "?"
    },
    {
+    "itemtype": "file",
     "path": "ignored",
     "status": "I"
    },
    {
+    "itemtype": "file",
     "path": ".hgignore",
     "status": "C"
    },
    {
+    "itemtype": "file",
     "path": "modified",
     "status": "C"
    }
@@ -558,6 +566,7 @@ 
   $ hg status --config ui.formatdebug=True --rev 1 1
   status = [
       {
+          'itemtype': 'file',
           'path': '1/2/3/4/5/b.txt',
           'status': 'R'
       },
diff --git a/tests/test-conflict.t b/tests/test-conflict.t
--- a/tests/test-conflict.t
+++ b/tests/test-conflict.t
@@ -63,16 +63,38 @@ 
   $ hg status -Tjson
   [
    {
+    "itemtype": "file",
     "path": "a",
     "status": "M",
     "unresolved": true
    },
    {
+    "itemtype": "file",
     "path": "a.orig",
     "status": "?"
    }
   ]
 
+  $ hg status -Tjson --config commands.status.morestatus-item=1
+  [
+   {
+    "itemtype": "file",
+    "path": "a",
+    "status": "M",
+    "unresolved": true
+   },
+   {
+    "itemtype": "file",
+    "path": "a.orig",
+    "status": "?"
+   },
+   {
+    "itemtype": "morestatus",
+    "unfinished": "merge",
+    "unfinishedmsg": "To continue:    hg commit\nTo abort:       hg merge --abort"
+   }
+  ]
+
   $ cat a
   Small Mathematical Series.
   1
diff --git a/mercurial/configitems.py b/mercurial/configitems.py
--- a/mercurial/configitems.py
+++ b/mercurial/configitems.py
@@ -244,6 +244,9 @@ 
     b'commands', b'show.aliasprefix', default=list,
 )
 coreconfigitem(
+    b'commands', b'status.morestatus-item', default=False,
+)
+coreconfigitem(
     b'commands', b'status.relative', default=False,
 )
 coreconfigitem(
diff --git a/mercurial/commands.py b/mercurial/commands.py
--- a/mercurial/commands.py
+++ b/mercurial/commands.py
@@ -6884,7 +6884,7 @@ 
             for f in files:
                 fm.startitem()
                 fm.context(ctx=ctx2)
-                fm.data(path=f)
+                fm.data(itemtype=b'file', path=f)
                 fm.condwrite(showchar, b'status', b'%s ', char, label=label)
                 fm.plain(fmt % uipathfn(f), label=label)
                 if f in copy:
diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
--- a/mercurial/cmdutil.py
+++ b/mercurial/cmdutil.py
@@ -786,6 +786,7 @@ 
     unfinishedmsg = attr.ib()
     inmergestate = attr.ib()
     unresolvedpaths = attr.ib()
+    adddataitem = attr.ib()
     _label = b'status.morestatus'
 
     def formatfile(self, path, fm):
@@ -797,6 +798,11 @@ 
                      ) % self.unfinishedop
         fm.plain(b'%s\n' % _commentlines(statemsg), label=self._label)
 
+        if self.adddataitem:
+            fm.startitem()
+            fm.data(itemtype=b'morestatus', unfinished=self.unfinishedop,
+                    unfinishedmsg=self.unfinishedmsg)
+
         self._formatconflicts(fm)
         if self.unfinishedmsg:
             fm.plain(b'%s\n' % _commentlines(self.unfinishedmsg),
@@ -841,8 +847,9 @@ 
     unresolved = None
     if mergestate.active():
         unresolved = sorted(mergestate.unresolved())
+    dataitem = repo.ui.config(b'commands', b'status.morestatus-item')
     return morestatus(repo.root, unfinishedop, unfinishedmsg,
-                      unresolved is not None, unresolved)
+                      unresolved is not None, unresolved, dataitem)
 
 
 def findpossible(cmd, table, strict=False):