Re: [PATCH v7 07/19] xfs: Factor out xfs_attr_leaf_addname helper

Allison Collins
2020-02-24T07:09:18Z

On 2/23/20 11:38 PM, Amir Goldstein wrote:
> On Sun, Feb 23, 2020 at 8:38 PM Allison Collins
> <allison.henderson@oracle.com> wrote:
>>
>> On 2/23/20 5:42 AM, Amir Goldstein wrote:
>>> On Sun, Feb 23, 2020 at 4:07 AM Allison Collins
>>> <allison.henderson@oracle.com> wrote:
>>>>
>>>> Factor out new helper function xfs_attr_leaf_try_add.
>>>
>>> Right. This should be the subject.
>>> Not factor out xfs_attr_leaf_addname helper.
>>>
>>>> Because new delayed attribute
>>>> routines cannot roll transactions, we carve off the parts of xfs_attr_leaf_addname
>>>> that we can use, and move the commit into the calling function.
>>>
>>> And that is a different change.
>>> It is hard enough to review a pure factor out of a helper.
>>> Adding changed inside a pure re-factor is not good.
>>>
>>> Belongs to another change - move transaction commit to caller.
>>
>> Yes, this came up in the last review, but the reason I avoid it is
>> because I think the transaction looks weird in either helper. The
>> reason the roll is here is because there is no room to add the attr as a
>> leaf, and so we need to hand it off to the node subroutines. But that
>> seems like something that should be managed by the caller, not leaf
>> helpers. So I was concerned that separating the split and the hoist
>> would generate more weird looks from people trying to understand the
>> split until the see the hoist in the next patch. If people really think
>> it looks better that way, I can split them up. But I know folks have a
>> tough enough time trying to recall the discussion history, so I'm trying
>> to avoid confusion of another type :-)
>>
>> Thoughts?
>>
>
> It's fine, so long as you document it properly in commit message.
> See, we are so bad in this review process, that we rely on humans
> to verify that logic preserving re-factoring patches are indeed logic
> preserving. This is so lame to begin with, because there are static
> checker bots out there that would gladly do that work for us :) ...
I didnt know there were tools out there that did that. They sound
helpful though!

> if we only annotated the logic preserving patches as such.
> In the mean while, while we are substituting the review robots,
> the least a developer could do is not call out a patch "re-factoring"
> and sneak in a logic change without due notice to reviewers.
Ok, what if the title were to say something like:
xfs: Split out node handling from xfs_attr_leaf_addname

Or maybe just "Split apart xfs_attr_leaf_addname"? Does that sound like
it would be a better description here?

Thanks!
Allison

>
> Thanks,
> Amir.
>