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

Amir Goldstein
2020-02-24T06:38:26Z

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 :) ...
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.

Thanks,
Amir.