install: Prompt when multiple remotes match

Currently if the user doesn't specify a remote name to the install
command, Flatpak chooses the first one that has a matching ref, either
with or without user interaction depending on whether --assumeyes/-y was
used. Then if more than one remote ref is similar to the one specified,
the user is prompted to choose between them. I think this asymmetry
doesn't make much sense; why not have the user choose between remotes
just as they choose between refs?

For example, the string "libre" matches refs from both flathub and
eos-apps, so if I do "flatpak install -y libre" a remote is arbitrarily
chosen for me but I'm still prompted to choose between the refs (since
-y can't choose for you when there are mutliple choices).

So this commit changes the install command implementation to present the
user with a list of remotes that have matching refs all at once rather
than one at a time. The reason I didn't implement it this way at first
is that I was worried about the performance impact, but in testing I'm
not able to measure a difference between stopping at the first remote
with a match and checking every remote (although the story might be
different if you have many remotes or don't have up-to-date metadata
downloaded).

If there's an error searching a remote, it's treated as a warning so
that one misconfigured remote doesn't take down the whole operation with
it.

This commit also updates the unit tests so they continue to succeed.

Closes: #2288
Approved by: matthiasclasen

4 files changed, 111 insertions, 27 deletions

diff --git a/app/flatpak-builtins-install.c b/app/flatpak-builtins-install.c
index bda1cc9c..506b2ca8 100644
--- a/app/flatpak-builtins-install.c
+++ b/app/flatpak-builtins-install.c
@@ -235,7 +235,7 @@ flatpak_builtin_install (int argc, char **argv, GCancellable *cancellable, GErro
 {
   g_autoptr(GOptionContext) context = NULL;
   g_autoptr(GPtrArray) dirs = NULL;
-  FlatpakDir *dir;
+  g_autoptr(FlatpakDir) dir = NULL;
   g_autofree char *remote = NULL;
   g_autofree char *remote_url = NULL;
   char **prefs = NULL;
@@ -256,7 +256,7 @@ flatpak_builtin_install (int argc, char **argv, GCancellable *cancellable, GErro
     return FALSE;
 
   /* Start with the default or specified dir, this is fine for opt_bundle or opt_from */
-  dir = g_ptr_array_index (dirs, 0);
+  dir = g_object_ref (g_ptr_array_index (dirs, 0));
 
   if (!opt_bundle && !opt_from && argc >= 2)
     {
@@ -312,13 +312,16 @@ flatpak_builtin_install (int argc, char **argv, GCancellable *cancellable, GErro
       if (!auto_remote)
         {
           remote = g_strdup (argv[1]);
-          dir = dir_with_remote;
+          dir = g_object_ref (dir_with_remote);
         }
       else
         {
-          gboolean found_remote = FALSE;
+          g_autoptr(GPtrArray) remote_dir_pairs = NULL;
+          RemoteDirPair *chosen_pair = NULL;
+
+          remote_dir_pairs = g_ptr_array_new_with_free_func ((GDestroyNotify) remote_dir_pair_free);
 
-          /* Try to find a remote with a matching ref. This is imperfect
+          /* Search all remotes for a matching ref. This is imperfect
            * because it only takes the first specified ref into account and
            * doesn't distinguish between an exact match and a fuzzy match, but
            * that's okay because the user will be asked to confirm the remote
@@ -342,6 +345,7 @@ flatpak_builtin_install (int argc, char **argv, GCancellable *cancellable, GErro
                   g_autofree char *branch = NULL;
                   FlatpakKinds matched_kinds;
                   g_auto(GStrv) refs = NULL;
+                  g_autoptr(GError) local_error = NULL;
 
                   if (flatpak_dir_get_remote_disabled (this_dir, this_remote))
                     continue;
@@ -355,42 +359,37 @@ flatpak_builtin_install (int argc, char **argv, GCancellable *cancellable, GErro
                     refs = flatpak_dir_find_local_refs (this_dir, this_remote, id, branch, this_default_branch, arch,
                                                         flatpak_get_default_arch (),
                                                         matched_kinds, FIND_MATCHING_REFS_FLAGS_FUZZY,
-                                                        cancellable, error);
+                                                        cancellable, &local_error);
                   else
                     refs = flatpak_dir_find_remote_refs (this_dir, this_remote, id, branch, this_default_branch, arch,
                                                          flatpak_get_default_arch (),
                                                          matched_kinds, FIND_MATCHING_REFS_FLAGS_FUZZY,
-                                                         cancellable, error);
+                                                         cancellable, &local_error);
 
                   if (refs == NULL)
-                    return FALSE;
+                    {
+                      g_warning ("An error was encountered searching remote ‘%s’ for ‘%s’: %s", this_remote, argv[1], local_error->message);
+                      continue;
+                    }
 
                   if (g_strv_length (refs) == 0)
                     continue;
                   else
                     {
-                      if (!flatpak_resolve_duplicate_remotes (dirs, this_remote, &dir_with_remote, cancellable, error))
-                        return FALSE;
-
-                      if (!opt_yes &&
-                          !flatpak_yes_no_prompt (TRUE, /* default to yes on Enter */
-                                                  _("Found refs similar to ‘%s’ in remote ‘%s’ (%s).\nIs that the remote you want to use?"),
-                                                  argv[1], this_remote, flatpak_dir_get_name (dir_with_remote)))
-                        continue;
-
-                      remote = g_strdup (this_remote);
-                      dir = dir_with_remote;
-                      found_remote = TRUE;
-                      break;
+                      RemoteDirPair *pair = remote_dir_pair_new (this_remote, this_dir);
+                      g_ptr_array_add (remote_dir_pairs, pair);
                     }
                 }
-
-                if (found_remote)
-                  break;
             }
 
-            if (remote == NULL)
-              return flatpak_fail (error, _("No remote selected to resolve matches for ‘%s’"), argv[1]);
+            if (remote_dir_pairs->len == 0)
+              return flatpak_fail (error, _("No remote refs found similar to ‘%s’"), argv[1]);
+
+            if (!flatpak_resolve_matching_remotes (opt_yes, remote_dir_pairs, argv[1], &chosen_pair, error))
+              return FALSE;
+
+            remote = g_strdup (chosen_pair->remote_name);
+            dir = g_object_ref (chosen_pair->dir);
         }
     }
 
diff --git a/app/flatpak-builtins-utils.c b/app/flatpak-builtins-utils.c
index 4449c5a7..deaa358e 100644
--- a/app/flatpak-builtins-utils.c
+++ b/app/flatpak-builtins-utils.c
@@ -30,6 +30,24 @@
 #include "flatpak-utils-private.h"
 
 
+void
+remote_dir_pair_free (RemoteDirPair *pair)
+{
+  g_free (pair->remote_name);
+  g_object_unref (pair->dir);
+  g_free (pair);
+}
+
+RemoteDirPair *
+remote_dir_pair_new (const char *remote_name, FlatpakDir *dir)
+{
+  RemoteDirPair *pair = g_new (RemoteDirPair, 1);
+
+  pair->remote_name = g_strdup (remote_name);
+  pair->dir = g_object_ref (dir);
+  return pair;
+}
+
 gboolean
 looks_like_branch (const char *branch)
 {
@@ -435,6 +453,40 @@ flatpak_resolve_matching_refs (gboolean      disable_interaction,
   return TRUE;
 }
 
+gboolean
+flatpak_resolve_matching_remotes (gboolean        disable_interaction,
+                                  GPtrArray      *remote_dir_pairs,
+                                  const char     *opt_search_ref,
+                                  RemoteDirPair **out_pair,
+                                  GError        **error)
+{
+  guint chosen = 0; /* 1 indexed */
+  guint i;
+
+  g_assert (remote_dir_pairs->len > 0);
+
+  if (disable_interaction && remote_dir_pairs->len == 1)
+    chosen = 1;
+
+  if (chosen == 0)
+    {
+      g_print (_("Multiple remotes found with refs similar to ‘%s’:\n"), opt_search_ref);
+      for (i = 0; i < remote_dir_pairs->len; i++)
+        {
+          RemoteDirPair *pair = g_ptr_array_index (remote_dir_pairs, i);
+          g_print ("%d) %s (%s)\n", i + 1, pair->remote_name, flatpak_dir_get_name (pair->dir));
+        }
+      chosen = flatpak_number_prompt (0, remote_dir_pairs->len, _("Which do you want to use (0 to abort)?"));
+      if (chosen == 0)
+        return flatpak_fail (error, _("No remote chosen to resolve matches for ‘%s’"), opt_search_ref);
+    }
+
+  if (out_pair)
+    *out_pair = g_ptr_array_index (remote_dir_pairs, chosen - 1);
+
+  return TRUE;
+}
+
 /* Returns: the time in seconds since the file was modified, or %G_MAXUINT64 on error */
 static guint64
 get_file_age (GFile *file)
diff --git a/app/flatpak-builtins-utils.h b/app/flatpak-builtins-utils.h
index a8c954fe..6a842d68 100644
--- a/app/flatpak-builtins-utils.h
+++ b/app/flatpak-builtins-utils.h
@@ -30,6 +30,16 @@
 /* Appstream data expires after a day */
 #define FLATPAK_APPSTREAM_TTL 86400
 
+typedef struct RemoteDirPair
+{
+  gchar              *remote_name;
+  FlatpakDir         *dir;
+} RemoteDirPair;
+
+void            remote_dir_pair_free (RemoteDirPair *pair);
+RemoteDirPair * remote_dir_pair_new (const char *remote_name,
+                                     FlatpakDir *dir);
+
 gboolean    looks_like_branch (const char *branch);
 GBytes *    download_uri (const char *url,
                           GError    **error);
@@ -62,6 +72,12 @@ gboolean flatpak_resolve_matching_refs (gboolean    disable_interaction,
                                         char      **out_ref,
                                         GError    **error);
 
+gboolean flatpak_resolve_matching_remotes (gboolean        disable_interaction,
+                                           GPtrArray      *remote_dir_pairs,
+                                           const char     *opt_search_ref,
+                                           RemoteDirPair **out_pair,
+                                           GError        **error);
+
 gboolean update_appstream (GPtrArray    *dirs,
                            const char   *remote,
                            const char   *arch,
diff --git a/tests/test-repo.sh b/tests/test-repo.sh
index 906c435a..8b532f86 100755
--- a/tests/test-repo.sh
+++ b/tests/test-repo.sh
@@ -143,13 +143,30 @@ echo "ok typo correction works for install"
 
 ${FLATPAK} ${U} uninstall -y org.test.Hello
 
+# Temporarily disable some remotes so that org.test.Hello only exists in one
+${FLATPAK} ${U} remote-modify --disable test-missing-gpg-repo
+${FLATPAK} ${U} remote-modify --disable test-wrong-gpg-repo
+${FLATPAK} ${U} remote-modify --disable test-gpg2-repo
+${FLATPAK} ${U} remote-modify --disable local-test-no-gpg-repo
+if [ x${USE_COLLECTIONS_IN_CLIENT-} != xyes ] ; then
+    ${FLATPAK} ${U} remote-modify --disable test-no-gpg-repo
+fi
+
 # Note: The missing remote is only auto-corrected without user interaction because we're using -y
-${FLATPAK} ${U} install -y org.test.Hello >install-log
+${FLATPAK} ${U} install -y org.test.Hello |& tee install-log
 assert_file_has_content install-log "org.test.Hello"
 
 ${FLATPAK} ${U} list -d > list-log
 assert_file_has_content list-log "^org.test.Hello"
 
+${FLATPAK} ${U} remote-modify --enable test-missing-gpg-repo
+${FLATPAK} ${U} remote-modify --enable test-wrong-gpg-repo
+${FLATPAK} ${U} remote-modify --enable test-gpg2-repo
+${FLATPAK} ${U} remote-modify --enable local-test-no-gpg-repo
+if [ x${USE_COLLECTIONS_IN_CLIENT-} != xyes ] ; then
+    ${FLATPAK} ${U} remote-modify --enable test-no-gpg-repo
+fi
+
 echo "ok missing remote name auto-corrects for install"
 
 ${FLATPAK} ${U} uninstall -y org.test.Platform org.test.Hello