lib/repo: Split archive/bare file parsing

Prep for future cleanup patches (in particular I want an internal-only
version at first that uses a fd+`struct stat`) to avoid allocations.

The new version avoids lots of deep nesting of conditionals as well
by hoisting the "not found" handling to an early return.

There's a bit of code duplication between the two cases but it's
quite worth the result.

Closes: #951
Approved by: jlebon

src/libostree/ostree-repo.c

@@ -2614,21 +2614,8 @@ _ostree_repo_read_bare_fd (OstreeRepo           *self,
   return TRUE;
 }
 
-/**
+static gboolean
- * ostree_repo_load_file:
+repo_load_file_archive (OstreeRepo *self,
- * @self: Repo
- * @checksum: ASCII SHA256 checksum
- * @out_input: (out) (optional) (nullable): File content
- * @out_file_info: (out) (optional) (nullable): File information
- * @out_xattrs: (out) (optional) (nullable): Extended attributes
- * @cancellable: Cancellable
- * @error: Error
- *
- * Load content object, decomposing it into three parts: the actual
- * content (for regular files), the metadata, and extended attributes.
- */
-gboolean
-ostree_repo_load_file (OstreeRepo         *self,
                         const char         *checksum,
                         GInputStream      **out_input,
                         GFileInfo         **out_file_info,
@@ -2636,22 +2623,11 @@ ostree_repo_load_file (OstreeRepo         *self,
                         GCancellable       *cancellable,
                         GError            **error)
 {
-  gboolean found = FALSE;
-  g_autoptr(GInputStream) ret_input = NULL;
-  g_autoptr(GFileInfo) ret_file_info = NULL;
-  g_autoptr(GVariant) ret_xattrs = NULL;
-
-  OstreeRepoMode repo_mode = ostree_repo_get_mode (self);
-
-  char loose_path_buf[_OSTREE_LOOSE_PATH_MAX];
-  _ostree_loose_path (loose_path_buf, checksum, OSTREE_OBJECT_TYPE_FILE, repo_mode);
-
-  if (repo_mode == OSTREE_REPO_MODE_ARCHIVE_Z2)
-    {
-      int fd = -1;
   struct stat stbuf;
-      g_autoptr(GInputStream) tmp_stream = NULL;
+  char loose_path_buf[_OSTREE_LOOSE_PATH_MAX];
+  _ostree_loose_path (loose_path_buf, checksum, OSTREE_OBJECT_TYPE_FILE, self->mode);
 
+  glnx_fd_close int fd = -1;
   if (!ot_openat_ignore_enoent (self->objects_dir_fd, loose_path_buf, &fd,
                                 error))
     return FALSE;
@@ -2665,24 +2641,44 @@ ostree_repo_load_file (OstreeRepo         *self,
 
   if (fd != -1)
     {
-          tmp_stream = g_unix_input_stream_new (fd, TRUE);
+      if (!glnx_fstat (fd, &stbuf, error))
-          fd = -1; /* Transfer ownership */
-
-          if (!glnx_stream_fstat ((GFileDescriptorBased*) tmp_stream, &stbuf,
-                                  error))
         return FALSE;
 
-          if (!ostree_content_stream_parse (TRUE, tmp_stream, stbuf.st_size, TRUE,
+      g_autoptr(GInputStream) tmp_stream = g_unix_input_stream_new (glnx_steal_fd (&fd), TRUE);
-                                            out_input ? &ret_input : NULL,
+      /* Note return here */
-                                            &ret_file_info, &ret_xattrs,
+      return ostree_content_stream_parse (TRUE, tmp_stream, stbuf.st_size, TRUE,
-                                            cancellable, error))
+                                          out_input, out_file_info, out_xattrs,
-            return FALSE;
+                                          cancellable, error);
-
-          found = TRUE;
     }
+  else if (self->parent_repo)
+    {
+      return ostree_repo_load_file (self->parent_repo, checksum,
+                                    out_input, out_file_info, out_xattrs,
+                                    cancellable, error);
     }
   else
     {
+      g_set_error (error, G_IO_ERROR, G_IO_ERROR_NOT_FOUND,
+                   "Couldn't find file object '%s'", checksum);
+      return FALSE;
+    }
+}
+
+static gboolean
+repo_load_file_bare (OstreeRepo *self,
+                     const char         *checksum,
+                     GInputStream      **out_input,
+                     GFileInfo         **out_file_info,
+                     GVariant          **out_xattrs,
+                     GCancellable       *cancellable,
+                     GError            **error)
+{
+  g_autoptr(GInputStream) ret_input = NULL;
+  g_autoptr(GFileInfo) ret_file_info = NULL;
+  g_autoptr(GVariant) ret_xattrs = NULL;
+  char loose_path_buf[_OSTREE_LOOSE_PATH_MAX];
+  _ostree_loose_path (loose_path_buf, checksum, OSTREE_OBJECT_TYPE_FILE, self->mode);
+
   int objdir_fd = -1; /* referenced */
   if (!stat_bare_content_object (self, loose_path_buf,
                                  &objdir_fd,
@@ -2690,55 +2686,54 @@ ostree_repo_load_file (OstreeRepo         *self,
                                  cancellable, error))
     return FALSE;
 
-      if (ret_file_info)
+  if (!ret_file_info)
     {
-          found = TRUE;
+      if (self->parent_repo)
-
-          if (repo_mode == OSTREE_REPO_MODE_BARE_USER)
         {
-              guint32 mode;
+          return ostree_repo_load_file (self->parent_repo, checksum,
-              g_autoptr(GVariant) metadata = NULL;
+                                        out_input, out_file_info, out_xattrs,
-              g_autoptr(GBytes) bytes = NULL;
+                                        cancellable, error);
-              glnx_fd_close int fd = -1;
+        }
+      else
+        {
+          g_set_error (error, G_IO_ERROR, G_IO_ERROR_NOT_FOUND,
+                       "Couldn't find file object '%s'", checksum);
+          return FALSE;
+        }
+    }
 
+  if (self->mode == OSTREE_REPO_MODE_BARE_USER)
+    {
       /* In bare-user, symlinks are stored as regular files, so we just
        * always do an open, then query the user.ostreemeta xattr for
        * more information.
        */
-              fd = openat (objdir_fd, loose_path_buf, O_RDONLY | O_CLOEXEC);
+      glnx_fd_close int fd = openat (objdir_fd, loose_path_buf, O_RDONLY | O_CLOEXEC);
       if (fd < 0)
         return glnx_throw_errno (error);
 
-              bytes = glnx_fgetxattr_bytes (fd, "user.ostreemeta", error);
+      g_autoptr(GBytes) bytes = glnx_fgetxattr_bytes (fd, "user.ostreemeta", error);
       if (bytes == NULL)
         return FALSE;
 
-              metadata = g_variant_new_from_bytes (OSTREE_FILEMETA_GVARIANT_FORMAT,
+      g_autoptr(GVariant) metadata = g_variant_ref_sink (g_variant_new_from_bytes (OSTREE_FILEMETA_GVARIANT_FORMAT,
-                                                   bytes, FALSE);
+                                                                                   bytes, FALSE));
-              g_variant_ref_sink (metadata);
-
       ret_xattrs = set_info_from_filemeta (ret_file_info, metadata);
 
-              mode = g_file_info_get_attribute_uint32 (ret_file_info, "unix::mode");
+      guint32 mode = g_file_info_get_attribute_uint32 (ret_file_info, "unix::mode");
-
       if (S_ISREG (mode) && out_input)
         {
           g_assert (fd != -1);
-                  ret_input = g_unix_input_stream_new (fd, TRUE);
+          ret_input = g_unix_input_stream_new (glnx_steal_fd (&fd), TRUE);
-                  fd = -1; /* Transfer ownership */
         }
       else if (S_ISLNK (mode))
         {
-                  g_autoptr(GInputStream) target_input = NULL;
-                  char targetbuf[PATH_MAX+1];
-                  gsize target_size;
-
           g_file_info_set_file_type (ret_file_info, G_FILE_TYPE_SYMBOLIC_LINK);
           g_file_info_set_size (ret_file_info, 0);
 
-                  target_input = g_unix_input_stream_new (fd, TRUE);
+          char targetbuf[PATH_MAX+1];
-                  fd = -1; /* Transfer ownership */
+          gsize target_size;
-
+          g_autoptr(GInputStream) target_input = g_unix_input_stream_new (fd, FALSE);
           if (!g_input_stream_read_all (target_input, targetbuf, sizeof (targetbuf),
                                         &target_size, cancellable, error))
             return FALSE;
@@ -2746,9 +2741,8 @@ ostree_repo_load_file (OstreeRepo         *self,
           g_file_info_set_symlink_target (ret_file_info, targetbuf);
         }
     }
-          else if (repo_mode == OSTREE_REPO_MODE_BARE_USER_ONLY)
+  else if (self->mode == OSTREE_REPO_MODE_BARE_USER_ONLY)
     {
-              glnx_fd_close int fd = -1;
 
       /* Canonical info is: uid/gid is 0 and no xattrs, which
          might be wrong and thus not validate correctly, but
@@ -2759,7 +2753,7 @@ ostree_repo_load_file (OstreeRepo         *self,
       if (g_file_info_get_file_type (ret_file_info) == G_FILE_TYPE_REGULAR &&
           out_input)
         {
-                  fd = openat (objdir_fd, loose_path_buf, O_RDONLY | O_CLOEXEC);
+          glnx_fd_close int fd = openat (objdir_fd, loose_path_buf, O_RDONLY | O_CLOEXEC);
           if (fd < 0)
             return glnx_throw_errno (error);
 
@@ -2776,14 +2770,12 @@ ostree_repo_load_file (OstreeRepo         *self,
     }
   else
     {
-              g_assert (repo_mode == OSTREE_REPO_MODE_BARE);
+      g_assert (self->mode == OSTREE_REPO_MODE_BARE);
 
       if (g_file_info_get_file_type (ret_file_info) == G_FILE_TYPE_REGULAR
           && (out_input || out_xattrs))
         {
-                  glnx_fd_close int fd = -1;
+          glnx_fd_close int fd = openat (objdir_fd, loose_path_buf, O_RDONLY | O_CLOEXEC);
-
-                  fd = openat (objdir_fd, loose_path_buf, O_RDONLY | O_CLOEXEC);
           if (fd < 0)
             return glnx_throw_errno (error);
 
@@ -2813,27 +2805,6 @@ ostree_repo_load_file (OstreeRepo         *self,
             return FALSE;
         }
     }
-        }
-    }
-
-  if (!found)
-    {
-      if (self->parent_repo)
-        {
-          if (!ostree_repo_load_file (self->parent_repo, checksum,
-                                      out_input ? &ret_input : NULL,
-                                      out_file_info ? &ret_file_info : NULL,
-                                      out_xattrs ? &ret_xattrs : NULL,
-                                      cancellable, error))
-            return FALSE;
-        }
-      else
-        {
-          g_set_error (error, G_IO_ERROR, G_IO_ERROR_NOT_FOUND,
-                       "Couldn't find file object '%s'", checksum);
-          return FALSE;
-        }
-    }
 
   ot_transfer_out_value (out_input, &ret_input);
   ot_transfer_out_value (out_file_info, &ret_file_info);
@@ -2841,6 +2812,36 @@ ostree_repo_load_file (OstreeRepo         *self,
   return TRUE;
 }
 
+/**
+ * ostree_repo_load_file:
+ * @self: Repo
+ * @checksum: ASCII SHA256 checksum
+ * @out_input: (out) (optional) (nullable): File content
+ * @out_file_info: (out) (optional) (nullable): File information
+ * @out_xattrs: (out) (optional) (nullable): Extended attributes
+ * @cancellable: Cancellable
+ * @error: Error
+ *
+ * Load content object, decomposing it into three parts: the actual
+ * content (for regular files), the metadata, and extended attributes.
+ */
+gboolean
+ostree_repo_load_file (OstreeRepo         *self,
+                       const char         *checksum,
+                       GInputStream      **out_input,
+                       GFileInfo         **out_file_info,
+                       GVariant          **out_xattrs,
+                       GCancellable       *cancellable,
+                       GError            **error)
+{
+  if (self->mode == OSTREE_REPO_MODE_ARCHIVE_Z2)
+    return repo_load_file_archive (self, checksum, out_input, out_file_info, out_xattrs,
+                                   cancellable, error);
+  else
+    return repo_load_file_bare (self, checksum, out_input, out_file_info, out_xattrs,
+                                cancellable, error);
+}
+
 /**
  * ostree_repo_load_object_stream:
  * @self: Repo