Patchwork [2,of,4] templater: fix if() to not evaluate False as bool('False')

login
register
mail settings
Submitter Yuya Nishihara
Date Aug. 21, 2016, 2:53 p.m.
Message ID <dfc5b7368fa170e85352.1471791236@mimosa>
Download mbox | patch
Permalink /patch/16373/
State Accepted
Headers show

Comments

Yuya Nishihara - Aug. 21, 2016, 2:53 p.m.
# HG changeset patch
# User Yuya Nishihara <yuya@tcha.org>
# Date 1471505362 -32400
#      Thu Aug 18 16:29:22 2016 +0900
# Node ID dfc5b7368fa170e8535202bc4c7057f53d65d3ff
# Parent  7e4ca26412a04f858f31f7c409734eb935a3586f
templater: fix if() to not evaluate False as bool('False')

Before, False was True. This patch fixes the issue by processing True/False
transparently. The other values (including integer 0) are tested as strings
for backward compatibility, which means "if(latesttagdistance)" never be False.

Should we change the behavior of "if(0)" as well?
timeless - Aug. 24, 2016, 5:35 p.m.
While I support this change, I think it needs a BC

On Sun, Aug 21, 2016 at 10:53 AM, Yuya Nishihara <yuya@tcha.org> wrote:
> # HG changeset patch
> # User Yuya Nishihara <yuya@tcha.org>
> # Date 1471505362 -32400
> #      Thu Aug 18 16:29:22 2016 +0900
> # Node ID dfc5b7368fa170e8535202bc4c7057f53d65d3ff
> # Parent  7e4ca26412a04f858f31f7c409734eb935a3586f
> templater: fix if() to not evaluate False as bool('False')
>
> Before, False was True. This patch fixes the issue by processing True/False
> transparently. The other values (including integer 0) are tested as strings
> for backward compatibility, which means "if(latesttagdistance)" never be False.
>
> Should we change the behavior of "if(0)" as well?
>
> diff --git a/mercurial/templater.py b/mercurial/templater.py
> --- a/mercurial/templater.py
> +++ b/mercurial/templater.py
> @@ -289,6 +289,15 @@ def evalfuncarg(context, mapping, arg):
>          thing = stringify(thing)
>      return thing
>
> +def evalboolean(context, mapping, arg):
> +    func, data = arg
> +    thing = func(context, mapping, data)
> +    if isinstance(thing, bool):
> +        return thing
> +    # other objects are evaluated as strings, which means 0 is True, but
> +    # empty dict/list should be False as they are expected to be ''
> +    return bool(stringify(thing))
> +
>  def evalinteger(context, mapping, arg, err):
>      v = evalfuncarg(context, mapping, arg)
>      try:
> @@ -560,7 +569,7 @@ def if_(context, mapping, args):
>          # i18n: "if" is a keyword
>          raise error.ParseError(_("if expects two or three arguments"))
>
> -    test = evalstring(context, mapping, args[0])
> +    test = evalboolean(context, mapping, args[0])
>      if test:
>          yield args[1][0](context, mapping, args[1][1])
>      elif len(args) == 3:
> diff --git a/tests/test-branches.t b/tests/test-branches.t
> --- a/tests/test-branches.t
> +++ b/tests/test-branches.t
> @@ -516,6 +516,8 @@ template output:
>     }
>    ]
>
> +  $ hg branches --closed -T '{if(closed, "{branch}\n")}'
> +  c
>
>  Tests of revision branch name caching
>
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Yuya Nishihara - Aug. 25, 2016, 3:07 p.m.
On Wed, 24 Aug 2016 13:35:40 -0400, timeless wrote:
> While I support this change, I think it needs a BC

Good point. I didn't mark it as a BC because log templates would never
generate a boolean object, but that's true there are BC where experimental
formatter is used.

> On Sun, Aug 21, 2016 at 10:53 AM, Yuya Nishihara <yuya@tcha.org> wrote:
> > templater: fix if() to not evaluate False as bool('False')
> >
> > Before, False was True. This patch fixes the issue by processing True/False
> > transparently. The other values (including integer 0) are tested as strings
> > for backward compatibility, which means "if(latesttagdistance)" never be False.

Patch

diff --git a/mercurial/templater.py b/mercurial/templater.py
--- a/mercurial/templater.py
+++ b/mercurial/templater.py
@@ -289,6 +289,15 @@  def evalfuncarg(context, mapping, arg):
         thing = stringify(thing)
     return thing
 
+def evalboolean(context, mapping, arg):
+    func, data = arg
+    thing = func(context, mapping, data)
+    if isinstance(thing, bool):
+        return thing
+    # other objects are evaluated as strings, which means 0 is True, but
+    # empty dict/list should be False as they are expected to be ''
+    return bool(stringify(thing))
+
 def evalinteger(context, mapping, arg, err):
     v = evalfuncarg(context, mapping, arg)
     try:
@@ -560,7 +569,7 @@  def if_(context, mapping, args):
         # i18n: "if" is a keyword
         raise error.ParseError(_("if expects two or three arguments"))
 
-    test = evalstring(context, mapping, args[0])
+    test = evalboolean(context, mapping, args[0])
     if test:
         yield args[1][0](context, mapping, args[1][1])
     elif len(args) == 3:
diff --git a/tests/test-branches.t b/tests/test-branches.t
--- a/tests/test-branches.t
+++ b/tests/test-branches.t
@@ -516,6 +516,8 @@  template output:
    }
   ]
 
+  $ hg branches --closed -T '{if(closed, "{branch}\n")}'
+  c
 
 Tests of revision branch name caching