| Message ID | cf58eed9c9b9134b94fb6872d37b75a0c0bbe914.1703838752.git.anand.jain@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fstests: add tests for btrfs' raid-stripe-tree feature | expand |
pls ignore this sole patch.
On Fri, Dec 29, 2023 at 12:30 PM Anand Jain <anand.jain@oracle.com> wrote: > > pls ignore this sole patch. > How can it be ignored if other patches in this patchset that introduce tests use the filter introduced by this patch? Thanks.
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+$//" 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... Thanks. > + > # make sure this script returns success > /bin/true > -- > 2.39.3 > >
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? Thanks, Anand
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
>>> 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... > Hmm. Yes. Also, a completely avoidable coding nitpick in btrfs-progs. > 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. > Yeah. > 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. > I'm fine with either way. Since there is a choice here, I will keep the former. I'll send a reroll. Thanks, Anand
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/ $//" +} + # make sure this script returns success /bin/true
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(+)