Re: [libvirt] [PATCH 1/5] virutil: Move string related functions to virstring.c

On 04/02/2013 08:22 AM, Michal Privoznik wrote: ... Looks like most of the patch is fallout from new include file needed for using existing functions in their new location,... ...and that the split of virutil.c into virstring.h is the meat of the patch. Just from these numbers, I'm wondering if it would have been worth splitting this patch into at least two, so that the code motion portion of the patch is easier to find without having to hunt to the middle of the giant patch. More thoughts on splitting below. It is also possible to use 'git format-patch/send-email -O/path/to/file' where /path/to/file contains a list of shell globs that control the order in which the patch file is generated, so that the important changes (src/util/virstring.[ch], src/util/virutil.[ch]) come first, and the mechanical fallout last. You mixed sorting include names alongside adding the new include name, which makes it a bit harder to follow, and makes the diff longer to read. I would have split this into two to do include sorting first, code motion and addition of include virstring.h second. But making you redo things to split it now seems like a lot of work for little gain. Oh well. One thing is for certain - your commit message doesn't mention that you did more than just add a new include. It took me this far into the review to realize WHY your diff was so large (I was expecting lots of 1-line-additions for a new include, and was anticipating that things like a +12/-11 would be due to use of a renamed function, only to find out that you didn't rename any functions). Sorting includes is not necessarily bad, but I should have found about it from the commit message alone, not by diving into the patch itself. On the surface, this hunk feels completely unrelated. Obviously, it is fallout from some other file changing includes to be in alphabetical order, and it looks correct in isolation, but this is again evidence that you crammed a bit much into one patch. This list looks like a reasonable set of string-related functions. While we generally like to name functions with a prefix that matches the file it is found in, I agree that these names and this particular file are common enough to be an exception to that rule (that is, requiring someone to type something like virStrVasprintf would defeat the point of having a convenient asprintf wrapper). My quick glance didn't spot any obvious flaws when doing the code motion. Pre-existing - but since we already rejected too-large n, we are already guaranteed that dest[n] is NUL without needing this statement. Don't clean it up during the code motion; if you do clean it, it should be a separate patch. Oh, and 'NUL' is the all-0-bit character (generally one byte unless you are coding with wchar_t), NULL is the pointer (generally multiple bytes), but there's enough comments in the code base that use NULL where NUL was intended that I've given up trying to make them all correct :) Indentation is off, but looks like that was pre-existing in the old location. ACK with a better commit message. While I pointed out a few things that are worth changing, those may be better as a followup patch (so that the code motion is straight motion); and it would be quite time-consuming to require you to split this into two patches. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org