Patchwork [RFC] debugobsolete: add formatter support (issue5134)

login
register
mail settings
Submitter liscju
Date July 5, 2016, 9:43 a.m.
Message ID <75dcd3200e5eb25cea41.1467711780@liscju-VirtualBox>
Download mbox | patch
Permalink /patch/15752/
State Changes Requested
Delegated to: Yuya Nishihara
Headers show

Comments

liscju - July 5, 2016, 9:43 a.m.
# HG changeset patch
# User liscju <piotr.listkiewicz@gmail.com>
# Date 1467710115 -7200
#      Tue Jul 05 11:15:15 2016 +0200
# Node ID 75dcd3200e5eb25cea41e39a26bcde12f50de376
# Parent  6a98f9408a504be455d4382801610daceac429e6
debugobsolete: add formatter support (issue5134)

This solution keeps the same output debugobsolete had
before formatter support, but there are few issues
I didnt manage to resolve:

1) succnodes, parents were space separated strings, in
formatter they should be passed as list to be printed
by jsonformatter output as for example:
succnodes: [succnode1, succnode2, etc]

Problem is how to pass it as list, but at the same time
keep the same output it had before.

2) metadata should be printed as dictionary, but when
it is passed as one it changes plain old formatting.

3) flags formatting, fm.write('flags', '%X', marker.flags())
behaves differently for plain formatting(it prints some flags
as C) but for json formatting it always prints number.
Yuya Nishihara - July 9, 2016, 11:12 a.m.
On Tue, 05 Jul 2016 11:43:00 +0200, liscju wrote:
> # HG changeset patch
> # User liscju <piotr.listkiewicz@gmail.com>
> # Date 1467710115 -7200
> #      Tue Jul 05 11:15:15 2016 +0200
> # Node ID 75dcd3200e5eb25cea41e39a26bcde12f50de376
> # Parent  6a98f9408a504be455d4382801610daceac429e6
> debugobsolete: add formatter support (issue5134)
> 
> This solution keeps the same output debugobsolete had
> before formatter support, but there are few issues
> I didnt manage to resolve:
> 
> 1) succnodes, parents were space separated strings, in
> formatter they should be passed as list to be printed
> by jsonformatter output as for example:
> succnodes: [succnode1, succnode2, etc]
> 
> Problem is how to pass it as list, but at the same time
> keep the same output it had before.

Yeah, it's known issue.

https://bz.mercurial-scm.org/show_bug.cgi?id=5217#c2

The idea I have in mind is to add new API for lists.

  # writelist(field: str, elemname: str, elemfmt: str, data: iterable,
  #           sep='': str)
  fm.writelist('succnodes', 'node', '%s ',
               (hex(repl) for repl in marker.succnodes()))

The point of the formatter API is to avoid divergence between plain and
template/machine outputs.  I think writelist() can handle common cases well.

> 2) metadata should be printed as dictionary, but when
> it is passed as one it changes plain old formatting.

For this particular case, you can use fm.data() and fix _jsonifyobj() to
support dicts. There are more complicated examples in evolve.py and journal.py,
but "{metadata}" can be a plain dict since it isn't a nested template structure.

> 3) flags formatting, fm.write('flags', '%X', marker.flags())
> behaves differently for plain formatting(it prints some flags
> as C) but for json formatting it always prints number.

"C" is hex value. It should be decimal integer in JSON.

> diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
> --- a/mercurial/cmdutil.py
> +++ b/mercurial/cmdutil.py
> @@ -1611,25 +1611,42 @@ def show_changeset(ui, repo, opts, buffe
>  
>      return changeset_templater(ui, repo, matchfn, opts, tmpl, mapfile, buffered)
>  
> -def showmarker(ui, marker, index=None):
> +def showmarker(fm, marker, index=None):
>      """utility function to display obsolescence marker in a readable way
>  
>      To be used by debug function."""
> +    fm.startitem()

Better to move fm.startitem() to the caller because this function doesn't
control the loop.

> +    fm.condwrite(index is not None, 'index', '%i', index)
>      if index is not None:
> -        ui.write("%i " % index)
> +        fm.plain(' ')

JSON and template outputs should always have index, i.e.

  fm.condwrite(showindex, 'index', '%i ', index)

> +    fm.write('date', '%s', util.datestr(marker.date()))

It should be a date tuple in template/machine format.
Yuya Nishihara - July 10, 2016, 12:31 p.m.
On Sat, 9 Jul 2016 20:12:38 +0900, Yuya Nishihara wrote:
> On Tue, 05 Jul 2016 11:43:00 +0200, liscju wrote:
> > Problem is how to pass it as list, but at the same time
> > keep the same output it had before.  
> 
> Yeah, it's known issue.
> 
> https://bz.mercurial-scm.org/show_bug.cgi?id=5217#c2
> 
> The idea I have in mind is to add new API for lists.
> 
>   # writelist(field: str, elemname: str, elemfmt: str, data: iterable,
>   #           sep='': str)
>   fm.writelist('succnodes', 'node', '%s ',
>                (hex(repl) for repl in marker.succnodes()))
> 
> The point of the formatter API is to avoid divergence between plain and
> template/machine outputs.  I think writelist() can handle common cases well.

I came up with a better idea.

  # formatlist(name: str, data: iterable, fmt='%s': str, sep='': str)
  #            -> str|list|templatekw._hybrid
  hexnodes = [hex(repl) for repl in marker.succnodes()]
  fm.write('succnodes', '%s', fm.formatlist('node', hexnodes, fmt='%s '))

It can avoid copying the functionality of (cond)?write(), and has fewer
arguments.

> > +    fm.write('date', '%s', util.datestr(marker.date()))  
> 
> It should be a date tuple in template/machine format.

Maybe we could handle datestr() in the same way?

  fm.formatdate(date: (int, int), format=...) -> str|(int, int)
liscju - July 12, 2016, 9:57 a.m.
>
> For this particular case, you can use fm.data() and fix _jsonifyobj() to
> support dicts. There are more complicated examples in evolve.py and
> journal.py,
> but "{metadata}" can be a plain dict since it isn't a nested template
> structure.


It wouldn't be a problem that fm.data(..) for plainformatter prints
nothing? It means we would need two invocation of writing metadata:

fm.plain( expression_to_stringify_metadata) # only for plain
fm.data(metadata) # only for templates != plain


> JSON and template outputs should always have index, i.e.
>   fm.condwrite(showindex, 'index', '%i ', index)


As far as i checked only a plainformatter care about the condition in the
condwrite, other templates prints value no matter what condition is. Or i
just don't get the point.

I came up with a better idea.
>   # formatlist(name: str, data: iterable, fmt='%s': str, sep='': str)
>   #            -> str|list|templatekw._hybrid
>   hexnodes = [hex(repl) for repl in marker.succnodes()]
>   fm.write('succnodes', '%s', fm.formatlist('node', hexnodes, fmt='%s '))
> It can avoid copying the functionality of (cond)?write(), and has fewer
> arguments.


I have a problem what formatlist should return for jsonformatter:
1) list - it crash because json doesnt support jsonifying list
2) string - it won't work because json fm.write escapes characters like '['
3) templatekw._hybrid - i have no idea how to do this, i see this returns
generator, i have no idea how to use it

Maybe we could handle datestr() in the same way?
>   fm.formatdate(date: (int, int), format=...) -> str|(int, int)


Plainformatter in this case should probably  return util.datestr(date) but
what about jsonformatter?. JSON spec does not specify how date should look
like, should it return tuple of int and as a result print date as tuple?

2016-07-10 14:31 GMT+02:00 Yuya Nishihara <yuya@tcha.org>:

> On Sat, 9 Jul 2016 20:12:38 +0900, Yuya Nishihara wrote:
> > On Tue, 05 Jul 2016 11:43:00 +0200, liscju wrote:
> > > Problem is how to pass it as list, but at the same time
> > > keep the same output it had before.
> >
> > Yeah, it's known issue.
> >
> > https://bz.mercurial-scm.org/show_bug.cgi?id=5217#c2
> >
> > The idea I have in mind is to add new API for lists.
> >
> >   # writelist(field: str, elemname: str, elemfmt: str, data: iterable,
> >   #           sep='': str)
> >   fm.writelist('succnodes', 'node', '%s ',
> >                (hex(repl) for repl in marker.succnodes()))
> >
> > The point of the formatter API is to avoid divergence between plain and
> > template/machine outputs.  I think writelist() can handle common cases
> well.
>
> I came up with a better idea.
>
>   # formatlist(name: str, data: iterable, fmt='%s': str, sep='': str)
>   #            -> str|list|templatekw._hybrid
>   hexnodes = [hex(repl) for repl in marker.succnodes()]
>   fm.write('succnodes', '%s', fm.formatlist('node', hexnodes, fmt='%s '))
>
> It can avoid copying the functionality of (cond)?write(), and has fewer
> arguments.
>
> > > +    fm.write('date', '%s', util.datestr(marker.date()))
> >
> > It should be a date tuple in template/machine format.
>
> Maybe we could handle datestr() in the same way?
>
>   fm.formatdate(date: (int, int), format=...) -> str|(int, int)
>
Yuya Nishihara - July 12, 2016, 1:23 p.m.
On Tue, 12 Jul 2016 11:57:17 +0200, Piotr Listkiewicz wrote:
> > For this particular case, you can use fm.data() and fix _jsonifyobj() to
> > support dicts. There are more complicated examples in evolve.py and
> > journal.py,
> > but "{metadata}" can be a plain dict since it isn't a nested template
> > structure.
> 
> It wouldn't be a problem that fm.data(..) for plainformatter prints
> nothing? It means we would need two invocation of writing metadata:
> 
> fm.plain( expression_to_stringify_metadata) # only for plain
> fm.data(metadata) # only for templates != plain

Yes. If we want to avoid two separate calls, we could add fm.formatdict().

> > JSON and template outputs should always have index, i.e.
> >   fm.condwrite(showindex, 'index', '%i ', index)
> 
> As far as i checked only a plainformatter care about the condition in the
> condwrite, other templates prints value no matter what condition is. Or i
> just don't get the point.

Printing index=None is wrong. It needs a real index value plus a condition
whether or not index is printed in plain output.

> I came up with a better idea.
> >   # formatlist(name: str, data: iterable, fmt='%s': str, sep='': str)
> >   #            -> str|list|templatekw._hybrid
> >   hexnodes = [hex(repl) for repl in marker.succnodes()]
> >   fm.write('succnodes', '%s', fm.formatlist('node', hexnodes, fmt='%s '))
> > It can avoid copying the functionality of (cond)?write(), and has fewer
> > arguments.
> 
> I have a problem what formatlist should return for jsonformatter:
> 1) list - it crash because json doesnt support jsonifying list
> 2) string - it won't work because json fm.write escapes characters like '['
> 3) templatekw._hybrid - i have no idea how to do this, i see this returns
> generator, i have no idea how to use it

plainformatter.formatlist() returns a formatted string,
templateformatter.formatlist() returns a list object designed for templater,
otherwise returns a list.

I wrote a draft patch:
https://bitbucket.org/yuja/hg-work/commits/05abf1351e40

> Maybe we could handle datestr() in the same way?
> >   fm.formatdate(date: (int, int), format=...) -> str|(int, int)
> 
> Plainformatter in this case should probably  return util.datestr(date) but
> what about jsonformatter?. JSON spec does not specify how date should look
> like, should it return tuple of int and as a result print date as tuple?

[timestamp, tzoffset]

See "hg log -Tjson" for how date should be rendered.

Patch

diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
--- a/mercurial/cmdutil.py
+++ b/mercurial/cmdutil.py
@@ -1611,25 +1611,42 @@  def show_changeset(ui, repo, opts, buffe
 
     return changeset_templater(ui, repo, matchfn, opts, tmpl, mapfile, buffered)
 
-def showmarker(ui, marker, index=None):
+def showmarker(fm, marker, index=None):
     """utility function to display obsolescence marker in a readable way
 
     To be used by debug function."""
+    fm.startitem()
+    fm.condwrite(index is not None, 'index', '%i', index)
     if index is not None:
-        ui.write("%i " % index)
-    ui.write(hex(marker.precnode()))
-    for repl in marker.succnodes():
-        ui.write(' ')
-        ui.write(hex(repl))
-    ui.write(' %X ' % marker.flags())
-    parents = marker.parentnodes()
-    if parents is not None:
-        ui.write('{%s} ' % ', '.join(hex(p) for p in parents))
-    ui.write('(%s) ' % util.datestr(marker.date()))
-    ui.write('{%s}' % (', '.join('%r: %r' % t for t in
+        fm.plain(' ')
+    fm.write('precnode', '%s', hex(marker.precnode()))
+    if marker.succnodes():
+        fm.plain(' ')
+    fm.condwrite(marker.succnodes(),
+                 'succnode',
+                 '%s',
+                 ' '.join(hex(repl) for repl in marker.succnodes()))
+    fm.plain(' ')
+    fm.write('flags', '%X', marker.flags())
+    fm.plain(' ')
+    parents = marker.parentnodes() or []
+    if parents:
+        fm.plain('{')
+    fm.condwrite(parents,
+             'parents',
+             '%s',
+             ', '.join(hex(p) for p in parents))
+    if parents:
+        fm.plain('} ')
+    fm.plain('(')
+    fm.write('date', '%s', util.datestr(marker.date()))
+    fm.plain(') {')
+    fm.write('metadata',
+             '%s',
+             (', '.join('%r: %r' % t for t in
                                  sorted(marker.metadata().items())
                                  if t[0] != 'date')))
-    ui.write('\n')
+    fm.plain('}\n')
 
 def finddate(ui, repo, date):
     """Find the tipmost changeset that matches the given date spec"""
diff --git a/mercurial/commands.py b/mercurial/commands.py
--- a/mercurial/commands.py
+++ b/mercurial/commands.py
@@ -3053,7 +3053,7 @@  def debuglocks(ui, repo, **opts):
          ('r', 'rev', [], _('display markers relevant to REV')),
          ('', 'index', False, _('display index of the marker')),
          ('', 'delete', [], _('delete markers specified by indices')),
-        ] + commitopts2,
+        ] + commitopts2 + formatteropts,
          _('[OBSOLETED [REPLACEMENT ...]]'))
 def debugobsolete(ui, repo, precursor=None, *successors, **opts):
     """create arbitrary obsolete marker
@@ -3141,6 +3141,7 @@  def debugobsolete(ui, repo, precursor=No
             markerset = set(markers)
             isrelevant = lambda m: m in markerset
 
+        fm = ui.formatter('debugobsolete', opts)
         for i, m in enumerate(markerstoiter):
             if not isrelevant(m):
                 # marker can be irrelevant when we're iterating over a set
@@ -3152,7 +3153,8 @@  def debugobsolete(ui, repo, precursor=No
                 # are relevant to --rev value
                 continue
             ind = i if opts.get('index') else None
-            cmdutil.showmarker(ui, m, index=ind)
+            cmdutil.showmarker(fm, m, index=ind)
+        fm.end()
 
 @command('debugpathcomplete',
          [('f', 'full', None, _('complete an entire path')),
diff --git a/tests/test-completion.t b/tests/test-completion.t
--- a/tests/test-completion.t
+++ b/tests/test-completion.t
@@ -261,7 +261,7 @@  Show all commands + options
   debuglocks: force-lock, force-wlock
   debugmergestate: 
   debugnamecomplete: 
-  debugobsolete: flags, record-parents, rev, index, delete, date, user
+  debugobsolete: flags, record-parents, rev, index, delete, date, user, template
   debugpathcomplete: full, normal, added, removed
   debugpushkey: 
   debugpvec: 
diff --git a/tests/test-obsolete.t b/tests/test-obsolete.t
--- a/tests/test-obsolete.t
+++ b/tests/test-obsolete.t
@@ -649,6 +649,82 @@  List of both
   cda648ca50f50482b7055c0b0c4c117bba6733d9 3de5eca88c00aa039da7399a220f4a5221faa585 0 (*) {'user': 'test'} (glob)
   cdbce2fbb16313928851e97e0d85413f3f7eb77f ca819180edb99ed25ceafb3e9584ac287e240b00 0 (Thu Jan 01 00:22:17 1970 +0000) {'user': 'test'}
 
+  $ hg debugobsolete --hidden --rev 3::6 -T json
+  [
+   {
+    "date": "Thu Jan 01 00:22:19 1970 +0000",
+    "flags": 0,
+    "index": null,
+    "metadata": "'user': 'test'",
+    "parents": "",
+    "precnode": "1337133713371337133713371337133713371337",
+    "succnode": "5601fb93a350734d935195fee37f4054c529ff39"
+   },
+   {
+    "date": "Thu Jan 01 00:22:19 1970 +0000",
+    "flags": 0,
+    "index": null,
+    "metadata": "'user': 'test'",
+    "parents": "",
+    "precnode": "1339133913391339133913391339133913391339",
+    "succnode": "ca819180edb99ed25ceafb3e9584ac287e240b00"
+   },
+   {
+    "date": "Thu Jan 01 00:00:01 1970 -0002",
+    "flags": 12,
+    "index": null,
+    "metadata": "'user': 'test'",
+    "parents": "",
+    "precnode": "245bde4270cd1072a27757984f9cda8ba26f08ca",
+    "succnode": "cdbce2fbb16313928851e97e0d85413f3f7eb77f"
+   },
+   {
+    "date": "Thu Jan 01 00:22:18 1970 +0000",
+    "flags": 1,
+    "index": null,
+    "metadata": "'user': 'test'",
+    "parents": "",
+    "precnode": "5601fb93a350734d935195fee37f4054c529ff39",
+    "succnode": "6f96419950729f3671185b847352890f074f7557"
+   },
+   {
+    "date": "Thu Jan 01 00:00:00 1970 +0000",
+    "flags": 0,
+    "index": null,
+    "metadata": "'user': 'test'",
+    "parents": "6f96419950729f3671185b847352890f074f7557",
+    "precnode": "94b33453f93bdb8d457ef9b770851a618bf413e1",
+    "succnode": ""
+   },
+   {
+    "date": "Thu Jan 01 00:22:18 1970 +0000",
+    "flags": 0,
+    "index": null,
+    "metadata": "'user': 'test'",
+    "parents": "",
+    "precnode": "ca819180edb99ed25ceafb3e9584ac287e240b00",
+    "succnode": "1337133713371337133713371337133713371337"
+   },
+   {
+    "date": "*", (glob)
+    "flags": 0,
+    "index": null,
+    "metadata": "'user': 'test'",
+    "parents": "",
+    "precnode": "cda648ca50f50482b7055c0b0c4c117bba6733d9",
+    "succnode": "3de5eca88c00aa039da7399a220f4a5221faa585"
+   },
+   {
+    "date": "Thu Jan 01 00:22:17 1970 +0000",
+    "flags": 0,
+    "index": null,
+    "metadata": "'user': 'test'",
+    "parents": "",
+    "precnode": "cdbce2fbb16313928851e97e0d85413f3f7eb77f",
+    "succnode": "ca819180edb99ed25ceafb3e9584ac287e240b00"
+   }
+  ]
+
 #if serve
 
 Test the debug output for exchange