rebase: Remove support for providing prefixes

This mainly undoes
coreos@2c270a6,
and follows up on coreos#2842.
Also serves as some cleanup to make way for the introduction of a
new `container-image-reference` refspec type.

We no longer have any use for `ostree://` or `rojig://`-style
prefixes in the refspecs. There had been efforts to "canonicalize"
refspecs to always include such prefixes into the code internally,
but that hasn't gotten too far. Crucially, since such prefixes were
never introduced to libostree, the refspecs in the origin file
were never meant to contain such prefixes anyway.

Following some discussion in
coreos#2940 (comment),
in the future we would disambiguate "refspec type" via the name of
the key in the origin file instead.

rust/src/lib.rs

@@ -1,4 +1,4 @@
-//! rpm-ostree a hybrid Rust and C/C++ application.  This is the
+//! rpm-ostree is a hybrid Rust and C/C++ application. This is the
 //! main library used by the executable, which also links to the
 //! C/C++ `librpmostreeinternals.a` static library.
 

src/app/rpmostree-builtin-rebase.cxx

@@ -118,44 +118,39 @@ rpmostree_builtin_rebase (int             argc,
       if (argc == 3)
         revision = argv[2];
 
-      /* Canonicalize */
-      new_provided_refspec = new_refspec_owned =
-        rpmostree_refspec_canonicalize (new_provided_refspec, error);
-      if (!new_provided_refspec)
-        return FALSE;
+      new_provided_refspec = new_refspec_owned = g_strdup (new_provided_refspec);
     }
   else
     {
       if (opt_remote)
         {
           new_provided_refspec = new_refspec_owned =
-            g_strconcat (RPMOSTREE_REFSPEC_OSTREE_PREFIX, opt_remote, ":", opt_branch ?: "", NULL);
+            g_strconcat (opt_remote, ":", opt_branch ?: "", NULL);
         }
       else
         new_provided_refspec = opt_branch;
     }
   (void)new_refspec_owned; /* Pacify static analysis */
 
-  const char *remainder = NULL;
   RpmOstreeRefspecType refspectype;
-  if (!rpmostree_refspec_classify (new_provided_refspec, &refspectype, &remainder, error))
+  if (!rpmostree_refspec_classify (new_provided_refspec, &refspectype, error))
     return FALSE;
 
-  /* catch "ostree://"; we'd error out much later in the daemon otherwise */
-  if (strlen (remainder) == 0)
+  /* catch empty refspec now; we'd error out much later in the daemon otherwise */
+  if (strlen (new_provided_refspec) == 0)
     return glnx_throw (error, "Refspec is empty");
 
   /* Check if remote refers to a local repo */
   g_autofree char *local_repo_remote = NULL;
   if (G_IN_SET (refspectype, RPMOSTREE_REFSPEC_TYPE_OSTREE,
                              RPMOSTREE_REFSPEC_TYPE_CHECKSUM))
     {
-      if (*remainder == '/')
+      if (*new_provided_refspec == '/')
         {
-          const char *ref = strrchr (remainder, ':');
+          const char *ref = strrchr (new_provided_refspec, ':');
           if (!ref)
             return glnx_throw (error, "Missing ':' in LOCALPATH:REF rebase");
-          local_repo_remote = g_strndup (remainder, ref - remainder);
+          local_repo_remote = g_strndup (new_provided_refspec, ref - new_provided_refspec);
           new_provided_refspec = ref + 1;
           /* just don't support "/path/to/repo:" for now */
           if (strlen (new_provided_refspec) == 0)

src/app/rpmostree-builtin-status.cxx

@@ -611,10 +611,8 @@ print_one_deployment (RPMOSTreeSysroot *sysroot_proxy,
   const char *custom_origin_description = NULL;
   if (origin_refspec)
     {
-      const char *refspec_data;
-      if (!rpmostree_refspec_classify (origin_refspec, &refspectype, &refspec_data, error))
+      if (!rpmostree_refspec_classify (origin_refspec, &refspectype, error))
         return FALSE;
-      g_autofree char *canonrefspec = rpmostree_refspec_to_string (refspectype, refspec_data);
       switch (refspectype)
         {
         case RPMOSTREE_REFSPEC_TYPE_CHECKSUM:
@@ -631,12 +629,12 @@ print_one_deployment (RPMOSTreeSysroot *sysroot_proxy,
                 g_print ("%s", custom_origin_url);
               }
             else
-              g_print ("%s", canonrefspec);
+              g_print ("%s", origin_refspec);
           }
           break;
         case RPMOSTREE_REFSPEC_TYPE_OSTREE:
           {
-            g_print ("%s", canonrefspec);
+            g_print ("%s", origin_refspec);
           }
           break;
         }

src/daemon/rpmostreed-deployment-utils.cxx

@@ -406,10 +406,8 @@ add_all_commit_details_to_vardict (OstreeDeployment *deployment,
     }
   else
     {
-      const char *refspec_remainder = NULL;
-      if (!rpmostree_refspec_classify (refspec, &refspec_type, &refspec_remainder, error))
+      if (!rpmostree_refspec_classify (refspec, &refspec_type, error))
         return FALSE;
-      refspec = refspec_remainder;
     }
   (void)refspec_owned; /* Pacify static analysis */
   refspec_is_ostree = refspec_type == RPMOSTREE_REFSPEC_TYPE_OSTREE;
@@ -800,12 +798,8 @@ rpmostreed_update_generate_variant (OstreeDeployment  *booted_deployment,
 
   const char *refspec = rpmostree_origin_get_refspec (origin);
   { RpmOstreeRefspecType refspectype = RPMOSTREE_REFSPEC_TYPE_OSTREE;
-    const char *refspec_data;
-    if (!rpmostree_refspec_classify (refspec, &refspectype, &refspec_data, error))
+    if (!rpmostree_refspec_classify (refspec, &refspectype, error))
       return FALSE;
-
-    /* just skip over "ostree://" so we can talk with libostree without thinking about it */
-    refspec = refspec_data;
   }
 
   /* let's start with the ostree side of things */

src/daemon/rpmostreed-transaction-types.cxx

@@ -53,39 +53,28 @@ static gboolean
 change_origin_refspec (GVariantDict    *options,
                        OstreeSysroot *sysroot,
                        RpmOstreeOrigin *origin,
-                       const gchar *src_refspec,
+                       const gchar *refspec,
                        GCancellable *cancellable,
                        gchar **out_old_refspec,
                        gchar **out_new_refspec,
                        GError **error)
 {
-  RpmOstreeRefspecType refspectype;
-  const char *refspecdata;
-  if (!rpmostree_refspec_classify (src_refspec, &refspectype, &refspecdata, error))
-    return FALSE;
-
   RpmOstreeRefspecType current_refspectype;
-  const char *current_refspecdata;
-  rpmostree_origin_classify_refspec (origin, &current_refspectype, &current_refspecdata);
-
-  /* Now here we "peel" it since the rest of the code assumes libostree */
-  const char *refspec = refspecdata;
-
-  g_autofree gchar *current_refspec =
-    g_strdup (rpmostree_origin_get_refspec (origin));
+  g_autofree gchar *current_refspec = rpmostree_origin_get_full_refspec (origin, &current_refspectype);
+
   g_autofree gchar *new_refspec = NULL;
-
   if (!rpmostreed_refspec_parse_partial (refspec,
                                          current_refspec,
                                          &new_refspec,
                                          error))
     return FALSE;
 
-  /* Re-classify after canonicalization to ensure we handle TYPE_CHECKSUM */
-  if (!rpmostree_refspec_classify (new_refspec, &refspectype, &refspecdata, error))
+  /* Classify to ensure we handle TYPE_CHECKSUM */
+  RpmOstreeRefspecType new_refspectype;
+  if (!rpmostree_refspec_classify (new_refspec, &new_refspectype, error))
     return FALSE;
 
-  if (refspectype == RPMOSTREE_REFSPEC_TYPE_CHECKSUM)
+  if (new_refspectype == RPMOSTREE_REFSPEC_TYPE_CHECKSUM)
     {
       const char *custom_origin_url = NULL;
       const char *custom_origin_description = NULL;
@@ -1041,16 +1030,15 @@ deploy_transaction_execute (RpmostreedTransaction *transaction,
       g_assert (self->refspec);
 
       RpmOstreeRefspecType refspectype;
-      const char *ref;
-      if (!rpmostree_refspec_classify (self->refspec, &refspectype, &ref, error))
+      if (!rpmostree_refspec_classify (self->refspec, &refspectype, error))
         return FALSE;
 
       g_autoptr(OstreeRepo) local_repo_remote =
         ostree_repo_open_at (self->local_repo_remote_dfd, ".", cancellable, error);
       if (!local_repo_remote)
         return glnx_prefix_error (error, "Failed to open local repo");
       g_autofree char *rev = NULL;
-      if (!ostree_repo_resolve_rev (local_repo_remote, ref, FALSE, &rev, error))
+      if (!ostree_repo_resolve_rev (local_repo_remote, self->refspec, FALSE, &rev, error))
         return FALSE;
 
       g_autoptr(OstreeAsyncProgress) progress = ostree_async_progress_new ();
@@ -1721,16 +1709,9 @@ rpmostreed_transaction_new_deploy (GDBusMethodInvocation *invocation,
   self->flags = deploy_flags_from_options (self->options, default_flags);
 
   auto refspec = (const char *)vardict_lookup_ptr (self->modifiers, "set-refspec", "&s");
-  /* Canonicalize here; the later code actually ends up peeling it
-   * again, but long term we want to manipulate canonicalized refspecs
-   * internally, and only peel when writing origin files for ostree:// types.
-   */
   if (refspec)
-    {
-      self->refspec = rpmostree_refspec_canonicalize (refspec, error);
-      if (!self->refspec)
-        return NULL;
-    }
+    self->refspec = g_strdup (refspec);
+
   const gboolean refspec_or_revision = (self->refspec != NULL || self->revision != NULL);
 
   self->revision = (char*)vardict_lookup_ptr (self->modifiers, "set-revision", "&s");

src/libpriv/rpmostree-core.cxx

@@ -50,65 +50,22 @@
 
 static OstreeRepo * get_pkgcache_repo (RpmOstreeContext *self);
 
-/* Given a string, look for ostree:// prefix and
- * return its type and the remainder of the string.  Note
- * that ostree:// can be either a refspec (TYPE_OSTREE) or
- * a bare commit (TYPE_COMMIT).
+/* Given a string, infer its type and return it in `out_type`.
+ * Could be either an ostree refspec (TYPE_OSTREE)
+ * or a bare commit (TYPE_COMMIT).
  */
 gboolean
 rpmostree_refspec_classify (const char *refspec,
                             RpmOstreeRefspecType *out_type,
-                            const char **out_remainder,
                             GError     **error)
 {
-  /* Add any other prefixes here */
-
-  /* For compatibility, fall back to ostree:// when we have no prefix. */
-  const char *remainder;
-  if (g_str_has_prefix (refspec, RPMOSTREE_REFSPEC_OSTREE_PREFIX))
-    remainder = refspec + strlen (RPMOSTREE_REFSPEC_OSTREE_PREFIX);
-  else
-    remainder = refspec;
-
-  if (ostree_validate_checksum_string (remainder, NULL))
+  /* Fall back to TYPE_OSTREE if we cannot infer type */
+  *out_type = RPMOSTREE_REFSPEC_TYPE_OSTREE;
+  if (ostree_validate_checksum_string (refspec, NULL))
     *out_type = RPMOSTREE_REFSPEC_TYPE_CHECKSUM;
-  else
-    *out_type = RPMOSTREE_REFSPEC_TYPE_OSTREE;
-  if (out_remainder)
-    *out_remainder = remainder;
   return TRUE;
 }
 
-char*
-rpmostree_refspec_to_string (RpmOstreeRefspecType  reftype,
-                             const char           *data)
-{
-  const char *prefix = NULL;
-  switch (reftype)
-    {
-    case RPMOSTREE_REFSPEC_TYPE_OSTREE:
-    case RPMOSTREE_REFSPEC_TYPE_CHECKSUM:
-      prefix = RPMOSTREE_REFSPEC_OSTREE_PREFIX;
-      break;
-    }
-  g_assert (prefix);
-  return g_strconcat (prefix, data, NULL);
-}
-
-
-/* Primarily this makes sure that ostree refspecs start with `ostree://`.
- */
-char*
-rpmostree_refspec_canonicalize (const char *orig_refspec,
-                                GError    **error)
-{
-  RpmOstreeRefspecType refspectype;
-  const char *refspec_data;
-  if (!rpmostree_refspec_classify (orig_refspec, &refspectype, &refspec_data, error))
-    return NULL;
-  return rpmostree_refspec_to_string (refspectype, refspec_data);
-}
-
 static int
 compare_pkgs (gconstpointer ap,
               gconstpointer bp)

src/libpriv/rpmostree-core.h

@@ -65,19 +65,10 @@ typedef enum {
   RPMOSTREE_REFSPEC_TYPE_CHECKSUM,
 } RpmOstreeRefspecType;
 
-#define RPMOSTREE_REFSPEC_OSTREE_PREFIX "ostree://"
-
 gboolean rpmostree_refspec_classify (const char *refspec,
                                      RpmOstreeRefspecType *out_type,
-                                     const char **out_remainder,
                                      GError     **error);
 
-char* rpmostree_refspec_to_string (RpmOstreeRefspecType  reftype,
-                                   const char           *data);
-
-char* rpmostree_refspec_canonicalize (const char           *orig_refspec,
-                                      GError              **error);
-
 namespace rpmostreecxx {
 void core_libdnf_process_global_init();
 }

src/libpriv/rpmostree-origin.cxx

@@ -123,7 +123,7 @@ rpmostree_origin_parse_keyfile (GKeyFile         *origin,
         return (RpmOstreeOrigin *)glnx_null_throw (error, "No origin/refspec, or origin/baserefspec in current deployment origin; cannot handle via rpm-ostree");
     }
 
-  if (!rpmostree_refspec_classify (refspec, &ret->refspec_type, NULL, error))
+  if (!rpmostree_refspec_classify (refspec, &ret->refspec_type, error))
     return FALSE;
   /* Note the lack of a prefix here so that code that just calls
    * rpmostree_origin_get_refspec() in the ostree:// case
@@ -208,11 +208,11 @@ rpmostree_origin_get_full_refspec (RpmOstreeOrigin *origin,
 void
 rpmostree_origin_classify_refspec (RpmOstreeOrigin      *origin,
                                    RpmOstreeRefspecType *out_type,
-                                   const char          **out_refspecdata)
+                                   const char          **out_refspec)
 {
   *out_type = origin->refspec_type;
-  if (out_refspecdata)
-    *out_refspecdata = origin->cached_refspec;
+  if (out_refspec)
+    *out_refspec = rpmostree_origin_get_refspec (origin);
 }
 
 static char *
@@ -500,11 +500,10 @@ rpmostree_origin_set_rebase_custom (RpmOstreeOrigin *origin,
   rpmostree_origin_set_override_commit (origin, NULL, NULL);
 
   /* See related code in rpmostree_origin_parse_keyfile() */
-  const char *refspecdata;
-  if (!rpmostree_refspec_classify (new_refspec, &origin->refspec_type, &refspecdata, error))
+  if (!rpmostree_refspec_classify (new_refspec, &origin->refspec_type, error))
     return FALSE;
   g_free (origin->cached_refspec);
-  origin->cached_refspec = g_strdup (refspecdata);
+  origin->cached_refspec = g_strdup (new_refspec);
   switch (origin->refspec_type)
     {
     case RPMOSTREE_REFSPEC_TYPE_CHECKSUM:

tests/kolainst/nondestructive/misc.sh

@@ -61,7 +61,6 @@ runuser -u bin rpm-ostree status
 echo "ok status doesn't require active PAM session"
 
 rpm-ostree status -b > status.txt
-assert_streq $(grep -F -e 'ostree://' status.txt | wc -l) "1"
 assert_file_has_content status.txt BootedDeployment:
 echo "ok status -b"
 

tests/vmcheck/test-pinned-commit.sh

@@ -28,7 +28,6 @@ checksum=$(vm_get_booted_csum)
 vm_rpmostree rebase :${checksum}
 vm_assert_status_jq ".deployments[0][\"origin\"] == \"${checksum}\""
 vm_rpmostree status > status.txt
-assert_file_has_content status.txt '^  ostree://'${checksum}
 echo "ok pin to commit"
 
 vm_rpmostree upgrade >out.txt