qemu-block
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-block] [PATCH v6 04/15] iotests: Move _filter_nbd into common.


From: Max Reitz
Subject: Re: [Qemu-block] [PATCH v6 04/15] iotests: Move _filter_nbd into common.filter
Date: Mon, 9 Nov 2015 19:17:40 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0

On 09.11.2015 17:04, Kevin Wolf wrote:
> Am 04.11.2015 um 19:57 hat Max Reitz geschrieben:
>> _filter_nbd can be useful for other NBD tests, too, therefore it should
>> reside in common.filter, and it should support URLs of the "nbd://"
>> format and export names.
>>
>> The NBD log lines ("/your/source/dir/nbd.c:function():line: error")
>> should not be converted to empty lines but removed altogether.
>>
>> Signed-off-by: Max Reitz <address@hidden>
> 
> Code motion and modification in the same patch is bad style. The changes
> look good, though.

Considering splitting this into two patches will result basically in
both of them each changing just as much as this single patch does
(because test 083 uses tabs instead of spaces) I'm inclined to just
change the commit title to "Remove filter_nbd and add _filter_nbd" instead.

There actually is no good style for this patch. One could argue that the
coding style in all of test 083 is broken since it uses tabs instead of
spaces, so first I'd need to fix up the style of 083 completely. Then,
in a second patch, I can drop those empty lines, and in a third patch I
can move the function. I consider that horrible when it's just about
getting the code to common.filter.

The second variant would be not to move the code, but to "move" it, i.e.
leave the coding style in 083 and just convert the style of this
function when moving it to common.filter. Well, if I'm already doing
that, I might just as well fix that empty line thing on the way.

The third variant would be to fix that empty line thing in 083, and fix
the code style of that single function along with it, and then move it
to common.filter in a separate patch.

And then we have the fourth way which would be to move nbd_filter to
common.filter as it is, and then fix up the coding style there.

So let's look at my opinion for each of them:
(1) I find it horrible, but I can do that.
(2) That's what I did and that's what I'd do again.
(3) I'm opposed to change the style of that one function inside of 083
    without changing the rest of the file, but not strongly enough not
    to do it.
(4) I will definitely not insert tabs, even if it's just code movement.

I still consider 2 very reasonable for the issue at hand since the
function is very small and it will have to be completely “rewritten”
anyway in some patch (because the tabs to spaces change is absolutely
necessary at some point when moving it from 083 to common.filter).
Therefore, I don't think reviewers gain anything from doing it any other
way.

I consider 1 (fixing up all of 083 just so that I can move this little
function) so horrible that I won't do it unless there is another way,
and 2 already is another way, so that's that.

I guess I'll go for 3 just so you can see why I chose 2 before. I can do
1 in v8 if you insist, so you can get to experience that, too.

Max

Attachment: signature.asc
Description: OpenPGP digital signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]