[PATCH STABLE] win32: backout 1a9ebc83a74c

[Steve Borho]

On Mon, May 5, 2014 at 3:37 PM, Adrian Buehlmann <adrian at cadifra.com> wrote:
> On 2014-05-05 21:37, Pierre-Yves David wrote:
>>
>>
>> On 05/05/2014 11:56 AM, Steve Borho wrote:
>>> On Sat, May 3, 2014 at 3:43 AM, Adrian Buehlmann <adrian at cadifra.com> wrote:
>>>> # HG changeset patch
>>>> # User Adrian Buehlmann <adrian at cadifra.com>
>>>> # Date 1399106034 -7200
>>>> # Branch stable
>>>> # Node ID b3bceb2a103e9f279f058e7a3bacf272968b6bb6
>>>> # Parent  d36440d843284ba546858b241da9cc4817811fe5
>>>> win32: backout 1a9ebc83a74c
>>>>
>>>> This appears to cause havoc on TortoiseHg. While 1a9ebc83a74c may be nice to
>>>> have (as soon as someone can prove it has no unwanted side effects), it is not
>>>> worth the troubles at this point.
>>>
>>> To add some context; the troubles are specifically that files which
>>> are being "watched" for changes by QFileSystemWatcher() are no longer
>>> safely updated by util.rename().  So things like updating .hg/hgrc or
>>> perhaps even the changelog are suddenly dangerous.  We're still not
>>> completely certain of the exact semantics of the problem so I don't
>>> know if this patch is mandatory, but we believe it would resolve the
>>> problem.
>>
>> Do you have actually example of bugs this changes introduce (I'm curious)
>
> [It's nice to see you being curious about hairy dirty win32 details. :-) ]
>
> Yep. But only together with TortoiseHg on Windows, which uses this layer
> of Mercurial:
>
> Steve lost his hgrc file on a local disk (as he reported), which is
> enough evidence for me to backout 1a9ebc83a74c.
>
> 1a9ebc83a74c was the only change on that layer, which has always been a
> delicate matter (Windows file access trying to emulate posix
> expectations of Mercurial, combined with more or less evil virus
> scanner-like environments).
>
> I really can no longer recommend to inflict 1a9ebc83a74c on everyone.
> TortoiseHg replacing win32.unlink is not really an option either, as
> that would reduce usage of this part of Mercurial to an almost
> negligible quantity.
>
> It wasn't found out earlier, apparently because no one built a
> TortoiseHg with 1a9ebc83a74c and used it. Steve was the guinea pig himself.
>
> 1a9ebc83a74c is a performance optimization for a non-essential use case.
> Not worth the trouble, IMHO.
>
> In fact I strongly recommend against publishing a TortoiseHg binary
> containing 1a9ebc83a74c.
>
> The other option would be to find someone who is cute enough to release
> such a binary and wait for another one which either proves all works
> fine or presents a theory of why 1a9ebc83a74c is harmful exactly.
>
> I won't be neither one.
>
> And Steve has already publicly declared that he will hold off tagging a
> TortoiseHg release until this issue is resolved.
>
> So, either we find hard evidence or we just backout.

I built two packages today to test this issue.

The first package has Mercurial 3.0 and a slightly old version of
TortoiseHg (prior to some workarounds we made for this issue).  And
with this package I was able to reproduce the problem very quickly by
editing a repository .hg/hgrc file using our settings tool (which uses
util.atomictempfile to swap the old with the new atomically).

The second package was the same as the first except I backed out
1a9ebc83a74c in Mercurial and manually moved the 3.0 tag locally to
include that backout (I have to move tags in order to build
test-release packages).

With the second package I was unable to reproduce the file overwrite errors.

-- 
Steve Borho