Re: [Qemu-devel] [PATCH v2 2/9] qcow2: Factor out refcount accounting for check

[Benoît Canet]

On Thu, Aug 21, 2014 at 01:13:20PM -0600, Eric Blake wrote:
> On 08/21/2014 12:57 PM, Benoît 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 check
> >> into an own function.
> >>
> >> Also, do not use g_realloc() for increasing the size of the in-memory
> >> refcount table, but rather g_try_realloc().
> 
> The "Also," may be a sign of doing two things in one patch that could
> have been two.
> 
> >>
> >> Signed-off-by: Max Reitz <address@hidden>
> >> ---
> >>  block/qcow2-refcount.c | 188 
> >> ++++++++++++++++++++++++++++++-------------------
> >>  1 file changed, 115 insertions(+), 73 deletions(-)
> 
> But overall, this patch seems like a reasonable refactor to me,
> splitting out a reusable piece.
> 
> > Another point is that I think these two extractions patches will totaly 
> > confuse
> > git blame the day we will need it.
> 
> 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 
appearing
elsewere with another author) and the operations could be reviewed more easily
with the various code review tools.

> 
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>