On Thu, Apr 15, 2010 at 9:34 PM, Luke Kanies <[email protected]> wrote:
> Mostly +1; comment below. > > > On Apr 15, 2010, at 4:35 PM, Markus Roberts wrote: > > The plussignment operator was constructing the new parameter value by >> modifying the param object's value in place (so as to preserve the file >> and line information for debugging). However, when multiple resources >> are overridden by the same plussignment this would result in all of the >> resources sharing the same value (the union of all the prior values and >> the new value), which is wrong. >> >> Instead, we need to give each resource its own copy of the value (e.g., >> a copy of the param object), which this patch implements. >> >> Signed-off-by: Markus Roberts <[email protected]> >> --- >> lib/puppet/parser/resource.rb | 14 ++++++++++---- >> spec/unit/parser/resource.rb | 18 ++++++++++++++++++ >> 2 files changed, 28 insertions(+), 4 deletions(-) >> >> diff --git a/lib/puppet/parser/resource.rb b/lib/puppet/parser/resource.rb >> index 651ed42..ee87886 100644 >> --- a/lib/puppet/parser/resource.rb >> +++ b/lib/puppet/parser/resource.rb >> @@ -377,10 +377,16 @@ class Puppet::Parser::Resource >> >> # If we've gotten this far, we're allowed to override. >> >> - # Merge with previous value, if the parameter was generated with >> the +> syntax. >> - # It's important that we use the new param instance here, not the >> old one, >> - # so that the source is registered correctly for later overrides. >> - param.value = [current.value, param.value].flatten if param.add >> + # Merge with previous value, if the parameter was generated with >> the +> >> + # syntax. It's important that we use a copy of the new param >> instance >> + # here, not the old one, and not the original new one, so that >> the source >> + # is registered correctly for later overrides but the values >> aren't >> + # implcitly shared when multiple resources are overrriden at once >> (see >> + # ticket #3556). >> + if param.add >> + param = param.dup >> + param.value = [current.value, param.value].flatten >> + end >> > > It would actually probably make more sense to just turn this around and use > current instead of param: > > current.value = [current.value, param.value].flatten > > You don't need the dupe, because each resource already has its own param. > > Not a huge difference, but it should have been written this way in the > first place, and avoids the dupe. It did use the current; you changed to param to preserve the source information, and added the comment to prevent reverting to using the current (in your words, "it's important to use the new param instance here, not the old one"). Thus my decision to use dup. If present Luke thinks prior Luke was mistaken, we could use current (with the aforementioned change in source information tracking) but the two of you will have to settle that between yourself. -- Markus -- You received this message because you are subscribed to the Google Groups "Puppet Developers" group. To post to this group, send email to [email protected]. To unsubscribe from this group, send email to [email protected]. For more options, visit this group at http://groups.google.com/group/puppet-dev?hl=en.
