These set of changes are intended to get the tests working and the
implementation in a stable state, though not strictly correct for the
reasons you mention and also UNC paths, etc. Rather than make those changes
in this patch series, I will be making a second pass, refactoring path
validation into utility methods like we taked about.
Josh
On Tue, Jul 19, 2011 at 3:19 PM, Daniel Pittman <dan...@puppetlabs.com>wrote:
> On Tue, Jul 19, 2011 at 14:59, Josh Cooper <j...@puppetlabs.com> wrote:
> > When testing whether a file path is absolute, the regexp was only
> > handling POSIX style file paths. This commit requires Windows
> > style file paths to start with a drive letter. A future commit
> > will refacter the various places we do path validation to
> > support both Windows drive letters and UNC paths.
> >
> > Reviewed-by: Jacob Helwig <ja...@puppetlabs.com>
> > Signed-off-by: Josh Cooper <j...@puppetlabs.com>
>
> > - path = path.chomp(File::SEPARATOR) unless path == File::SEPARATOR
> > - raise ArgumentError.new("Fileset paths must be fully qualified")
> unless File.expand_path(path) == path
> > + if Puppet.features.microsoft_windows?
> > + # REMIND: UNC path
> > + path = path.chomp(File::SEPARATOR) unless path =~ /^[A-Za-z]:\/$/
>
> Doesn't this fail in the face of platforms that have both
> File::SEPARATOR and File::ALT_SEPARATOR? It wouldn't strip the right
> character from the end of the path, at least, in one of the two valid
> cases.
>
> > + else
> > + path = path.chomp(File::SEPARATOR) unless path == File::SEPARATOR
> > + end
>
> Wouldn't it be better to have unified these tests, so that we don't
> have to open-code these Windows-specific tests in every line of code
> that needs to handle paths? That sounds like an invitation to a
> constant stream of bugs where we fail to handle these cases to me.
>
> > def validate_dirs(dirs)
> > + dir_regex = Puppet.features.microsoft_windows? ?
> /^[A-Za-z]:#{File::SEPARATOR}/ : /^#{File::SEPARATOR}/
>
> This will fail if File::SEPARATOR is '\', because you just shoved a
> regexp escape character into your string. On the plus side, though,
> because you are not actually getting it right WRT ALT_SEPARATOR, this
> won't blow up. Two bugs make it work. ;)
>
> > - unless file =~ /^#{File::SEPARATOR}/
> > + regex = Puppet.features.microsoft_windows? ?
> /^[A-Za-z]:#{File::SEPARATOR}/ : /^#{File::SEPARATOR}/
>
> Hey, I think I recognise this code. Maybe we should have somewhere
> central for it, rather than open-coding it every place that needs it?
>
> Daniel
> --
> ⎋ Puppet Labs Developer – http://puppetlabs.com
> ✉ Daniel Pittman <dan...@puppetlabs.com>
> ✆ Contact me via gtalk, email, or phone: +1 (877) 575-9775
> ♲ Made with 100 percent post-consumer electrons
>
> --
> You received this message because you are subscribed to the Google Groups
> "Puppet Developers" group.
> To post to this group, send email to puppet-dev@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.
>
>
--
You received this message because you are subscribed to the Google Groups
"Puppet Developers" group.
To post to this group, send email to puppet-dev@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.
