Re: [Xen-devel] [PATCH v4 02/15] libxl: sanitize error handling in libxl_get_max_{cpus, nodes}

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Dario Faggioli writes ("Re: [Xen-devel] [PATCH v4 02/15] libxl: sanitize error 
handling in libxl_get_max_{cpus, nodes}"):
> Thanks for the explanation, and sorry for not getting what you mean/want
> sooner. It just differs from the way I've done things up to now, so I
> really wasn't sure I was understanding, rather than not wanting to do
> it. :-)

I think it would be helpful for me to expand on and motivate what Ian
C has said, which I agree with.

In general, code motion makes it very hard to review substantive
changes.  It makes it difficult to spot even very small substantive
changes.

On the other hand, it is much easier to review a patch that says in
its commit message that it contains _only_ code motion and a
specifically enumerated set of absolutely required changes (new
function beginnings and endings, GC_INIT and GC_FREE, etc. etc. (but
don't write "etc. etc." in the commit message - spell it out)).

Firstly, if you know a patch is supposed to be just code motion, it is
fairly easy to check this with an ad hoc approach.  And secondly, if
we trust that the person who wrote the commit message is scrupulous
about not introducing other stuff into it, we can take more of it on
trust.

And then the non-code motion patch is much easier to review too,
because it just says "add missing LOG calls" and we can verify that
that's what it is doing.

In general IMO splitting patches up should be done as much as
possible, so long as the changes themselves are not interrelated.  The
reason for this is that reviewing involves understanding the commit
message and then reviewing the code changes against the claimed
changes from the commit message.  This is task is much harder when the
commit message is a rag bag of small changes.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel