Thanks for a bug report. It sounds like you'd like to achieve something what offers FleetCommander [1], it only make things in more generic way. Possibly easier is to prefill the .source files, which will still need to be distributed on users' machines (I suppose it's what you do right now).

I do not know whether it would make any sense to create for example
   /usr/share/evolution-data-server/sources/
folder, which would contain any template .source files with some kind of "revision" entry and those would be automatically merged on the evolution-source-registry start, when it still means to distribute such files between machines. There's no gain when it's one user per one machine, though I understand that in a scenario of multiple users per one machine it makes more sense.

[1] https://wiki.gnome.org/Projects/FleetCommander

Thanks for the quick reply!

To elaborate a bit, what we do is we build OS images that get updated using OSTree [1] to a read-only /usr partition. At the moment we don't have the use case I mention in this issue implemented, I'm trying to figure out the best way to do it.

Being /usr read-only, we'd like to avoid using any directory in there. Also, there's an argument in favor of /etc. The .source files are configuration files living in .config ($XDG_CONFIG_HOME) and the system-wide equivalent (see [2]) is /etc/xdg/ ($XDG_CONFIG_DIRS). So adding support for something like
  /etc/xdg/evolution/sources/
with a merging approach like the one you mention would work for us.

By the way, I could work on implementing that feature if we decide it's the way to go.

Thanks again!

[1] https://ostree.readthedocs.io/en/latest/
[2] https://specifications.freedesktop.org/basedir-spec/basedir-spec-latest.html

Right, thus the evolution-data-server should read files from
   $XDG_CONFIG_DIRS/evolution-data-server/sources/
and eventually copy them to its
   $XDG_CONFIG_HOME/evolution/sources

Yes, I see it, there is an inconsistency in the parent directory naming of the 'sources' directory, but I think it'll be better to name the folder in /etc/xdg/ as eds, rather than evo, because everything would reside in the eds and be done on the eds side, completely unrelated to the evo itself.

You would want to do many things, for those obvious:
a) have a way to define the data revision, which would be a new unique
   section in the .source file and also a reference, which user's .source
   file corresponds to which /etc/... file (this can be done by adding a new
   ESourceExtension descendant, with which you can easily search for available
   sources with e_source_registry_list_sources(), eventually with
   e_source_registry_list_enabled() (though the former looks better).
b) there cannot be only a copy of the file from /etc/... to ~/.config/...,
   you should also replace (and be able to define) some variables, possibly
   at least environment variables, thus for example when some service
   would require user credentials the ~/.config... .source file will use
   current user's name; similarly for emails
c) add a code to the evolution-source-registry process which will do the merge
   of the /etc/ files with the ~/.config files, including removal of
   nonexistent .source files, those without their mate in /etc/...
   Note you might ideally assing new UIDs for the copied sources in
   the ~/.config/, which means you will also need to fix any Parent= values
   when copying the files.
d) the second level merge, on the file content level, would be also good to
   do, rather than blindly overwrite the files, thus you'll give the users
   an option to overwrite some values and these overwrites will survive
   another later changes in the /etc/... files

With respect of a), it can be, for example, something like:
  /etc/xdg/evolution-data-server/sources/company-calendar.source
     [AutoConfig]
     Revision=20161020.0

     [DataSource]
     ....

and its corresponding ~/.config/evolution/sources/${UniqueID}.source
     [AutoConfig]
     Revision=20161020.0
     Source=company-calendar

     [DataSource]
     ....

where the Revision key doesn't have any particular meaning, it either matches or not. The Source key is a reference to the .source file in the /etc/..., without the extension and it is not included in the /etc/... file.

With respect of d), the simplest example is a color of the Calendar. If it'll not be forced in the /etc/... source file, then it'll be good to preserve it when a change is recognized. Users will be able to change everything in the .source files, but it would stick only until a new revision found in the /etc/... files. That would mean to "copy" the files from /etc/... to ~/.config/... not as a file copy routine, but by a GKeyFile which will copy existing sections and keys to the destination file, with some exceptions, like the above suggested [AutoConfig] section.

It'll be nice if you could look on it. The above is only a quick idea, maybe when you get to it you'll find some obstacles or ways to do it in a much easier way. I mean, we can fine tune the way it'll be done as we go.

(In reply to Milan Crha from comment #3)
>      [AutoConfig]
>      Revision=20161020.0
>      Source=company-calendar

s/Source/Template/ it'll be better, there are too many 'Source' usages here :)

Thanks for the detailed planning, that helps a lot!

It looks good at first glance, I'll dive into the code and try to get a better understanding. I'll probably come back with questions :)

(In reply to Iago López Galeiras from comment #5)
> I'll probably come back with questions

Sure thing, feel free to ask. It will be easier on IRC [1], you'll know both I'm listening and you'll get the answer in a much quicker way than through bugzilla, which I usually check only once or twice a day.

[1] See the IRC part of https://wiki.gnome.org/Apps/Evolution/#Online_Support

Hi again!

I've been trying to implement this [1] and I think I have most of it working so
far (except fixing up the Parents and handle more than $USER in the source
templates).

However, I've been implementing it manipulating raw GKeyFiles instead of using
ESource objects. I did it this way because I see the merging step as something
you do at the start of evolution-source-registry and from what I've seen in the
ESourceRegistry API, reading ESources is quite coupled with adding them to the
ESourceRegistry, which was inconvenient for me. I understand that this solution
might not be acceptable even though it seems to work.

At this point, (1) I'd like to get some feedback on this and, since I don't
really have a good overview of the code, (2) perhaps you have some suggestions
on how to implement this in a better way by using ESource objects (maybe adding
some new functions to the ESourceRegistry API).

Any advice or suggestion will be welcome.

Thanks!

[1] WIP branch: https://github.com/kinvolk/evolution-data-server/commits/iaguis/system-wide-config

This is bad. I cannot comment on the changes. There are coding style issues (misaligned parameters, function names and such. There are unfinished texts in the comments and in the property definitions. There is missing set_property() in the e-source-autoconfig. The comment lines beginning with '//' are bad, they are for C++, not C. One-letter variables are bad, use for example 'ff', or even better 'func'. The MergeSourcePopulateHashtable might be better to name as MergeSourcePopulateHashtableFunc. It's slightly confusing that you use both g_key_file_free() and g_key_file_unref(), maybe using only the unref() will be cleaner, even I see why you use one in one place but the other in the other place). You should not use "Autoconfig", use the #define.
// if we fail to remove it, keep going
a) bad comment type; b) read the return value and at least warn on the console, thus there is some sign of the failure. The double-allocation around evolution_source_replace_env_vars() feels unneeded, unless you have it for any future needs. Do not use g_assert(). Possible memory leaks around evolution_source_get_revision(). This construction "(*error)->message" is not good, what if the caller passes NULL for 'error'? What if the called function fails, but doesn't set the error? The code usually uses 'local_error' for such cases and 'local_error ? local_error->message : "Unknown error"'.

I think the first commit is not needed. You can also fix '// TODO handle several system directories?' when you'll not use it.

// FIXME should this be fatal?
The answer is 'no'.

I didn't test it, it was only a read-review, but apart of those things and those "todo" it looks fine.

For the next review, please merge the two top commits into one and attach it here as a patch. You'll see how nice the integration in the GNOME bugzilla for patch reviews is.

Created attachment 340689 [details] [review]
system-wide sources support

Here's an updated version. Some comments:

* I dropped the generation of a new UID to copy the file to the user directory
  and manitain the name to avoid fixing the Parent= field. I assume the sysadmin
  doesn't choose the same name for templates in different system directories so
  we shouldn't have problems with clashes.

* I dropped the first commit as you suggested.

* About the double-allocation around evolution_source_replace_env_vars().
  I was assuming we'll add more env variables (although I can't think of
  more right now) so I separated it to a function. Since
  g_get_user_name() returns a newly allocated string and I didn't wanna
  pass the ownership of the string to the function, I created the
  separate value and freed it outside to be explicit about it. Happy to
  change it to something else if you have a better idea.

* I tried to fix all the review comments and code-style issues, hope I
  didn't forget about any.

Thanks!

(In reply to Iago López Galeiras from comment #9)
> Since g_get_user_name() returns a newly allocated string

It returns 'const gchar *', a string owned by GLib, not a newly allocated string.

I'm thinking, what about extend the evolution_source_replace_env_vars() and teach it about:

   $USER | ${USER}         ... g_get_user_name ()
   $REALNAME | ${REALNAME} ... g_get_real_name ()
   $HOST | ${HOST}         ... g_get_host_name ()

Anything else with $xxx or ${xxx} ... g_getenv (xxx)

Defaulting not to the passed-in value, but to an empty string (rather not NULL) and a debugging output on console about "not-found" variable (using e_source_registry_debug_print()).

I'm also afraid that users will need to replace variables in the middle of any string, evolution_source_registry_replace_env_vars() currently supports only whole values. Imagine that you'll have these keys:

Email=${USER}@company.com
Host=http://instranet.company.com/dav/${USER}/calendar?from=${HOST}

These and many other things are pretty common. In that case, maybe support only variables in curly brackets, not those bare variables, to avoid confusion and make the code simpler.

Just an idea for an enhancement. I'm going to read-review the patch.

Review of attachment 340689 [details] [review]:

Please see also: https://wiki.gnome.org/Apps/Evolution/Patches Pity I didn't start with it at the beginning. I'm sorry about that.

To see the review in a more better way click the [Review] link and you'll get a clickable list, also with the text being shown in the full context. I sometimes didn't write each single thing repeatedly (sometimes I did).

::: src/libedataserver/e-source-autoconfig.c
@@ +67,3 @@
+			g_value_set_string (
+				value,
+				e_source_autoconfig_get_revision (

use g_value_take_string() and e_source_autoconfig_dup_ functions, instead of e_source_autoconfig_get_ functions here (both cases). See for example this: https://git.gnome.org/browse/evolution-data-server/tree/src/libedataserver/e-source-backend.c#n151

@@ +82,3 @@
+
+static void
+source_autoconfig_set_property (GObject *object,

this one is not used - compiler should tell you, as it does for me

@@ +197,3 @@
+
+/**
+ * e_source_autoconfig_set_template:

"glue" the functions by the property (template, then revision), not by type (get, then set)

::: src/libedataserver/e-source-autoconfig.h
@@ +82,3 @@
+						(ESourceAutoconfig *extension);
+const gchar *		e_source_autoconfig_get_template
+						(ESourceAutoconfig *extension);

The same as in source file, "glue" by property, not by function type.

::: src/services/evolution-source-registry/evolution-source-registry.c
@@ +82,3 @@
+evolution_source_registry_free_merge_source_data (gpointer mem)
+{
+	MergeSourceData *source_data = (MergeSourceData *) mem;

check whether source_data is not NULL, just in case

@@ +97,3 @@
+	gchar *uid;
+
+	uid = g_strndup (basename, strlen(basename) - 7);

coding style: missing space after strlen()

@@ +101,3 @@
+	data = g_new0 (MergeSourceData, 1);
+	data->key_file = g_key_file_ref(key_file);
+	data->path = g_strdup(filename);

coding style: similar as with the strlen(), both above lines

@@ +116,3 @@
+	if (g_key_file_has_group (key_file, E_SOURCE_EXTENSION_AUTOCONFIG)) {
+		gchar *template;
+		if ((template = g_key_file_get_value (

coding style: add an empty line before the 'if'

@@ +120,3 @@
+					E_SOURCE_EXTENSION_AUTOCONFIG,
+					"Template",
+					&error)) == NULL) {

what does happen to the 'error'? It looks like is is leaked, if set, and also unused.

@@ +127,3 @@
+				"Malformed source: ",
+				E_SOURCE_EXTENSION_AUTOCONFIG,
+				"group found, but no Template key present");

why to replace those static strings with '%s'? It makes the actual message very cryptic.

@@ +147,3 @@
+	GDir *dir = NULL;
+	const gchar *basename;
+	MergeSourceData *data = NULL;

compiler claims that this one is unused.

@@ +192,3 @@
+	for (index = keys;
+	     index != NULL;
+	     index = index->next) {

index = g_list_next (index); all can be on one line

@@ +203,3 @@
+						   "Error cleaning orphan source",
+						   data->path,
+						   g_strerror(errno));

coding style: space between function name and the arguments start

@@ +214,3 @@
+
+static gchar *
+evolution_source_registry_get_revision (MergeSourceData *data, GError **error)

coding style: one parameter per line

@@ +216,3 @@
+evolution_source_registry_get_revision (MergeSourceData *data, GError **error)
+{
+	GError *local_error = NULL;

compiler claims that this one is unused.

@@ +225,3 @@
+	}
+
+	g_set_error(

coding style: space between function name and the arguments start

@@ +246,3 @@
+
+static void
+evolution_source_registry_copy_source (GKeyFile *target, GKeyFile *source)

coding style: one parameter per line

@@ +249,3 @@
+{
+	gchar **groups = NULL;
+	gsize group_length;

what about 'ngroups'? the 'group_length' sounds like 'a length of one group, like its name'.

@@ +250,3 @@
+	gchar **groups = NULL;
+	gsize group_length;
+	gint i;

coding style: make it `gint ii;`

@@ +291,3 @@
+static gboolean
+evolution_source_registry_merge_source (GHashTable *home_sources,
+                                        gchar *key,

const gchar *key,

@@ +327,3 @@
+	if (skip_copy)
+		/* revisions are the same, don't copy */
+		return TRUE;

coding style: when there are multiple lines, then enclose in a block, aka {}

@@ +352,3 @@
+
+static void
+evolution_source_registry_generate_source_from_system (gchar *key,

const gchar *key,

@@ +389,3 @@
+	GHashTableIter iter;
+	MergeSourceData *system_key_file;
+	MergeSourceData *new_data;

compiler claims that this one is unused.

@@ +406,3 @@
+					&key_files_to_copy,
+					&error)) {
+				g_warning ("%s: %s", G_STRFUNC, error->message);

error-s can be NULL, check for it. Also prefix the second '%s' with some explanation, like "evolution_source_registry_merge_source() failed: %s"

@@ +407,3 @@
+					&error)) {
+				g_warning ("%s: %s", G_STRFUNC, error->message);
+				g_error_free (error);

use g_clear_error(), otherwise if there will be more than one failure then the second will do use-after-free in the cycle (the 'error' will not be NULL). Also consider to rename 'error' to 'local_error'.

@@ +429,3 @@
+
+static gboolean
+evolution_source_registry_write_key_files (GList *list, GError **error)

coding style: one parameter per line

@@ +435,3 @@
+	gboolean success;
+
+	for (index = list; index != NULL; index = index->next) {

index = g_list_next (index);

@@ +436,3 @@
+
+	for (index = list; index != NULL; index = index->next) {
+		data = (MergeSourceData *)index->data;

coding style, missing space before 'index->data'. The typecast isn't even needed, but it would be a good idea to check for not-NULL of 'data'.

@@ +453,3 @@
+	gboolean success = FALSE;
+	const gchar * const *config_dirs;
+	gint i;

gint ii;

@@ +501,3 @@
+
+	success = TRUE;
+exit:

add a space in front of the label 'exit:'; it helps with `diff -p`.

@@ +580,3 @@
+	if (!success) {
+		g_warning ("%s: %s", G_STRFUNC, error->message);
+		g_error_free (error);

as above:
a) prefix in the g_warning() what it was trying to do
b) the error can be NULL (error ? error->message : "Unknown error")
c) use g_clear_error(), thus the below usage of the 'error' is NULL. This call can be done out of the 'if (!success)' too.

Created attachment 341040 [details] [review]
system-wide sources support v2

Another stab at this.

Fixed review issues and handled https://bugzilla.gnome.org/show_bug.cgi?id=773156#c10

Created attachment 341051 [details] [review]
system-wide sources support v2

Wrong attachment, sorry.

Created attachment 341059 [details] [review]
system-wide sources support v3

Updated again to fix some nits found by Krzesimir Nowak.

Review of attachment 341059 [details] [review]:

Thanks for the update. The change looks pretty well. I've couple more nitpicks, but nothing against functionality as such.

I'm also wondering, what about adding some debug prints that any (which) system sources were found, which needed update and why and which not, and which had been removed? Thus the admins will be able to debug what their changes do in the background. That can be a follow-up enhancement too, to not update whole patch again and again.

I'll test the actual functionality and let you know my results soon. Feel free to postpone the changes until that.

::: src/libedataserver/e-source-autoconfig.c
@@ +280,3 @@
+/**
+ *
+ * e_source_autoconfig_dup_revision:

The above "empty" line generates a compile time warning:
src/libedataserver/e-source-autoconfig.c:281: Error: EDataServer: identifier not found on the first line:
 *
  ^

::: src/libedataserver/e-source-autoconfig.h
@@ +69,3 @@
+ **/
+struct _ESourceAutoconfig {
+	ESourceExtension parent;

add a comment: /*< private >*/
before this line, as it is in the other extensions. It's a note for gtk-doc or GObjectIntrospection, I do not recall precisely for what I added it there.

::: src/services/evolution-source-registry/evolution-source-registry.c
@@ +121,3 @@
+{
+	if (g_key_file_has_group (key_file, E_SOURCE_EXTENSION_AUTOCONFIG)) {
+		GError *error = NULL;

the 'error' variable is basically unused here, you can simply remove it and pass NULL as the argument of g_key_file_get_value().

@@ +242,3 @@
+		{ "USER", g_get_user_name ()},
+		{ "REALNAME", g_get_real_name ()},
+		{ "HOST", g_get_host_name ()}

coding style: missing white space before '}'

I'm also wondering whether using function calls as initializers is portable (I'm pretty sure that for example a Borland compiler used to have trouble with such constructions).

@@ +260,3 @@
+	} else {
+		e_source_registry_debug_print (
+			"Environment variable ${%s} not found.\n", var_name);

I  know I requested to use e_source_registry_debug_print(), but the code is not consistent with it, because it uses g_warning() on some places and this function elsewhere. It would be good to be consistent. Either we'll consider all issues important (then use g_warning()), or we'll consider all errors "boring for the regular users", in which case this debug function should be used. What do you think?

@@ +261,3 @@
+		e_source_registry_debug_print (
+			"Environment variable ${%s} not found.\n", var_name);
+		g_string_append (result, "");

This is a no-op, better to replace it with a comment telling that the variable had been replaced with an empty string instead.

@@ +292,3 @@
+				NULL);
+
+	g_return_val_if_fail (new != NULL, g_strdup (old));

could it be made an if() instead, please? That way you won't leak 'regex'. I would even use a 'local_error' here, to know what failed (you pass a user input to the function, after all, thus the runtime check (the g_return_val_if_fail) doesn't look like the best thing to do here).

@@ +564,3 @@
+		goto exit;
+
+	success = TRUE;

The 'success' is already TRUE here; the failure of the evolution_source_registry_write_key_files() will report overall TRUE as well, even it failed.

(In reply to Milan Crha from comment #3)
> Right, thus the evolution-data-server should read files from
>    $XDG_CONFIG_DIRS/evolution-data-server/sources/

Thinking of it, a better folder name would be:
   $XDG_CONFIG_DIRS/evolution-data-server/autoconfig/
aka do not use 'sources', but 'autoconfig', to avoid confusion between the two, in which does what.

There would be also good to write a documentation about the usage, ideally with an example and a step-by-step guide, notes (on the Parent key, ...) and so on, possibly on the evolution's WiKi [1], thus one doesn't need to dig this out of the sources and the code reading.

Also, splitting the changes in the evolution-source-registry.c into a separate file, like the other parts are (evolution-source-registry-migrate*.c) would be good to do, to separate the autoconfig code from the other. That might be evolution-source-registry-autoconfig.c and wherever you call the functions with "_system_sources" make it "_autoconfig". I mean, if you think such change would make any sense at all.

[1] https://wiki.gnome.org/Apps/Evolution

Just the first run says:
> (evolution-source-registry:2101): evolution-source-registry-WARNING **:
> evolution_source_registry_load_sources:
> evolution_source_registry_merge_system_sources() failed: Error opening
> directory '/etc/xdg/evolution-data-server/sources': No such file or directory

That's no problem that the directory doesn't exist, this error can be silently ignored.

(In reply to Milan Crha from comment #15)
> I'm also wondering, what about adding some debug prints that any (which)
> system sources were found, which needed update and why and which not, and
> which had been removed? Thus the admins will be able to debug what their
> changes do in the background. That can be a follow-up enhancement too, to
> not update whole patch again and again.

Sure, that makes sense.


(In reply to Milan Crha from comment #16)
> aka do not use 'sources', but 'autoconfig', to avoid confusion between the
> two, in which does what.

OK.

> There would be also good to write a documentation about the usage, ideally
> with an example and a step-by-step guide, notes (on the Parent key, ...) and
> so on, possibly on the evolution's WiKi [1], thus one doesn't need to dig
> this out of the sources and the code reading.

I can do that. I'm trying to find a good place for it in the wiki. Not sure if this fits in "Developer Resources" and I don't see any other candidate sections. Maybe somewhere in [1] and an example in [2]?

> Also, splitting the changes in the evolution-source-registry.c into a
> separate file, like the other parts are
> (evolution-source-registry-migrate*.c) would be good to do, to separate the
> autoconfig code from the other. That might be
> evolution-source-registry-autoconfig.c and wherever you call the functions
> with "_system_sources" make it "_autoconfig". I mean, if you think such
> change would make any sense at all.

Makes sense to me, I'll do that.


[1]:https://developer.gnome.org/eds/3.22/
[2]:https://developer.gnome.org/eds/3.22/examples.html

I made 4 files in /etc/xdg/evolution-data-server/sources, three of them define an IMAPx account, the fourth is for a calendar. I run the evolution-source-registry and it shows me this warning:

> (evolution-source-registry:2429): evolution-source-registry-WARNING **:
> evolution_source_registry_load_sources:
> evolution_source_registry_merge_system_sources() failed: No such file or directory

I do not see from it what exactly is wrong. It would be good to claim more details in the error, thus I can do anything to correct (possibly my own?) error. No files had been copied at this point.

I'm not investigating further, you are on turn now. :)

the [Autoconfig] Template=xxx can be filled automatically on the copy from the autoconfig/ to user's sources/ dir; it's a pita to repeat the file name, too easy to fail/break/make a typo when it can be done automatically. That way the .source file in the ..../autoconfig/ folder can contain only the Revision key.

Thinking of it (yeah, I'm sorry, I miss many details until I actually touch the feature and try it out), there might be people willing to have one corporate signature shared on all the machines. Signatures are also .source files, but they also reference files from ~/.config/evolution/signatures (looking for the same filename in that folder). It would be good to handle these too. See the e-source-mail-signature.c extension, it doesn't save many things in the .source file, but there are still tricky parts like embedded images in HTML signatures. We can figure out how to do that best after we finish the "initial" part. This is just to have you think of another (extended) usage.

A new GSettings key with a path where to look for custom sources (like some corporate NFS volume) in addition to the XDG folder(s), might be also nice to have. I can imagine that admins will populate the key with their path once and then will setup everything from that folder. (data/org.gnome.evolution-data-server.gschema.xml.in, add a string key "autoconfig-directory"; this directory should claim an error when it's filled and cannot be opened (see the difference with the XDG folder(s) from comment #17)).

(In reply to Iago López Galeiras from comment #18)
> I can do that. I'm trying to find a good place for it in the wiki. Not sure
> if this fits in "Developer Resources" and I don't see any other candidate
> sections. Maybe somewhere in [1] and an example in [2]?

Let's have it as a page (currently non-existent):
https://wiki.gnome.org/Apps/Evolution/Autoconfig

which will be references from the main Apps/Evolution WiKi page.

Created attachment 341172 [details] [review]
system-wide sources support v4

Updated.

I think I addressed everything except https://bugzilla.gnome.org/show_bug.cgi?id=773156#c20, the wiki part (but that's not part of the patch :) and the function calls as initializers (which I think should be fine but I could change it to function pointers if we're really worried).

I couldn't reproduce your g_warning. I added a better error to the calls to g_key_file_set_value() to help with finding the problem (now that I changed all the g_warning() to debug prints you'll need to start the registry with ESR_DEBUG=1).

(In reply to Milan Crha from comment #20)
> A new GSettings key with a path where to look for custom sources (like some
> corporate NFS volume)

I just got yet another idea, but maybe it's nonsense. When there'll be "autoconfig-directory", then a complementary "autoconfig-variables" (an array of strings of format 'name=value') would be probably also nice to have, then the variable replacement order would be: predefined variables | variables from GSettings | environment variables. That's just to be able to not change machine environment variables, which can be (sometimes) problematic.

I'll test your current change and let you know.

I added a .source into the autoconfig/ folder which didn't have the [Autconfig] section at all. It had been happily copied the first time I ran evolution-source-registry. The next start I see on the console (with debugging on):
> evolution_source_registry_merge_sources:
> evolution_source_registry_merge_source() failed: Key file does not have
> group 'Autoconfig'.

a) the error message should tell which key file it is
b) the initial copy should reject files without the section and
   the mandatory keys (and tell about it)

I added the [Autoconfig] into my source template and run esr again. This had been shown:

> evolution_source_registry_merge_sources:
> evolution_source_registry_merge_source() failed: Key file does not have key
> 'Revision' in group 'Autoconfig'.

The a) from above applies to this error message as well.

> Autoconfig: Environment variable ${RRR} not found.

Might be change to:

> Autoconfig: Environment variable ${RRR} not found, used in '%s'.

Looking at this error message, I like what you did, the prefix "Autoconfig: ", it'll be very useful to prefix all the messages from that part with it, thus it'll be grep-able and/or simply easily recognized between all of the esr messages.

When we do not rename the files, the Template= key got useless and can be removed. That will left us with simplified [Autoconfig] section containing only the Revision key, which will be mandatory in both files.

Other my tests show that the patch works as expected. Good work. Thanks for it.

Review of attachment 341172 [details] [review]:

One thing found (apart of below includes), you should reject to overwrite existing file in user's home when it doesn't contain the [Autoconfig] section, to avoid data loss. It's unlikely that the names (ESource UIDs) will match, but better to be prepared.

::: src/services/evolution-source-registry/evolution-source-registry-autoconfig.c
@@ +22,3 @@
+#include <glib/gstdio.h>
+#include <camel/camel.h>
+#include <libsoup/soup.h>

What is camel and libsoup good for here? Neither of them is used here.

::: src/services/evolution-source-registry/evolution-source-registry.c
@@ +22,3 @@
 #include <stdlib.h>
 #include <glib/gi18n.h>
+#include <glib/gstdio.h>

both added includes are not needed here anymore

Created attachment 341229 [details] [review]
system-wide sources support v5

Version 5 :)

Review of attachment 341229 [details] [review]:

Thanks for the update. You can see my final notes below. I changed it myself this time, to avoid one more round trip, and committed your changes into the source.

::: src/services/evolution-source-registry/evolution-source-registry-autoconfig.c
@@ +342,3 @@
+				error);
+	if (autoconfig_revision == NULL) {
+		g_propagate_error (error, local_error);

The 'local_error' is always NULL here

@@ +356,3 @@
+				error);
+	if (home_revision == NULL) {
+		g_propagate_error (error, local_error);

The 'local_error' is always NULL here

@@ +395,3 @@
+			G_KEY_FILE_NONE,
+			&local_error)) {
+		g_propagate_error (error, local_error);

This is the first use of the 'local_error', but it's unneeded, because you do not dereference it, thus a direct use of the passed-in 'error' can be done here.

@@ +440,3 @@
+	 * list of files to copy to avoid data loss. */
+	if (g_file_test (dest_source_path, G_FILE_TEST_EXISTS)) {
+		g_free (dest_source_path);

it should tell so on the console too.

Created commit df37ebf in eds master (3.23.3+)