Created attachment 326852 [details] [review]
GResources: add support for resource overlays

Review of attachment 326852 [details] [review]:

Looks very handy, thanks! Ideally, there should be some documentation about the new env var, explaining details such as that it is colon-separated, that it can't handle resource paths containing =, etc

::: gio/gresource.c
@@ +269,3 @@
+      gint i, j;
+
+      envvar = g_getenv ("G_RESOURCES_OVERLAY_DIRS");

The DIRS in the name is a bit misleading since you also support replacing individual files.
I might suggest to go with the shorter G_RESOURCE_OVERLAYS

@@ +354,3 @@
+        /* hold off on dst_len because we will probably fail the checks below */
+      }
+

If you want to be very fast here, you could just split the lines once and keep parallel src and dst arrays... it probably doesn't matter in practice

The reason I did this was because we need to know the length of 'src' anyway, for the memcmp.

One thing I considered was to keep pairs of src/eq (since dst comes direct from 'eq').  Keeping a pointer to 'dst' is more or less equivalent to that as long as we keep it in the form that src and dst are stored together in a single string.  I doubt that this is going to show up on anyone's profiles, though...

I'll rename the variable, and add some docs to the intro.  I'll just more or less copy the commit message.

Created attachment 326856 [details] [review]
GResources: add support for resource overlays

I changed the variable name and added the documentation, as well as
improving the commit message a bit.  I also fixed a slight bug where we
were trying to free the GMappedFile even in the error case (where we
failed to create it).

Created attachment 326861 [details] [review]
GResources: add support for resource overlays

I forgot to change the name of the envvar in the g_critical() messages as well.

Review of attachment 326861 [details] [review]:

Naming bikeshed I know...but how about `G_DEBUG_RESOURCE_OVERLAYS` so it's clear this is Not For Production Use?

to me, this sounds like I would be setting it to 'yes' to get additional outputs regarding a feature called 'resource overlays'...

Review of attachment 326861 [details] [review]:

::: gio/gresource.c
@@ +300,3 @@
+        parts = g_strsplit (envvar, ":", 0);
+      else
+        parts = g_new0 (char *, 0 + 1);

Consider `NULL` rather than the empty array?  The main reason I say this is it'll be another thing everyone's cargo culted valgrind suppressions file will have to start ignoring.

Yeah, true, would be different from the other vars.  I don't have a strong opinion on it, I'm okay if it stays as is too.

(In reply to Colin Walters from comment #8)
> Review of attachment 326861 [details] [review] [review]:

> Consider `NULL` rather than the empty array?  The main reason I say this is
> it'll be another thing everyone's cargo culted valgrind suppressions file
> will have to start ignoring.

Which is a very concrete pain point for me (and I'm sure others) BTW,
https://github.com/rpm-software-management/libhif/pull/91

The reason I put the empty array (in addition to consistency reasons) is because otherwise I have to step up my use of g_once_init().

I guess I could use a magic value like (void*)0x1...

actually, I have a better idea.  Patch coming.

Created attachment 326875 [details] [review]
GResources: add support for resource overlays

I use a static empty array and point the variable to it in case there are no overlays.

Review of attachment 326875 [details] [review]:

Looked like mclasen was reviewing this originally, I assume he's OK though with this.

I only have one other minor comment that the last cleanup hunk using `g_hash_table_get_keys_as_array` might be nicer as a separate patch, but we have a relatively heavyweight review here, so fine by me as is.

Thanks for catching that!  I actually intended to do that separately but forgot to commit it before proceeding.

I'll split it out and let mclasen take a final crack at this when he wakes up.

Created attachment 326918 [details] [review]
GResources: use g_hash_table_get_keys_as_array()

Replace the hand-written equivalent of this with the call to the
GHashTable built-in version to save a few lines of code.

The GResource code was written a couple of years before this function
existed.

Similarly, replace a set-mode usage of g_hash_table_insert() with a call
to g_hash_table_add().

Created attachment 326919 [details] [review]
GResources: add support for resource overlays

When debugging a program or testing a change to an installed version, it
is often useful to be able to replace resources in the program or a
library, without recompiling.

To support this, for debugging and hacking purposes, it's now possible
to define a G_RESOURCE_OVERLAYS environment variable as a
colon-separated list of substitutions to perform when looking up
GResources.

A substitution has the form

  "/org/gtk/libgtk=/home/desrt/gtk-overlay"

The part before the '=' is the resource subpath for which the overlay
applies.  The part after is a filesystem path which contains files and
subdirectories as you would like to be loaded as resources with the
equivalent names.

In the example above, if an application tried to load a resource with
the resource path '/org/gtk/libgtk/ui/gtkdialog.ui' then GResource would
check the filesystem path '/home/desrt/gtk-overlay/ui/gtkdialog.ui'.  If
a file was found there, it would be used instead.

Substitutions must start with a slash, and must not have a trailing
slash before the '='.  It is possible to overlay the location of a
single resource with an individual file.

Review of attachment 326918 [details] [review]:

sure

Review of attachment 326919 [details] [review]:

looks good to me now

Attachment 326918 [details] pushed as 3c7c0af - GResources: use g_hash_table_get_keys_as_array()
Attachment 326919 [details] pushed as 55ab3af - GResources: add support for resource overlays