After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 420686 - g_key_file_to_data alters original data
g_key_file_to_data alters original data
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: general
2.13.x
Other All
: Normal minor
: ---
Assigned To: gtkdev
gtkdev
Depends on:
Blocks:
 
 
Reported: 2007-03-20 20:23 UTC by Bobby Jack
Modified: 2007-03-22 09:21 UTC
See Also:
GNOME target: ---
GNOME version: 2.15/2.16


Attachments
Add a test case for reloading GKeyFiles (2.42 KB, patch)
2007-03-21 22:09 UTC, Chris Wilson
none Details | Review
Make reloading GKeyFiles idempotent (after a single beautifying pass) (9.74 KB, patch)
2007-03-22 00:25 UTC, Chris Wilson
none Details | Review
Make reloading GKeyFiles idempotent (after a single beautifying pass) (4.42 KB, patch)
2007-03-22 00:34 UTC, Chris Wilson
committed Details | Review

Description Bobby Jack 2007-03-20 20:23:44 UTC
Please describe the problem:
In gkeyfile.c, g_key_file_to_data() unconditionally appends a newline between groups (the comment is simply "separate groups by at least an empty line"). The resulting string is, therefore, different from the original contents of the file, by 1 newline character per group. If one writes back the contents of this string to the original file, that file then grows.

Bug 344868 explains why this was done, but I do not agree with that bug, specifically the comment:

"Keyfiles written by g_key_file_to_data are a bit hard to read since the groups aren't separated by empty lines."

is only valid for keyfiles that have the lack of newlines anyway. In my view, the exact contents of the original file should be maintained when calling
g_key_file_to_data.

I've seen an example of this problem 'in the wild' (attachment 58956 [details] [review]) and another user has reported the same problem to the gtk-list.

I have a patch to remove the extra newlines, and to test that g_key_file_to_data leaves that data unchanged.

Steps to reproduce:


Actual results:


Expected results:


Does this happen every time?


Other information:
Comment 1 Christian Persch 2007-03-20 23:59:29 UTC
I filed bug 344868 because I _did_ find that "Keyfiles written by g_key_file_to_data are a bit hard to read since the groups aren't separated by empty lines." which does apply for keyfiles you generate entirely from code, i.e. g_key_file_new() + add keys + to_data.

I don't think there's any guarantee implied that a load_from_file + a to_data will give you the exact same string as the original. In fact it will not; for example

--snip--
[Group1]


[Group2]
--snip--

will be re-serialised with just one newline.
Comment 2 Bobby Jack 2007-03-21 21:19:58 UTC
OK, Christian, I believe you and I understand your test case :) To be honest, I can't think of an ideal solution, short of code that inserts a single newline before a group, if there wasn't one already. This would ensure that the file only grows to a finite size and would maintain all 'nicely formatted' keyfiles as is.

However, this is not the behaviour I currently see; to_data() *always* appends extra newlines, in addition to any that are in the original data. Hence the file grows indefinitely.
Comment 3 Chris Wilson 2007-03-21 21:45:35 UTC
(In reply to comment #2)
> However, this is not the behaviour I currently see; to_data() *always* appends
> extra newlines, in addition to any that are in the original data. Hence the
> file grows indefinitely.
This behaviour was identified and fixed in bug 349825, using exactly the method you proposed. Which version of glib are you testing, as the unconditional growth was introduced in 2.11.4 and fixed in 2.12.2?

Comment 4 Christian Persch 2007-03-21 21:47:19 UTC
Looks like bug 349825 but that's supposed to be fixed.
Comment 5 Chris Wilson 2007-03-21 22:09:45 UTC
Created attachment 85080 [details] [review]
Add a test case for reloading GKeyFiles

As this test case indicates the bug was not fixed.
Comment 6 Matthias Clasen 2007-03-21 22:17:10 UTC
Thanks for the testcase
Comment 7 Bobby Jack 2007-03-21 23:14:46 UTC
For the record, I'm seeing this in 2.12.9, 2.12.11, and today's trunk.

Chris's test case is very similar to the one I was working from before I even filed this bug, and at that time I believed the operation should be idempotent. Then Christian made the point about programatically generated files, so I'm confused as to exactly what the functionality should be. Should to_data() be idempotent or should it only (and always) guarantee a single blank line between groups?
Comment 8 Christian Persch 2007-03-21 23:59:13 UTC
Btw, my testing was without the G_KEY_FILE_KEEP_COMMENTS flags, so that's why I didn't see the problem.
Comment 9 Chris Wilson 2007-03-22 00:25:49 UTC
Created attachment 85086 [details] [review]
Make reloading GKeyFiles idempotent (after a single beautifying pass)

This patch endeavours to insert only a single line between the key=value pairs of one group and the next [group].

In the process of doing this I changed the semantics of the comments slightly.
Previously:
#top comment1
#top comment2
[group1]
#key1 comment
key1=value
#key2 comment
key2=value
#group2 comment
[group2]
#key3 comment

Now the comment before the group is tightly bound to that group and we use blank lines to break up comment blocks, viz:
#top comment1
#top comment2
<blank>
#group1 comment
#group1 comment
[group1]
#key1 comment
key1=value
#group2 comment
group2
Comment 10 Chris Wilson 2007-03-22 00:34:44 UTC
Created attachment 85087 [details] [review]
Make reloading GKeyFiles idempotent (after a single beautifying pass)

This time without changing comment semantics.
Comment 11 Matthias Clasen 2007-03-22 02:04:28 UTC
Looks fine, please commit to both branches.
Comment 12 Chris Wilson 2007-03-22 09:21:05 UTC
Commited to trunk r5431 and glib-2.12 r5432:

2007-03-22  Chris Wilson  <chris@chris-wilson.co.uk>

	* glib/gkeyfile.c: Track whether the last key=value pair in a group
	is a blank line and during to_data() only insert a new blank line
	betweens group in its absence. This allows the beautification of the
	GKeyFile and prevents newlines being inserted indefinitely. (#420686)

	* tests/keyfile-test.c (test_reload_idempotency): Test that after a 
	single beautification pass, g_key_file_to_data() does not alter its
	input data.



This will be defeated by someone using g_key_file_set_comment(keyfile, group, NULL, "\nComment\n") but only on the first pass, subsequent reloads should not add further newlines.