On Wed 01-07-15 21:38:15, Tejun Heo wrote: > Hello, Jan. > > On Tue, Jun 30, 2015 at 05:02:54PM +0200, Jan Kara wrote: > > BTW, I'd prefer if this was merged with the following patch. I was > > wondering for a while about the condition at the beginning of > > blk_clear_congested() only to learn it gets modified to the one I'd expect > > in the following patch :) > > The patches are already merged, it's a bit too late to discuss but I > usually try to keep each step quite granular. e.g. I try hard to > avoid combining code relocation / restructuring with actual functional > changes so that the code change A -> B -> C where B is functionally > identical to A and C is different from B only where the actual > functional changes occur. Yeah, I didn't mean this comment as something you should change even if the series wasn't applied yet (it isn't that bad). I meant it more as a feedback for future. > I think your argument is that as C is the final form, introducing B is > actually harder for reviewing. I have to disagree with that pretty > strongly. When you only think about the functional transformations A > -> C might seem easier but given that we also want to verify the > changes - both during development and review - it's far more > beneficial to go through the intermediate stage as that isolates > functional changes from mere code transformation. > > Another thing to consider is that there's a difference when one is > reviewing a patch series as a whole tracking the development of big > picture and later when somebody tries to debug or bisect a bug the > patchset introduces. At that point, the general larger flow isn't > really in the picture and combining structural and functional changes > may make understanding what's going on significantly harder in > addition to making such errors more likely and less detectable in the > first place. In general I agree with you - separating refactoring from functional changes is useful. I just think you took it a bit to the extreme in this series :) When I'm reviewing patches, I'm also checking whether the function does what it's "supposed" to do. So in case of splitting patches like this I have to go through the series and verify that in the end we end up with what one would expect. And sometimes the correctness is so much easier to verify when changes are split that the extra patch chasing is worth it. But in simple cases like this one, merged patch would have been easier for me. I guess it's a matter of taste... Honza -- Jan Kara <jack@xxxxxxx> SUSE Labs, CR -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
