On 12/05/2012 08:28 AM, Ondrej Kos wrote:
> On 05/12/12 02:17, Dmitri Pal wrote:
>> Hello,
>>
>> Please find attached the list of the 10 patches for ding-libs.
>> These patches implement merge capabilities of the ding-libs and refine
>> parser to consistently deal with different parse modes.
>>
>>
>> Patches 1-5 are self contained patches that can be applied independently
>> from each other but in the given order.
>> After applying each of those the tree should build-able.
>>
>> Patches 6-10 are related to each other and need to be viewed and applied
>> in relation. They are a logically split for an easier review one piece
>> of functionality .
>>
>> Patch names and commit comments give an idea of what happens in each
>> patch.
>> Most complicated patches are 6, 7 and 10.
>>
>> Let me know if I missed something.
>>
>>
>>
>> _______________________________________________
>> sssd-devel mailing list
>> sssd-devel(a)lists.fedorahosted.org
>>
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>>
> [PATCH 1/10]
>
> *ACK*
>
>
> [PATCH 2/10]
>
> just a nitpick, I wouldn't delete the empty line after introductory
> comment (maybe leave it for the future code re-styling)
>
> @@ -18,7 +18,6 @@
> You should have received a copy of the GNU Lesser General Public
> License
> along with Collection Library. If not, see
> <
http://www.gnu.org/licenses/>.
> */
> -
> #define _GNU_SOURCE
> #include <string.h>
> #include <stdlib.h>
>
> otherwise *ACK*
A new patch is ready.
>
>
> [PATCH 3/10]
>
> git am doesn't like the two empty lines added to the end of file
I can't find place where there are two lines at the end of the file.
> $ gam 0003-INI-Merge-and-serialize-comments.patch
> Applying: Merge and serialize comments
> /home/okos/devel/ding-libs/.git/rebase-apply/patch:89: trailing
> whitespace.
> struct simplebuffer *sbobj)
> warning: 1 line adds whitespace errors.
That I found and fixed.
Will be in the next set of patches.
> and *NACK*:
>
> ini_comment_serialize doesn't work as expected
> it is appending multiple CRs, not sure when exactly, but it probably
> has something to do with multi-line comments. and those empty lines
> result into
> = =
> thus breaking config file
> example:
>
> #Hoho section
> [hoho]
> #Hoho value
> val = hoho
> #End of hoho
> #Start of section
> [foo]
> #Second value
> bar = second value
> #End of section
> = =
> #Hoho section
> [hoho]
> #Hoho value
> val = hoho
> #Hoho section
> [hoho]
> #Hoho value
> val = hoho
> #End of hoho
>
>
Are you looking at the ini_parse_ut?
Hm let me try applying these patches one by one.
May be I need to restructure unit tests
> [PATCH 4/10]
>
> make-check fails on:
>
> Files ./merge.conf.out and ./expect.conf.out differ
> Failed to run copy command 256 1.
> FAIL: ini_parse_ut
>
> caused by PATCH 3
Same thing
>
>
> [PATCH 5/10]
>
> *Ack* (i noticed the warning with the first patch)
>
>
>
> review for patches 6-10 will follow.
>
> Ondra
>
I will send the refined patch set shortly
I reattached the set of patches.
Here is the problem. I was doing the changes jumping between different
parts of code so they are interconnected.
It turns out that I was wrong with my assessment that the changes are
independent in the first 5 patches from the later patches.
The change introduced in patch 3 to the serialization is later corrected
in patch 7.
Patch 3 is for comment object and as long as ini_comment_ut returns
success we are OK.
After patch 4 check ini_valueobj_ut - it should return success but
ini_parse_ut would fail.
The reason is the following lines:
- if (strncmp(key, INI_SPECIAL_KEY, sizeof(INI_SPECIAL_KEY)) == 0) {
- /* Special key carries only a comment */
- TRACE_FLOW_EXIT();
- return EOK;
}
I removed this change from the patch 4 and moved it to the later patch.
The regenerated patch set is attached.
Thanks for the review!
--
Thank you,
Dmitri Pal
Sr. Engineering Manager for IdM portfolio
Red Hat Inc.
-------------------------------
Looking to carve out IT costs?