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

Amir Goldstein
2020-02-24T07:27:13Z

On Mon, Feb 24, 2020 at 9:09 AM Allison Collins
<allison.henderson@oracle.com> wrote:
>
>
>
> 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?
>

Sure, both are fine.
All I'm saying is that "factor out" is a well established keyword
that shouldn't be abused.

Thanks,
Amir.