On (01/10/15 13:56), Michal Židek wrote:
> On 09/16/2015 03:56 PM, Michal Židek wrote:
>> On 09/15/2015 04:03 PM, Jakub Hrozek wrote:
>>> On Tue, Sep 15, 2015 at 03:53:41PM +0200, Michal Židek wrote:
>>>> On 09/14/2015 05:43 PM, Jakub Hrozek wrote:
>>>>> On Wed, Sep 09, 2015 at 02:43:59PM +0200, Michal Židek wrote:
>>>>>> Hi,
>>>>>>
>>>>>> patch for ticket
>>>>>>
https://fedorahosted.org/sssd/ticket/2673
>>>>>> is in the attachment.
>>>>>>
>>>>>> Thanks.
>>>>>> Michal
>>>>>
>>>>>> From 7c454bc2a737be05068418a5eef7fe9446bb5fa8 Mon Sep 17
00:00:00
>>>>>> 2001
>>>>>> From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzidek(a)redhat.com>
>>>>>> Date: Wed, 9 Sep 2015 14:37:48 +0200
>>>>>> Subject: [PATCH] util: Include disabled domains in
link_forest_roots
>>>>>>
>>>>>> Ticket:
>>>>>>
https://fedorahosted.org/sssd/ticket/2673
>>>>>> ---
>>>>>> src/db/sysdb_subdomains.c | 6 +++---
>>>>>> src/tests/cmocka/test_utils.c | 3 +++
>>>>>> src/util/domain_info_utils.c | 21 ++++++++++++++++++---
>>>>>> src/util/util.h | 3 +++
>>>>>> 4 files changed, 27 insertions(+), 6 deletions(-)
>>>>>
>>>>> The patch looks good but whenever I see us adding more and more
boolean
>>>>> switches, I wonder if we should just use flags instead?
>>>>>
>>>>> This:
>>>>> get_next_domain_ex(d, GND_USE_DISABLED | GND_DESCEND);
>>>>> Reads quite a bit easier to me than:
>>>>> get_next_domain_ex(d, true, false);
>>>>>
>>>>> Also, bonus point are acquired next time we add a new flag, because
not
>>>>> all callers of the function must be converted..
>>>>>
>>>>> What do you think?
>>>>
>>>> I think this is very good point. It will also give us implicit
>>>> boolean parameter value (if not set, it is automatically false)
>>>> which improves readability a lot. I wander if it is good to add
>>>> the _ex function in this case. Would you agree if I changed the
>>>> original get_next_domain to use flags? I know, it needs to be changed
>>>> on more places, but I think such small refactoring makes more
>>>> sence than adding _ex function with flags.
>>>>
>>>> Or is this what you were saying? I am not sure because
>>>> you used the _ex function in your example of "nice"
>>>> usage.
>>>
>>> If we have a test for different usages of the flag-based function, then
>>> I guess it would be better than keeping the existing get_next_domain
>>> function.
>>>
>>> We can also split master from sssd-1-13 already..
>>>
>>> btw the flag names were completely pulled out of thin air, feel free to
>>> suggest better ones.
>>
>> I split the refactoring of get_next_domain to separate patch.
>> The second patch is just one liner that switches on the
>> disabled domains in link_forest_roots.
>>
>> Michal
>>
> ____ _ _ __ __ ____
> __/\_| __ )| | | | \/ | _ \__/\__
> \ / _ \| | | | |\/| | |_) \ /
> /_ _\ |_) | |_| | | | | __//_ _\
> \/ |____/ \___/|_| |_|_| \/
>
> (low priority, just keeping the thread alive :) )
Would you mind to write unit tests for get_next_domain?
LS