Comments on; h=refs/heads/transGroups

Stefan (metze) Metzmacher metze at
Thu Jul 30 06:18:48 MDT 2009

Hi Matthias,

I just mean that

 if (sid == NULL) {
      return NT_STATUS_OK;

 already_there = sids_contains_sid((const struct dom_sid**) *res_sids,
                                   *num_res_sids, sid);
 if (already_there) {
      return NT_STATUS_OK;

would do the same, but is more readable.

I mostly follow the rule to do only one statement in a line.

1.) assing the return values of a function to variables


2.) check some logic.

I think that makes it easier to follow the logic.

I also always review my own patches, like I would review patches from
others, typically a day or some hours later.
(git rebase -i is my best friend :-)

I also wonder why you add the new code to sidmap.c.
I'd prefer if use just make the new function static in
source4/auth/sam.c. And I would rename calc_trans_clos_sid()
to something like authsam_expand_nested_groups() or

the code is sidmap.c is not used anymore, and I think I'll remove it
(at from the build) soon.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 252 bytes
Desc: OpenPGP digital signature
URL: <>

More information about the samba-technical mailing list