Patchwork win32: backout 1a9ebc83a74c

login
register
mail settings
Submitter Steve Borho
Date May 5, 2014, 11:02 p.m.
Message ID <4898c37e03c0b804d6ca.1399330921@70.1.168.192.in-addr.arpa>
Download mbox | patch
Permalink /patch/4641/
State Accepted
Commit 4898c37e03c0b804d6ca8c3ad341dceb53ff5806
Headers show

Comments

Steve Borho - May 5, 2014, 11:02 p.m.
# HG changeset patch
# User Steve Borho <steve@borho.org>
# Date 1399106034 -7200
#      Sat May 03 10:33:54 2014 +0200
# Branch stable
# Node ID 4898c37e03c0b804d6ca8c3ad341dceb53ff5806
# Parent  cadad384c97c7956c98f3c9b92d8cc40fa16d93b
win32: backout 1a9ebc83a74c

This change conflicted with TortoiseHg's use of QFileSystemWatcher. Files which
were being monitored (for file-system events) were unable to be reliably updated
using util.atomictempfile.  Often the update would error out in the middle of
the process leaving neither the old or the new file in place.

My guess is that _kernel32.CreateFileA() is triggering an exception that is
not handled correctly within unlink()
Pierre-Yves David - May 5, 2014, 11:50 p.m.
On 05/05/2014 04:02 PM, Steve Borho wrote:
> # HG changeset patch
> # User Steve Borho <steve@borho.org>
> # Date 1399106034 -7200
> #      Sat May 03 10:33:54 2014 +0200
> # Branch stable
> # Node ID 4898c37e03c0b804d6ca8c3ad341dceb53ff5806
> # Parent  cadad384c97c7956c98f3c9b92d8cc40fa16d93b
> win32: backout 1a9ebc83a74c

This have been queued in the clowncopter. Thanks!

You can now prepare to advocate for a quick 3.0.1 along side and excuse 
for not finding this bug during the 3.0-rc period.
Adrian Buehlmann - May 6, 2014, 5:49 a.m.
On 2014-05-06 01:50, Pierre-Yves David wrote:
> On 05/05/2014 04:02 PM, Steve Borho wrote:
>> # HG changeset patch
>> # User Steve Borho <steve@borho.org>
>> # Date 1399106034 -7200
>> #      Sat May 03 10:33:54 2014 +0200
>> # Branch stable
>> # Node ID 4898c37e03c0b804d6ca8c3ad341dceb53ff5806
>> # Parent  cadad384c97c7956c98f3c9b92d8cc40fa16d93b
>> win32: backout 1a9ebc83a74c
> 
> This have been queued in the clowncopter. Thanks!

Thanks. And - for what's worth - fine be me.

(funny "crew" repo names you guys have nowadays...)

> You can now prepare to advocate for a quick 3.0.1 along side and excuse 
> for not finding this bug during the 3.0-rc period.

Given how small the TortoiseHg team nowadays is and what little
resources they have, they really don't have much to excuse (Steve in
particular).

Perhaps, we should be a bit more restrictive on mercurial-devel for
things like 1a9ebc83a74c in the future. I know it is not easy... New
patches and "improvements" are tempting...
Pierre-Yves David - May 6, 2014, 5:55 a.m.
On 05/05/2014 10:49 PM, Adrian Buehlmann wrote:
> On 2014-05-06 01:50, Pierre-Yves David wrote:
>> On 05/05/2014 04:02 PM, Steve Borho wrote:
>>> # HG changeset patch
>>> # User Steve Borho <steve@borho.org>
>>> # Date 1399106034 -7200
>>> #      Sat May 03 10:33:54 2014 +0200
>>> # Branch stable
>>> # Node ID 4898c37e03c0b804d6ca8c3ad341dceb53ff5806
>>> # Parent  cadad384c97c7956c98f3c9b92d8cc40fa16d93b
>>> win32: backout 1a9ebc83a74c
>>
>> This have been queued in the clowncopter. Thanks!
>
> Thanks. And - for what's worth - fine be me.
>
> (funny "crew" repo names you guys have nowadays...)

It is here if you missed it: http://42.netv6.net/clowncopter/
Angel Ezquerra - May 6, 2014, 6 a.m.
On Tue, May 6, 2014 at 7:49 AM, Adrian Buehlmann <adrian@cadifra.com> wrote:
> On 2014-05-06 01:50, Pierre-Yves David wrote:
>> On 05/05/2014 04:02 PM, Steve Borho wrote:
>>> # HG changeset patch
>>> # User Steve Borho <steve@borho.org>
>>> # Date 1399106034 -7200
>>> #      Sat May 03 10:33:54 2014 +0200
>>> # Branch stable
>>> # Node ID 4898c37e03c0b804d6ca8c3ad341dceb53ff5806
>>> # Parent  cadad384c97c7956c98f3c9b92d8cc40fa16d93b
>>> win32: backout 1a9ebc83a74c
>>
>> This have been queued in the clowncopter. Thanks!
>
> Thanks. And - for what's worth - fine be me.
>
> (funny "crew" repo names you guys have nowadays...)
>
>> You can now prepare to advocate for a quick 3.0.1 along side and excuse
>> for not finding this bug during the 3.0-rc period.
>
> Given how small the TortoiseHg team nowadays is and what little
> resources they have, they really don't have much to excuse (Steve in
> particular).

Actually I suspect that I hit this error during the RC period (I also
lost my whole mercurial.ini file at some point). Unfortunately at the
time I was hacking on the TortoiseHg settings code and I just thought
that it was my changes that had caused the problem.

I think the main reason why this bug was not caught earlier is because
there were no TortoiseHg RC builds during the RC period! If there had
been it is quite likely that we would have found the issue much sooner
(I am pretty sure I would have since I tend to use the newest build
TortoiseHg we have).

IMHO the solution to this problem (or at least a big part of the
solution) would be to have an automated build system for the
TortoiseHg installers.

Cheers,

Angel
Angel Ezquerra - May 6, 2014, 6:01 a.m.
On Tue, May 6, 2014 at 7:55 AM, Pierre-Yves David
<pierre-yves.david@ens-lyon.org> wrote:
>
>
> On 05/05/2014 10:49 PM, Adrian Buehlmann wrote:
>>
>> On 2014-05-06 01:50, Pierre-Yves David wrote:
>>>
>>> On 05/05/2014 04:02 PM, Steve Borho wrote:
>>>>
>>>> # HG changeset patch
>>>> # User Steve Borho <steve@borho.org>
>>>> # Date 1399106034 -7200
>>>> #      Sat May 03 10:33:54 2014 +0200
>>>> # Branch stable
>>>> # Node ID 4898c37e03c0b804d6ca8c3ad341dceb53ff5806
>>>> # Parent  cadad384c97c7956c98f3c9b92d8cc40fa16d93b
>>>> win32: backout 1a9ebc83a74c
>>>
>>>
>>> This have been queued in the clowncopter. Thanks!
>>
>>
>> Thanks. And - for what's worth - fine be me.
>>
>> (funny "crew" repo names you guys have nowadays...)
>
>
> It is here if you missed it: http://42.netv6.net/clowncopter/

Apparently I missed it too. Is this the new crew repo?

Angel
Augie Fackler - May 13, 2014, 12:39 a.m.
On Tue, May 06, 2014 at 08:01:25AM +0200, Angel Ezquerra wrote:
> On Tue, May 6, 2014 at 7:55 AM, Pierre-Yves David
> <pierre-yves.david@ens-lyon.org> wrote:
> >
> >
> > On 05/05/2014 10:49 PM, Adrian Buehlmann wrote:
> >>
> >> On 2014-05-06 01:50, Pierre-Yves David wrote:
> >>>
> >>> On 05/05/2014 04:02 PM, Steve Borho wrote:
> >>>>
> >>>> # HG changeset patch
> >>>> # User Steve Borho <steve@borho.org>
> >>>> # Date 1399106034 -7200
> >>>> #      Sat May 03 10:33:54 2014 +0200
> >>>> # Branch stable
> >>>> # Node ID 4898c37e03c0b804d6ca8c3ad341dceb53ff5806
> >>>> # Parent  cadad384c97c7956c98f3c9b92d8cc40fa16d93b
> >>>> win32: backout 1a9ebc83a74c
> >>>
> >>>
> >>> This have been queued in the clowncopter. Thanks!
> >>
> >>
> >> Thanks. And - for what's worth - fine be me.
> >>
> >> (funny "crew" repo names you guys have nowadays...)
> >
> >
> > It is here if you missed it: http://42.netv6.net/clowncopter/
>
> Apparently I missed it too. Is this the new crew repo?

No - crew is still crew and exists, I'm just low on time in the last
~month due to some stuff in my "real job".

The clowncopter is where facebook's reviewer rotation pushes things.

>
> Angel
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
Angel Ezquerra - May 13, 2014, 6:56 a.m.
On Tue, May 13, 2014 at 2:39 AM, Augie Fackler <raf@durin42.com> wrote:
> On Tue, May 06, 2014 at 08:01:25AM +0200, Angel Ezquerra wrote:
>> On Tue, May 6, 2014 at 7:55 AM, Pierre-Yves David
>> <pierre-yves.david@ens-lyon.org> wrote:
>> >
>> >
>> > On 05/05/2014 10:49 PM, Adrian Buehlmann wrote:
>> >>
>> >> On 2014-05-06 01:50, Pierre-Yves David wrote:
>> >>>
>> >>> On 05/05/2014 04:02 PM, Steve Borho wrote:
>> >>>>
>> >>>> # HG changeset patch
>> >>>> # User Steve Borho <steve@borho.org>
>> >>>> # Date 1399106034 -7200
>> >>>> #      Sat May 03 10:33:54 2014 +0200
>> >>>> # Branch stable
>> >>>> # Node ID 4898c37e03c0b804d6ca8c3ad341dceb53ff5806
>> >>>> # Parent  cadad384c97c7956c98f3c9b92d8cc40fa16d93b
>> >>>> win32: backout 1a9ebc83a74c
>> >>>
>> >>>
>> >>> This have been queued in the clowncopter. Thanks!
>> >>
>> >>
>> >> Thanks. And - for what's worth - fine be me.
>> >>
>> >> (funny "crew" repo names you guys have nowadays...)
>> >
>> >
>> > It is here if you missed it: http://42.netv6.net/clowncopter/
>>
>> Apparently I missed it too. Is this the new crew repo?
>
> No - crew is still crew and exists, I'm just low on time in the last
> ~month due to some stuff in my "real job".
>
> The clowncopter is where facebook's reviewer rotation pushes things.

Thanks for the info. Interesting choice of name...

Angel

Patch

diff -r cadad384c97c -r 4898c37e03c0 mercurial/win32.py
--- a/mercurial/win32.py	Thu May 01 17:48:02 2014 -0500
+++ b/mercurial/win32.py	Sat May 03 10:33:54 2014 +0200
@@ -25,7 +25,6 @@ 
 # GetLastError
 _ERROR_SUCCESS = 0
 _ERROR_NO_MORE_FILES = 18
-_ERROR_SHARING_VIOLATION = 32
 _ERROR_INVALID_PARAMETER = 87
 _ERROR_INSUFFICIENT_BUFFER = 122
 
@@ -61,9 +60,7 @@ 
 
 _OPEN_EXISTING = 3
 
-_FILE_FLAG_OPEN_REPARSE_POINT = 0x00200000
 _FILE_FLAG_BACKUP_SEMANTICS = 0x02000000
-_FILE_FLAG_DELETE_ON_CLOSE = 0x04000000
 
 # SetFileAttributes
 _FILE_ATTRIBUTE_NORMAL = 0x80
@@ -424,18 +421,11 @@ 
 def unlink(f):
     '''try to implement POSIX' unlink semantics on Windows'''
 
-    # If we can open f exclusively, no other processes must have open handles
-    # for it and we can expect its name will be deleted immediately when we
-    # close the handle unless we have another in the same process.  We also
-    # expect we shall simply fail to open f if it is a directory.
-    fh = _kernel32.CreateFileA(f, 0, 0, None, _OPEN_EXISTING,
-        _FILE_FLAG_OPEN_REPARSE_POINT | _FILE_FLAG_DELETE_ON_CLOSE, None)
-    if fh != _INVALID_HANDLE_VALUE:
-        _kernel32.CloseHandle(fh)
-        return
-    error = _kernel32.GetLastError()
-    if error != _ERROR_SHARING_VIOLATION:
-        raise ctypes.WinError(error)
+    if os.path.isdir(f):
+        # use EPERM because it is POSIX prescribed value, even though
+        # unlink(2) on directories returns EISDIR on Linux
+        raise IOError(errno.EPERM,
+                      "Unlinking directory not permitted: '%s'" % f)
 
     # POSIX allows to unlink and rename open files. Windows has serious
     # problems with doing that: