On 12/20/19 3:54 PM, Michal Prívozník wrote: > 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. > Yes I agree. Dropping the ALLOC < 0 bits shouldn't be done incrementally. It should be a largely mechanical change, everything under the 'if' is already dead code. - Cole -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
