>>> index e13c796..93d1735 100644
> >> --- a/lib/puppet/type/file.rb
> >> +++ b/lib/puppet/type/file.rb
> >> @@ -706,10 +706,10 @@ module Puppet
> >>
> >> # Write out the file. Requires the content to be written,
> >> # the property name for logging, and the checksum for
> validation.
> >> - def write(content, property)
> >> + def write(property)
> >>
> >
> > It appears that the comment here is now at least two refactors out of
> date,
> > as we are nolonger passing in the content or the checksum. This might be
> a
> > good argument for removing the comment altogether as it is both a) wrong
> and
> > b) being consistently ignored.
>
> You're right. I'm so used to never read comments, that I don't see them
> anymore.
>
Ditto, unless I'm reviewing code, and then it's to spot things like this.
When I'm working with the code I either ignore than or read them in a "the
little devil sitting on my shoulder says..." monad.
> >> remove_existing(:file)
> >>
> >> - use_temporary_file = (content.length != 0)
> >> + use_temporary_file = write_temporary_file?
> >> if use_temporary_file
> >> path = "#{self[:path]}.puppettmp_#{rand(10000)}"
> >> while File.exists?(path) or File.symlink?(path)
> >> @@ -722,14 +722,17 @@ module Puppet
> >> mode = self.should(:mode) # might be nil
> >> umask = mode ? 000 : 022
> >>
> >> + content_checksum = nil
> >> Puppet::Util.withumask(umask) do
> >> - File.open(path, File::CREAT|File::WRONLY|File::TRUNC,
> >> mode) { |f| f.print content }
> >> + File.open(path, File::CREAT|File::WRONLY|File::TRUNC,
> >> mode) do |f|
> >> + content_checksum = write_content(f)
> >> + end
> >> end
> >>
> >>
> > This could be reduced to:
> >
> > content_checksum = Puppet::Util.withumask(umask) do
> > File.open(path, File::CREAT|File::WRONLY|File::TRUNC,
> mode)
> > { |f| write_content(f) }
> > end
> >
> > Or if I'm not mistaken:
> >
> > content_checksum = Puppet::Util.withumask(umask) {
> > File.open(path,'w' , mode) { |f| write_content(f) } }
> >
>
> I don't know. Does withumask return what the block returns (and same
> question for File.open)?
>
Yes, it does.
> That's the kind of things I'm never sure of, so I tend to do defensive
> code :-)
>
I can see that, although too much defensive code sows doubt; people come
along and copy it and don't know if it had to be done that way or not, and
decades later you can wind up with abominations like this (from traumatic
memory of actual C code I once had to debug):
if (abs(a) < 128) { b = b + a } else if (a < 0) { b = b - abs(a) }
else if (a > 0) then {b = b + a}
It's a long slope and we aren't that far down it, but I'd like to keep from
sliding further.
What about using "w" instead of File::CREAT|File::WRONLY|File::TRUNC? I
_think_ they're the same, but I wouldn't be surprised if there was a
subtlety I'm missing. Anyone?
> >> + def write_content(file)
> >> + checksum = nil
> >> + if content = property(:content)
> >> + checksum = content.write(file)
> >> + else # we have a sole ensure property
> >> + file.print ""
> >> + end
> >> + checksum
> >> + end
> >>
> >> What is the purpose of the
> >
> > file.print ""
>
> I mimicked what ensure was doing when there was no source nor content.
>
> > That is, opening a file and then closing it without writing anything
> still
> > creates it in all OSes, right? Isn't this routine doing the same thing
> as:
> >
> > def write_content(file)
> > (content = property(:content)) && content.write(file)l
> > end
>
> You might be right because the file is open with trunc and create.
>
Looking at the original code, I think that writing an empty string was a
just consequence of the code path ("f.print content" even when content
happened to be empty).
-- Markus
P.S. (and reiterating our discussion at puppet camp) I'm again catching
myself going into more detail on your patches than I do on some of the
others not because yours need it (they generally don't) but because I enjoy
it more, and I feel like I missed most the fun on the event queuing thread.
So if this cuts to far into the 0.35% of each day you have for this, feel
free to disregard my musings.
--
You received this message because you are subscribed to the Google Groups
"Puppet Developers" group.
To post to this group, send email to puppet-...@googlegroups.com.
To unsubscribe from this group, send email to
puppet-dev+unsubscr...@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/puppet-dev?hl=en.
