Patchwork [5,of,5] templater: heuristic to force header/footer in empty logs (issue4135) (BC)

login
register
mail settings
Submitter Yuya Nishihara
Date Aug. 26, 2015, 2:33 p.m.
Message ID <20150826233326.7bd10237169d220e54a6b86a@tcha.org>
Download mbox | patch
Permalink /patch/10282/
State Changes Requested
Headers show

Comments

Yuya Nishihara - Aug. 26, 2015, 2:33 p.m.
On Tue, 25 Aug 2015 14:56:41 -0500, Matt Mackall wrote:
> # HG changeset patch
> # User Matt Mackall <mpm@selenic.com>
> # Date 1440528293 18000
> #      Tue Aug 25 13:44:53 2015 -0500
> # Node ID 2bc31865e766c9ca08472889a689dbf5e302d48e
> # Parent  1b6f85c04ba5de786ea02badb8bd4bab81ac58bc
> templater: heuristic to force header/footer in empty logs (issue4135) (BC)
> 
> This gets us a valid XML output with the xml style for empty logs.
> Empty documents are not valid XML because they require a single root
> element besides the optional header.
> 
> Prior to this change, there was no opportunity to print headers or
> footers because their output was potentially dependent on the
> changesets printed.
> 
> This heuristic will get into trouble if someone wants to use bare "{"
> in a header.. which isn't that unreasonable.
> 
> diff -r 1b6f85c04ba5 -r 2bc31865e766 mercurial/cmdutil.py
> --- a/mercurial/cmdutil.py	Tue Aug 25 13:38:20 2015 -0500
> +++ b/mercurial/cmdutil.py	Tue Aug 25 13:44:53 2015 -0500
> @@ -1431,6 +1431,30 @@
>                  if mode and cur in self.t:
>                      self._parts[t] = cur
>  
> +    def close(self):
> +        # the commandline templater is problematic with headers vs footers:
> +        # - headers are potentially variable based on changeset
> +        # - duplicate headers are elided (eg for changelog style)
> +        # - only the last computed footer is printed
> +        # - with no csets, we printed nothing
> +        # - this made our XML output invalid
> +        #
> +        # to fix this, we detect:
> +        # - no header or footer calculated yet
> +        # - constant header
> +        # - constant footer
> +        # (we can't print variable ones because we have no cset to supply)
> +
> +        if not self.lastheader and not self.footer:
> +            self.footer = ""
> +            h = self._parts['header']
> +            if h and "{" not in self.t(h):
> +                self.footer += templater.stringify(self.t(h))
> +            f = self._parts['footer']
> +            if f and "{" not in self.t(f):
> +                self.footer += templater.stringify(self.t(f))
> +        return super(changeset_templater, self).close()

These patches look good to me, but can't we add new keywords instead of
heuristic?

For example,

  doc_header = '<?xml version="1.0"?>\n<log>\n'
  doc_footer = '</log>\n'

and print them no matter if there was no entry.

Mercurial-devel mailing list
Mercurial-devel@selenic.com
https://selenic.com/mailman/listinfo/mercurial-devel
Matt Mackall - Aug. 26, 2015, 8:35 p.m.
On Wed, 2015-08-26 at 23:33 +0900, Yuya Nishihara wrote:
> On Tue, 25 Aug 2015 14:56:41 -0500, Matt Mackall wrote:
> > # HG changeset patch
> > # User Matt Mackall <mpm@selenic.com>
> > # Date 1440528293 18000
> > #      Tue Aug 25 13:44:53 2015 -0500
> > # Node ID 2bc31865e766c9ca08472889a689dbf5e302d48e
> > # Parent  1b6f85c04ba5de786ea02badb8bd4bab81ac58bc
> > templater: heuristic to force header/footer in empty logs (issue4135) (BC)
> > 
> > This gets us a valid XML output with the xml style for empty logs.
> > Empty documents are not valid XML because they require a single root
> > element besides the optional header.
> > 
> > Prior to this change, there was no opportunity to print headers or
> > footers because their output was potentially dependent on the
> > changesets printed.
> > 
> > This heuristic will get into trouble if someone wants to use bare "{"
> > in a header.. which isn't that unreasonable.
> > 
> > diff -r 1b6f85c04ba5 -r 2bc31865e766 mercurial/cmdutil.py
> > --- a/mercurial/cmdutil.py	Tue Aug 25 13:38:20 2015 -0500
> > +++ b/mercurial/cmdutil.py	Tue Aug 25 13:44:53 2015 -0500
> > @@ -1431,6 +1431,30 @@
> >                  if mode and cur in self.t:
> >                      self._parts[t] = cur
> >  
> > +    def close(self):
> > +        # the commandline templater is problematic with headers vs footers:
> > +        # - headers are potentially variable based on changeset
> > +        # - duplicate headers are elided (eg for changelog style)
> > +        # - only the last computed footer is printed
> > +        # - with no csets, we printed nothing
> > +        # - this made our XML output invalid
> > +        #
> > +        # to fix this, we detect:
> > +        # - no header or footer calculated yet
> > +        # - constant header
> > +        # - constant footer
> > +        # (we can't print variable ones because we have no cset to supply)
> > +
> > +        if not self.lastheader and not self.footer:
> > +            self.footer = ""
> > +            h = self._parts['header']
> > +            if h and "{" not in self.t(h):
> > +                self.footer += templater.stringify(self.t(h))
> > +            f = self._parts['footer']
> > +            if f and "{" not in self.t(f):
> > +                self.footer += templater.stringify(self.t(f))
> > +        return super(changeset_templater, self).close()
> 
> These patches look good to me, but can't we add new keywords instead of
> heuristic?
> 
> For example,
> 
>   doc_header = '<?xml version="1.0"?>\n<log>\n'
>   doc_footer = '</log>\n'
> 
> and print them no matter if there was no entry.

That's probably a better answer. We've got a similar problem with diffs
in XML logs that'll need a fix like that.

Patch

--- a/mercurial/cmdutil.py
+++ b/mercurial/cmdutil.py
@@ -1422,7 +1422,8 @@  class changeset_templater(changeset_prin
             (self.ui.debugflag, 'debug'),
         ]
 
-        self._parts = {'header': '', 'footer': '', 'changeset': 'changeset'}
+        self._parts = {'header': '', 'footer': '', 'changeset': 'changeset',
+                       'doc_header': '', 'doc_footer': ''}
         for mode, postfix in tmplmodes:
             for t in self._parts:
                 cur = t
@@ -1431,6 +1432,16 @@  class changeset_templater(changeset_prin
                 if mode and cur in self.t:
                     self._parts[t] = cur
 
+        if self._parts['doc_header']:
+            h = templater.stringify(self.t(self._parts['doc_header']))
+            self.ui.write(h)
+
+    def close(self):
+        super(changeset_templater, self).close()
+        if self._parts['doc_footer']:
+            h = templater.stringify(self.t(self._parts['doc_footer']))
+            self.ui.write(h)
+
     def _show(self, ctx, copies, matchfn, props):
         '''show a single changeset or file revision'''
_______________________________________________