s3-talloc Change TALLOC_ZERO_P() to talloc_zero()

[Andrew Bartlett]

On Fri, 2011-06-10 at 13:25 +0200, Michael Adam wrote:
> Hi Andrew,
> 
> Andrew Bartlett wrote:
> > The branch, master has been updated
> >        ...
> >        via  ad0a07c s3-talloc Change TALLOC_ZERO_P() to talloc_zero()
> >        ...
> >
> > commit ad0a07c531fadd1639c5298951cfaf5cfe0cb10e
> > Author: Andrew Bartlett <abartlet at samba.org>
> > Date:   Tue Jun 7 11:44:43 2011 +1000
> > 
> >     s3-talloc Change TALLOC_ZERO_P() to talloc_zero()
> >     
> >     Using the standard macro makes it easier to move code into common, as
> >     TALLOC_ZERO_P isn't standard talloc.
> 
> You really need to be extremely careful with monster commits like
> this, changing so many files.
> 
> E.g. Did you do this talloc hierarchy change intentionally:
> 
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> diff --git a/source3/winbindd/idmap_autorid.c b/source3/winbindd/idmap_autorid.c
> index 80d8ed1..0e1750d 100644
> --- a/source3/winbindd/idmap_autorid.c
> +++ b/source3/winbindd/idmap_autorid.c
> @@ -446,7 +446,7 @@ static NTSTATUS idmap_autorid_initialize(struct idmap_domain *dom)
>                 goto error;
>         }
>  
> -       config = TALLOC_ZERO_P(frame, struct autorid_global_config);
> +       config = talloc_zero(dom, struct autorid_global_config);
>         if (!config) {
>                 DEBUG(0, ("Out of memory!\n"));
>                 status = NT_STATUS_NO_MEMORY;
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> I guess this file had just changed between you writing the patch and
> pushing the patch to autobuild.
>
> In fact the chang would be correct, because my previous commit to
> autobuild has an error. But I was wondering whether you noticed this
> and fixed it in stealth mode, or if this was just accidentially "fixed"
> unnoticed in conflict resolution...

Yes, this was a case of incorrect conflict resolution, and for that I do
apologise.  There was only one conflict, so hopefully there are no other
dragons lurking in this change. 

Rest assured I would never slip in 'fixup' changes unannounced into a
commit such as this.

> Don't worry about changin the code, please: I am just changing/fixing
> this code file anyways. But I was just curious because I stumbled
> over this conflict when I now tried to rebase changes I made... :-)
> 
> Again, my lesson is: Try to change as little files in one
> commit as possible.

There are still a number of other issues (types without _t, True and
False) that come up every time I move code from source3 to the common
code, so I am very keen to learn the lessons here:  How would you have
handled this better?

So far to reduce the patch risk I split the change up into one commit
per global replace, and confirmed each one twice (emacs interactive
tag-replace and git add --patch).  In some senses the safest thing to do
then would have been to just put it in the tree, when the only actions
were programmatic search and replace.  But for a change this big it
seemed in-prudent, which in turn brings the risk of the tree changing,
which then introduces the unsafe thing: the human editor. 

It is critical to me that this kind of harmonisation is regarded as
low-risk, so what additional steps would you recommend?  How would you
have handled this with 'as little files in one commit as possible'?
Would the same patches, but of sub-trees 'remove TALLOC macros under
smbd/' been better?

Thanks,

Andrew Bartlett

-- 
Andrew Bartlett                                http://samba.org/~abartlet/
Authentication Developer, Samba Team           http://samba.org