[PATCH] Allow manipulating files with long names on Windows

[Aaron Cohen]

Thanks for the review.


On Thu, Jan 20, 2011 at 3:17 PM, Mads Kiilerich <mads at kiilerich.com> wrote:
> On 01/20/2011 06:02 PM, Aaron Cohen wrote:
>>
>> # Parent  9f707b297b0f52278acc6c4a4f7c6d801001acb7
>> Allow manipulating files with long names on Windows
>
> A minor detail from http://mercurial.selenic.com/wiki/ContributingChanges :
> "lowercase summary line"
>
> And start the summary line with the extension name.

There's some tension here between fitting in 80 characters for the
description, and having a good summary. ;)
I also wasn't sure whether it was supposed to start with a subsystem
prefix before the subsystem was accepted.
Duly noted though.

> Such history might be more appropriate in an introduction email than in the
> changeset description.

Yes, but I'm actually not sure how to do the introductory email
correctly at all. I'm new to the patchbomb extension. I guess I'm
supposed to use --intro and then compose the message, and end up with
a [0/1] email? I've been fruitlessly looking for a way to send just
the patch with introduction before "---". I also haven't been able to
get "versioned patches" going the way I'd like. I was expecting
--flagV3 to produce a subject like [PATCH][V3], but it does not.

I'll try to do better in the future, I guess using --intro.

>> +''Author: Aaron Cohen<aaron at assonance.org>''
>
> This should be in a standard copyright / license comment like in (most)
> other extensions.

OK

>> +    [extensions]
>> +    win32lfn=
>
> "lfn" is new abbreviation to learn. Would it make sense to call it "unc" instead?

That was actually what I originally named the extension, but I
reconsidered. UNC seems to be more of a technical detail to me, where
LFN (for long file names) is more descriptive of the extension's
purpose. I can switch back if you feel strongly about it.

>> +_win32 = False
>> +try:
>> +    import win32api, win32file, winerror, pywintypes
>> +
>> +    _win32 = True
>
> Beware of demandload that might delay the import error to winerror is used
> in the next lines.

So the _win32 = True needs to come after the first usage? OK. I
probably need to test more without pywin32 installed.

>> +    _errmap = {
>> +        winerror.ERROR_ALREADY_EXISTS: errno.EEXIST,
>> +        winerror.ERROR_PATH_NOT_FOUND: errno.ENOENT
>> +    }
>> +except ImportError:
>> +    pass

>> +# UNC filenames require different normalization than mercurial and python
>> want
>> +def unc(path):
>> +    global _suppressunc
>> +    if not _suppressunc:
>> +        _suppressunc += 1
>
> The trick here is that abspath is patched and might recurse back here? That
> might deserve a comment.
>
> Would it be possible to store the original abspath and call that instead?

Yes, that was the trick. Unfortunately, the unpatched abspath doesn't
behave correctly for long paths. However, the patched one no longer
recurses, this is now just an unneeded holdover from an earlier
version and I'll delete it.

>> +def wrap(method):
>
> "wrap1" would be more descriptive, especially compared to wrap2.

OK

>> +# vanilla os.listdir handles UNC ok, but breaks if they're longer than
>> MAX_PATH
>
> Use docstring instead of comment.

Won't that replace the docstring for listdir with mine? If so do I
need to copy all the original docstring too? Maybe I need to do that
anyway.

>> +# vanilla returns a relative path for filenames longer than MAX_PATH
>> +# os.path.abspath(30 * "123456789\\") ->  30 * "123456789\\"
>
> You could perhaps phrase this as a doctest in the docstring.

That's indicating the incorrect behavior of the original rather than
the desired behavior.

>> +# vanilla loses a trailing backslash:
>
> Why is that a problem?

If you do os.path.join(os.path.split("\\\\?\\C:\\"), "foo") you end up
with \\?\C:foo which is invalid. Same problem with dirname.

>> +# os.path.split('\\\\?\\C:\\') ->  ('\\\\?\\C:', '')
>> +def wrapsplit(split):
>> +    def lfnsplit(path):
>> +        result = split(path)
>> +def wrapchdir(chdir):
>> +    def lfnchdir(path):
>> +        if len(os.path.abspath(path))>= 248:
>
> A magic number? Put it in a "constant" with a descriptive name or add a
> comment.

Will fix.

>> +            path = unc(path)
>> +        if os.path.exists(path):
>> +            # Use an environment variable so subprocesses get the correct
>> cwd
>> +            os.environ["CD"] = path
>
> Is this CD variable magic or used in other places? If it is for this use
> only we might want to make sure it doesn't collide with other uses - for
> example HGWIN32LFNCWD.

CD is the variable cmd.exe sets as you change directories. It's not
set when hg is running though, so I thought I'd repurpose it. I can
change it though.

> But it seems like a bad idea that chdir doesn't do a chdir. That will most
> likely have unexpected consequences. Wouldn't it be better to just fail if
> cwd is too long?

That would be possible I guess, there aren't that many places in hg
where os.chdir is used, and it's almost always to repo.root. The most
important usages seem to be in the record extension and the patch
command.

I originally just documented that the root of the repo shouldn't be
more than 244 characters long, but figured I'd give it my best shot to
fix it "right".

>> +def uisetup(ui):
>> +    if not _win32:
>> +        ui.warn(_("This extension requires the pywin32 extensions\n"))
>
> It will be hard for the user who get this message to figure which extensions
> "this" refers to.

Will fix

>> +    if hasattr(util, "unlinkpath"):
>> +        util.unlinkpath = wrap(util.unlinkpath)
>> +    if hasattr(util, "unlink"):
>> +        util.unlink = wrap(util.unlink)
>
> This invasive monkey patching looks a bit scary and fragile and might make
> it less easy for it to become an "official" extension.

This is actually pretty much identical to what win32mbcs patches, just
written out directly instead of with a fancy loop. I find this easier
to read, but could switch to a style like mbcs is using. In fact,
anywhere win32mbcs isn't patching a function I patched, I'd bet they
have a bug. (util.makedirs for instance).