Created attachment 198850 [details] [review]
GFileEnumerator: Add g_file_enumerator_get_next()

Review of attachment 198850 [details] [review]:

I think it is a bit of a stretch to call this an incorrect use of GError. 
Nothing wrong with adding an alternative that is more convenient for bindings, though.

This doesn't just impact bindings - these kinds of GError misuses are more painful to use from *C* too.

static gboolean
myfunc (..., GError **error)
{
  GError *temp_error = NULL;

  while ((filename = g_data_input_stream_read_upto (datain, &separator, 1,
                                                    &filename_len, NULL, &temp_error)) != NULL)
  {
    ..
}
  if (filename == NULL && temp_error != NULL)
    {
      g_propagate_prefixed_error (error, temp_error, "%s", "While reading filelist: ");
      goto out;
    }

Is the only right way to use this function.  You have to allocate a temporary GError.

Actually the tradeoff made here is they are slightly easier to use if your function itself doesn't take a GError.

But if you're wrapping any of these GIO calls in a library which itself throws GError, following the rules you have to allow the caller to pass NULL for your GError, so you have to allocate a temporary.

In the end really all functions that throw a GError should just always return gboolean - otherwise it's a mess.

Hmm...so it turns out GIO has this problem rather pervasively, and I'm less sure it makes sense to add a lot of new functions.

It may be better to just stick with the status quo, add documentation for each function that has this issue, and have all callers allocate temporary errors.

Partial list of broken functions:

g_file_enumerator_next_file
g_file_enumerator_next_files_finish
g_data_input_stream_read_byte
g_data_input_stream_read_int16
g_data_input_stream_read_uint16
g_data_input_stream_read_int32
g_data_input_stream_read_uint32
g_data_input_stream_read_int64
g_data_input_stream_read_uint64
g_data_input_stream_read_line
g_data_input_stream_read_line_utf8
g_data_input_stream_read_line_finish

For all of these functions, the return value is also a valid result, so to distinguish errors from non-errors, you have to pass a non-NULL GError.

(In reply to comment #4)
> But if you're wrapping any of these GIO calls in a library which itself throws
> GError, following the rules you have to allow the caller to pass NULL for your
> GError, so you have to allocate a temporary.
> 
> In the end really all functions that throw a GError should just always return
> gboolean - otherwise it's a mess.

FWIW, even then you need to allocate a temporary GError sometimes, if you need to "catch" some errors and "re-throw" the rest. (This pops up all over the place in the tls code with G_IO_ERROR_WOULD_BLOCK.)

g_file_enumerator_next_file is pretty bad.  Even when it fails (and returns a GError), you don't know why: it could have failed to get the attributes of a single file (so you should try again to get the next file) or it could be completely dead (so you should quit).

I think this API would be better:


gboolean    g_file_enumerator_next     (GFileEnumerator  *enumerator);
GFileInfo * g_file_enumerator_get_info (GFileEnumerator  *enumerator,
                                        GError          **error);

then you'd write this:

  while (enumerator.next ()) {
      try {
          info = enumerator.get_info();
      } catch {
          ... handle failure of single file ...
      }
  }

  try {
      enumerator.close ();
  } catch {
      ... report higher-level failure to iterate ...
  }

we could also add

const gchar * g_file_enumerator_get_name (GFileEnumerator *enumerator);

that you could call to find out the name of the file that we failed to get the info for.

As long as we're poking GFileEnumerator, I pretty much always want to get a GFile* for the child too...so an API that gave me both would be cool.

(In reply to comment #8)
 
> I think this API would be better:
> 
> 
> gboolean    g_file_enumerator_next     (GFileEnumerator  *enumerator);
> GFileInfo * g_file_enumerator_get_info (GFileEnumerator  *enumerator,
>                                         GError          **error);

Yeah...the main problem with this is that we need to implement it on top of the old API; adding vfuncs isn't really an option since it would break "third party" GFileEnumerators (and I actually have one in ostree).

The problem with your proposal could be best phrased as "where does the GCancellable go"?  Under the constraint of building on top of the old API, it needs to go in _next(), but the GError would stay on _get_info(), which is weird.

I'm not sure if we have any other precedent for iterators that take GError?

Created attachment 218240 [details] [review]
GFileEnumerator: Add API that's more friendly to propgating GError

The current synchronous g_file_enumerator_next_file() API makes life
painful for callers which want to re-throw a GError; one has to
allocate a temporary GError to correctly distinguish between a normal
end-of-stream versus an error condition.

Doing this "right" requires separating the two cases, and that really
requires two different API calls.

Created attachment 218241 [details]
sample diff between users

This diff shows that the new API is nicer - in particular we can drop this ugly bit:

   if (temp_error)
     {
       g_propagate_error (error, temp_error);
       goto out;
     }

The (conceptually simpler) alternative to the above patch is my original patch; would require callers to distinguish between end-of-stream versus error by testing whether or not the return info is NULL, but that's a lot less annoying than having to allocate a temporary GError.

I kind of prefer my original patch actually, so I'm going to un-obsolete it and we can discuss between.

(In reply to comment #13)
> The (conceptually simpler) alternative to the above patch is my original patch;
> would require callers to distinguish between end-of-stream versus error by
> testing whether or not the return info is NULL

you mean whether the error is NULL, right? the return info would be NULL in both cases

I think I like your version better too, given the constraints around the GCancellable.

Also, g_socket_address_enumerator_next() has exactly the same problem.

Comment on attachment 198850 [details] [review]
GFileEnumerator: Add g_file_enumerator_get_next()

Actually no, forget this first one.  When I tried to use it in practice it didn't really work.  The second one does work.

Created attachment 296725 [details] [review]
Add a better enumeration API

3.5 years later, here's a better patch.

Created attachment 296726 [details] [review]
Also check the value of "count"

In quick self review, I noticed I wasn't testing the value of "count".  Fix that. 

Also I should note this function has been in use for a few years in libgsystem:
https://git.gnome.org/browse/libgsystem/tree/src/gsystem-file-utils.c#n926

*** Bug 702333 has been marked as a duplicate of this bug. ***

Review of attachment 296726 [details] [review]:

::: gio/gfileenumerator.c
@@ +577,3 @@
+ * g_file_enumerator_iterate:
+ * @direnum: an open #GFileEnumerator
+ * @out_info: (out) (transfer none) (allow-none): Output location for the next #GFileInfo

This is not allow-none. We unconditionally assign to it.
I think it should be though. You can as well check for if child != NULL for end-of-stream.

@@ +589,3 @@
+ * In contrast, with this function, a %FALSE return from
+ * gs_file_enumerator_iterate() <emphasis>always</emphasis> means
+ * "error".  End of iteration is signaled by @out_info being %NULL.

@out_info or @out_child being %NULL

@@ +641,3 @@
+  if (g_once_init_enter (&quarks_initialized))
+    {
+      cached_info_quark = g_quark_from_static_string ("gsystem-cached-info");

g-cached-info?

@@ +646,3 @@
+    }
+
+  *out_info = g_file_enumerator_next_file (direnum, cancellable, &temp_error);

I think we should have a temporary info, and allow out_info == NULL.

@@ +652,3 @@
+    {
+      g_propagate_error (error, temp_error);
+      goto out;

I don't really see why we have to use goto here. There is no cleanup, so just return FALSE.

@@ +656,3 @@
+  else if (*out_info != NULL)
+    {
+      g_object_set_qdata_full ((GObject*)direnum, cached_info_quark, *out_info, (GDestroyNotify)g_object_unref);

We should only set this if out_info is != NULL, otherwise we can unref it after we created the child.

::: gio/tests/file.c
@@ +701,3 @@
+    {
+      (void)g_file_enumerator_iterate (fenum, &info, NULL, NULL, &error);
+      g_assert_no_error (error);

Also assert it returns TRUE?

Review of attachment 296726 [details] [review]:

(other than that it looks good to me)

Ok, pushed with all of those changes, though I kept the "goto out"
since I just find it cleaner personally.

One thing though - I had to add a warning if the enumerator was
created without "standard::name" being specified, and we weren't
requesting a GFileInfo return.  In that case we have no way to return
the child.

Maybe a future version of this could silently upgrade the iterator to
force querying the name - I can't think of a use case for not getting
the name.