Require an explicit registry instead of defaulting to one

The shipped default named one private, internet-unreachable host, so an
unconfigured install failed against a server it could never reach rather
than saying what to set.

- drop the registry default and make the option required
- reject a non-string, empty, or non-http(s) registry with the option name
  and how to set it
- ask for a registry only when a run has to resolve one, so clean, list,
  and specs with an explicit url keep working without it

README.md

@@ -39,8 +39,16 @@ require('arti-pack').setup({
 })
 ```
 
-`registry` is the one option you have to set: the URL prefix your artifact
-repository serves upstream tag archives under.
+`registry` is required and has no default: it is the URL prefix your own
+artifact repository serves upstream tag archives under, so there is no sensible
+value to ship. Until you set it, `:ArtiPackSync` refuses to run and says so:
+
+```
+[arti-pack] registry is not configured; set it in require('arti-pack').setup{ registry = 'https://artifacts.example.com/artifactory/github-remote' }
+```
+
+`:ArtiPackClean` and `:ArtiPackList` never resolve a URL, so they work without
+it, as do plugins that carry an explicit `url`.
 
 Every plugin needs an explicit `tag` — that is the point. Plugins install to
 `stdpath('data')/site/pack/arti-pack/start/<name>`, or `.../opt/<name>` when
@@ -61,7 +69,7 @@ Every plugin needs an explicit `tag` — that is the point. Plugins install to
 
 | Option        | Default | Meaning                                     |
 | ------------- | ------- | ------------------------------------------- |
-| `registry`    | *site-specific* | Archive URL prefix placed in front of `owner/repo` |
+| `registry`    | *required* | Archive URL prefix placed in front of `owner/repo` |
 | `auto_sync`   | `true`  | Sync missing and changed plugins on startup |
 | `concurrency` | `4`     | Plugins downloaded in parallel              |
 | `timeout`     | `120`   | Per-download timeout in seconds             |

doc/arti-pack.txt

@@ -38,9 +38,13 @@ Requires curl, and unzip or tar.
 <
 Options:
 
-  registry     Archive URL prefix placed in front of "owner/repo". The
-               shipped default is site-specific; set it to your own
-               artifact repository.
+  registry     Required, no default. Archive URL prefix placed in front of
+               "owner/repo", pointing at your own artifact repository. It
+               must be an http:// or https:// URL; anything else is
+               rejected at setup. Until it is set, |:ArtiPackSync| refuses
+               to run and says which option to set. |:ArtiPackClean| and
+               |:ArtiPackList| never resolve a URL and work without it, as
+               do plugin specs carrying an explicit "url".
   plugins      List of plugin specs. See |arti-pack-specs|.
   auto_sync    Sync missing and changed plugins on startup. Default true.
   concurrency  Plugins downloaded in parallel. Default 4.

lua/arti-pack/init.lua

@@ -5,8 +5,9 @@ local spec_mod = require('arti-pack.spec')
 
 local M = {}
 
+-- registry has no default: it is site-specific, so any shipped value would point
+-- every install at a host it cannot reach. See M.setup.
 local DEFAULTS = {
-  registry = 'https://artifactapi.k8s.syd1.au.unkin.net/api/v1/remote/github',
   plugins = {},
   auto_sync = true,
   concurrency = 4,
@@ -42,6 +43,17 @@ local function context()
   }
 end
 
+-- A spec carrying its own url never touches the registry, so only demand one
+-- when something in this run would have to be resolved against it.
+local function registry_error(specs, registry)
+  for _, spec in ipairs(specs) do
+    if not spec.url then
+      local _, err = spec_mod.check_registry(registry)
+      return err
+    end
+  end
+end
+
 local function installed_dir(ctx, name, opt)
   return fs.join(ctx.pack_dir, opt and 'opt' or 'start', name)
 end
@@ -110,6 +122,11 @@ function M.sync(opts)
     return notify('everything up to date')
   end
 
+  local reg_err = registry_error(pending, ctx.registry)
+  if reg_err then
+    return notify(reg_err, vim.log.levels.ERROR)
+  end
+
   running = true
   notify(string.format('syncing %d plugin(s)', #pending))
   installer.run(ctx, pending, function(results)
@@ -213,6 +230,13 @@ function M.setup(opts)
   -- a user's plugin list; the caller's list is always authoritative.
   M.config.plugins = opts.plugins or {}
 
+  if opts.registry ~= nil then
+    local _, err = spec_mod.check_registry(opts.registry)
+    if err then
+      notify(err, vim.log.levels.ERROR)
+    end
+  end
+
   local p = paths()
   fs.mkdirp(fs.join(p.pack_dir, 'start'))
   fs.mkdirp(fs.join(p.pack_dir, 'opt'))

lua/arti-pack/spec.lua

@@ -2,6 +2,8 @@ local M = {}
 
 local SOURCE_PATTERN = '^([%w%.%_%-]+)/([%w%.%_%-]+)$'
 local EXTENSIONS = { ['zip'] = true, ['tar.gz'] = true }
+local SETUP_HINT =
+  "set it in require('arti-pack').setup{ registry = 'https://artifacts.example.com/artifactory/github-remote' }"
 
 local function trim(s)
   return (s:gsub('^%s+', ''):gsub('%s+$', ''))
@@ -65,16 +67,29 @@ function M.parse(entry)
   }
 end
 
+-- There is no default registry: the URL is site-specific, and a wrong one only
+-- surfaces as an opaque curl failure. Returns the base with trailing slashes
+-- stripped so joining it to a path never produces "//".
+function M.check_registry(registry)
+  if registry == nil then
+    return nil, 'registry is not configured; ' .. SETUP_HINT
+  end
+  if type(registry) ~= 'string' or not registry:match('^https?://[^%s/]') then
+    return nil, string.format('registry %s is not an http(s) URL; %s', vim.inspect(registry), SETUP_HINT)
+  end
+  return (registry:gsub('/+$', ''))
+end
+
 -- Tagged source archives live at the same path shape on every GitHub-alike, so
 -- the registry is just the proxy prefix in front of "<owner>/<repo>".
 function M.url(spec, registry)
   if spec.url then
     return spec.url
   end
-  if type(registry) ~= 'string' or registry == '' then
-    return nil, 'registry is not configured'
+  local base, err = M.check_registry(registry)
+  if not base then
+    return nil, err
   end
-  local base = registry:gsub('/+$', '')
   return table.concat({
     base,
     spec.owner,

tests/init_spec.lua

@@ -142,11 +142,25 @@ t.describe('arti-pack.setup', function()
   t.it('keeps the defaults for options the caller omits', function()
     use_env()
     arti.setup({ auto_sync = false })
-    t.eq('https://artifactapi.k8s.syd1.au.unkin.net/api/v1/remote/github', arti.config.registry)
     t.eq(4, arti.config.concurrency)
     t.eq(120, arti.config.timeout)
   end)
 
+  t.it('ships no registry default', function()
+    use_env()
+    arti.setup({ auto_sync = false })
+    t.eq(nil, arti.config.registry, 'registry is required, so nothing is assumed')
+  end)
+
+  t.it('rejects a registry that is not an http(s) url', function()
+    for _, bad in ipairs({ '', 'artifacts.example.com/gh', 'ftp://artifacts.example.com', 42, {} }) do
+      use_env()
+      arti.setup({ auto_sync = false, registry = bad })
+      t.matches('is not an http%(s%) URL', last_notice(), 'rejects ' .. vim.inspect(bad))
+      t.matches("setup{ registry = 'https://", last_notice(), 'says how to set it')
+    end
+  end)
+
   t.it('applies the options the caller does set', function()
     use_env()
     arti.setup({ auto_sync = false, registry = 'https://example.test/gh', concurrency = 1 })
@@ -179,6 +193,63 @@ t.describe('arti-pack.setup', function()
   end)
 end)
 
+t.describe('arti-pack without a registry', function()
+  t.it('refuses to sync a plugin it would have to resolve against the registry', function()
+    use_env()
+    arti.setup({ auto_sync = false, plugins = { { 'owner/thing.nvim', tag = 'v1' } } })
+
+    arti.sync()
+
+    t.matches('registry is not configured', last_notice())
+    t.matches("setup{ registry = 'https://", last_notice(), 'says how to set it')
+    t.falsy(fs.exists(env.pack_dir .. '/start/thing.nvim'), 'nothing is downloaded')
+  end)
+
+  t.it('names the registry option when a command runs before setup', function()
+    use_env()
+    arti.config.registry = nil
+    arti.config.plugins = { { 'owner/thing.nvim', tag = 'v1' } }
+
+    arti.sync()
+
+    t.matches('registry is not configured', last_notice())
+  end)
+
+  t.it('still syncs plugins that carry their own url', function()
+    if not have('curl') then
+      return
+    end
+    use_env()
+    local before = #env.notices
+    arti.setup({
+      auto_sync = false,
+      plugins = { plugin_entry('owner/thing.nvim', 'v1.0.0', tmpdir() .. '/absent.zip') },
+    })
+    arti.sync()
+    drain(before)
+
+    t.falsy(notices():find('registry', 1, true), 'a url-only run never asks for a registry')
+  end)
+
+  t.it('cleans without one', function()
+    use_env()
+    arti.setup({ auto_sync = false, plugins = {} })
+    install_fake('drop.nvim', 'start')
+
+    arti.clean()
+
+    t.matches('removed drop.nvim', last_notice())
+  end)
+
+  t.it('lists without one', function()
+    use_env()
+    arti.setup({ auto_sync = false, plugins = { { 'owner/here.nvim', tag = 'v1' } } })
+    install_fake('here.nvim', 'start', 'v1')
+
+    t.matches('installed$', arti.list()[1])
+  end)
+end)
+
 t.describe('arti-pack.sync', function()
   if not have('zip') or not have('unzip') then
     t.it('SKIPPED: zip/unzip unavailable', function() end)

tests/spec_spec.lua

@@ -101,9 +101,31 @@ t.describe('spec.url', function()
 
   t.it('errors when no registry is configured', function()
     local s = assert(spec.parse({ 'owner/repo', tag = 'v1' }))
-    local url, err = spec.url(s, '')
+    local url, err = spec.url(s, nil)
     t.falsy(url)
-    t.matches('registry', err)
+    t.matches('registry is not configured', err)
+    t.matches("setup{ registry = 'https://", err, 'the error says how to set it')
+  end)
+
+  t.it('errors instead of handing a bad registry to curl', function()
+    local s = assert(spec.parse({ 'owner/repo', tag = 'v1' }))
+    for _, bad in ipairs({ '', 'https://', 'artifacts.example.com/gh', 'ftp://artifacts.example.com', 42 }) do
+      local url, err = spec.url(s, bad)
+      t.falsy(url, 'rejects ' .. vim.inspect(bad))
+      t.matches('is not an http%(s%) URL', err)
+    end
+  end)
+end)
+
+t.describe('spec.check_registry', function()
+  t.it('returns the registry with trailing slashes stripped', function()
+    t.eq(REGISTRY, spec.check_registry(REGISTRY))
+    t.eq(REGISTRY, spec.check_registry(REGISTRY .. '/'))
+    t.eq(REGISTRY, spec.check_registry(REGISTRY .. '///'))
+  end)
+
+  t.it('accepts a plain http registry', function()
+    t.eq('http://artifacts.example.com/gh', spec.check_registry('http://artifacts.example.com/gh'))
   end)
 end)