On 7.01.19 г. 20:46 ч., David Sterba wrote: > On Mon, Jan 07, 2019 at 04:22:01PM +0100, Johannes Thumshirn wrote: >> On 02/01/2019 18:43, Nikolay Borisov wrote: >>> On 2.01.19 г. 19:05 ч., David Sterba wrote: >>>> This is repeating code and could be simplified ... to the original code. >>> >>> It does and this is intentional. What I've strived to do is make the >>> idea of the code obvious and not try to reduce the total line of code. >>> It is a massive improvement given the code which modifies extent_end >>> triggers only in case ret is > 0. I discussed this with Johannes and he >>> agreed with my assessment. >>> >>>> I'm not sure this patch is an improvement. >> >> I think it really helps when trying to understand the code. >> >> This is why the patch was created in the first place, I asked Nik about >> something and we went over the code together. > > I've tried to read the new version again with fresh eyes and still don't > see any improvement in readability. Not that the whole function is easy > to follow, quite the opposite, but I'd rather see continued refactoring > that untangles the gotos. > > The return codes of btrfs_lookup_file_extent follow the common > btrfs_search_slot pattern < 0 error, 0 found, > 0 not found. And in the > last case there's an adjustment needed. Read the item type, check and > branch if necessary. No surprises there. Actually there is a surprise, this surprise is due to the fact that it's not explicitly clear the adjustments happens when > 0 is returned. With my patch it's clear which code gets executed only when the respective extent item wasn't found. > > The amount of duplicated code is also not something trivial like > a variable asisgnment, there are arguments passed etc. The amount of duplication are 3 lines of linear code, I think this is just being overly nitpicky given the (IMO) upside of this change. >
