code review 85240043: html/template: fix two unrelated bugs

html/template: fix two unrelated bugs
1) The code to catch an exception marked the template as escaped
when it was not yet, which caused subsequent executions of the
template to not escape properly.
2) ensurePipelineContains needs to handled Field as well as
Identifier nodes.

Fixes issue 7379.

[r, 2014-04-08 04:06:15]

Hello mikesamuel@gmail.com (cc: golang-codereviews@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go/

[MikeSamuel, 2014-04-08 18:18:48]

https://codereview.appspot.com/85240043/diff/40001/src/pkg/html/template/esca...
File src/pkg/html/template/escape.go (right):

https://codereview.appspot.com/85240043/diff/40001/src/pkg/html/template/esca...
src/pkg/html/template/escape.go:46: tmpl.escaped = true
I'm fuzzy on the relationship between templates and their sets.

Maybe this should be

    for _, name := range names {
             if t := tmpl.set[name]; t != nil {
                     t.escaped = true
                     t.Tree = t.text.Tree
             }
    }

and line 43 can go away too.

https://codereview.appspot.com/85240043/diff/40001/src/pkg/html/template/esca...
src/pkg/html/template/escape.go:211: // which might be a singleton (Identifier)
or a slice (Field).
Is this exercised by any existing tests?

[r, 2014-04-09 03:04:25]

Hello mikesamuel@gmail.com (cc: golang-codereviews@googlegroups.com),

Please take another look.

[r, 2014-04-09 03:04:27]

https://codereview.appspot.com/85240043/diff/40001/src/pkg/html/template/esca...
File src/pkg/html/template/escape.go (right):

https://codereview.appspot.com/85240043/diff/40001/src/pkg/html/template/esca...
src/pkg/html/template/escape.go:46: tmpl.escaped = true
On 2014/04/08 18:18:49, MikeSamuel wrote:
> I'm fuzzy on the relationship between templates and their sets.
> 
> Maybe this should be
> 
>     for _, name := range names {
>              if t := tmpl.set[name]; t != nil {
>                      t.escaped = true
>                      t.Tree = t.text.Tree
>              }
>     }
> 
> and line 43 can go away too.

Done.

https://codereview.appspot.com/85240043/diff/40001/src/pkg/html/template/esca...
src/pkg/html/template/escape.go:211: // which might be a singleton (Identifier)
or a slice (Field).
On 2014/04/08 18:18:49, MikeSamuel wrote:
> Is this exercised by any existing tests?

The newly added test exercises this and passes with this fix, panics without.
However, this function is not tested directly, only through
ensurePipelineContains.
It's trivial enough that doesn't bother me, but if pressured I'll see if I can
come up with something.

[MikeSamuel, 2014-04-09 05:20:46]

LGTM

[r, 2014-04-09 05:57:38]

*** Submitted as https://code.google.com/p/go/source/detail?r=8b7f9e17e68d ***

html/template: fix two unrelated bugs
1) The code to catch an exception marked the template as escaped
when it was not yet, which caused subsequent executions of the
template to not escape properly.
2) ensurePipelineContains needs to handled Field as well as
Identifier nodes.

Fixes issue 7379.

LGTM=mikesamuel
R=mikesamuel
CC=golang-codereviews
https://codereview.appspot.com/85240043