On Mon, Aug 5, 2013 at 3:45 PM, Michele Tartara <[email protected]> wrote: > On Mon, Aug 5, 2013 at 3:29 PM, Thomas Thrainer <[email protected]> wrote: >> >> >> >> >> On Mon, Aug 5, 2013 at 2:45 PM, Michele Tartara <[email protected]> >> wrote: >>> >>> On Mon, Aug 5, 2013 at 2:41 PM, Thomas Thrainer <[email protected]> >>> wrote: >>>> >>>> >>>> >>>> >>>> On Mon, Aug 5, 2013 at 1:20 PM, Michele Tartara <[email protected]> >>>> wrote: >>>>> >>>>> On Thu, Aug 1, 2013 at 11:18 AM, Thomas Thrainer <[email protected]> >>>>> wrote: >>>>>> >>>>>> Some configuration objects are accessed quite often, so introduce >>>>>> shortcut properties for those. >>>>>> >>>>>> Signed-off-by: Thomas Thrainer <[email protected]> >>>>>> --- >>>>>> test/py/cmdlib/cluster_unittest.py | 46 >>>>>> +++++++++++---------------- >>>>>> test/py/cmdlib/test_unittest.py | 14 ++++---- >>>>>> test/py/cmdlib/testsupport/cmdlib_testcase.py | 7 ++++ >>>>>> 3 files changed, 33 insertions(+), 34 deletions(-) >>>>> >>>>> >>>>> >>>>> Shouldn't these be introduced earlier given that they are part of the >>>>> same patchset? >>>> >>>> >>>> Yeah, they could. But actually I wanted to save some work by not faking >>>> history ;-). >>>> I would have to distribute this change over a couple of other commits, >>>> making sure in the progress that all tests continue to work after each >>>> commit. Given the fact that I changed and refined the test framework >>>> throughout the patch series again and again, I considered this change as a >>>> refinement too, which does not need to be hidden away... >>> >>> >>> I'm saying that just because sending already clean patches is the default >>> approach that has always been used in the past... >> >> >> Hmm, true. In this case, I probably should re-shape the whole patch series >> again, in which the first commits add the test framework in its final state, >> and only then actual tests are added. I'm not quite sure if this has >> advantages, as I guess it's even harder to review a patch series where a lot >> of support code or extractions are already made before the actual need is >> visible. >> In the past I remember some patch series where in early patches code >> duplication or other "bad" style was introduced, only to have it corrected >> in later patches of the series (especially from Helga, I guess this matches >> her style of work as well). I don't know what the "official" opinion about >> such patch series is. >> >> The ultimate goal, IMHO, should be to make the life of the developer and >> the reviewer as easy as possible. I like during reviews to have some context >> (which new requirements required a change, etc.), but that's just a matter >> of taste. >> >> I see multiple options for this case: >> - re-shape the whole patch series to be "framework-first" >> - only redistribute this "shortcut properties" patch over the other ones >> - leave it as is >> >> What do you think? Should we figure out some guidelines for patch series? >> Or are there already guidelines I don't know of? >> > > > As far as I know, this is the guideline that was used when I started > submitting patches, I guess to keep the history as clean as possible. And > that's the reason why I replied with this to your patch. > > Personally, I'd be in favor of changing that to allow things being > implemented in one way at the beginning of a patch set and corrected before > the end of it series, without the need for spreading the changes all over > the original patches, given that it's quite time consuming. And probably, > changing the code in later patches doesn't really do any harm, provided the > code is as it should be by the end of the patch set. > > Let's see if Guido (or anybody else) has some suggestion on how we should > deal with this. >
In general we've always tried to have patch series as small and clean as possible. This means not asking the reviewer to review code that will be removed later, not introduce breakage, etc. This doesn't mean it has to be brought to the extreme: it's ok to introduce a function and then make changes to it. Introducing it and moving it later to another file perhaps should be avoided. For this particular patch we can go for it, in general the guidelines are: - yes, we change history all the time (before code gets submitted), due to reviews, etc - don't introduce breakage you can avoid (makes bisects harder, etc) (but no need to commit-check at all intermediate points) - it's ok to alter code in later patches, but only as long as (a) it would be a lot more work to bring it back to a clean history and (b) it's not terribly radical (introduce function + delete function == no). For this patch i'd say it's LGTM, nothing too bad is happening Thanks, Guido
