This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

Author nadeem.vawda
Recipients Adam.Groszer, benjamin.peterson, cjw296, eric.araujo, georg.brandl, jaraco, nadeem.vawda, tarek
Date 2012-02-25.08:07:24
SpamBayes Score 4.9960036e-16
Marked as misclassified No
Message-id <1330157245.34.0.217800195625.issue6884@psf.upfronthosting.co.za>
In-reply-to
Content
There are some minor errors in your v3 patch. I've attached a v4 that fixes
them (as usual, tested on Windows and Linux):

 - s/special/sep/ in glob_to_re()
 - Add missing l(.) to filenames in test_process_template under 'graft d'


>> - test_glob_to_re() was doing two levels of escaping (r'\' -> r'\\\\')
>>   for its expected output when it should only do one (r'\' -> r'\\').
> Fix merged.  I don’t fully understand why one place needs two escapes and the others just one.

The places that use one level of escaping are the ones that deal with the
regex string directly. In glob_to_re() itself, the string you're building
is a regex replacement pattern that *operates on* the regex that gets
returned, so it needs one level of escaping for when the output regex is
actually applied, and a second one for the immediate call to re.sub().
When re.sub is called, it eats one level of the escaping, and operates on
a string with one level, returning a string with one level.


>> I also took the liberty of changing the checks for whether the separator needs to be escaped
>> - it's better to escape everything except "/", just in case someone decides to port Python to
>> some platform using a weird directory separator that is neither "/" nor r"\".
> I didn’t take that part.  If someone wants to port Python with a new style of os.sep or os.extsep, I’ll deal with it when they report that.

If you want to leave it as it is, that's your choice. But I do think it's
better to make this Do The Right Thing now, given how easy it is.


> On a related subject, the code in sdist that excludes VCS dirs uses r'/|\\'; I thought that os.altsep was an alternate *accepted* separator, but not actually produced by OS calls.  IOW, I’m asking if os.walk (used to create filelist.allfiles) can ever give paths with '/' on Windows.  If not, then the regex is redundant, and I can clean it in packaging.  My patch removes the redundancy, just for curiosity, but I won’t actually commit that part to distutils.

Well, as far as I can make out, the current implementation always uses
r'\' to join paths. But it won't convert existing '/'s in the start
path to r'\'s, so you might still end up encountering both separators
(assuming the initial path is something you get from the user somewhere
along the line).

(These remarks are not specific to os.walk, but are relevant to anything
based on os.path.join - for example, distutils.filelist.findall and
packaging.manifest._findall)
History
Date User Action Args
2012-02-25 08:07:26nadeem.vawdasetrecipients: + nadeem.vawda, georg.brandl, jaraco, cjw296, benjamin.peterson, tarek, eric.araujo, Adam.Groszer
2012-02-25 08:07:25nadeem.vawdasetmessageid: <1330157245.34.0.217800195625.issue6884@psf.upfronthosting.co.za>
2012-02-25 08:07:24nadeem.vawdalinkissue6884 messages
2012-02-25 08:07:24nadeem.vawdacreate