| 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
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
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 >>
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=
> 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