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

[Max Reitz <mreitz@redhat.com>]

On 22.08.2014 17:37, Benoît Canet wrote:
> On Fri, Aug 22, 2014 at 05:26:45PM +0200, Max Reitz wrote:
>> 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 <mreitz@redhat.com>
>>>>>> ---
>>>>>>   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.
> Do a:  git show f75c744 | kompare -
>
> You will see a whole block of code being morphed to int64_t i and the same whole
> block reapear below.
> I think this is a consequence of the extracted functions moving up at the same time
> they are extracted.

This is not a consequence of the extracted functions moving up, but 
because of check_refblocks() preceding calculate_refcounts(). Splitting 
the check won't help; and I know that Eric dislikes 
(forward-)declarations of static functions (which I have to do if I pull 
calculate_refcounts() before check_refblocks()).

Max