Patchwork [1,of,5] posix: move checkexec test file to .hg/cache

login
register
mail settings
Submitter Mads Kiilerich
Date Nov. 17, 2016, 6:44 p.m.
Message ID <1b5e959ebd859c27b336.1479408257@madski>
Download mbox | patch
Permalink /patch/17622/
State Accepted
Headers show

Comments

Mads Kiilerich - Nov. 17, 2016, 6:44 p.m.
# HG changeset patch
# User Mads Kiilerich <madski@unity3d.com>
# Date 1479383976 -3600
#      Thu Nov 17 12:59:36 2016 +0100
# Node ID 1b5e959ebd859c27b3369124c926a512e222545c
# Parent  854190becacb8abecddddbf5c18a777b02bbc045
posix: move checkexec test file to .hg/cache

This avoids unnecessary churn in the working directory.

It is not necessarily a fully valid assumption that .hg/cache is on the same
filesystem as the working directory, but I think it is an acceptable
approximation. It could also be the case that different parts of the working
directory is on different mount points so checking in the root folder could
also be wrong.
Siddharth Agarwal - Nov. 29, 2016, 10:52 p.m.
On 11/17/16 10:44, Mads Kiilerich wrote:
> # HG changeset patch
> # User Mads Kiilerich <madski@unity3d.com>
> # Date 1479383976 -3600
> #      Thu Nov 17 12:59:36 2016 +0100
> # Node ID 1b5e959ebd859c27b3369124c926a512e222545c
> # Parent  854190becacb8abecddddbf5c18a777b02bbc045
> posix: move checkexec test file to .hg/cache
>
> This avoids unnecessary churn in the working directory.
>
> It is not necessarily a fully valid assumption that .hg/cache is on the same
> filesystem as the working directory, but I think it is an acceptable
> approximation.

I remember Matt talking about this exact issue and saying that this is 
not a reasonable assumption.

- Siddharth

>   It could also be the case that different parts of the working
> directory is on different mount points so checking in the root folder could
> also be wrong.
>
> diff --git a/mercurial/posix.py b/mercurial/posix.py
> --- a/mercurial/posix.py
> +++ b/mercurial/posix.py
> @@ -160,7 +160,10 @@ def checkexec(path):
>   
>       try:
>           EXECFLAGS = stat.S_IXUSR | stat.S_IXGRP | stat.S_IXOTH
> -        fh, fn = tempfile.mkstemp(dir=path, prefix='hg-checkexec-')
> +        cachedir = os.path.join(path, '.hg', 'cache')
> +        if not os.path.isdir(cachedir):
> +            cachedir = path
> +        fh, fn = tempfile.mkstemp(dir=cachedir, prefix='hg-checkexec-')
>           try:
>               os.close(fh)
>               m = os.stat(fn).st_mode & 0o777
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Gregory Szorc - Nov. 30, 2016, 3:51 a.m.
On Tue, Nov 29, 2016 at 2:52 PM, Siddharth Agarwal <sid@less-broken.com>
wrote:

> On 11/17/16 10:44, Mads Kiilerich wrote:
>
>> # HG changeset patch
>> # User Mads Kiilerich <madski@unity3d.com>
>> # Date 1479383976 -3600
>> #      Thu Nov 17 12:59:36 2016 +0100
>> # Node ID 1b5e959ebd859c27b3369124c926a512e222545c
>> # Parent  854190becacb8abecddddbf5c18a777b02bbc045
>> posix: move checkexec test file to .hg/cache
>>
>> This avoids unnecessary churn in the working directory.
>>
>> It is not necessarily a fully valid assumption that .hg/cache is on the
>> same
>> filesystem as the working directory, but I think it is an acceptable
>> approximation.
>>
>
> I remember Matt talking about this exact issue and saying that this is not
> a reasonable assumption.
>
>
Source? I could easily see:

* .hg/cache being on a network/shared filesystem
* .hg/cache being on a separate filesystem from .hg/store (due to shared
repos)

But the main .hg directory and .hg/cache /should/ be on the same filesystem
as the working directory since we don't allow detaching the .hg directory.

The only scenario I can think of is someone who has moved .hg to another
filesystem and symlinked .hg in the working directory to it. I guess that's
plausible. Ugh.


>
>   It could also be the case that different parts of the working
>> directory is on different mount points so checking in the root folder
>> could
>> also be wrong.
>>
>> diff --git a/mercurial/posix.py b/mercurial/posix.py
>> --- a/mercurial/posix.py
>> +++ b/mercurial/posix.py
>> @@ -160,7 +160,10 @@ def checkexec(path):
>>         try:
>>           EXECFLAGS = stat.S_IXUSR | stat.S_IXGRP | stat.S_IXOTH
>> -        fh, fn = tempfile.mkstemp(dir=path, prefix='hg-checkexec-')
>> +        cachedir = os.path.join(path, '.hg', 'cache')
>> +        if not os.path.isdir(cachedir):
>> +            cachedir = path
>> +        fh, fn = tempfile.mkstemp(dir=cachedir, prefix='hg-checkexec-')
>>           try:
>>               os.close(fh)
>>               m = os.stat(fn).st_mode & 0o777
>> _______________________________________________
>> 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
>
timeless - Dec. 12, 2016, 2:45 a.m.
Gregory Szorc wrote:
> The only scenario I can think of is someone who has moved .hg to another
> filesystem and symlinked .hg in the working directory to it. I guess that's
> plausible. Ugh.

I can picture doing this. It's possible I've already done it...

Patch

diff --git a/mercurial/posix.py b/mercurial/posix.py
--- a/mercurial/posix.py
+++ b/mercurial/posix.py
@@ -160,7 +160,10 @@  def checkexec(path):
 
     try:
         EXECFLAGS = stat.S_IXUSR | stat.S_IXGRP | stat.S_IXOTH
-        fh, fn = tempfile.mkstemp(dir=path, prefix='hg-checkexec-')
+        cachedir = os.path.join(path, '.hg', 'cache')
+        if not os.path.isdir(cachedir):
+            cachedir = path
+        fh, fn = tempfile.mkstemp(dir=cachedir, prefix='hg-checkexec-')
         try:
             os.close(fh)
             m = os.stat(fn).st_mode & 0o777