Re: [PATCH] subrepo: config option to disable subrepositories

[Adrian Buehlmann]

On 2017-11-04 07:57, Yuya Nishihara wrote:
> On Fri, 03 Nov 2017 17:28:27 -0700, Gregory Szorc wrote:
>> # HG changeset patch
>> # User Gregory Szorc 
>> # Date 1509755155 25200
>> #  Fri Nov 03 17:25:55 2017 -0700
>> # Branch stable
>> # Node ID f2390c369bfebf32f26f5a2e4aa5620224a7c8ea
>> # Parent  f445b10dc7fb3495d24d1c22b0996148864c77f7
>> subrepo: config option to disable subrepositories
> 
>> +``enablesubrepos``
>> +Whether the subrepositories feature is enabled. If disabled,
>> +subrepositories are effectively ignored by the Mercurial client.
>> +(default: True)
> 
> We might want to select subrepo types to be enabled since hg subrepo is
> more widely used and considered less broken.
> 

After looking at subrepo.gitsubrepo._checkversion in the mercurial sources:

Assuming the problem is limited to specific vulnerable versions of
external scm tools:

Perhaps, this might be narrowed to requiring specific versions of git
and svn using a config knob (which defaults to the hard-coded values).
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

D1069: bitmanipulation: reformat with clang-format

[abuehl (Adrian Buehlmann)]

abuehl added inline comments.

INLINE COMMENTS

> bitmanipulation.h:34
>   c[2] = (x >> 8) & 0xff;
> - c[3] = (x) & 0xff;
> + c[3] = (x)&0xff;
>  }

this one looks bad

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1069

To: durin42, #hg-reviewers
Cc: abuehl, mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

D851: [RFC] setup: increase MAX_PATH limit on Windows 10 Anniversary Update

[abuehl (Adrian Buehlmann)]

abuehl added a comment.


  In addition, Mercurial is not using the ...W Windows API functions.
  
  I think, for example
  

https://phab.mercurial-scm.org/diffusion/HG/browse/default/mercurial/cext/osutil.c;da8bdeb1be28b976909a963c89e974264686e2bb$1207
  
  would have to be changed to call CreateFileW in order for this to have an 
effect.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D851

To: indygreg, #hg-reviewers
Cc: abuehl, mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

D851: [RFC] setup: increase MAX_PATH limit on Windows 10 Anniversary Update

[abuehl (Adrian Buehlmann)]

abuehl added a comment.


  In https://phab.mercurial-scm.org/D851#14301, @indygreg wrote:
  
  > In https://phab.mercurial-scm.org/D851#14300, @abuehl wrote:
  >
  > > What about file I/O done via Python lib (e.g. python27.dll)?
  >
  >
  > I /think/ this works at the process - not DLL - level. But I need to test 
on a Windows machine to be sure. One of the reasons this is marked RFC.
  
  
  Maybe, but what does the Python library itself do with long paths?
  What do you expect from calling e.g. os.unlink with a long path? Can the 
Python lib handle it?

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D851

To: indygreg, #hg-reviewers
Cc: abuehl, mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

D851: [RFC] setup: increase MAX_PATH limit on Windows 10 Anniversary Update

[abuehl (Adrian Buehlmann)]

abuehl added a comment.


  What about file I/O done via Python lib (e.g. python27.dll)?

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D851

To: indygreg, #hg-reviewers
Cc: abuehl, mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

D588: win32: use fewer system calls for unlink()

[abuehl (Adrian Buehlmann)]

abuehl added a comment.


  In https://phab.mercurial-scm.org/D588#9989, @indygreg wrote:
  
  > Would it be safe to keep the ``os.stat()`` code and return if the file 
doesn't exist? That at least allows us to do the "is directory" and "file 
missing" check with a single system call. That will avoid the random number 
generation and the 2nd system call for the rename when the file doesn't exist.
  
  
  I guess something like
  
diff --git a/mercurial/win32.py b/mercurial/win32.py
--- a/mercurial/win32.py
+++ b/mercurial/win32.py
@@ -12,6 +12,7 @@
 import msvcrt
 import os
 import random
+import stat
 import subprocess

 from . import (
@@ -522,7 +523,8 @@
 def unlink(f):
 '''try to implement POSIX' unlink semantics on Windows'''

-if os.path.isdir(f):
+st = os.stat(f)
+if stat.S_ISDIR(st.st_mode):
 # use EPERM because it is POSIX prescribed value, even though
 # unlink(2) on directories returns EISDIR on Linux
 raise IOError(errno.EPERM,
  
  might work (not tested).
  
  If f does not exist, the os.stat call will raise directly, which would spare 
us another system call in that case.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D588

To: indygreg, #hg-reviewers, quark
Cc: abuehl, durin42, quark, mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

D588: win32: use fewer system calls for unlink()

[abuehl (Adrian Buehlmann)]

abuehl added inline comments.

INLINE COMMENTS

> win32.py:537
> +if e.errno == errno.ENOENT:
> +raise
> +

why continue on errors != ENOENT ?

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D588

To: indygreg, #hg-reviewers, quark
Cc: abuehl, durin42, quark, mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

D588: win32: use fewer system calls for unlink()

[abuehl (Adrian Buehlmann)]

abuehl added a comment.


  In https://phab.mercurial-scm.org/D588#9989, @indygreg wrote:
  
  > OK. I failed to grok the unlink semantics on Windows. I'll need to read up 
on MSDN.
  
  
  I doubt this is documented on MSDN, but in case you find it there, please 
share the link (perhaps add it on the UnlinkingFilesOnWindows 
 wiki page).
  
  The code has pretty big comment right underneath where you inserted your 
change. The important part is this one:
  
# - Calling os.unlink on a file that has been opened with Mercurial's
#   posixfile (or comparable methods) will delay the actual deletion of
#   the file for as long as the file is held open. The filename is blocked
#   during that time and cannot be used for recreating a new file under
#   that same name ("zombie file"). Directories containing such zombie files
#   cannot be removed or moved.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D588

To: indygreg, #hg-reviewers, quark
Cc: abuehl, durin42, quark, mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

D588: win32: use fewer system calls for unlink()

[abuehl (Adrian Buehlmann)]

abuehl added a comment.


  In https://phab.mercurial-scm.org/D588#9925, @quark wrote:
  
  > I can confirm `os.unlink` may succeed without actually removing the file:
  >
  >   >>> from mercurial import util
  >   >>> f=util.posixfile('c:\\users\\quark\\a.txt', 'w')
  >   >>> os.unlink('c:\\users\\quark\\a.txt') # success, but file is still 
there
  >   >>> f.close() # removes the file
  >   
  >
  > There is a proposed `boost.afio` that seems to take a same approach: 
https://boostgsoc13.github.io/boost.afio/doc/html/afio/FAQ/deleting_open_files.html
  
  
  (finally created a login to this phabricator madness.. argh).
  
  Yep. You just redescovered what is already known.
  
  The bad thing is, the "zombie file" (can be held open by another process, 
even by opening any hardlinked name to the same file) makes it impossible to 
**create a new file under the same name** after "unlinking" the previous one. 
For as long as the other process holds the "unlinked" file open.
  
  Awesome pointer, BTW!
  
  I'll quote the interesting part found on that link you posted:
  
  > AFIO works around these un-POSIX file semantics by taking a dual step to 
deleting files. Firstly, it renames the file to a 128 bit cryptographically 
strong random name prefixed by “.afiod” into as high up the directory hierarchy 
as it is able to, and only then does it request the deletion of the file. As 
AFIO always opens files with FILE_SHARE_DELETE permission enabled, with that 
flag Windows permits renaming and deletion, and because the name was changed to 
a very random name somewhere not in its origin directory before deletion, you 
don't see those unexpected and random errors when creating files with the same 
name as a recently deleted file as you do anywhere else on Windows. Because the 
file is probably not in its original containing directory any more, deletions 
of that directory will not fail with “directory not empty” as they otherwise 
normally would and indeed do in most Windows programs. Handy eh?
  
  Very handy indeed and we already do something similar on Mercurial. We don't 
rename the "zombie file" to a different directory though, which sounds pretty 
clever. We could steal that trick from AFIO for Mercurial.
  
  For manual tests: I usually opened the file with an other process to match 
the scenario we were facing back then when we introduced the "rename to a 
random name before unlinking" dance.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D588

To: indygreg, #hg-reviewers, quark
Cc: abuehl, durin42, quark, mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Re: D588: win32: use fewer system calls for unlink()

[Adrian Buehlmann]

On 2017-09-01 08:16, Adrian Buehlmann wrote:
> Ugh. Can I reply to a phabricator notification by email?
> 
> Adding gregory.sz...@gmail.com manually, as I'm not sure replaying to
> those nasty phabricator emails is going to work...
> 
> On 2017-09-01 00:32, indygreg (Gregory Szorc) wrote:
>> diff --git a/mercurial/win32.py b/mercurial/win32.py
>> --- a/mercurial/win32.py
>> +++ b/mercurial/win32.py
>> @@ -12,6 +12,7 @@
>>  import msvcrt
>>  import os
>>  import random
>> +import stat
>>  import subprocess
>>  
>>  from . import (
>> @@ -522,11 +523,26 @@
>>  def unlink(f):
>>  '''try to implement POSIX' unlink semantics on Windows'''
>>  
>> -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)
>> +# If the path doesn't exist, raise that exception.
>> +# If it is a directory, emulate POSIX behavior.
>> +try:
>> +st = os.stat(f)
>> +if stat.S_ISDIR(st.st_mode):
>> +# 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)
>> +except OSError as e:
>> +if e.errno == errno.ENOENT:
>> +raise
>> +
>> +# In the common case, a normal unlink will work. Try that first and fall
>> +# back to more complexity if and only if we need to.
>> +try:
>> +os.unlink(f)
>> +return
>> +except (IOError, OSError) as e:
>> +pass
>>  
>>  # POSIX allows to unlink and rename open files. Windows has serious
>>  # problems with doing that:
> 
> Do you get an error at all, if a file, which is in open state, is unlinked?
> 
> My fear is: You won't get an error, but instead the filename is blocked
> by the file being held in place by the other process, until the other
> process closes it. Which means: You already lost the game.
> 
> Which would explain why we didn't do things like you propose here.
> 
> See also
> 
>https://www.mercurial-scm.org/wiki/UnlinkingFilesOnWindows
> 
> (specifically, heading 2)
> 

In other words:

The "zombie state" - which the rename dance afterwards tries to avoid -
has already been initiated (by calling unlink) when the rename dance is
started. Which is pointless. You then might as well remove the rename
step: It can't help any more. It will even fail, as you can't rename a
file in zombie state.
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Re: D588: win32: use fewer system calls for unlink()

[Adrian Buehlmann]

Ugh. Can I reply to a phabricator notification by email?

Adding gregory.sz...@gmail.com manually, as I'm not sure replaying to
those nasty phabricator emails is going to work...

On 2017-09-01 00:32, indygreg (Gregory Szorc) wrote:
> indygreg created this revision.
> Herald added a subscriber: mercurial-devel.
> Herald added a reviewer: hg-reviewers.
> 
> REVISION SUMMARY
>   unlink() on Windows is complicated by the fact that Windows doesn't
>   support removing an open file. Our unlink wrapper currently goes
>   through a dance where it generates a random filename, renames the
>   file, then attempts to unlink.
>   
>   This is a lot of extra work for what is likely the special case of
>   attempting to unlink a file that is open or can't be simply unlinked.
>   
>   In this commit, we refactor the code to use fewer system calls in
>   the common cases of 1) unlink just works 2) file doesn't exist.
>   
>   For the file doesn't exist case (before)
>   
>   1. stat() to determine if path is directory
>   2. generate random filename
>   3. rename()
>   
>   after:
>   
>   1. stat() to get path info
>   
>   For the file not open case (before)
>   
>   1. stat() to determine if path is directory
>   2. generate random filename
>   3. rename()
>   4. unlink()
>   
>   after:
>   
>   1. stat()
>   2. unlink()
>   
>   For the file open case (before)
>   
>   1. stat() to determine if path is directory
>   2. generate random filename
>   3. rename()
>   4. unlink()
>   
>   after:
>   
>   1. stat()
>   2. unlink()
>   3. generate random filename
>   4. rename()
>   5. unlink()
>   
>   There is also a scenario where the unlink fails due to the file being
>   marked as read-only. In this case we also introduce an extra unlink()
>   call. However, I reckon the common case is the file isn't marked as
>   read-only and skipping the random generation and rename is worth it.
>   
>   I /think/ this change makes bulk file writing on Windows faster. This
>   is because vfs.__call__ calls unlink() when a file is opened for
>   writing. When writing a few hundred files files as part of a clone or
>   working directory update, the extra system calls can matter.
>   win32.unlink() did show up in performance profiles, which is what
>   caused me to look at this code. But I/O performance is hard to measure
>   and I'm not sure if the ~30s off of ~620s for a stream clone unbundle
>   on the Firefox repo is indicative of real world performance. I do know
>   the new code uses fewer system calls and shouldn't be slower.
> 
> REPOSITORY
>   rHG Mercurial
> 
> REVISION DETAIL
>   https://phab.mercurial-scm.org/D588
> 
> AFFECTED FILES
>   mercurial/win32.py
> 
> CHANGE DETAILS
> 
> diff --git a/mercurial/win32.py b/mercurial/win32.py
> --- a/mercurial/win32.py
> +++ b/mercurial/win32.py
> @@ -12,6 +12,7 @@
>  import msvcrt
>  import os
>  import random
> +import stat
>  import subprocess
>  
>  from . import (
> @@ -522,11 +523,26 @@
>  def unlink(f):
>  '''try to implement POSIX' unlink semantics on Windows'''
>  
> -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)
> +# If the path doesn't exist, raise that exception.
> +# If it is a directory, emulate POSIX behavior.
> +try:
> +st = os.stat(f)
> +if stat.S_ISDIR(st.st_mode):
> +# 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)
> +except OSError as e:
> +if e.errno == errno.ENOENT:
> +raise
> +
> +# In the common case, a normal unlink will work. Try that first and fall
> +# back to more complexity if and only if we need to.
> +try:
> +os.unlink(f)
> +return
> +except (IOError, OSError) as e:
> +pass
>  
>  # POSIX allows to unlink and rename open files. Windows has serious
>  # problems with doing that:

Do you get an error at all, if a file, which is in open state, is unlinked?

My fear is: You won't get an error, but instead the filename is blocked
by the file being held in place by the other process, until the other
process closes it. Which means: You already lost the game.

Which would explain why we didn't do things like you propose here.

See also

   https://www.mercurial-scm.org/wiki/UnlinkingFilesOnWindows

(specifically, heading 2)

___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Re: Experimenting with Phabricator for reviews

[Adrian Buehlmann]

On 2017-07-10 22:00, Kevin Bullock wrote:
> Greetings, Mercurial hackers!
> 
> We've set up an instance of Phabricator that we're going to experiment with 
> for code reviews. At this time, we're not planning on using any of the other 
> features of Phabricator, and the use of Phabricator for reviews is very much 
> *an experiment* - we intend to see how this is going in a little while 
> (probably around the start of the 4.4 cycle). If you're more comfortable 
> submitting and reviewing patches as plain email, that's fine - use of 
> Phabricator is optional. In the meantime, you'll see e-mails coming to the 
> mailing list when activity happens in Phabricator--hopefully we've hit the 
> right settings to make this useful without being too chatty.
> 
> See https://www.mercurial-scm.org/wiki/Phabricator for a little more detail, 
> including how to submit new reviews.

Ugh. Looks way too chatty/cluttered for my taste so far (I'm mostly
lurking the list).

I'm likely going to unsubscribe Mercurial-devel, if it stays like that...

___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Re: [PATCH 2 of 4] killdaemons: close pid file before killing processes

[Adrian Buehlmann]

On 2017-06-05 05:38, Matt Harbison wrote:
> # HG changeset patch
> # User Matt Harbison 
> # Date 1495503902 14400
> #  Mon May 22 21:45:02 2017 -0400
> # Node ID eaed97a21942af11c122607bf37f7399c68cae9d
> # Parent  ecc27f3123ea173f2dc66e20abbedad5741ea5e1
> killdaemons: close pid file before killing processes
> 
> With #serve enabled on Windows, I was getting occasional stacktraces like 
> this:
> 
>   Errored test-hgweb-json.t: Traceback (most recent call last):
> File "./run-tests.py", line 724, in run
>   self.tearDown()
> File "./run-tests.py", line 805, in tearDown
>   killdaemons(entry)
> File "./run-tests.py", line 540, in killdaemons
>   logfn=vlog)
> File "...\tests\killdaemons.py", line 94, in killdaemons
>   os.unlink(pidfile)
>   WindowsError: [Error 32] The process cannot access the file because it is
>  being used by another process: 
> '...\\hgtests.zmpqj3\\child80\\daemon.pids'

..

> Adrian suggested using util.posixfile, (..)

Not really. I was just trying to point out that if you use Python's
open(), you get the "used by another process" WindowsError on
os.unlink(), if the file in question is still open.

https://www.mercurial-scm.org/wiki/UnlinkingFilesOnWindows

> However, the 'mercurial'
> package isn't in sys.path when invoking run-tests.py, and it isn't clear that
> hacking[1] it in is a good thing (especially for test-run-tests.t, which uses 
> an
> installation in a temp folder).
> 
> I tried using ProcessMonitor to figure out what the other process is, but that
> monitoring slows things down to such a degree that the issue doesn't occur.  I
> was ready to blame the virus scanner, but it happens without that too.
> 
> Looking at the code, I don't see anything that would have the pid file open.
> But I was able to get through about 20 full test runs without an issue with 
> this
> minor change, whereas before it was pretty certain to hit this at least once 
> in
> two or three runs.
> 
> [1] 
> https://www.mercurial-scm.org/pipermail/mercurial-devel/2017-May/097907.html
> 
> diff --git a/tests/killdaemons.py b/tests/killdaemons.py
> --- a/tests/killdaemons.py
> +++ b/tests/killdaemons.py
> @@ -78,18 +78,20 @@
>  logfn = lambda s: s
>  # Kill off any leftover daemon processes
>  try:
> -fp = open(pidfile)
> -for line in fp:
> -try:
> -pid = int(line)
> -if pid <= 0:
> -raise ValueError
> -except ValueError:
> -logfn('# Not killing daemon process %s - invalid pid'
> -  % line.rstrip())
> -continue
> +pids = []
> +with open(pidfile) as fp:
> +for line in fp:
> +try:
> +pid = int(line)
> +if pid <= 0:
> +raise ValueError
> +except ValueError:
> +logfn('# Not killing daemon process %s - invalid pid'
> +  % line.rstrip())
> +continue
> +pids.append(pid)
> +for pid in pids:
>  kill(pid, logfn, tryhard)
> -fp.close()
>  if remove:
>  os.unlink(pidfile)
>  except IOError:

This looks good to me and the change is an improvement.

Making sure that files are closed before acting is always good for Windows.

I'd suggest taking this patch, but I haven't tested it.
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Re: [PATCH 1 of 2] hghave: enable 'serve' on Windows

[Adrian Buehlmann]

On 2017-05-08 06:39, Matt Harbison wrote:
> # HG changeset patch
> # User Matt Harbison 
> # Date 1494183520 14400
> #  Sun May 07 14:58:40 2017 -0400
> # Node ID 36d9a659b9d76837faaf73fde3f5c5455231c2f9
> # Parent  c6cbd0b66465bcaa41f03c9498555f04d3dfbe7c
> hghave: enable 'serve' on Windows
> 
> I've been using a local hghaveaddon.py to enable this for a couple of months
> with reasonable success, and 'killdaemons' is already enabled on Windows.
> There's one failure[1] in test-http-proxy.t that this adds, which I can't 
> figure
> out.  On occasion, there is also a stacktrace at the end of a run:
> 
> Errored test-serve.t: Traceback (most recent call last):
>   File "./run-tests.py", line 724, in run
> self.tearDown()
>   File "./run-tests.py", line 805, in tearDown
> killdaemons(entry)
>   File "./run-tests.py", line 540, in killdaemons
> logfn=vlog)
>   File "c:\Users\Matt\Projects\hg\tests\killdaemons.py", line 94, in 
> killdaemons
> os.unlink(pidfile)
> WindowsError: [Error 32] The process cannot access the file because it is 
> being
>   used by another process: 
> '...\\hgtests.gubapm\\child449\\daemon.pids'

https://www.mercurial-scm.org/wiki/UnlinkingFilesOnWindows

> The affected test(s) vary from run to run (and most times the error doesn't
> occur).  The common thread is that the affected tests are missing a 
> killdaemons
> call.
> 
> Still, it seems better to enable a whole class of tests by default, to catch
> actual regressions when they occur.
> 
> [1] 
> https://www.mercurial-scm.org/pipermail/mercurial-devel/2017-April/096987.html
> 
> diff --git a/tests/hghave.py b/tests/hghave.py
> --- a/tests/hghave.py
> +++ b/tests/hghave.py
> @@ -502,7 +502,7 @@
>  
>  @check("serve", "platform and python can manage 'hg serve -d'")
>  def has_serve():
> -return os.name != 'nt' # gross approximation
> +return True
>  
>  @check("test-repo", "running tests from repository")
>  def has_test_repo():
> ___
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Re: [PATCH V3] util: move checknlink away from the dependency of a hard-coded filename

[Adrian Buehlmann]

On 2016-08-25 20:11, tt...@fb.com wrote:
> # HG changeset patch
> # User Tony Tung 
> # Date 1472148645 25200
> #  Thu Aug 25 11:10:45 2016 -0700
> # Node ID 625f3bb4833b47c4d35754fced91f37b077f7866
> # Parent  2efc5a05d80a6d4253767f2ce0c2fb062ba83cb6
> util: move checknlink away from the dependency of a hard-coded filename
> 
> This somewhat insulates us against bugs in checknlink when a stale file
> left behind could result in checknlink always returning False.
> 
> diff --git a/mercurial/util.py b/mercurial/util.py
> --- a/mercurial/util.py
> +++ b/mercurial/util.py
> @@ -1305,38 +1305,53 @@
>  def checknlink(testfile):
>  '''check whether hardlink count reporting works properly'''
>  
> -# testfile may be open, so we need a separate file for checking to
> -# work around issue2543 (or testfile may get lost on Samba shares)
> -f1 = testfile + ".hgtmp1"
> -if os.path.lexists(f1):
> -return False
> +f1 = {}
> +f2 = {}
> +
>  try:
> -posixfile(f1, 'w').close()
> -except IOError:
> -try:
> -os.unlink(f1)
> -except OSError:
> -pass
> -return False
> -
> -f2 = testfile + ".hgtmp2"
> -fd = None
> -try:
> -oslink(f1, f2)
> +dirpath, filename = os.path.split(testfile)
> +
> +# testfile may be open, so we need a separate file for checking to
> +# work around issue2543 (or testfile may get lost on Samba shares)
> +f1['fd'], f1['path'] = tempfile.mkstemp(prefix=filename, dir=dirpath)
> +os.close(f1['fd'])
> +del f1['fd']
> +
> +f2['fd'], f2['path'] = tempfile.mkstemp(prefix=filename, dir=dirpath)
> +os.close(f2['fd'])
> +del f2['fd']
> +
> +# there's a small race condition that another file can jam itself in
> +# between the time we remove f2 and the time we create the hard link.
> +# in the unlikely scenario that happens, we'll treat it as nlink 
> being
> +# unreliable.
> +os.unlink(f2['path'])
> +oslink(f1['path'], f2['path'])
>  # nlinks() may behave differently for files on Windows shares if
>  # the file is open.
> -fd = posixfile(f2)
> -return nlinks(f2) > 1
> -except OSError:
> +f2['handle'] = posixfile(f2['path'])
> +return nlinks(f2['path']) > 1
> +except EnvironmentError:
>  return False
>  finally:
> -if fd is not None:
> -fd.close()
> -for f in (f1, f2):
> -try:
> -os.unlink(f)
> -except OSError:
> -pass
> +for fi in (f1, f2):
> +if 'fd' in fi:
> +try:
> +os.close(fi['fd'])
> +except EnvironmentError:
> +pass
> +
> +if 'handle' in fi:
> +try:
> +fi['handle'].close()
> +except EnvironmentError:
> +pass
> +
> +if 'path' in fi:
> +try:
> +os.unlink(fi['path'])
> +except EnvironmentError:
> +pass

If I'm not mistaken, this will try to unlink file f1 while hardlinked
alias name f2 still has an open handle (opened using posixfile).

This may not be a problem with posixfile, but Windows can have problems,
if a hardlinked file is deleted while it is still in open() state under
an alias name (see [1]).

I would thus recommend closing all files before unlinking.

I'd additionally prefer doing cleanup things in reverse order, i.e.
close 'handle' on f2 first.

So, I think I'd prefer doing the cleanup this way:

# close files
for fi in (f2, f1): # intentionally in reverse order
if 'handle' in fi:
try:
fi['handle'].close()
except EnvironmentError:
pass
if 'fd' in fi:
try:
os.close(fi['fd'])
except EnvironmentError:
pass

# unlink files
for fi in (f2, f1):  # intentionally in reverse order
if 'path' in fi:
try:
os.unlink(fi['path'])
except EnvironmentError:
pass

>  
>  def endswithsep(path):
>  '''Check path ends with os.sep or os.altsep.'''

[1] https://www.mercurial-scm.org/wiki/UnlinkingFilesOnWindows
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel