From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53643) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zvr0r-0000GM-Hl for qemu-devel@nongnu.org; Mon, 09 Nov 2015 13:17:54 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zvr0q-0001M3-FW for qemu-devel@nongnu.org; Mon, 09 Nov 2015 13:17:53 -0500 References: <1446663467-22485-1-git-send-email-mreitz@redhat.com> <1446663467-22485-5-git-send-email-mreitz@redhat.com> <20151109160432.GF3621@noname.redhat.com> From: Max Reitz Message-ID: <5640E344.4050406@redhat.com> Date: Mon, 9 Nov 2015 19:17:40 +0100 MIME-Version: 1.0 In-Reply-To: <20151109160432.GF3621@noname.redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="V33pogqVWrPJNElR2tNjfV4DIn0jK06an" Subject: Re: [Qemu-devel] [PATCH v6 04/15] iotests: Move _filter_nbd into common.filter List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: Alberto Garcia , qemu-block@nongnu.org, qemu-devel@nongnu.org, Markus Armbruster , Stefan Hajnoczi , Paolo Bonzini This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --V33pogqVWrPJNElR2tNjfV4DIn0jK06an Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: quoted-printable 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 shoul= d >> 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 >=20 > Code motion and modification in the same patch is bad style. The change= s > 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" instea= d. 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 =93rewritten=94 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 --V33pogqVWrPJNElR2tNjfV4DIn0jK06an Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQEcBAEBCAAGBQJWQONEAAoJEDuxQgLoOKytTz8H/2w+1lET+OZF3rFixDB94Vbk 3uTpDeWltkh0S/81uCRXu+PXXzH0n80OJfv4Io8TmfxBuVG6zMtHnL04rwLceRNA sSXIM61yHDo1i8npyqOvvFdOEfk+VbG58Qsa3sEGbYwhLfMqMCrFVICQPhr9TX5i dX7ej2WX91AD8XirY+Fa+GNh6k+eMM3n4FxdGxP/J5xS6GM8iTw/0VO4D2IIrMzj mhLQAJ3hKoP+j6wmToPFrd3BeMC/xbgK/rTW+tma60r8B0tAxyp+x4YIP6eFZ+5U H3OnfGNumQ3RGiOtCQxOlAh2VcUbOZ8c18fyCsYEjnuJZr6fa8wCabksGVxlpPQ= =Oizg -----END PGP SIGNATURE----- --V33pogqVWrPJNElR2tNjfV4DIn0jK06an--