GNOME Bugzilla – Bug 676594
[Patch] fix g_reload_user_special_dirs_cache
Last modified: 2012-06-23 21:51:14 UTC
Created attachment 214698 [details] [review] Fix We handle a special case for G_USER_DIRECTORY_DESKTOP when we init the values but drop it when we reload them. Fix this by preferring old values to NULL
Btw, there were a couple of tabs in this function, replaced them with spaces to be consistent and amended locally
Review of attachment 214698 [details] [review]: Looks good, thanks. It would be nice to add a testcase for this special problem too.
Created attachment 214774 [details] [review] Fix previous test This value can be free'd in g_reload_user_special_dirs_cache
Created attachment 214775 [details] [review] Rename previous test and add a new one Previous test was a good test for this fix, but not for a general case, since it's basically the only value we handle in a different way when initializing.
Or maybe the new test should test for the result being !null instead ? Since it's always set at initilization ?
Review of attachment 214774 [details] [review]: ::: glib/tests/utils.c @@ +430,2 @@ g_assert_cmpstr (dir, ==, dir2); + g_free (dir); We can't free the values returned by g_get_user_special_dir - it doesn't return a newly allocated string. We have to live with the fact that some strings leak if you call reload.
Review of attachment 214775 [details] [review]: ::: glib/tests/utils.c @@ +441,3 @@ + dir2 = g_get_user_special_dir (G_USER_DIRECTORY_DOCUMENTS); + + g_assert_cmpstr (dir, ==, dir2); I agree that it would be more useful to check that dir != NULL and dir2 != NULL. Since that is what is currently broken. @@ +442,3 @@ + + g_assert_cmpstr (dir, ==, dir2); + g_free (dir); Same comment as before - can't free these strings.
Actually, it was possible to free these strings as there was a g_strdup before. It was to avoid using a pointer which points to an area of memory we do not manage anymore in case the value had changed and it was free'd, but it's pointless since we directly compare the pointers and not the strings (actually, I do not even get why the tests passed with this here...).
Oh, actually, after getting back in it, we're comparing the strings, so I think either the first patch _is_ needed in order not to dereference a pointer we shouldn't or we should just used g_assert instead of g_assert_cmpstr since when the strings are equals, the same pointer is used. What do you think of this last solution ?
Created attachment 214815 [details] [review] Compare pointers instead of strings
Created attachment 214816 [details] [review] Add test_desktop_special_dit
Created attachment 214817 [details] [review] Add test_desktop_special_dir fix typo
Thanks, I've now added a testcase like this.