daemon/upgrader: make use of override-commit-ids

Currently, when setting the `override-commit` key in the origin, the
upgrader pulls that commit checksum directly and then updates the
refspec to point to it. This behaviour was inherited from its ostree
version; at the time it was implemented, the pull code didn't support
passing a specific commit for a given refspec. However, we now have
the override-commit-ids option, which will make libostree update the ref
for us.

We change the code here to make use of it and simplify the function.
This also fixes the corner case of local branches: we shouldn't change
the ref if we're on a local branch. This is actually what drove me to
this patch as I was debugging coreos#981.

(Aside: I'm still not convinced updating the refspec is always the
correct thing to do even in the remote case, though it's a bit messy to
disentangle).

src/daemon/rpmostree-sysroot-upgrader.c

@@ -373,21 +373,18 @@ rpmostree_sysroot_upgrader_pull_base (RpmOstreeSysrootUpgrader  *self,
                                       GCancellable           *cancellable,
                                       GError                **error)
 {
+  const char *refspec = rpmostree_origin_get_refspec (self->origin);
+
   g_autofree char *origin_remote = NULL;
   g_autofree char *origin_ref = NULL;
-  if (!ostree_parse_refspec (rpmostree_origin_get_refspec (self->origin),
-                             &origin_remote, &origin_ref, error))
+  if (!ostree_parse_refspec (refspec, &origin_remote, &origin_ref, error))
     return FALSE;
 
-  char *refs_to_fetch[] = { NULL, NULL };
-  if (rpmostree_origin_get_override_commit (self->origin) != NULL)
-    refs_to_fetch[0] = (char*)rpmostree_origin_get_override_commit (self->origin);
-  else
-    refs_to_fetch[0] = origin_ref;
-
   const gboolean allow_older =
     (self->flags & RPMOSTREE_SYSROOT_UPGRADER_FLAGS_ALLOW_OLDER) > 0;
 
+  const char *override_commit = rpmostree_origin_get_override_commit (self->origin);
+
   g_assert (self->origin_merge_deployment);
   if (origin_remote)
     {
@@ -403,7 +400,12 @@ rpmostree_sysroot_upgrader_pull_base (RpmOstreeSysrootUpgrader  *self,
         g_variant_builder_add (optbuilder, "{s@v}", "timestamp-check",
                                g_variant_new_variant (g_variant_new_boolean (TRUE)));
       g_variant_builder_add (optbuilder, "{s@v}", "refs",
-                             g_variant_new_variant (g_variant_new_strv ((const char *const*) refs_to_fetch, -1)));
+                             g_variant_new_variant (g_variant_new_strv (
+                                 (const char *const *)&origin_ref, 1)));
+      if (override_commit)
+        g_variant_builder_add (optbuilder, "{s@v}", "override-commit-ids",
+                               g_variant_new_variant (g_variant_new_strv (
+                                   (const char *const *)&override_commit, 1)));
 
       g_autoptr(GVariant) opts = g_variant_ref_sink (g_variant_builder_end (optbuilder));
       if (!ostree_repo_pull_with_options (self->repo, origin_remote, opts, progress,
@@ -414,48 +416,32 @@ rpmostree_sysroot_upgrader_pull_base (RpmOstreeSysrootUpgrader  *self,
         ostree_async_progress_finish (progress);
     }
 
-  g_autofree char *new_base_revision = NULL;
-  if (rpmostree_origin_get_override_commit (self->origin) != NULL)
-    {
-      if (!ostree_repo_set_ref_immediate (self->repo,
-                                          origin_remote,
-                                          origin_ref,
-                                          rpmostree_origin_get_override_commit (self->origin),
-                                          cancellable,
-                                          error))
-        return FALSE;
-
-      new_base_revision = g_strdup (rpmostree_origin_get_override_commit (self->origin));
-    }
+  g_autofree char *new_base_rev = NULL;
+  if (override_commit)
+    new_base_rev = g_strdup (override_commit);
   else
     {
-      if (!ostree_repo_resolve_rev (self->repo, rpmostree_origin_get_refspec (self->origin), FALSE,
-                                    &new_base_revision, error))
+      if (!ostree_repo_resolve_rev (self->repo, refspec, FALSE, &new_base_rev, error))
         return FALSE;
     }
 
-  {
-    gboolean changed = FALSE;
-
-    if (strcmp (new_base_revision, self->base_revision) != 0)
-      {
-        changed = TRUE;
-
-        if (!allow_older)
-          {
-            if (!ostree_sysroot_upgrader_check_timestamps (self->repo, self->base_revision,
-                                                           new_base_revision,
-                                                           error))
-              return FALSE;
-          }
-
-        g_free (self->base_revision);
-        self->base_revision = g_steal_pointer (&new_base_revision);
-      }
+  gboolean changed = !g_str_equal (new_base_rev, self->base_revision);
+  if (changed)
+    {
+      /* check timestamps here too in case the commit was already pulled (or the
+       * refspec is local) */
+      if (!allow_older)
+        {
+          if (!ostree_sysroot_upgrader_check_timestamps (self->repo, self->base_revision,
+                                                         new_base_rev, error))
+            return FALSE;
+        }
 
-    *out_changed = changed;
-  }
+      g_free (self->base_revision);
+      self->base_revision = g_steal_pointer (&new_base_rev);
+    }
 
+  *out_changed = changed;
   return TRUE;
 }
 

tests/check/test-basic.sh

@@ -24,7 +24,7 @@ export RPMOSTREE_SUPPRESS_REQUIRES_ROOT_CHECK=yes
 
 ensure_dbus
 
-echo "1..24"
+echo "1..25"
 
 setup_os_repository "archive-z2" "syslinux"
 
@@ -157,6 +157,14 @@ rpm-ostree rebase --os=testos another-branch
 assert_status_jq '.deployments[0].origin == "another-branch"'
 echo "ok rebase from local branch to local branch"
 
+csum1=$($OSTREE commit -b another-branch --tree=ref=$branch)
+csum2=$($OSTREE commit -b another-branch --tree=ref=$branch)
+rpm-ostree deploy --os=testos $csum1
+if [ "$($OSTREE rev-parse another-branch)" != $csum2 ]; then
+    assert_not_reached "deploying from local branch changes branch"
+fi
+echo "ok local deploy doesn't affect branch"
+
 rpm-ostree rebase --skip-purge --os=testos testos:testos/buildmaster/x86_64-runtime
 assert_status_jq '.deployments[0].origin == "testos:testos/buildmaster/x86_64-runtime"'
 ostree --repo=${test_tmpdir}/testos-repo commit --add-metadata-string version=1.0.2 -b testos/stable/x86_64-runtime -s "Build" \