On Fri, 2011-07-29 at 08:39 +0200, Jan Zelený wrote:
> > 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
Just for the record, I'm intentionally deferring this commit until we
branch sssd-1-6, because it's too sizable a change to make this close to
release.
Jan, please keep an eye on these patches and make sure they continue to
apply against master. It will be the first set to go in once we branch.
Ok, will do
Jan