On 21.08.2014 23:16, Benoît Canet wrote:
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.
The second change fit really well into this patch, so I left it in.
On the other hand, I just noticed I don't need to do that at all,
because that call will be dropped in patch 5 anyway.
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.
The main cause for the diff cluttering is making nb_clusters and
refcount_table pointers in the function. Unfortunately (or maybe
fortunately?) this is not C++, so I cannot make them references, but
they need to be passed by reference. The function absolutely must be
able to change their values and I don't see any way of avoiding this
even in the first extraction patch.