Patchwork [V2] util: add allowhardlinks module variable

login
register
mail settings
Submitter Durham Goode
Date March 2, 2017, 6:12 p.m.
Message ID <32c17aa5fc546a112b35.1488478371@dev111.prn1.facebook.com>
Download mbox | patch
Permalink /patch/18874/
State Accepted
Headers show

Comments

Durham Goode - March 2, 2017, 6:12 p.m.
# HG changeset patch
# User Durham Goode <durham@fb.com>
# Date 1488478360 28800
#      Thu Mar 02 10:12:40 2017 -0800
# Node ID 32c17aa5fc546a112b355e734fb71b740172cf09
# Parent  a8458fe51a9d155f1daeaffdcf503e674d4d4588
util: add allowhardlinks module variable

To enable extensions to enable hardlinks for certain environments, let's move
the 'if False' to be an 'if allowhardlinks' and let extensions modify the
allowhardlinks variable.

Tests on linux ext4 pass with it set to True and to False.
Augie Fackler - March 3, 2017, 1:31 a.m.
On Thu, Mar 02, 2017 at 10:12:51AM -0800, Durham Goode wrote:
> # HG changeset patch
> # User Durham Goode <durham@fb.com>
> # Date 1488478360 28800
> #      Thu Mar 02 10:12:40 2017 -0800
> # Node ID 32c17aa5fc546a112b355e734fb71b740172cf09
> # Parent  a8458fe51a9d155f1daeaffdcf503e674d4d4588
> util: add allowhardlinks module variable

Queued, thanks.

>
> To enable extensions to enable hardlinks for certain environments, let's move
> the 'if False' to be an 'if allowhardlinks' and let extensions modify the
> allowhardlinks variable.
>
> Tests on linux ext4 pass with it set to True and to False.
>
> diff --git a/mercurial/util.py b/mercurial/util.py
> --- a/mercurial/util.py
> +++ b/mercurial/util.py
> @@ -1056,6 +1056,11 @@ def checksignature(func):
>
>      return check
>
> +# Hardlinks are problematic on CIFS, do not allow hardlinks
> +# until we find a way to work around it cleanly (issue4546).
> +# This is a variable so extensions can opt-in to using them.
> +allowhardlinks = False
> +
>  def copyfile(src, dest, hardlink=False, copystat=False, checkambig=False):
>      '''copy a file, preserving mode and optionally other stat info like
>      atime/mtime
> @@ -1072,9 +1077,7 @@ def copyfile(src, dest, hardlink=False,
>          if checkambig:
>              oldstat = checkambig and filestat(dest)
>          unlink(dest)
> -    # hardlinks are problematic on CIFS, quietly ignore this flag
> -    # until we find a way to work around it cleanly (issue4546)
> -    if False and hardlink:
> +    if allowhardlinks and hardlink:
>          try:
>              oslink(src, dest)
>              return
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Ryan McElroy - March 12, 2017, 6:37 a.m.
On 3/2/17 5:31 PM, Augie Fackler wrote:
> On Thu, Mar 02, 2017 at 10:12:51AM -0800, Durham Goode wrote:
>> # HG changeset patch
>> # User Durham Goode <durham@fb.com>
>> # Date 1488478360 28800
>> #      Thu Mar 02 10:12:40 2017 -0800
>> # Node ID 32c17aa5fc546a112b355e734fb71b740172cf09
>> # Parent  a8458fe51a9d155f1daeaffdcf503e674d4d4588
>> util: add allowhardlinks module variable
> Queued, thanks.

Late to the party here, but why not do this as a config option so one 
doesn't need an extension to override this?

>
>> To enable extensions to enable hardlinks for certain environments, let's move
>> the 'if False' to be an 'if allowhardlinks' and let extensions modify the
>> allowhardlinks variable.
>>
>> Tests on linux ext4 pass with it set to True and to False.
>>
>> diff --git a/mercurial/util.py b/mercurial/util.py
>> --- a/mercurial/util.py
>> +++ b/mercurial/util.py
>> @@ -1056,6 +1056,11 @@ def checksignature(func):
>>
>>       return check
>>
>> +# Hardlinks are problematic on CIFS, do not allow hardlinks
>> +# until we find a way to work around it cleanly (issue4546).
>> +# This is a variable so extensions can opt-in to using them.
>> +allowhardlinks = False
>> +
>>   def copyfile(src, dest, hardlink=False, copystat=False, checkambig=False):
>>       '''copy a file, preserving mode and optionally other stat info like
>>       atime/mtime
>> @@ -1072,9 +1077,7 @@ def copyfile(src, dest, hardlink=False,
>>           if checkambig:
>>               oldstat = checkambig and filestat(dest)
>>           unlink(dest)
>> -    # hardlinks are problematic on CIFS, quietly ignore this flag
>> -    # until we find a way to work around it cleanly (issue4546)
>> -    if False and hardlink:
>> +    if allowhardlinks and hardlink:
>>           try:
>>               oslink(src, dest)
>>               return
>>
Ryan McElroy - March 12, 2017, 6:44 a.m.
On 3/11/17 10:37 PM, Ryan McElroy wrote:
>
>
> On 3/2/17 5:31 PM, Augie Fackler wrote:
>> On Thu, Mar 02, 2017 at 10:12:51AM -0800, Durham Goode wrote:
>>> # HG changeset patch
>>> # User Durham Goode <durham@fb.com>
>>> # Date 1488478360 28800
>>> #      Thu Mar 02 10:12:40 2017 -0800
>>> # Node ID 32c17aa5fc546a112b355e734fb71b740172cf09
>>> # Parent  a8458fe51a9d155f1daeaffdcf503e674d4d4588
>>> util: add allowhardlinks module variable
>> Queued, thanks.
>
> Late to the party here, but why not do this as a config option so one 
> doesn't need an extension to override this?
>

Durham already answered my question in a previous email chain, but I 
missed it until just now. The answer is this:

"Might be hard to get a config option down to this level, since there's 
no ui object around here, but we could probably move the 'False' value 
to a module level variable so extensions could at least set it to True."

That resolves my concerns. Thanks!

>>
>>> To enable extensions to enable hardlinks for certain environments, 
>>> let's move
>>> the 'if False' to be an 'if allowhardlinks' and let extensions 
>>> modify the
>>> allowhardlinks variable.
>>>
>>> Tests on linux ext4 pass with it set to True and to False.
>>>
>>> diff --git a/mercurial/util.py b/mercurial/util.py
>>> --- a/mercurial/util.py
>>> +++ b/mercurial/util.py
>>> @@ -1056,6 +1056,11 @@ def checksignature(func):
>>>
>>>       return check
>>>
>>> +# Hardlinks are problematic on CIFS, do not allow hardlinks
>>> +# until we find a way to work around it cleanly (issue4546).
>>> +# This is a variable so extensions can opt-in to using them.
>>> +allowhardlinks = False
>>> +
>>>   def copyfile(src, dest, hardlink=False, copystat=False, 
>>> checkambig=False):
>>>       '''copy a file, preserving mode and optionally other stat info 
>>> like
>>>       atime/mtime
>>> @@ -1072,9 +1077,7 @@ def copyfile(src, dest, hardlink=False,
>>>           if checkambig:
>>>               oldstat = checkambig and filestat(dest)
>>>           unlink(dest)
>>> -    # hardlinks are problematic on CIFS, quietly ignore this flag
>>> -    # until we find a way to work around it cleanly (issue4546)
>>> -    if False and hardlink:
>>> +    if allowhardlinks and hardlink:
>>>           try:
>>>               oslink(src, dest)
>>>               return
>>>
>
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://urldefense.proofpoint.com/v2/url?u=https-3A__www.mercurial-2Dscm.org_mailman_listinfo_mercurial-2Ddevel&d=DwIGaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=Jw8rundaE7TbmqBYd1txIQ&m=TY1yyCzQsFkWRqltmIihi_jJR6XvVkwU3D2zz2BjrjA&s=LPqDB6t4fdZmNHb1m1wO9MZKN9-wYENAZelXyDROO84&e=
Augie Fackler - March 12, 2017, 6:47 a.m.
> On Mar 11, 2017, at 22:44, Ryan McElroy <rm@fb.com> wrote:
> 
>>>> util: add allowhardlinks module variable
>>> Queued, thanks.
>> 
>> Late to the party here, but why not do this as a config option so one doesn't need an extension to override this?
>> 
> 
> Durham already answered my question in a previous email chain, but I missed it until just now. The answer is this:
> 
> "Might be hard to get a config option down to this level, since there's no ui object around here, but we could probably move the 'False' value to a module level variable so extensions could at least set it to True."
> 
> That resolves my concerns. Thanks!

More rationale:

https://www.mercurial-scm.org/pipermail/mercurial-devel/2017-March/093816.html

(happy to elaborate if needed)

Patch

diff --git a/mercurial/util.py b/mercurial/util.py
--- a/mercurial/util.py
+++ b/mercurial/util.py
@@ -1056,6 +1056,11 @@  def checksignature(func):
 
     return check
 
+# Hardlinks are problematic on CIFS, do not allow hardlinks
+# until we find a way to work around it cleanly (issue4546).
+# This is a variable so extensions can opt-in to using them.
+allowhardlinks = False
+
 def copyfile(src, dest, hardlink=False, copystat=False, checkambig=False):
     '''copy a file, preserving mode and optionally other stat info like
     atime/mtime
@@ -1072,9 +1077,7 @@  def copyfile(src, dest, hardlink=False, 
         if checkambig:
             oldstat = checkambig and filestat(dest)
         unlink(dest)
-    # hardlinks are problematic on CIFS, quietly ignore this flag
-    # until we find a way to work around it cleanly (issue4546)
-    if False and hardlink:
+    if allowhardlinks and hardlink:
         try:
             oslink(src, dest)
             return