Re: [PATCH 05/10] common: add _filter_trailing_whitespace

[Filipe Manana <fdmanana@kernel.org>]

On Tue, Jan 2, 2024 at 8:02 AM Anand Jain <anand.jain@oracle.com> wrote:
>
>
>
> On 12/29/23 20:57, Filipe Manana wrote:
> > On Fri, Dec 29, 2023 at 11:02 AM Anand Jain <anand.jain@oracle.com> wrote:
> >>
> >> The command 'btrfs inspect-internal dump-tree -t raid_stripe'
> >> introduces trailing whitespace in its output.
> >> Apply a filter to remove it. Used in btrfs/30[4-8][.out].
> >>
> >> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> >> ---
> >>   common/filter | 5 +++++
> >>   1 file changed, 5 insertions(+)
> >>
> >> diff --git a/common/filter b/common/filter
> >> index 509ee95039ac..016d213b8bee 100644
> >> --- a/common/filter
> >> +++ b/common/filter
> >> @@ -651,5 +651,10 @@ _filter_bash()
> >>          sed -e "s/^bash: line 1: /bash: /"
> >>   }
> >>
> >> +_filter_trailing_whitespace()
> >> +{
> >> +       sed -e "s/ $//"
> >> +}
> >
> > If we're having such a generic filter in common file, than I'd rather
> > have it delete any number of trailing white spaces, not just a single
> > one, and also account for tabs and other white spaces, so:
> >
> > sed -e "s/\s+$//"
> >
>
>   I'll amend.
>
>
> > Also, since this is so specific to the raid stripe tree, I'd rather
> > have this filter included in the raid stripe tree filter introduced in
> > patch 2, _filter_stripe_tree(). That would make the tests shorter and
> > cleaner by avoiding piping yet over another filter that is used only
> > for the raid stripe tree dump...
>
>   I kept this as a separate function so that it can be used elsewhere
>   when needed. Doesn't that make sense?

Not so much if there's only one use case for it... specially if it's
such a trivial filter...

Even if we had multiple cases, doing this pattern in the tests:

$BTRFS_UTIL_PROG inspect-internal dump-tree (... ) |
_filter_trailing_whitespace | _filter_btrfs_version |
_filter_stripe_tree

Is ugly and verbose. The filtering could be done in
_filter_stripe_tree() by calling "_filter_triling_whitespace" there...
And mentioning that, we could also call _filter_btrfs_version there,
since it's always wanted and to make tests shorter and easier to read.

So in the end it would only be

$BTRFS_UTIL_PROG inspect-internal dump-tree (... ) | _filter_stripe_tree

With _filter_stripe_tree() as:

_filter_stripe_tree()
{
    _filter_trailing_whitespace | _filter_btrfs_version | sed -E -e (....)
}

Or:

_filter_stripe_tree()
{
    _filter_btrfs_version | sed -E -e "s/\s+$//" -e (...)
}

A lot more clean.

Thanks.


>
> Thanks, Anand