Re: [PATCH] generic/411: change sub-path name that's duplicate of TEST_DIR

[Eryu Guan <eguan@redhat.com>]

On Wed, Mar 08, 2017 at 04:26:22PM +0200, Amir Goldstein wrote:
> On Wed, Mar 8, 2017 at 1:51 PM, Eryu Guan <eguan@redhat.com> wrote:
> > On Wed, Mar 08, 2017 at 12:01:33PM +0200, Amir Goldstein wrote:
> >> On Wed, Mar 8, 2017 at 9:26 AM, Zorro Lang <zlang@redhat.com> wrote:
> >> > Darrick found generic/411 golden output mismatch if use
> >> > TEST_DIR=/mnt. Because g/411 use some test path named
> >> > /mnt/XXXX/mnt1/mnt2, _filter_test_dir will replace all
> >> > "/mnt" things to "TEST_DIR".
> >> >
> >> > For stop this failure, change all directory names to be
> >> > "$seq-XXX", that's less likely to be mistaken for TEST_*
> >> > and SCRATCH_*.
> >> >
> >>
> >> Although you have a right to choose whichever names you
> >> want top use for your test, this is papering over a bug.
> >>
> >> I re-read the docuemtnation for \B:
> >> http://www.rexegg.com/regex-boundaries.html#bengines
> >>
> >> To my understanding, the expression "\B$TEST_DIR" will
> >> match every instance of $TEST_DIR, where preceding character
> >> is NOT a letter, number or underscore.
> >> This is because $TEST_DIR must start with '/', which is not
> >> a letter, number or underscore.
> >>
> >> I think it should be safe to fix _filter_test_dir and _filter_scratch.
> >
> > I agreed, according to the document above, \B matches all positions
> > where \b doesn't match. And \b matches positions where "one side is a
> > word character and the other side is not", so \B matches "neither side
> > is a word character" and "both sides are a word character".
> >
> > This is also because we canonicalized all mount points, there's no path
> > like //mnt/mnt1 is allowed in fstests. And this leads me to wonder if we
> > should canonicalize all test devices (if they're block device), to avoid
> > something like //dev/sda5? The double "/" will break the \B match.
> >
> 
> //dev/sda5 does to break \B match.
> \B$TEST_DEV match, as you wrote, means that character before
> $TEST_DEV *is not* a word character, because $TEST_DEV begins
> with a non-word character. it doesn't matter the the second character
> is a non-word as well.
> 
> Attached a patch that I tested -g quick on xfs and overlayfs
> and did not see any regressions, but you better test it with a wider
> variety of fs types and a larger group of tests.

Sure, I'll give it more tests. Thanks!

> 
> > Further more, if we decide to use \B to improve _filter_test_dir and
> > _filter_scratch, it appears to me that the fix from commit 4e965d8
> > ("fstests: fix test and scratch filters for overlapping DEV/MNT paths")
> > can be discarded, the order is not a problem anymore.
> >
> 
> Well, not exactly.
> 
> The case that commit 4e965d8 fixes is:
> TEST_DEV=/mnt
> TEST_DIR=/mnt/ovl-mnt
> 
> So \B$TEST_DEV will still match the prefix of $TEST_DIR and this is
> wrong.

You're right, I missed that.

Thanks,
Eryu