On Tue, Jan 2, 2024 at 8:02 AM Anand Jain <anand.jain@xxxxxxxxxx> wrote:
>
>
>
> On 12/29/23 20:57, Filipe Manana wrote:
> > On Fri, Dec 29, 2023 at 11:02 AM Anand Jain <anand.jain@xxxxxxxxxx> 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@xxxxxxxxxx>
> >> ---
> >> 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
