Re: [PATCH 3/4] btrfs: Refactor retval handling of btrfs_lookup_file_extent in btrfs_get_extent

[Nikolay Borisov <nborisov@suse.com>]

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.

>