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