Patchwork [STABLE,RFC] largefiles: load the convert extension as needed to register config items

login
register
mail settings
Submitter Matt Harbison
Date Nov. 23, 2017, 7:29 p.m.
Message ID <939e30e7ca1c925e5209.1511465365@Envy>
Download mbox | patch
Permalink /patch/25728/
State Accepted, archived
Headers show

Comments

Matt Harbison - Nov. 23, 2017, 7:29 p.m.
# HG changeset patch
# User Matt Harbison <matt_harbison@yahoo.com>
# Date 1511417702 18000
#      Thu Nov 23 01:15:02 2017 -0500
# Branch stable
# Node ID 939e30e7ca1c925e5209f03805cd7f18583f2550
# Parent  02845f7441aff30bc01975a5881cabfa922c12d4
largefiles: load the convert extension as needed to register config items

The `hg lfconvert --to-normal` command uses the convert extension internally to
work its magic, but that produces devel-warn messages if the convert extension
isn't loaded by the user.  The test in fcd2f9b06629 (modified here) wasn't
showing the warnings because the convert extension was loaded via $HGRCPATH.
Most of the config options default to None/False, but 'hg.usebranchnames' and
'hg.tagsbranch' are supposed to default to True and 'default' respectively.

Note that check-config is complaining that the extension isn't documented.  It
is, but I assume fixing up the script is outside the scope of stable.  We could
abort instead, and make the user enable the convert extension.  But I don't see
any reason why they should suddenly have to start doing that.

Some options were moved to core in 0f5521e56b77, which would probably also avoid
the problem.  However, these are actually implemented in the extension, and
loading the extension allows some of the convert extension to be wrapped by the
lfs extension.  (That patch is in my outbound queue.)
Yuya Nishihara - Nov. 26, 2017, 2:34 a.m.
On Thu, 23 Nov 2017 14:29:25 -0500, Matt Harbison wrote:
> # HG changeset patch
> # User Matt Harbison <matt_harbison@yahoo.com>
> # Date 1511417702 18000
> #      Thu Nov 23 01:15:02 2017 -0500
> # Branch stable
> # Node ID 939e30e7ca1c925e5209f03805cd7f18583f2550
> # Parent  02845f7441aff30bc01975a5881cabfa922c12d4
> largefiles: load the convert extension as needed to register config items

Perhaps the easiest way around is to move all configitems into core as we did
for the share extension.
Matt Harbison - Nov. 26, 2017, 2:46 a.m.
> On Nov 25, 2017, at 9:34 PM, Yuya Nishihara <yuya@tcha.org> wrote:
> 
>> On Thu, 23 Nov 2017 14:29:25 -0500, Matt Harbison wrote:
>> # HG changeset patch
>> # User Matt Harbison <matt_harbison@yahoo.com>
>> # Date 1511417702 18000
>> #      Thu Nov 23 01:15:02 2017 -0500
>> # Branch stable
>> # Node ID 939e30e7ca1c925e5209f03805cd7f18583f2550
>> # Parent  02845f7441aff30bc01975a5881cabfa922c12d4
>> largefiles: load the convert extension as needed to register config items
> 
> Perhaps the easiest way around is to move all configitems into core as we did
> for the share extension.

Yes, that was the hash I referenced.  But like I said, the share functionality is actually implemented in the core, and these aren’t, so it seems a bit odd.

The bigger issue though is I need to have the convert extension load, in order for it to be wrapped by the lfs extension.  (The basic flow is to wrap convert, but if isn’t loaded, wrap largefiles.  Then when largefiles loads, wrap convert in an lfconvert override.)  It would be weird for the user to not need to load convert when using lfconvert, unless also using lfs.  I can submit that series as an RFC too (or put it on bitbucket) if you’d like to see what I mean.
Yuya Nishihara - Nov. 26, 2017, 3:01 a.m.
On Sat, 25 Nov 2017 21:46:55 -0500, Matt Harbison wrote:
> > On Nov 25, 2017, at 9:34 PM, Yuya Nishihara <yuya@tcha.org> wrote:
> >> On Thu, 23 Nov 2017 14:29:25 -0500, Matt Harbison wrote:
> >> # HG changeset patch
> >> # User Matt Harbison <matt_harbison@yahoo.com>
> >> # Date 1511417702 18000
> >> #      Thu Nov 23 01:15:02 2017 -0500
> >> # Branch stable
> >> # Node ID 939e30e7ca1c925e5209f03805cd7f18583f2550
> >> # Parent  02845f7441aff30bc01975a5881cabfa922c12d4
> >> largefiles: load the convert extension as needed to register config items
> > 
> > Perhaps the easiest way around is to move all configitems into core as we did
> > for the share extension.
> 
> Yes, that was the hash I referenced.  But like I said, the share functionality is actually implemented in the core, and these aren’t, so it seems a bit odd.
> 
> The bigger issue though is I need to have the convert extension load, in order for it to be wrapped by the lfs extension.  (The basic flow is to wrap convert, but if isn’t loaded, wrap largefiles.  Then when largefiles loads, wrap convert in an lfconvert override.)  It would be weird for the user to not need to load convert when using lfconvert, unless also using lfs.  I can submit that series as an RFC too (or put it on bitbucket) if you’d like to see what I mean.

I think some logic of the convert extension can be moved to core as well,
so the largefiles will no longer need to wrap extension functions. I don't
know if that will work for lfs, though.

Patch

diff --git a/hgext/largefiles/lfcommands.py b/hgext/largefiles/lfcommands.py
--- a/hgext/largefiles/lfcommands.py
+++ b/hgext/largefiles/lfcommands.py
@@ -85,6 +85,15 @@ 
     if not hg.islocal(dest):
         raise error.Abort(_('%s is not a local Mercurial repo') % dest)
 
+    # Converting to normal files leverages the convert extension as an
+    # implementation detail.  If not enabled (or explicitly disabled) force it
+    # on if converting in that direction, to ensure that the config items are
+    # properly registered.
+    if not tolfile:
+        ext = ui.config('extensions', 'convert')
+        if ext is None or ext.startswith('!'):
+            ui.setconfig('extensions', 'convert', '', source='largefiles')
+
     rsrc = hg.repository(ui, src)
     ui.status(_('initializing destination %s\n') % dest)
     rdst = hg.repository(ui, dest, create=True)
diff --git a/tests/test-check-config.t b/tests/test-check-config.t
--- a/tests/test-check-config.t
+++ b/tests/test-check-config.t
@@ -45,3 +45,4 @@ 
 
   $ testrepohg files "set:(**.py or **.txt) - tests/**" | sed 's|\\|/|g' |
   >   $PYTHON contrib/check-config.py
+  undocumented: extensions.convert (str)\r (esc)
diff --git a/tests/test-lfconvert.t b/tests/test-lfconvert.t
--- a/tests/test-lfconvert.t
+++ b/tests/test-lfconvert.t
@@ -233,9 +233,12 @@ 
   $ cd ..
 
 round-trip: converting back to a normal (non-largefiles) repo with
-"lfconvert --to-normal" should give the same as ../bigfile-repo
+"lfconvert --to-normal" should give the same as ../bigfile-repo.  The
+global hgrc is ignored to ensure the convert extension doesn't load,
+to force the largefiles extension to handle that.
   $ cd largefiles-repo
-  $ hg lfconvert --to-normal . ../normal-repo
+  $ HGRCPATH=/dev/null hg --config extensions.largefiles= \
+  >     lfconvert --to-normal . ../normal-repo
   initializing destination ../normal-repo
   0 additional largefiles cached
   scanning source...