On 12/20/19 7:57 PM, Cole Robinson wrote: > On 12/19/19 4:09 PM, Daniel Henrique Barboza wrote: >> This series got buried a few months ago. Let's go onward unto >> the 20s with no one left behind. >> >> changes from version 3 [1]: >> - rebased to master at commit 330b556829 >> - removed former patch 4. The 'g_strdup_printf' change was >> made along the road >> - new patch 4: I am sending this one in separate to patch 3 >> because this unneeded label was already in the code, being >> unrelated to the changes of this series. The maintainer is >> welcome to squash it into patch 3. >> >> [1] https://www.redhat.com/archives/libvir-list/2019-October/msg01080.html >> >> Daniel Henrique Barboza (4): >> qemu_process.c: use g_autofree >> qemu_process.c: use g_autoptr() >> qemu_process.c: remove cleanup labels after g_auto*() changes >> qemu_process.c: remove 'cleanup' label from >> qemuProcessCreatePretendCmd() >> >> src/qemu/qemu_process.c | 429 +++++++++++++++------------------------- >> 1 file changed, 157 insertions(+), 272 deletions(-) >> > > Patch 3 and 4 look fine, some comments on the first two. > > I can't really decide what is the best way to approach cleanups like > this. Should patches be split by function, and do all cleanups in one > pass, or do one type of cleanup, but over a larger surface per file? > Like you have done here. > > The first method is more tedious, but it's easier for reviewers, and > good patches can go in first while patches with issues can be kicked out > for v2. But it could be thousands of patches judging by the 3000+ > cleanup labels we have in the code base, which sounds extreme. > > I think in general you will find the list more receptive to series of > small per function patches though. Optimizing for ease of review will > always give things a better chance of getting committed in my > experience. This is just recommendation for future series though, I'll > review this one as is I think the sooner we get this over with the better (i.e. less rebase conflicts). It's like tearing off a patch (bandage?) - it hurts less if you do it all at once. Having said that, it still makes sense to honour our rules and have one semantic chnage per commit. Michal -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
