Make discovery explicit (#439)

* Make discovery explicit

Fix #438

docs/tutorial.rst

@@ -89,12 +89,12 @@ default addressbook to ``~/.contacts/``::
 
     Configuration for other servers can be found at :ref:`supported-servers`.
 
-After running ``vdirsyncer sync``, ``~/.contacts/`` will contain a bunch of
+After running ``vdirsyncer discover`` and ``vdirsyncer sync``, ``~/.contacts/``
-``.vcf`` files which all contain a contact in ``VCARD`` format each. You can
+will contain a bunch of ``.vcf`` files which all contain a contact in ``VCARD``
-modify their content, add new ones and delete some [1]_, and your changes will be
+format each. You can modify their content, add new ones and delete some [1]_,
-synchronized to the CalDAV server after you run ``vdirsyncer sync`` again. For
+and your changes will be synchronized to the CalDAV server after you run
-further reference, it uses the storages :storage:`filesystem` and
+``vdirsyncer sync`` again. For further reference, it uses the storages
-:storage:`carddav`.
+:storage:`filesystem` and :storage:`carddav`.
 
 .. [1] You'll want to :doc:`use a helper program for this <supported>`.
 
@@ -145,15 +145,16 @@ line to let vdirsyncer automatically sync all addressbooks it can find::
     username = bob
     password = asdf
 
-With the above configuration, vdirsyncer will fetch all available collections
+With the above configuration, ``vdirsyncer discover`` will fetch all available
-from the server, and create subdirectories for each of them in
+collections from the server, and create subdirectories for each of them in
-``~/.contacts/``. For example, ownCloud's default addressbook ``"default"``
+``~/.contacts/`` after confirmation. For example, ownCloud's default
-would be synchronized to the location ``~/.contacts/default/``.
+addressbook ``"default"`` would be synchronized to the location
+``~/.contacts/default/``.
 
-Vdirsyncer fetches this list on first sync, and will re-fetch it if you change
+After that, ``vdirsyncer sync`` will synchronize all your addressbooks as
-your configuration file. However, if new collections are created on the server,
+expected. However, if new collections are created on the server, it will not
-it will not automatically start synchronizing those [2]_. You should run
+automatically start synchronizing those [2]_. You need to run ``vdirsyncer
-``vdirsyncer discover`` to re-fetch this list instead.
+discover`` again to re-fetch this list instead.
 
 .. [2] Because collections are added rarely, and checking for this case before
    every synchronization isn't worth the overhead.

tests/cli/test_discover.py

@@ -27,18 +27,13 @@ def test_discover_command(tmpdir, runner):
         bar.mkdir(x)
     bar.mkdir('d')
 
-    result = runner.invoke(['sync'])
+    result = runner.invoke(['discover'])
     assert not result.exception
-    lines = result.output.splitlines()
-    assert lines[0].startswith('Discovering')
-    assert 'Syncing foobar/a' in lines
-    assert 'Syncing foobar/b' in lines
-    assert 'Syncing foobar/c' in lines
-    assert 'Syncing foobar/d' not in lines
 
     foo.mkdir('d')
     result = runner.invoke(['sync'])
     assert not result.exception
+    lines = result.output.splitlines()
     assert 'Syncing foobar/a' in lines
     assert 'Syncing foobar/b' in lines
     assert 'Syncing foobar/c' in lines

tests/cli/test_sync.py

@@ -4,8 +4,6 @@ import json
 import unicodedata
 from textwrap import dedent
 
-from click.testing import CliRunner
-
 from hypothesis import example, given
 import hypothesis.strategies as st
 
@@ -36,6 +34,9 @@ def test_simple_run(tmpdir, runner):
     tmpdir.mkdir('path_a')
     tmpdir.mkdir('path_b')
 
+    result = runner.invoke(['discover'])
+    assert not result.exception
+
     result = runner.invoke(['sync'])
     assert not result.exception
 
@@ -47,6 +48,7 @@ def test_simple_run(tmpdir, runner):
 
 def test_sync_inexistant_pair(tmpdir, runner):
     runner.write_with_general("")
+
     result = runner.invoke(['sync', 'foo'])
     assert result.exception
     assert 'pair foo does not exist.' in result.output.lower()
@@ -73,6 +75,9 @@ def test_debug_connections(tmpdir, runner):
     tmpdir.mkdir('path_a')
     tmpdir.mkdir('path_b')
 
+    result = runner.invoke(['discover'])
+    assert not result.exception
+
     result = runner.invoke(['-vdebug', 'sync', '--max-workers=3'])
     assert 'using 3 maximal workers' in result.output.lower()
 
@@ -101,6 +106,9 @@ def test_empty_storage(tmpdir, runner):
     tmpdir.mkdir('path_a')
     tmpdir.mkdir('path_b')
 
+    result = runner.invoke(['discover'])
+    assert not result.exception
+
     result = runner.invoke(['sync'])
     assert not result.exception
 
@@ -116,15 +124,9 @@ def test_empty_storage(tmpdir, runner):
     assert result.exception
 
 
-def test_verbosity(tmpdir):
+def test_verbosity(tmpdir, runner):
-    runner = CliRunner()
+    runner.write_with_general('')
-    config_file = tmpdir.join('config')
+    result = runner.invoke(['--verbosity=HAHA', 'sync'])
-    config_file.write('')
-
-    result = runner.invoke(
-        cli.app, ['--verbosity=HAHA', 'sync'],
-        env={'VDIRSYNCER_CONFIG': str(config_file)}
-    )
     assert result.exception
     assert 'invalid value for "--verbosity"' in result.output.lower()
 
@@ -151,6 +153,12 @@ def test_deprecated_item_status(tmpdir):
 
 
 def test_collections_cache_invalidation(tmpdir, runner):
+    foo = tmpdir.mkdir('foo')
+    bar = tmpdir.mkdir('bar')
+    for x in 'abc':
+        foo.mkdir(x)
+        bar.mkdir(x)
+
     runner.write_with_general(dedent('''
     [storage foo]
     type = filesystem
@@ -168,13 +176,11 @@ def test_collections_cache_invalidation(tmpdir, runner):
     collections = ["a", "b", "c"]
     ''').format(str(tmpdir)))
 
-    foo = tmpdir.mkdir('foo')
-    bar = tmpdir.mkdir('bar')
-    for x in 'abc':
-        foo.mkdir(x)
-        bar.mkdir(x)
     foo.join('a/itemone.txt').write('UID:itemone')
 
+    result = runner.invoke(['discover'])
+    assert not result.exception
+
     result = runner.invoke(['sync'])
     assert not result.exception
     assert 'detected change in config file' not in result.output.lower()
@@ -208,6 +214,12 @@ def test_collections_cache_invalidation(tmpdir, runner):
         bar2.mkdir(x)
     result = runner.invoke(['sync'])
     assert 'detected change in config file' in result.output.lower()
+    assert result.exception
+
+    result = runner.invoke(['discover'])
+    assert not result.exception
+
+    result = runner.invoke(['sync'])
     assert not result.exception
 
     rv = bar.join('a').listdir()
@@ -239,6 +251,9 @@ def test_invalid_pairs_as_cli_arg(tmpdir, runner):
         for c in 'abc':
             base.mkdir(c)
 
+    result = runner.invoke(['discover'])
+    assert not result.exception
+
     result = runner.invoke(['sync', 'foobar/d'])
     assert result.exception
     assert 'pair foobar: collection d not found' in result.output.lower()
@@ -258,19 +273,25 @@ def test_multiple_pairs(tmpdir, runner):
                 yield dedent('''
                 [storage {name}]
                 type = filesystem
-                path = {base}/{name}/
+                path = {path}
                 fileext = .txt
-                ''').format(name=name, base=str(tmpdir))
+                ''').format(name=name, path=str(tmpdir.mkdir(name)))
 
     runner.write_with_general(''.join(get_cfg()))
 
-    result = runner.invoke(['sync'])
+    result = runner.invoke(['discover'])
     assert set(result.output.splitlines()) > set([
         'Discovering collections for pair bambaz',
-        'Discovering collections for pair foobar',
+        'Discovering collections for pair foobar'
+    ])
+    assert not result.exception
+
+    result = runner.invoke(['sync'])
+    assert set(result.output.splitlines()) == set([
         'Syncing bambaz',
         'Syncing foobar',
     ])
+    assert not result.exception
 
 
 @given(collections=st.sets(
@@ -311,13 +332,8 @@ def test_create_collections(subtest, collections):
         fileext = .txt
         '''.format(base=str(tmpdir), colls=json.dumps(list(collections)))))
 
-        result = runner.invoke(['sync'])
-        assert result.exception
-        entries = set(x.basename for x in tmpdir.listdir())
-        assert 'foo' not in entries and 'bar' not in entries
-
         result = runner.invoke(
-            ['sync'],
+            ['discover'],
             input='y\n' * 2 * (len(collections) + 1)
         )
         assert not result.exception
@@ -369,6 +385,9 @@ def test_ident_conflict(tmpdir, runner):
     foo.join('two.txt').write('UID:1')
     foo.join('three.txt').write('UID:1')
 
+    result = runner.invoke(['discover'])
+    assert not result.exception
+
     result = runner.invoke(['sync'])
     assert result.exception
     assert ('error: foobar: Storage "foo" contains multiple items with the '
@@ -399,7 +418,7 @@ def test_unknown_storage(tmpdir, runner, existing, missing):
 
     tmpdir.mkdir(existing)
 
-    result = runner.invoke(['sync'])
+    result = runner.invoke(['discover'])
     assert result.exception
 
     assert (

vdirsyncer/cli/__init__.py

@@ -208,7 +208,7 @@ def discover(ctx, pairs, max_workers, list):
                 discover_collections,
                 status_path=config.general['status_path'],
                 pair=pair,
-                skip_cache=True,
+                from_cache=False,
                 list_collections=list,
             ))
             wq.spawn_worker()

vdirsyncer/cli/utils.py

@@ -156,27 +156,32 @@ def _get_collections_cache_key(pair):
     return m.hexdigest()
 
 
-def collections_for_pair(status_path, pair, skip_cache=False,
+def collections_for_pair(status_path, pair, from_cache=True,
                          list_collections=False):
     '''Determine all configured collections for a given pair. Takes care of
     shortcut expansion and result caching.
 
     :param status_path: The path to the status directory.
-    :param skip_cache: Whether to skip the cached data and always do discovery.
+    :param from_cache: Whether to load from cache (aborting on cache miss) or
-        Even with this option enabled, the new cache is written.
+        discover and save to cache.
 
     :returns: iterable of (collection, (a_args, b_args))
     '''
-    rv = load_status(status_path, pair.name, data_type='collections')
     cache_key = _get_collections_cache_key(pair)
-    if rv and not skip_cache:
+    if from_cache:
-        if rv.get('cache_key', None) == cache_key:
+        rv = load_status(status_path, pair.name, data_type='collections')
+        if rv and rv.get('cache_key', None) == cache_key:
             return list(_expand_collections_cache(
                 rv['collections'], pair.config_a, pair.config_b
             ))
         elif rv:
-            cli_logger.info('Detected change in config file, discovering '
+            raise exceptions.UserError('Detected change in config file, '
-                            'collections for {}'.format(pair.name))
+                                       'please run `vdirsyncer discover {}`.'
+                                       .format(pair.name))
+        else:
+            raise exceptions.UserError('Please run `vdirsyncer discover {}` '
+                                       ' before synchronization.'
+                                       .format(pair.name))
 
     cli_logger.info('Discovering collections for pair {}'
                     .format(pair.name))