On 07/20/2011 12:58 PM, Jan Zelený wrote:
> I'm sending several patches with the sysdb refactoring. Originally, I was
> planning one more step - to change return type from int to errno_t where
> it was convenient. If someone wants to do it, feel free. Otherwise I'm
> going to make the patch myself in couple days.
>
> When reviewing, please note that patches 40-43 have to be tested
> together. The same applies for patches 44-46.
>
> Jan
039 - what do you think about renaming "sysdb_ctx_list *ctx_list" as
well? Otherwise ACK.
I think the way the patches are split is wrong -- after you apply 40,
you can't even *compile* the code. If we ever wanted to cherry-pick any
of the changes to some other branch, it would be all-or-nothing.
The only reason to do it this way was to separate the change in sysdb itself
from the rest of code changes - mainly for the sake of lucidity.
I think the split should either be per-function, or, if you think
that's
too many patches perhaps per group of functions ("all functions that
handle users", "all functions that handle groups" etc.).
We already did a huge sysdb API rewrite once, in 1.3, and the patches
were per-function back then -- check out
d86fc9163127f7c5bd0c3af950fcddff7911867f and earlier.
Considering that it may be a lot of work to regroup the patches, I think
it may be even better to have squash 40-43 and 44-46 together, but a
patch that does not compile seems bad to me.
This sounds reasonable. Should I do the job and send them back or were you
thinking about squashing them during the push?
After applying the patches (there were two minor conflicts),
"make
check" and a simple smoke test passed. I didn't review the patches
completely yet.
Thanks for doing the review
Jan