On Fri, Nov 29, 2013 at 01:34:27AM -0500, Dmitri Pal wrote: > On 11/28/2013 03:52 AM, Jakub Hrozek wrote: > > On Wed, Nov 27, 2013 at 03:24:44PM +0100, Pavel Reichl wrote: > >> On Tue, 2013-11-26 at 18:25 +0100, Jakub Hrozek wrote: > >>> On Tue, Nov 26, 2013 at 05:28:18PM +0100, Pavel Reichl wrote: > >>>> Hi Jakub, > >>>> > >>>> thanks for review. I have just a few little remarks, please see inline. > >>>> > >>>> On Tue, 2013-11-26 at 15:23 +0100, Jakub Hrozek wrote: > >>>>> On Tue, Nov 19, 2013 at 10:14:49PM +0100, Pavel Reichl wrote: > >>>>>> On Fri, 2013-11-15 at 21:01 +0100, Jakub Hrozek wrote: > >>>>>>> On Fri, Nov 15, 2013 at 11:15:53AM +0100, Pavel Reichl wrote: > >>>>>>>> Hello, > >>>>>>>> > >>>>>>>> First patch improves domain detection taking matched length into > >>>>>>>> account > >>>>>>>> as proposed and elaborated by Jakub. > >>>>>>>> > >>>>>>>> Second patch is a simple unit test testing > >>>>>>>> sss_ldap_dn_in_search_bases > >>>>>>>> and sdap_domain_get_by_dn. > >>>>>>>> > >>>>>>>> Pavel Reichl > >>>>>>> Hi Pavel, > >>>>>>> > >>>>>>> I haven't really read the full patches yet, just scrolled through > >>>>>>> them, > >>>>>>> so I have only one comment so far: > >>>>>>> > >>>>>>>> +++ b/src/tests/cmocka/test_search_bases.c > >>>>>>>> @@ -0,0 +1,214 @@ > >>>>>>>> +/* > >>>>>>>> + SSSD > >>>>>>>> + > >>>>>>>> + find_uid - Utilities tests > >>>>>>>> + > >>>>>>>> + Authors: > >>>>>>>> + Abhishek Singh <abhishekkumarsingh....@gmail.com> > >>>>>>>> + > >>>>>>>> + Copyright (C) 2013 Red Hat > >>>>>>> You should credit yourself :-) > >>>>>> Hi Jakub, > >>>>>> > >>>>>> you are right, thanks for noticing. > >>>>> Hi, in general the patches are working well and looking good. I only > >>>>> have > >>>>> a couple of comments, see inline. > >>>>> > >>>>>> From c7632a2310442cfc10708c5728bd63e6a8c916d2 Mon Sep 17 00:00:00 2001 > >>>>>> From: Pavel Reichl <pavel.rei...@redhat.com> > >>>>>> Date: Thu, 14 Nov 2013 21:34:51 +0000 > >>>>>> Subject: [PATCH 1/2] SSSD: Improved domain detection > >>>>>> > >>>>>> A bit more elegant way of detection of what domain the group member > >>>>>> belongs to > >>>>>> > >>>>>> Resolves: > >>>>>> https://fedorahosted.org/sssd/ticket/2132 > >>>>> I only have code style comments about this patch. > >>>>> > >>>>>> + tmp_ctx = talloc_new(NULL); > >>>>>> + if (tmp_ctx == NULL) > >>>>>> + return NULL; > >>>>> We use brackets around single-line conditionals as well. > >>>> OK, I have no trouble fixing this, but accordingly to > >>>> http://www.freeipa.org/page/Coding_Style#IF_Statements > >>>> I got an idea that statements like: > >>>> > >>>> if (foo) > >>>> bar(); > >>>> else > >>>> baz(); > >>>> > >>>> are legal so I assumed that: > >>>> > >>>> if (foo) > >>>> bar(); > >>>> > >>>> would be legal too. So it may be a good idea to clarify this in above > >>>> mentioned document or maybe It's just my bad reading. :-) > >>> Per IPA developers not using braces is discouraged. I agree the wiki > >>> page should be clarified. > >>> _______________________________________________ > >>> sssd-devel mailing list > >>> sssd-devel@lists.fedorahosted.org > >>> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel > >> Hi, > >> > >> thanks for information. Improved patches are attached. > >> > >> PR > >> > > Looks good and works fine. > > > > I will just squash in two small style changes I didn't tell you about > > earlier, but no need to resend the patches: > > > > --- a/src/tests/cmocka/test_search_bases.c > > +++ b/src/tests/cmocka/test_search_bases.c > > @@ -53,7 +53,7 @@ static struct sdap_search_base** > > generate_bases(TALLOC_CTX *mem_ctx, > > search_bases = talloc_array(mem_ctx, struct sdap_search_base *, n + 1); > > assert_non_null(search_bases); > > > > - for(i=0; i < n; ++i) { > > + for (i=0; i < n; ++i) { > > I hate to rain on parade but ++i is really confusing. > I know that for "for" loops it makes no difference (at least this is > what Goggle said) but for me any use of ++i seems odd. > It is "increment then do" with languages like C that is 0 based for > arrays it is more "do and then increment". > Does the use of ++i (though legitimate in this case) makes only me > uncomfortable or I am not the only one? > Can we use i++ and reserve ++i only for the cases where it is really > meaningful and justified? > I do not see anything in the style guide about this but should we have > something?
I'm personally indifferent to this, although I prefer the postfix notation (i++) as well. But that's just my personal preference. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
