Updated the patch with the suggested changes. The last commit for the new patch is https://www.redhat.com/archives/libvir-list/2020-November/msg01324.html.
On Fri, Nov 20, 2020 at 2:55 PM Laine Stump <[email protected]> wrote: > On 11/20/20 11:43 AM, Barrett J Schonefeld wrote: > > I appreciate the feedback on this patch! > > > > I will work on splitting this into multiple patches. I believe this will > > involve redoing much of the work because I will need to split this patch > > (a single commit) into many commits. > > One suggestion on how to more easily split one patch into multiple > patches (keeping in mind there's probably a much cleaner way of doing > the same thing; this is just how I've evolved to do it): > > 1) make a new branch "X-v2" based off the branch "X" that has this > current patch > > 2) "git reset HEAD^" on the new branch - this will remove the last > commit from git, but leave the working copies of the file unchanged. > > 3) use a tool like "git meld" to interactively go through all the > changes (hunks) you've made in this single commit, *un*doing the ones > that aren't related to basic g_autofree conversion. > > 4) git add / git commit -m"convert to g_autofree" > > 5) "git meld X" to compare the original commit on the *old* branch to > the tip of the new branch, interactively re-applying all the hunks that > you had just removed. > > 5) git add / git commit -m"remove unnecessary cleanup labels and return > variables" > > > Hence, I'd like to get some > > confirmation on how I should approach the patch. > > > > I plan to: > > > > 1. Address the feedback on returning `-errno`, `0`, `-1`, etc. directly > > instead of setting the local variable, `ret`, and returning `ret`. > > 2. Submit a patch per file with only the g_autofree changes. > > 3. Submit a patch per file that removes the cleanup sections. > > A couple of qualifiers: > > a) changing the return values to constants will of course happen as a > part of the "cleanup label removal" patch (item (3)), not on its own. > > b) a recent patch from jtomko reminded me of two occasions when you > *would* want a separate patch for the g_autofree changed in a single > function all by itself: > > ii) if that change fixes a bug (which would usually be a memory leak), > > and/or > > iii) if it requires any change in the logic of the function beyond > simply adding "g_autofree virBlahPtr blah = NULL;" and > removing the VIR_FREE(blah); at the end of the scope. > > Both of these would warrant extra explanation in the commit log, and > that's easier to follow if it's isolated. > > >
