s3-talloc Change TALLOC_ZERO_P() to talloc_zero()

[Michael Adam]

Andrew Bartlett wrote:
> On Fri, 2011-06-10 at 13:25 +0200, Michael Adam wrote:

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

Yeah, I did not assume that, either. :-)

> > 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?

I don't say that I have a recipe here that would catch all such
errors. My personal feeling is simply that I is better to make
patches as minimal/atomic as possible. If you have a conflict to resolve
in a very small patch, it may be easier not to move too quickly over
it but be watchful than in a big patch with possibly many
conflicts.

Also I generally think that it is more easy to read and
understand smaller patches (such search and replace patches are
not hard to understand of course... :-). And if one later wants
to cherry-pick suche changes, the chance that a single patch
still applies ist the greater the smaller the patch is.
(Touch too many files and chances are it won't apply to another branch.)

So my personal strategy is:
1. Try to touch as little files as possible in one patch
   (if possible, just one)
2. Try to touch as little functions (or structure) in one patch
   (if possible, just one)

It is of course not always possible or desirable to follow this,
but my personal experience is: most of the times this increases
the patch quality.

Well, again, this is my personal strategy. Of course I may not be
the more senior developer to give advice, and I dont want to
appear assuming, but I think these do make a lot of sense.
(And, of course, I am always hunting for ohloh-points myself,
so I might overdo this sometimes... ;-)

> 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.

Right, this would have been the safest thing.

> 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,

It definitely is.

> 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?

As I said above: yep. committing Per sub-tree or even per file.
In this case this would have been an awful lot of patches, but
my thought was that maybe one might have been more watchful in
conflict resolution.

Finally: Don't get me wrong: This could have happend to everyone!
And I am not blaming you for having done it. I just wanted to
take the chance mention my impression that it might have been
easier (maybe only for me) to spot the problem with smaller
patches. And again, generally praise the usefulness of small and
atomic commits...

Cheers - Michael

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 206 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20110623/031156c7/attachment.pgp>