[PATCH STABLE V2] windows: implement nlinks() using Python's ctypes (issue1922)

[Steve Borho]

On Wed, Jan 26, 2011 at 10:56 AM, Adrian Buehlmann <adrian at cadifra.com> wrote:
> On 2011-01-25 23:45, Matt Mackall wrote:
>> On Sun, 2011-01-23 at 16:12 +0100, Adrian Buehlmann wrote:
>>> On 2011-01-23 15:17, Adrian Buehlmann wrote:
>>>> # HG changeset patch
>>>> # User Adrian Buehlmann <adrian at cadifra.com>
>>>> # Date 1295790385 -3600
>>>> # Branch stable
>>>> # Node ID 99f2b980756d1c22775cf6ec6e93c646bc910a55
>>>> # Parent  d0e0d3d43e1439d63564ab4dddfe0daa69ae2d86
>>>> windows: implement nlinks() using Python's ctypes (issue1922)
>>>>
>>>> If the pywin32 package was not installed, the import of win32 in
>>>> windows.py silently failed and (before this patch) util.nlinks was then
>>>> used as a fallback when running on Windows.
>>>>
>>>> util.nlinks() returned 0 for all files when called on Windows, because
>>>> Python's
>>>>
>>>>     os.lstat(name).st_nlink
>>>>
>>>> is 0 on Windows for all files.
>>>>
>>>> If nlinks() returns 0, util.opener failed to break up hardlinks, which
>>>> could lead to repository corruption when committing or pushing to a
>>>> hardlinked clone (hg verify detects it).
>>>>
>>>> We now provide our own nlinks() in windows.py by using Python's ctypes
>>>> library, so we don't depend on the pywin32 package being installed for
>>>> nlinks().
>>>>
>>>>   ** Since Python's ctypes were introduced in Python 2.5, we now
>>>>   ** require Python 2.5 or later for Mercurial on Windows
>>>>
>>>> Using ctypes also has the benefit that nlinks() also works correctly
>>>> for the pure Python Mercurial.
>>>>
>>>> And we force breaking up hardlinks on every append file access in the
>>>> opener if nlinks() returns < 1, thus making sure that we can't cause
>>>> any hardlink repository corruption.
>>>>
>>>> It would have been possible to simply require the pywin32 package on
>>>> Windows and abort with an import error if it's not installed, but such
>>>> a policy change should be avoided on the stable branch. Previous packages
>>>> like for example
>>>>
>>>>     mercurial-1.7.3-1.win32-py2.6.exe
>>>>
>>>> didn't make it obvious that pywin32 was needed as a dependency. It just
>>>> silently caused repository corruption in hardlinked clones if pywin32
>>>> was not installed.
>>
>> Alright, now I've lost the plot. Let me see if I can recover it:
>>
>> require pywin32
>>  - adds a new dependency
>>  - solves problem for native
>>  - pure may or may not be able to use it (ie jython)
>> extend osutils
>>  - no new dependency
>>  - solves problem for native
>>  - pure still broken
>> use ctypes
>>  - requires py2.5 or separate ctypes
>>  - solves problem for native
>>  - pure may or may not be able to use it
>>
>> It's worth noting that pure has pretty substantial problems on Windows
>> due to the use of "native" open() and the like.
>>
>> So in this analysis, osutils looks like the winner. Is your osutils
>> patch good for 1.7.4?
>
> Looking at your statements in this mail, I'm tempted to say no.
>
> I think my osutil.c patch is quite risky for 1.7.4, it lacks a nlinks()
> implementation for pure and it is going in a wrong direction.
>
> IMHO, the right direction for the future is using Python's ctypes.
>
> I'd rather like to see
>
> diff --git a/mercurial/util.py b/mercurial/util.py
> --- a/mercurial/util.py
> +++ b/mercurial/util.py
> @@ -913,6 +909,8 @@ class opener(object):
>                     # shares if the file is open.
>                     fd = open(f)
>                     nlink = nlinks(f)
> +                    if nlink < 1:
> +                        nlink = 2 # force mktempcopy (issue1922)
>                     fd.close()
>             except (OSError, IOError):
>                 nlink = 0
>
> alone in 1.7.4.
>
> This simply forces a file copy on 'a'ppend if pywin32 is not installed.
>
> I'll send a patch for this.
>
>> Another way to look at it is "what else breaks without pywin32"?
>> The answer looks like:
>>
>> - finding stale locks
>> - finding rc paths
>> - finding usernames
>> - signal handling
>> - terminal width
>> - window hiding??
>>
>> Looks like pretty minor stuff.
>>
>> A related question is "can we replace pywin32 with ctypes"? If the
>> answer is yes, it might make sense to do that so we'll have no
>> dependencies when we move to Py2.5 a few years from now. Switching to
>> ctypes might make sense for 1.8 or 1.9.
>>
>
> I think we can get rid of the pywin32 dependency and we should.
>
> I hope we can switch to ctypes for 1.8.
>
> Can we require Python >=2.5 (at least for Windows) for 1.8?

Someone mentioned ctypes is available as a back-port for 2.4.  So I
think it's safe to require Python 2.5 or Python 2.4 + ctypes.

-- 
Steve Borho