Comment 5 for bug 724052

Comment on attachment 54450
Added netgroup support and expanded unit tests with MockLibc

Review of attachment 54450:
-----------------------------------------------------------------

Generally the patch looks very good, good job! High-level comments

 - I'm not sure I like git submodules (but TBH I don't have any good reason
   except "it's different!" ... but I think that's reason enough).

   So at this point I would prefer just adding the C and H files from mocklibc
   into tests/mocklibc/*.[ch] and instead of a full buildsystem (with configure.ac
   and all that entails) just having tests/mocklibc/Makefile.am.

   We can always sync with upstream mocklibc if there are bug-fixes or new
   features needed. I just think it's a lot easier to do it this way.

 - Please use 'git format-patch' to format the patch. That way I don't have
   to a) use 'git add' on all the new files; and b) write the commit message.
   (As other developers, I too am lazy!)

 - I think we should probably call it PolkitUnixNetgroup (and
   polkit_unix_netgroup and unix-netgroup:name) instead of
   PolkitNetGroup (and polkit_net_group and netgroup:name).
   Yeah, I agree it's a bit superfluous with the Unix prefix but we
   already do this for users and groups

 - The coding standards generally use "p != NULL" and "p == NULL" to make
   it explicit that p is a pointer and not a boolean. It's not a must to
   follow these conventions in new code but I would appreciate if we did
   in existing code (consistency matters etc.)

 - Please avoid using C++-style comments .. the main reason for this is
   that I use "//" only when temporarily commenting out code. This makes
   it easy to search for it. Yeah, it's a weird reason but bear with me :-)

::: test/polkit/polkitidentitytest.c
@@ +28,5 @@
> +struct comparison_test_data_type {
> + const gchar *subject_a;
> + const gchar *subject_b;
> + gboolean equal;
> +};

Not a biggie but the code generally uses CamelCase and typedef ... and _type is a bit unneeded... so this would simply be "type struct { } ComparisonTestData;"