From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54320) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zvr3a-00020i-Ir for qemu-devel@nongnu.org; Mon, 09 Nov 2015 13:20:43 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zvr3Z-0001ya-Ai for qemu-devel@nongnu.org; Mon, 09 Nov 2015 13:20:42 -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> <5640E344.4050406@redhat.com> From: Max Reitz Message-ID: <5640E3F0.10201@redhat.com> Date: Mon, 9 Nov 2015 19:20:32 +0100 MIME-Version: 1.0 In-Reply-To: <5640E344.4050406@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="kEbuB0t2VsNnCMwbw0bINufNLT3w7Oosd" 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) --kEbuB0t2VsNnCMwbw0bINufNLT3w7Oosd Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: quoted-printable 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 shou= ld >>> 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 >> >> Code motion and modification in the same patch is bad style. The chang= es >> look good, though. >=20 > 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" inst= ead. >=20 > There actually is no good style for this patch. One could argue that th= e > 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. >=20 > The second variant would be not to move the code, but to "move" it, i.e= =2E > 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. >=20 > 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. >=20 > 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. >=20 > 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.= >=20 > 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 othe= r > way. >=20 > 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. >=20 > I guess I'll go for 3 just so you can see why I chose 2 before. I can d= o > 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 --kEbuB0t2VsNnCMwbw0bINufNLT3w7Oosd 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 iQEcBAEBCAAGBQJWQOPwAAoJEDuxQgLoOKytl0EH+wRS6HHjA2qaiFcOFMbvcJbn YkND7ZZf5RPkha5qkiG543eqMyd+UdKy4ZyYjIA+BVBNDUzmEpdV5kLGDryztGbM tFcc7MuXsnMwq16n6RNo7QhMqCaoA0YCzVEuZpBro8fvea21pVCU9ZL/m+yG0D1w aale+T64+VITNROsvRAtalxej/c6N62ESfJXOYwoS+IYEJTO9/mVY7sIdc5t24m/ bfnGYw0WZskut/94CV5v3T2sbhgIM8qWvThnlhtlbB8MeyftYHVzCSkJCh7IuAbX XQs2/ufGw41DfpviEWtmzOztaI+1WkNabgfnk9mxQKJnPiiw+RovO/M3wyuc9gA= =w52f -----END PGP SIGNATURE----- --kEbuB0t2VsNnCMwbw0bINufNLT3w7Oosd--