From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34390) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XKZiR-0007TM-5U for qemu-devel@nongnu.org; Thu, 21 Aug 2014 17:16:20 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XKZiM-0005Tv-7d for qemu-devel@nongnu.org; Thu, 21 Aug 2014 17:16:15 -0400 Received: from dew.nodalink.com ([95.130.14.197]:39709) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XKZiM-0005Tp-1J for qemu-devel@nongnu.org; Thu, 21 Aug 2014 17:16:10 -0400 Date: Thu, 21 Aug 2014 21:16:07 +0000 From: =?iso-8859-1?Q?Beno=EEt?= Canet Message-ID: <20140821211607.GC15636@nodalink.com> References: <1408115786-13640-1-git-send-email-mreitz@redhat.com> <1408115786-13640-3-git-send-email-mreitz@redhat.com> <20140821185737.GB15636@nodalink.com> <53F644D0.2020701@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <53F644D0.2020701@redhat.com> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v2 2/9] qcow2: Factor out refcount accounting for check List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: Kevin Wolf , Stefan Hajnoczi , qemu-devel@nongnu.org, =?iso-8859-1?Q?Beno=EEt?= Canet , Max Reitz On Thu, Aug 21, 2014 at 01:13:20PM -0600, Eric Blake wrote: > On 08/21/2014 12:57 PM, Beno=EEt Canet wrote: > > On Fri, Aug 15, 2014 at 05:16:19PM +0200, Max Reitz wrote: > >> Put the code for calculating the reference counts during qemu-img ch= eck > >> into an own function. > >> > >> Also, do not use g_realloc() for increasing the size of the in-memor= y > >> refcount table, but rather g_try_realloc(). >=20 > The "Also," may be a sign of doing two things in one patch that could > have been two. >=20 > >> > >> Signed-off-by: Max Reitz > >> --- > >> block/qcow2-refcount.c | 188 ++++++++++++++++++++++++++++++--------= ----------- > >> 1 file changed, 115 insertions(+), 73 deletions(-) >=20 > But overall, this patch seems like a reasonable refactor to me, > splitting out a reusable piece. >=20 > > Another point is that I think these two extractions patches will tota= ly confuse > > git blame the day we will need it. >=20 > I disagree. When refactoring to split a large function into multiple > smaller functions, where the original function calls out to a helper > function for what it used to do inline, git blame tracks things > perfectly. Someone following blame may end up on this commit as the > source of a given line in its new location, but the blame viewer will > also show this commit is a refactor, and it should be fairly easy to > find where the line was moved from, and resume blaming even further. I think we could separate the extractions and the respectives moves into = their own patches. This way the extraction patch would be cleaner (no code disapearing and a= ppearing elsewere with another author) and the operations could be reviewed more e= asily with the various code review tools. >=20 > --=20 > Eric Blake eblake redhat com +1-919-301-3266 > Libvirt virtualization library http://libvirt.org >=20