On 09.11.2015 19:17, Max Reitz wrote:
> 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 <mre...@redhat.com>
>>
>> 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.Oh, even better, I'll go for a mix of 1, 3 and 4: I'll first fix the code style of that function alone, then I'll rename it to _filter_nbd, then I'll move it, and then I'll fix it. Even more patches than 1, but not as many changes, as I don't have to fix all of 083. Max
signature.asc
Description: OpenPGP digital signature
