Refactor UX of discovery (#437)

* Refactor UX of discovery

- The new `--list` option for `vdirsyncer discover`, enabled by default,
  makes vdirsyncer list all collections before saving the configuration.
  It is not enabled when vdirsyncer tries to discover when invoked via
  `vdirsyncer sync`. Fix #424

- There are no duplicate collections anymore. Fix #419.

- `vdirsyncer discover` is running with one worker by default now, to
  avoid broken output. See also #404.

vdirsyncer/cli/__init__.py

@@ -74,13 +74,19 @@ def max_workers_callback(ctx, param, value):
     return value
 
 
-max_workers_option = click.option(
-    '--max-workers', default=0, type=click.IntRange(min=0, max=None),
-    callback=max_workers_callback,
-    help=('Use at most this many connections. With debug messages enabled, '
-          'the default is 1, otherwise one connection per collection is '
-          'opened.')
-)
+def max_workers_option(default=0):
+    help = 'Use at most this many connections. '
+    if default == 0:
+        help += 'The default is 0, which means "as many as necessary". ' \
+                'With -vdebug enabled, the default is 1.'
+    else:
+        help += 'The default is {}.'.format(default)
+
+    return click.option(
+        '--max-workers', default=default, type=click.IntRange(min=0, max=None),
+        callback=max_workers_callback,
+        help=help
+    )
 
 
 def collections_arg_callback(ctx, param, value):
@@ -112,7 +118,7 @@ collections_arg = click.argument('collections', nargs=-1,
 @click.option('--force-delete/--no-force-delete',
               help=('Do/Don\'t abort synchronization when all items are about '
                     'to be deleted from both sides.'))
-@max_workers_option
+@max_workers_option()
 @pass_context
 @catch_errors
 def sync(ctx, collections, force_delete, max_workers):
@@ -149,7 +155,7 @@ def sync(ctx, collections, force_delete, max_workers):
 
 @app.command()
 @collections_arg
-@max_workers_option
+@max_workers_option()
 @pass_context
 @catch_errors
 def metasync(ctx, collections, max_workers):
@@ -174,10 +180,18 @@ def metasync(ctx, collections, max_workers):
 
 @app.command()
 @click.argument('pairs', nargs=-1)
-@max_workers_option
+@click.option(
+    '--list/--no-list', default=True,
+    help=(
+        'Whether to list all collections from both sides during discovery, '
+        'for debugging. This is quite slow. For faster discovery, disable '
+        'with --no-list.'
+    )
+)
+@max_workers_option(default=1)
 @pass_context
 @catch_errors
-def discover(ctx, pairs, max_workers):
+def discover(ctx, pairs, max_workers, list):
     '''
     Refresh collection cache for the given pairs.
     '''
@@ -195,6 +209,7 @@ def discover(ctx, pairs, max_workers):
                 status_path=config.general['status_path'],
                 pair=pair,
                 skip_cache=True,
+                list_collections=list,
             ))
             wq.spawn_worker()
 

vdirsyncer/cli/utils.py

@@ -156,7 +156,8 @@ 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, skip_cache=False,
+                         list_collections=False):
     '''Determine all configured collections for a given pair. Takes care of
     shortcut expansion and result caching.
 
@@ -182,7 +183,8 @@ def collections_for_pair(status_path, pair, skip_cache=False):
 
     # We have to use a list here because the special None/null value would get
     # mangled to string (because JSON objects always have string keys).
-    rv = list(_collections_for_pair_impl(status_path, pair))
+    rv = list(_collections_for_pair_impl(status_path, pair,
+                                         list_collections=list_collections))
 
     save_status(status_path, pair.name, data_type='collections',
                 data={
@@ -267,7 +269,27 @@ def _handle_collection_not_found(config, collection, e=None):
                            storage=storage_name))
 
 
-def _collections_for_pair_impl(status_path, pair):
+def _print_collections(base_config, discovered):
+    instance_name = base_config['instance_name']
+    cli_logger.info('{}:'.format(coerce_native(instance_name)))
+    for args in discovered.values():
+        args['instance_name'] = instance_name
+        try:
+            storage = storage_instance_from_config(args)
+            displayname = storage.get_meta('displayname')
+        except Exception:
+            displayname = u''
+
+        cli_logger.info('  - {}{}'.format(
+            storage.collection,
+            ' ("{}")'.format(coerce_native(displayname))
+            if displayname and displayname != storage.collection
+            else ''
+        ))
+
+
+def _collections_for_pair_impl(status_path, pair, list_collections=False):
+    handled_collections = set()
 
     shortcuts = pair.options['collections']
     if shortcuts is None:
@@ -276,6 +298,10 @@ def _collections_for_pair_impl(status_path, pair):
         a_discovered = _discover_from_config(pair.config_a)
         b_discovered = _discover_from_config(pair.config_b)
 
+        if list_collections:
+            _print_collections(pair.config_a, a_discovered)
+            _print_collections(pair.config_b, b_discovered)
+
         for shortcut in shortcuts:
             if shortcut == 'from a':
                 collections = a_discovered
@@ -296,6 +322,10 @@ def _collections_for_pair_impl(status_path, pair):
                 else:
                     collection_a = collection_b = collection
 
+                if collection in handled_collections:
+                    continue
+                handled_collections.add(collection)
+
                 try:
                     a_args = a_discovered[collection_a]
                 except KeyError: